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

practicalswift · 2018-07-19T08:33:16Z

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:

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

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

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

maflcko · 2018-07-19T11:10:42Z

Would be nice if this was covered by a test case, no?

practicalswift · 2018-07-19T13:10:01Z

@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:

wallet/test/psbt_wallet_tests.cpp(…): error: in "psbt_wallet_tests/parse_hd_keypath": 
  check !ParseHDKeypath("///////////////////////////", keypath) has failed
  check !ParseHDKeypath("//////////////////////////'/", keypath) has failed
  check !ParseHDKeypath("1///////////////////////////", keypath) has failed
  check !ParseHDKeypath("1/'//////////////////////////", keypath) has failed
  check !ParseHDKeypath("4294967296", keypath) has failed
  check !ParseHDKeypath("m/1/1/111111111111111111111111111111111111111111111111111111111111111111111111111111111111", keypath) has failed
  check !ParseHDKeypath("m/1//", keypath) has failed
  check !ParseHDKeypath("m/0/4294967296", keypath) has failed
  check !ParseHDKeypath("m/4294967296", keypath) has failed

Please re-review :-)

Empact · 2018-07-19T14:52:07Z

src/wallet/rpcwallet.h

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


Rather than declare it in the header, you could declare it extern in the test file.

… ParseHDKeypath(...).

achow101 · 2018-07-19T21:07:10Z

utACK 7223263

maflcko · 2018-07-19T21:19:47Z

utACK 7223263

maflcko · 2018-07-19T21:21:47Z

utACK 27ee53c

sipa · 2018-07-19T21:31:46Z

utACK 27ee53c

…id using an uninitialized variable in path calculation. 27ee53c wallet: Add error handling. Check return value of ParseUInt32(...) in ParseHDKeypath(...). (practicalswift) 7223263 wallet: Add tests for ParseHDKeypath(...) (practicalswift) Pull request description: 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`: ``` ParseHDKeypath("m/0/4294967296", keypath); ``` `4294967296` is `1` + `0xFFFFFFFF` (`uint32_t` max: `4294967295`). Introduced in a4b06fb which was merged into `master` 14 hours ago as part of #13557 ("BIP 174 PSBT Serializations and RPCs"). Tree-SHA512: e5ff423f67c18d82c1231bde6343587a453e793c32004d93dc9b61be6d9372b57a6b2c9978d9eb1000d6cc82fd180f2486013f928dca737fb92daad22c16e467

practicalswift · 2018-08-02T16:27:43Z

@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 :-)

… functions returning bool 9cc0230 Add NODISCARD to all {Decode,Parse}[...](...) functions returning bool. Sort includes. (practicalswift) 579497e tests: Explicitly ignore the return value of DecodeBase58(...) (practicalswift) 145fe95 tests: Check return value of ParseParameters(...) (practicalswift) 7c5bc2a miner: Default to DEFAULT_BLOCK_MIN_TX_FEE if unable to parse -blockmintxfee (practicalswift) Pull request description: Changes in this PR: * ~~Add linter to make sure the return value of `Parse[...](...)` is checked~~ * Add `__attribute__((warn_unused_result))` to all `{Decode,Parse}[...](...)` functions returning `bool` * Fix violations Context: * #13712: `wallet: Fix non-determinism in ParseHDKeypath(...). Avoid using an uninitialized variable in path calculation.` would have been prevented by this Tree-SHA512: 41a97899f2d5a26584235fa02b1ebfb4faacd81ea97e927022955a658fa7e15d07a1443b4b7635151a43259a1adf8f2f4de3c1c75d7b5f09f0d5496463a1dae6

practicalswift changed the title ~~wallet: Add error handling. Check return value of ParseUInt32(...) in…~~ wallet: Add error handling in ParseHDKeypath(...). Check return value of ParseUInt32(...) in… Jul 19, 2018

practicalswift changed the title ~~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. Jul 19, 2018

practicalswift changed the title ~~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. Jul 19, 2018

fanquake added the Wallet label Jul 19, 2018

practicalswift force-pushed the check-ParseUInt32-return-value branch from 7893b01 to f239417 Compare July 19, 2018 12:56

practicalswift force-pushed the check-ParseUInt32-return-value branch from f239417 to 44a5042 Compare July 19, 2018 13:38

Empact reviewed Jul 19, 2018

View reviewed changes

practicalswift force-pushed the check-ParseUInt32-return-value branch from 44a5042 to 7e788ff Compare July 19, 2018 19:07

laanwj added this to the 0.17.0 milestone Jul 19, 2018

practicalswift added 2 commits July 19, 2018 21:13

wallet: Add tests for ParseHDKeypath(...)

7223263

wallet: Add error handling. Check return value of ParseUInt32(...) in…

27ee53c

… ParseHDKeypath(...).

practicalswift force-pushed the check-ParseUInt32-return-value branch from 7e788ff to 27ee53c Compare July 19, 2018 19:13

maflcko merged commit 27ee53c into bitcoin:master Jul 19, 2018

Bushstar mentioned this pull request Jul 24, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#357

Merged

practicalswift mentioned this pull request Jul 31, 2018

util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815

Merged

practicalswift mentioned this pull request Mar 10, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

practicalswift mentioned this pull request Oct 1, 2020

signet: Fix uninitialized read in validation #20035

Merged

practicalswift mentioned this pull request Nov 11, 2020

ci: Remove redundant valgrind fuzz task #20368

Merged

practicalswift deleted the check-ParseUInt32-return-value branch April 10, 2021 19:35

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

practicalswift commented Jul 19, 2018 •

edited

Loading

Uh oh!

maflcko commented Jul 19, 2018

Uh oh!

practicalswift commented Jul 19, 2018 •

edited

Loading

Uh oh!

Empact Jul 19, 2018

Uh oh!

achow101 commented Jul 19, 2018

Uh oh!

maflcko commented Jul 19, 2018

Uh oh!

maflcko commented Jul 19, 2018

Uh oh!

sipa commented Jul 19, 2018

Uh oh!

practicalswift commented Aug 2, 2018 •

edited

Loading

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

practicalswift commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 19, 2018

Uh oh!

practicalswift commented Jul 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact Jul 19, 2018

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 19, 2018

Uh oh!

maflcko commented Jul 19, 2018

Uh oh!

maflcko commented Jul 19, 2018

Uh oh!

sipa commented Jul 19, 2018

Uh oh!

practicalswift commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Jul 19, 2018 •

edited

Loading

practicalswift commented Jul 19, 2018 •

edited

Loading

practicalswift commented Aug 2, 2018 •

edited

Loading