Don't return a CExtPubKey filled with random data when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...) #12789

1. 
   practicalswift commented at 10:15 AM on March 26, 2018: contributor

   Don't return a CExtPubKey filled with random data when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...).

   Before this patch:

   $ cat > test.cpp
   …
   int main(void) {
     CExtPubKey key = DecodeExtPubKey("foo");
     std::cout << key.nChild << "\n";
   }
   ^D
   $ g++ -o test test.cpp
   $ ./test
   109452364
   $ ./test
   -1789494196
   $ ./test
   -1568076724
   $ ./test
   1483189324
   $ ./test
   -1168606132
   

   Introduced in ebfe217b15d21656a173e5c102f826d17c6c8be4.

2. Don't return a CExtPubKey filled with junk when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...) d677161361
3. fanquake added the label Refactoring on Mar 26, 2018
4. fanquake requested review from sipa on Mar 26, 2018
5. practicalswift renamed this:
   Don't return a CExtPubKey filled with junk when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...)
   Don't return a CExtPubKey filled with random data when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...)
   on Mar 26, 2018
6. 
   jonasschnelli commented at 11:44 AM on March 26, 2018: contributor

   utACK d6771613617e24d964378056919fe4ad6ab049c8

7. 
   MarcoFalke commented at 12:48 PM on March 26, 2018: member

   Why?

   The underlying CPubKey is invalid, so nothing bad should happen. (And it seems your fix woulnd't prevent anything bad, if it was happening, either.)

8. 
   practicalswift commented at 1:15 PM on March 26, 2018: contributor

   @MarcoFalke Perhaps I'm misunderstanding you but wouldn't it be better if DecodeExt{Pub,}Key was deterministic (in the sense that a given input always produces the same output)? :-)

9. 
   MarcoFalke commented at 2:10 PM on March 26, 2018: member

   It is a bug elsewhere if it was possible to get non-deterministic results from that.

10. 
    practicalswift commented at 2:16 PM on March 26, 2018: contributor

    @MarcoFalke You mean that the caller guarantees that it is providing input that passes DecodeBase58Check(…)? Then we should document this obligation for the caller and/or assert(false); in DecodeExt{Pub,}Key if the DecodeBase58Check(…) fails, shouldn't we? Silently returning a CExtPubKey filled with random garbage in case of invalid input feels really icky.

11. 
    practicalswift commented at 3:18 PM on March 26, 2018: contributor

    Before this patch:

    $ cat > test.cpp
    …
    int main(void) {
      CExtPubKey key = DecodeExtPubKey("foo");
      std::cout << key.nChild << "\n";
    }
    ^D
    $ g++ -o test test.cpp
    $ ./test
    109452364
    $ ./test
    -1789494196
    $ ./test
    -1568076724
    $ ./test
    1483189324
    $ ./test
    -1168606132
    

    Are we really happy with that? :-)

12. 
    MarcoFalke commented at 4:13 PM on March 26, 2018: member

    You should check if key is valid before using it, I guess. Your patch doesn't help solve that bug, only makes it always fail in the same way.

13. 
    practicalswift commented at 7:44 PM on March 26, 2018: contributor

    @MarcoFalke Please help me out here :-) What do you suggest as the proper way to fix this assuming we agree that we don't want to silently return a CExtPubKey filled with random data when DecodeExt{Pub,}Key is given input not passing DecodeBase58Check(...)? :-)

14. 
    sipa commented at 7:47 PM on March 26, 2018: member

    I think the contents of CPubKey/CKey/... for which IsValid is false just has undefined contents. Anything accessing it without checking whether it's valid is violating the contract.

15. 
    practicalswift commented at 8:50 PM on March 26, 2018: contributor

    @MarcoFalke @sipa If this is expected behaviour I should probably document that somewhere. What would be the appropriate place to put it? :-)

16. 
    practicalswift commented at 2:06 PM on March 29, 2018: contributor

    Seems to be consensus NACK on this one. Closing! :-)

17. practicalswift closed this on Mar 29, 2018

    ---

18. practicalswift deleted the branch on Apr 10, 2021
19. DrahtBot locked this on Aug 16, 2022

Contributors

practicalswiftjonasschnelliMarcoFalkesipa

Review Requested
sipa