wallet: Check size after unserializing a pubkey #19237

elichai · 2020-06-10T14:39:53Z

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.

practicalswift · 2020-06-10T15:37:14Z

Thanks for addressing this @elichai! :)

Concept ACK

Regarding the implementation: if ReadCompactSize(s) returns zero won't vch[0] be read uninitialized in size()
(unsigned int size() const { return GetLen(vch[0]); }) before the call to Invalidate() takes place?

Also, what about adding a test case for the fixed issue that would fail under MemorySanitizer or Valgrind? Something exercising these code paths (plus assertions):

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;
(void)pub_key.IsFullyValid();

And perhaps also a test case where ReadCompactSize(s) returns zero to cover that case too :)

elichai · 2020-06-10T15:40:27Z

Regarding the implementation: if ReadCompactSize(s) returns zero won't vch[0] be read uninitialized in size()

I thought so at first, but no the constructor invalidates it (which writes 0xFF into vch[0])

bitcoin/src/pubkey.h

Lines 79 to 82 in 6762a62

    
           CPubKey() 
        
           { 
        
               Invalidate(); 
        
           }

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;
(void)pub_key.IsFullyValid();

I'll add a few tests.

practicalswift · 2020-06-10T19:12:13Z

@elichai Great! Another very nice thing with your fix is that it addresses another CPubKey oddity which I reported back in issue #17238: serialization-deserialization roundtrip of CPubKey did not necessarily result in an equal object. CPubKey was the only deserializable class that had that unexpected behaviour which made it a potential future gotcha. Glad to see it fixed too :)

More specifically …

CPubKey pk1;
CDataStream s1{std::vector<uint8_t>{'\x01', '\x72'}, SER_NETWORK, INIT_PROTO_VERSION};
s1 >> pk1;

CDataStream s2{SER_NETWORK, INIT_PROTO_VERSION};
s2 << pk1;

CPubKey pk2;
s2 >> pk2;

assert(pk1 == pk2);

… used to result in …

Assertion `pk1 == pk2' failed.

… but now passes (as expected). Note that the assertion failure was not due to UUM: no UUM takes place in this specific example.

Might be worth adding a test case also for that too, and perhaps also enable this commented out code from the CPubKey deserialization fuzzer:

bitcoin/src/test/fuzz/deserialize.cpp

Lines 118 to 122 in 6762a62

    
           #elif PUB_KEY_DESERIALIZE 
        
                   CPubKey pub_key; 
        
                   DeserializeFromFuzzingInput(buffer, pub_key); 
        
                   // TODO: The following equivalence should hold for CPubKey? Fix. 
        
                   // AssertEqualAfterSerializeDeserialize(pub_key);

pstratem · 2020-06-11T04:27:32Z

for anybody else wondering this doesn't look like a consensus change as the script interpreter uses the CPubKey constructor which calls Set.

laanwj · 2020-06-11T18:01:38Z

Concept ACK. I think it's good to assure the correctness of deserialized data. Especially as you say, a size() > len could potentially end up with uninitialized data in the key.

luke-jr

utACK

Github-Pull: bitcoin#19237 Rebased-From: 9b8907f

elichai · 2020-06-15T12:29:10Z

Added a few tests, these tests should fail before the fix commit and should trigger valgrind before the fix commit.

practicalswift · 2020-06-15T12:56:49Z

Thanks for adding tests!

ACK eab8ee3211e661dfb41f0363f6bf6bcabfc521fa -- patch looks correct

jonatack

ACK eab8ee3211e661dfb41f0363f6bf6bcabfc521fa code review, verified new tests fail on master but pass with this patch, both with and without valgrind.

valgrind unit test with this patch rebased on master

$ valgrind src/test/test_bitcoin -t key_tests -l test_suite
==27199== Memcheck, a memory error detector
==27199== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==27199== Using Valgrind-3.16.0.GIT and LibVEX; rerun with -h for copyright info
==27199== Command: src/test/test_bitcoin -t key_tests -l test_suite
==27199== 
Running 4 test cases...
Entering test module "Bitcoin Core Test Suite"
test/key_tests.cpp(32): Entering test suite "key_tests"
test/key_tests.cpp(34): Entering test case "key_test1"
test/key_tests.cpp(34): Leaving test case "key_test1"; testing time: 7488280us
test/key_tests.cpp(156): Entering test case "key_signature_tests"
test/key_tests.cpp(156): Leaving test case "key_signature_tests"; testing time: 1860096us
test/key_tests.cpp(192): Entering test case "key_key_negation"
test/key_tests.cpp(192): Leaving test case "key_key_negation"; testing time: 314970us
test/key_tests.cpp(251): Entering test case "pubkey_unserialize"
test/key_tests.cpp(251): Leaving test case "pubkey_unserialize"; testing time: 297818us
test/key_tests.cpp(32): Leaving test suite "key_tests"; testing time: 9969383us
Leaving test module "Bitcoin Core Test Suite"; testing time: 9975082us

*** No errors detected
==27199== 
==27199== HEAP SUMMARY:
==27199==     in use at exit: 505 bytes in 5 blocks
==27199==   total heap usage: 16,462 allocs, 16,457 frees, 41,640,410 bytes allocated
==27199== 
==27199== LEAK SUMMARY:
==27199==    definitely lost: 0 bytes in 0 blocks
==27199==    indirectly lost: 0 bytes in 0 blocks
==27199==      possibly lost: 0 bytes in 0 blocks
==27199==    still reachable: 505 bytes in 5 blocks
==27199==         suppressed: 0 bytes in 0 blocks
==27199== Rerun with --leak-check=full to see details of leaked memory
==27199== 
==27199== For lists of detected and suppressed errors, rerun with: -s
==27199== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

unit test failures without this patch

src/test/test_bitcoin -t key_tests -l test_suite
Running 4 test cases...
Entering test module "Bitcoin Core Test Suite"
test/key_tests.cpp(32): Entering test suite "key_tests"
test/key_tests.cpp(34): Entering test case "key_test1"
test/key_tests.cpp(34): Leaving test case "key_test1"; testing time: 72983us
test/key_tests.cpp(156): Entering test case "key_signature_tests"
test/key_tests.cpp(156): Leaving test case "key_signature_tests"; testing time: 46559us
test/key_tests.cpp(192): Entering test case "key_key_negation"
test/key_tests.cpp(192): Leaving test case "key_key_negation"; testing time: 10107us
test/key_tests.cpp(251): Entering test case "pubkey_unserialize"
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(251): Leaving test case "pubkey_unserialize"; testing time: 7067us
test/key_tests.cpp(32): Leaving test suite "key_tests"; testing time: 136897us
Leaving test module "Bitcoin Core Test Suite"; testing time: 137038us

*** 6 failures are detected in the test module "Bitcoin Core Test Suite"

valgrind unit test failures without this patch

$ valgrind src/test/test_bitcoin -t key_tests -l test_suite
==25033== Memcheck, a memory error detector
==25033== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==25033== Using Valgrind-3.16.0.GIT and LibVEX; rerun with -h for copyright info
==25033== Command: src/test/test_bitcoin -t key_tests -l test_suite
==25033== 
Running 4 test cases...
Entering test module "Bitcoin Core Test Suite"
test/key_tests.cpp(32): Entering test suite "key_tests"
test/key_tests.cpp(34): Entering test case "key_test1"
test/key_tests.cpp(34): Leaving test case "key_test1"; testing time: 6917131us
test/key_tests.cpp(156): Entering test case "key_signature_tests"
test/key_tests.cpp(156): Leaving test case "key_signature_tests"; testing time: 1514039us
test/key_tests.cpp(192): Entering test case "key_key_negation"
test/key_tests.cpp(192): Leaving test case "key_key_negation"; testing time: 300821us
test/key_tests.cpp(251): Entering test case "pubkey_unserialize"
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
==25033== Conditional jump or move depends on uninitialised value(s)
==25033==    at 0x483BDFF: bcmp (vg_replace_strmem.c:1111)
==25033==    by 0x285948: operator==(CPubKey const&, CPubKey const&) (pubkey.h:119)
==25033==    by 0x439B11: key_tests::CmpSerializationPubkey(CPubKey) (key_tests.cpp:248)
==25033==    by 0x43937C: key_tests::pubkey_unserialize::test_method() (key_tests.cpp:256)
==25033==    by 0x438D45: key_tests::pubkey_unserialize_invoker() (key_tests.cpp:251)
==25033==    by 0x20A369: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:118)
==25033==    by 0x4936DBD: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49369A4: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936A70: boost::execution_monitor::execute(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936B3C: boost::execution_monitor::vexecute(boost::function<void ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x495E538: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned int) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49392AA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033== 
==25033== Conditional jump or move depends on uninitialised value(s)
==25033==    at 0x483BE0E: bcmp (vg_replace_strmem.c:1111)
==25033==    by 0x285948: operator==(CPubKey const&, CPubKey const&) (pubkey.h:119)
==25033==    by 0x439B11: key_tests::CmpSerializationPubkey(CPubKey) (key_tests.cpp:248)
==25033==    by 0x43937C: key_tests::pubkey_unserialize::test_method() (key_tests.cpp:256)
==25033==    by 0x438D45: key_tests::pubkey_unserialize_invoker() (key_tests.cpp:251)
==25033==    by 0x20A369: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:118)
==25033==    by 0x4936DBD: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49369A4: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936A70: boost::execution_monitor::execute(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936B3C: boost::execution_monitor::vexecute(boost::function<void ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x495E538: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned int) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49392AA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033== 
==25033== Conditional jump or move depends on uninitialised value(s)
==25033==    at 0x483BE37: bcmp (vg_replace_strmem.c:1111)
==25033==    by 0x285948: operator==(CPubKey const&, CPubKey const&) (pubkey.h:119)
==25033==    by 0x439B11: key_tests::CmpSerializationPubkey(CPubKey) (key_tests.cpp:248)
==25033==    by 0x43937C: key_tests::pubkey_unserialize::test_method() (key_tests.cpp:256)
==25033==    by 0x438D45: key_tests::pubkey_unserialize_invoker() (key_tests.cpp:251)
==25033==    by 0x20A369: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:118)
==25033==    by 0x4936DBD: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49369A4: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936A70: boost::execution_monitor::execute(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936B3C: boost::execution_monitor::vexecute(boost::function<void ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x495E538: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned int) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49392AA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033== 
==25033== Conditional jump or move depends on uninitialised value(s)
==25033==    at 0x494AE92: boost::test_tools::tt_detail::report_assertion(boost::test_tools::assertion_result const&, boost::unit_test::lazy_ostream const&, boost::unit_test::basic_cstring<char const>, unsigned long, boost::test_tools::tt_detail::tool_level, boost::test_tools::tt_detail::check_type, unsigned long, ...) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x439C0B: key_tests::CmpSerializationPubkey(CPubKey) (key_tests.cpp:248)
==25033==    by 0x43937C: key_tests::pubkey_unserialize::test_method() (key_tests.cpp:256)
==25033==    by 0x438D45: key_tests::pubkey_unserialize_invoker() (key_tests.cpp:251)
==25033==    by 0x20A369: boost::detail::function::void_function_invoker0<void (*)(), void>::invoke(boost::detail::function::function_buffer&) (function_template.hpp:118)
==25033==    by 0x4936DBD: boost::detail::function::function_obj_invoker0<boost::detail::forward, int>::invoke(boost::detail::function::function_buffer&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49369A4: boost::execution_monitor::catch_signals(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936A70: boost::execution_monitor::execute(boost::function<int ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x4936B3C: boost::execution_monitor::vexecute(boost::function<void ()> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x495E538: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::function<void ()> const&, unsigned int) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49392AA: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033==    by 0x49395A3: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.67.0)
==25033== 
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(255): error: in "key_tests/pubkey_unserialize": check !key.IsValid() has failed
test/key_tests.cpp(251): Leaving test case "pubkey_unserialize"; testing time: 334009us
test/key_tests.cpp(32): Leaving test suite "key_tests"; testing time: 9073960us
Leaving test module "Bitcoin Core Test Suite"; testing time: 9080205us

*** 6 failures are detected in the test module "Bitcoin Core Test Suite"
==25033== 
==25033== HEAP SUMMARY:
==25033==     in use at exit: 505 bytes in 5 blocks
==25033==   total heap usage: 16,485 allocs, 16,480 frees, 41,641,596 bytes allocated
==25033== 
==25033== LEAK SUMMARY:
==25033==    definitely lost: 0 bytes in 0 blocks
==25033==    indirectly lost: 0 bytes in 0 blocks
==25033==      possibly lost: 0 bytes in 0 blocks
==25033==    still reachable: 505 bytes in 5 blocks
==25033==         suppressed: 0 bytes in 0 blocks
==25033== Rerun with --leak-check=full to see details of leaked memory
==25033== 
==25033== Use --track-origins=yes to see where uninitialised values come from
==25033== For lists of detected and suppressed errors, rerun with: -s
==25033== ERROR SUMMARY: 6 errors from 4 contexts (suppressed: 0 from 0)

Thanks for adding the tests.

maflcko · 2020-06-15T23:47:27Z

To determine which label to apply I patched as follows and checked which files won't compile:

diff --git a/src/pubkey.h b/src/pubkey.h
index 4c28af4a4d..35ccfe304d 100644
--- a/src/pubkey.h
+++ b/src/pubkey.h
@@ -137,7 +137,7 @@ public:
         s.write((char*)vch, len);
     }
     template <typename Stream>
-    void Unserialize(Stream& s)
+    void Unserializee(Stream& s)
     {
         unsigned int len = ::ReadCompactSize(s);
         if (len <= SIZE) {

The result was:

  CXX      test/test_bitcoin-key_tests.o
  CXX      wallet/libbitcoin_wallet_a-walletdb.o
...
make[2]: *** [Makefile:17241: test/test_bitcoin-key_tests.o] Error 1
...
make[2]: *** [Makefile:12299: wallet/libbitcoin_wallet_a-walletdb.o] Error 1

maflcko · 2020-06-15T23:48:03Z

So I applied the wallet label. Hope that makes sense and let me know if I should change it.

maflcko

Concept ACK. Feel free to ignore the style nits in the test.

maflcko · 2020-06-15T23:50:16Z

src/test/key_tests.cpp

nit: Either single-line or with { }

I thought about changing that but I decided not to because it's a straight copy out of the CPubKey class (a private function)
I'm fine changing it though

Yeah, no worries its just the tests.

src/test/key_tests.cpp

practicalswift · 2020-06-17T13:26:32Z

re-ACK 37ae687

jonatack · 2020-06-17T14:08:44Z

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

maflcko · 2020-06-25T12:01:29Z

src/pubkey.h

        if (len <= SIZE) {
            s.read((char*)vch, len);
+            if (len != size()) {
+                Invalidate();


Instead of enumerating what could go wrong, what do you think about having one path for a valid deserialization and let all other paths end in an invalid deserialization?

The diff would be:

diff --git a/src/pubkey.h b/src/pubkey.h index 261842b7f7..0f4787bc90 100644 --- a/src/pubkey.h +++ b/src/pubkey.h @@ -142,13 +142,14 @@ public: unsigned int len = ::ReadCompactSize(s); if (len <= SIZE) { s.read((char*)vch, len); + if (len == size()) return; } else { // invalid pubkey, skip available data char dummy; while (len--) s.read(&dummy, 1); - Invalidate(); } + Invalidate(); } //! Get the KeyID of this public key (hash of its serialization)

maflcko · 2020-06-25T12:07:12Z

ACK 37ae687

37ae687 Add tests for CPubKey serialization/unserialization (Elichai Turkel) 9b8907f Check size after Unserializing CPubKey (Elichai Turkel) Pull request description: Found by practicalswift, closes bitcoin#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 37ae687 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 37ae687 Tree-SHA512: 30173755555dfc76d6263fb6a59f41be36049ffae7b4e1b92b922d668f5e5e2331f7374d5fa10d5d59fc53020d2966156905ffcfa8b8129c1f6d0ca062174ff1

Github-Pull: bitcoin#19237 Rebased-From: 37ae687

…bKey` 9550dff fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner) Pull request description: This PR is a (quite late) follow-up to #19237 (bitcoin/bitcoin#19237 (comment)). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold. ACKs for top commit: jamesob: crACK bitcoin/bitcoin@9550dff pending CI Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55

9550dff fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner) Pull request description: This PR is a (quite late) follow-up to bitcoin#19237 (bitcoin#19237 (comment)). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold. ACKs for top commit: jamesob: crACK bitcoin@9550dff pending CI Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55

Summary: > 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`. This is a backport of [[bitcoin/bitcoin#19237 | core#19237]] Test Plan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9954

Check size after Unserializing CPubKey

9b8907f

luke-jr approved these changes Jun 11, 2020

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 12, 2020

Check size after Unserializing CPubKey

3c69994

Github-Pull: bitcoin#19237 Rebased-From: 9b8907f

jonatack reviewed Jun 15, 2020

View reviewed changes

DrahtBot added the Tests label Jun 15, 2020

maflcko added Wallet and removed Tests labels Jun 15, 2020

maflcko changed the title ~~Check size after unserializing a pubkey~~ wallet: Check size after unserializing a pubkey Jun 15, 2020

maflcko reviewed Jun 15, 2020

View reviewed changes

Add tests for CPubKey serialization/unserialization

37ae687

elichai force-pushed the 2020-06-pubkey branch from eab8ee3 to 37ae687 Compare June 17, 2020 12:00

jonatack mentioned this pull request Jun 18, 2020

p2p: Move all header verification into the network layer, extend logging #19107

Merged

practicalswift mentioned this pull request Jun 21, 2020

Serialization-deserialization roundtrip of CPubKey does not necessarily result in an equal object #17238

Closed

maflcko reviewed Jun 25, 2020

View reviewed changes

maflcko merged commit c9d1040 into bitcoin:master Jun 25, 2020

elichai deleted the 2020-06-pubkey branch July 5, 2020 08:57

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020

Add tests for CPubKey serialization/unserialization

b14efe6

Github-Pull: bitcoin#19237 Rebased-From: 37ae687

theStack mentioned this pull request Jun 17, 2021

fuzz: Assert roundtrip equality for CPubKey #22271

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallet: Check size after unserializing a pubkey #19237

wallet: Check size after unserializing a pubkey #19237

Uh oh!

Conversation

elichai commented Jun 10, 2020 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elichai commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pstratem commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 11, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

elichai commented Jun 15, 2020

Uh oh!

practicalswift commented Jun 15, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 15, 2020

Uh oh!

maflcko commented Jun 15, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

elichai Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Jun 17, 2020

Uh oh!

jonatack commented Jun 17, 2020

Uh oh!

maflcko Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

elichai commented Jun 10, 2020 •

edited by fanquake

Loading

practicalswift commented Jun 10, 2020 •

edited

Loading

elichai commented Jun 10, 2020 •

edited

Loading

practicalswift commented Jun 10, 2020 •

edited

Loading

pstratem commented Jun 11, 2020 •

edited

Loading

elichai Jun 16, 2020 •

edited

Loading