Use of uninitialized memory when validating successfully deserialized CPubKey #19235

Travis errors on #18912 (PR adding Valgrind checking) due to the following CPubKey deserialization issue. See Travis log for details.

Issue:

It is possible to successfully deserialize a public key (CPubKey) which when used -- such as checking for validity using IsFullyValid() -- leads to use of uninitialized memory (UUM):

Example:

const std::vector<uint8_t> serialized_pub_key{4, 4, 0, 0, 0};
CDataStream data_stream{serialized_pub_key, SER_NETWORK, INIT_PROTO_VERSION};
CPubKey pub_key;
data_stream >> pub_key;
if (pub_key.IsValid()) {
    (void)pub_key.IsFullyValid();
}

The example code leads to the following use of uninitialized memory (UUM) (in IsFullyValid()) ...

==6417== Conditional jump or move depends on uninitialised value(s)
==6417==    at 0x1077684: secp256k1_fe_set_b32 (field_5x52_impl.h:320)
==6417==    by 0x1056B82: secp256k1_eckey_pubkey_parse (eckey_impl.h:23)
==6417==    by 0x1056B82: secp256k1_ec_pubkey_parse (secp256k1.c:178)
==6417==    by 0xF4A6A3: CPubKey::IsFullyValid() const (pubkey.cpp:213)

... instead of the expected std::ios_base::failure when deserializing (on data_stream >> pub_key).

Uses of uninitialized memory (UUM) is unfortunately a quite common type of bug, but luckily MemorySanitizer and Valgrind can help us guard against such issues entering our code base. Travis MSan checking and Valgrind checking is provided in the following two PRs which should be ready for merge pending final code review (they have multiple "Concept ACKs"):

• #18288 -- "build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory"
• #18912 -- "ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors"

Hopefully uses of uninitialized memory will soon be a thing of the past when we have the proper UUM detection in place :)

Does this still happen with #19228 ?

@MarcoFalke I would assume so: AFAICT the problem isn't in libsecp256k1 but in CPubKey::Unserialize. More specifically I think the problem is that CPubKey::Unserialize isn't throwing the expected serialization error: we wan't std::ios_base::failure to be thrown when deserializating an invalid object :)

Or alternatively I guess Invalidate() could be called from CPubKey::Unserialize to invalidate the object. That would make GetLen(…) return a size of zero which avoids the issue.

CPubKey::Unserialize Doesn't validate anything (except a max size) https://github.com/bitcoin/bitcoin/blob/bc933aeaf04420218b46318e316ee5eaa772823c/src/pubkey.h#L144 And then when you call IsFullyValid() it will get the length via GetLen which will check the first byte and return a size accordingly, so in you example the first byte is 4 so it will return 65. https://github.com/bitcoin/bitcoin/blob/bc933aeaf04420218b46318e316ee5eaa772823c/src/pubkey.h#L57-L64.

I believe one solution can be calling CPubKey::Set from CPubKey::Unserialize OR check after reading if(len != GetLen(vch[0])) Invalidate();