Merge #19237: wallet: Check size after unserializing a pubkey

37ae687f95c82f2d64ed880533d158060d4fc3de Add tests for CPubKey serialization/unserialization (Elichai Turkel)
9b8907faded8e4ec312c0dd4b4b15e1793876acd Check size after Unserializing CPubKey (Elichai Turkel)

Pull request description:

  Found by practicalswift, closes #19235
  Currently all the public API(except the pointer-like API) in CPubKey that sets/constructs a pubkey goes through `CPubKey::Set` which checks if that the length and size match and if not invalidates the key.

  This adds the same check to `CPubKey::Unserialize`, sadly I don't see an easy way to just push this to the existing checks in `CPubKey::Set` but it's only a simple condition.

  The problem with not invalidating is that if you write a pubkey like: `{0x02,0x00}` it will think the actual length is 33(because of `size()`) and will access uninitialized memory if you call any of the functions on CPubKey.

ACKs for top commit:
  practicalswift:
    re-ACK 37ae687f95c82f2d64ed880533d158060d4fc3de
  jonatack:
    Code review re-ACK 37ae687 per `git diff eab8ee3 37ae687` only change since last review at eab8ee3 is passing the `pubkey` param by reference to const instead of by value in `src/test/key_tests.cpp::CmpSerializationPubkey`
  MarcoFalke:
    ACK 37ae687f95c82f2d64ed880533d158060d4fc3de

Tree-SHA512: 30173755555dfc76d6263fb6a59f41be36049ffae7b4e1b92b922d668f5e5e2331f7374d5fa10d5d59fc53020d2966156905ffcfa8b8129c1f6d0ca062174ff1

2 files changed, 47 insertions, 0 deletions

diff --git a/src/pubkey.h b/src/pubkey.h
index 261842b7f7..4c28af4a4d 100644
--- a/src/pubkey.h
+++ b/src/pubkey.h
@@ -142,6 +142,9 @@ public:
         unsigned int len = ::ReadCompactSize(s);
         if (len <= SIZE) {
             s.read((char*)vch, len);
+            if (len != size()) {
+                Invalidate();
+            }
         } else {
             // invalid pubkey, skip available data
             char dummy;
diff --git a/src/test/key_tests.cpp b/src/test/key_tests.cpp
index cf2bd03698..fd35537c77 100644
--- a/src/test/key_tests.cpp
+++ b/src/test/key_tests.cpp
@@ -5,6 +5,7 @@
 #include <key.h>
 
 #include <key_io.h>
+#include <streams.h>
 #include <test/util/setup_common.h>
 #include <uint256.h>
 #include <util/strencodings.h>
@@ -220,4 +221,47 @@ BOOST_AUTO_TEST_CASE(key_key_negation)
     BOOST_CHECK(key.GetPubKey().data()[0] == 0x03);
 }
 
+static CPubKey UnserializePubkey(const std::vector<uint8_t>& data)
+{
+    CDataStream stream{SER_NETWORK, INIT_PROTO_VERSION};
+    stream << data;
+    CPubKey pubkey;
+    stream >> pubkey;
+    return pubkey;
+}
+
+static unsigned int GetLen(unsigned char chHeader)
+{
+    if (chHeader == 2 || chHeader == 3)
+        return CPubKey::COMPRESSED_SIZE;
+    if (chHeader == 4 || chHeader == 6 || chHeader == 7)
+        return CPubKey::SIZE;
+    return 0;
+}
+
+static void CmpSerializationPubkey(const CPubKey& pubkey)
+{
+    CDataStream stream{SER_NETWORK, INIT_PROTO_VERSION};
+    stream << pubkey;
+    CPubKey pubkey2;
+    stream >> pubkey2;
+    BOOST_CHECK(pubkey == pubkey2);
+}
+
+BOOST_AUTO_TEST_CASE(pubkey_unserialize)
+{
+    for (uint8_t i = 2; i <= 7; ++i) {
+        CPubKey key = UnserializePubkey({0x02});
+        BOOST_CHECK(!key.IsValid());
+        CmpSerializationPubkey(key);
+        key = UnserializePubkey(std::vector<uint8_t>(GetLen(i), i));
+        CmpSerializationPubkey(key);
+        if (i == 5) {
+            BOOST_CHECK(!key.IsValid());
+        } else {
+            BOOST_CHECK(key.IsValid());
+        }
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()