wallet: Fix non-determinism in ParseHDKeypath(…). Avoid using an uninitialized variable in path calculation. #13712

1. 
   practicalswift commented at 8:33 am on July 19, 2018: contributor

   Add error handling. Check return value of ParseUInt32(...) in ParseHDKeypath(...).

   ParseUInt32(...) returns false if the entire string could not be parsed or when an overflow or underflow occurred. In such case the uninitialized variable number would be used in the calculation of path (prior to this commit).

   An example key path triggering this is m/0/4294967296:

   0  ParseHDKeypath("m/0/4294967296", keypath);
   

   4294967296 is 1 + 0xFFFFFFFF (uint32_t max: 4294967295).

   Introduced in a4b06fb42eb0ad94e562ca839391b57e69285136 which was merged into master 14 hours ago as part of #13557 (“BIP 174 PSBT Serializations and RPCs”).

2. practicalswift renamed this:
   wallet: Add error handling. Check return value of ParseUInt32(...) in…
   wallet: Add error handling in ParseHDKeypath(...). Check return value of ParseUInt32(...) in…
   on Jul 19, 2018
3. practicalswift renamed this:
   wallet: Add error handling in ParseHDKeypath(...). Check return value of ParseUInt32(...) in…
   wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized value in path calculation.
   on Jul 19, 2018
4. practicalswift renamed this:
   wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized value in path calculation.
   wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation.
   on Jul 19, 2018
5. fanquake added the label Wallet on Jul 19, 2018
6. 
   MarcoFalke commented at 11:10 am on July 19, 2018: member

   Would be nice if this was covered by a test case, no?
7. practicalswift force-pushed on Jul 19, 2018
8. 
   practicalswift commented at 1:10 pm on July 19, 2018: contributor

   @MarcoFalke Thanks for the quick review. I’ve now added some tests.

   This is the subset of the added tests that failed before the fix commit and passes after the fix commit:

   0wallet/test/psbt_wallet_tests.cpp(…): error: in "psbt_wallet_tests/parse_hd_keypath": 
   1  check !ParseHDKeypath("///////////////////////////", keypath) has failed
   2  check !ParseHDKeypath("//////////////////////////'/", keypath) has failed
   3  check !ParseHDKeypath("1///////////////////////////", keypath) has failed
   4  check !ParseHDKeypath("1/'//////////////////////////", keypath) has failed
   5  check !ParseHDKeypath("4294967296", keypath) has failed
   6  check !ParseHDKeypath("m/1/1/111111111111111111111111111111111111111111111111111111111111111111111111111111111111", keypath) has failed
   7  check !ParseHDKeypath("m/1//", keypath) has failed
   8  check !ParseHDKeypath("m/0/4294967296", keypath) has failed
   9  check !ParseHDKeypath("m/4294967296", keypath) has failed
   

   Please re-review :-)

9. practicalswift force-pushed on Jul 19, 2018
10. in src/wallet/rpcwallet.h:34 in 44a50424b5 outdated

    30@@ -31,4 +31,5 @@ bool EnsureWalletIsAvailable(CWallet *, bool avoidException);
    31 UniValue getaddressinfo(const JSONRPCRequest& request);
    32 UniValue signrawtransactionwithwallet(const JSONRPCRequest& request);
    33 bool FillPSBT(const CWallet* pwallet, PartiallySignedTransaction& psbtx, const CTransaction* txConst, int sighash_type = 1, bool sign = true, bool bip32derivs = false);
    34+bool ParseHDKeypath(std::string keypath_str, std::vector<uint32_t>& keypath);
    

    ---

    ---

    

    Empact commented at 2:52 pm on July 19, 2018:

    Rather than declare it in the header, you could declare it extern in the test file.
11. practicalswift force-pushed on Jul 19, 2018
12. laanwj added this to the milestone 0.17.0 on Jul 19, 2018
13. wallet: Add tests for ParseHDKeypath(...) 7223263899
14. wallet: Add error handling. Check return value of ParseUInt32(...) in ParseHDKeypath(...). 27ee53c1ae
15. practicalswift force-pushed on Jul 19, 2018
16. 
    achow101 commented at 9:07 pm on July 19, 2018: member

    utACK 722326389989757c58879feb74cc2480bd58bdfc
17. 
    MarcoFalke commented at 9:19 pm on July 19, 2018: member

    utACK 7223263
18. 
    MarcoFalke commented at 9:21 pm on July 19, 2018: member

    utACK 27ee53c1aedae4f27458537c61804b6fef34ce3c
19. 
    sipa commented at 9:31 pm on July 19, 2018: member

    utACK 27ee53c1aedae4f27458537c61804b6fef34ce3c
20. MarcoFalke merged this on Jul 19, 2018
21. MarcoFalke closed this on Jul 19, 2018

    ---

22. MarcoFalke referenced this in commit aba2e666d7 on Jul 19, 2018
23. 
    practicalswift commented at 4:27 pm on August 2, 2018: contributor

    @achow101 @MarcoFalke @sipa As reviewers of this bug fix you might be interested in reviewing PR #13815 which adds annotations (C++17-style [[nodiscard]] or __attribute__((warn_unused_result)) depending on availability) which could help us guard against unintentionally ignored return values going forward :-)
24. MarcoFalke referenced this in commit 384967f311 on Nov 15, 2018
25. practicalswift deleted the branch on Apr 10, 2021
26. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift MarcoFalke Empact achow101 sipa

Labels
Wallet

Milestone
0.17.0