wallettool: Check that the dumpfile checksum is the correct size #23834

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:dump-checksum-size changing 2 files +10 −1

achow101 commented at 4:05 PM on December 21, 2021: member

After parsing the checksum, make sure that it is the size that we expect it to be.

This issue was reported by Pedro Baptista.
achow101 force-pushed on Dec 21, 2021
DrahtBot added the label Wallet on Dec 21, 2021
w0xlt approved
w0xlt commented at 10:44 AM on December 23, 2021: contributor

crACK dafa16c

in src/wallet/dump.cpp:218 in dafa16c75b outdated

 213 | @@ -214,6 +214,11 @@ bool CreateFromDump(const std::string& name, const fs::path& wallet_path, biling
 214 |  
 215 |              if (key == "checksum") {
 216 |                  std::vector<unsigned char> parsed_checksum = ParseHex(value);
 217 | +                if (parsed_checksum.size() != checksum.size()) {
 218 | +                    error = strprintf(_("Error: Checksum is not the correct size"));

laanwj commented at 1:05 PM on December 26, 2021:

Please make this an untranslated message, it's very unlikely to appear and extremely technical.

achow101 commented at 5:34 PM on December 26, 2021:

Done

wallettool: Check that the dumpfile checksum is the correct size

After parsing the checksum, make sure that it is the size that we expect
it to be.

ac617cc141

achow101 force-pushed on Dec 26, 2021

in test/functional/tool_wallet.py:395 in ac617cc141

 389 | @@ -390,7 +390,11 @@ def test_dump_createfromdump(self):
 390 |          bad_sum_wallet_dump = os.path.join(self.nodes[0].datadir, "wallet-bad_sum3.dump")
 391 |          dump_data["checksum"] = "2" * 10
 392 |          self.write_dump(dump_data, bad_sum_wallet_dump)
 393 | -        self.assert_raises_tool_error('Error: Dumpfile checksum does not match. Computed {}, expected {}{}'.format(checksum, "2" * 10, "0" * 54), '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
 394 | +        self.assert_raises_tool_error('Error: Checksum is not the correct size', '-wallet=badload', '-dumpfile={}'.format(bad_sum_wallet_dump), 'createfromdump')
 395 | +        assert not os.path.isdir(os.path.join(self.nodes[0].datadir, "regtest/wallets", "badload"))
 396 | +        dump_data["checksum"] = "3" * 66

hebasto commented at 6:16 PM on December 26, 2021:

My naive assumption was that the following change

        dump_data["checksum"] = "3" * 65

will keep test passing. But it is wrong.

Maybe verify the size of value before calling ParseHex?

laanwj commented at 6:04 PM on January 5, 2022:

A hexadecimal string with an odd number of characters should cause a parse error anyway. Thinking of it, it's pretty bad that we don't have parse error feedback at all in ParseHex. Even if you'd add a non-hex character in between it's just break off the parsing early instead of return, say, nullopt.

(to be clear: not an issue to be fixed in this PR, but just to illustrate that checking the length before is not sufficient)

laanwj commented at 6:12 PM on January 5, 2022: member

Code review ACK ac617cc141fe05bea0dc5e8f9df3da43c0945842
laanwj merged this on Jan 5, 2022
laanwj closed this on Jan 5, 2022
sidhujag referenced this in commit 6867dcd153 on Jan 6, 2022
DrahtBot locked this on Jan 5, 2023

Contributors

Labels