wallet: bugfix, invalid crypted key "checksum

furszy commented at 2:55 PM on November 18, 2022: member

At wallet load time, the crypted key "checksum_valid" variable is always set to false. Which, on every wallet decryption call, forces the process to re-write all the ckeys to db when it's not needed.

Note: The first commit fixes the issue, the two commits in the middle are cleanups so DuplicateMockDatabase can be used without duplicating code. And, the last one is pure test coverage for the crypted keys loading process.

Includes test coverage for the following scenarios:

"All ckeys checksums valid" test: Loads an encrypted wallet with all the crypted keys with a valid checksum and verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.

(we force a complete ckeys re-write if we find any missing crypted key checksum during the wallet loading process)
"Missing checksum in one ckey" test: Verifies that loading up a wallet with, at least one, 'ckey' with no checksum triggers a complete re-write of the crypted keys.
"Invalid ckey checksum error" test: Verifies that loading up a ckey with an invalid checksum stops the wallet loading process with a corruption error.
"Invalid ckey pubkey error" test: Verifies that loading up a ckey with an invalid pubkey stops the wallet loading process with a corruption error.

wallet: bugfix, invalid crypted key "checksum_valid" set

At wallet load time, we set the crypted key "checksum_valid" variable always to false.
Which, on every wallet decryption call, forces the process to re-write the entire ckeys to db when
it's not needed.

cc5a5e8121

DrahtBot commented at 2:55 PM on November 18, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	aureleoules, achow101
Stale ACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
#25909 (wallet: coverage for receiving txes with same id but different witness data by furszy)
#25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
#25361 (BIP324: Cipher suite by dhruv)
#25325 (Add pool based memory resource by martinus)
#24914 (wallet: Load database records in a particular order by achow101)
#23561 (BIP324: Handshake prerequisites by dhruv)
#23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Nov 18, 2022

achow101 commented at 4:31 PM on November 18, 2022: member

Good catch! Do you think it's possible to have a test for this?

bitcoin deleted a comment on Nov 19, 2022

furszy commented at 2:02 PM on November 19, 2022: member

Good catch! Do you think it's possible to have a test for this?

Yep :), added test coverage for it and more inside 4db80c90.

Covered the following scenarios

"All ckeys checksums valid" test: Loads an encrypted wallet with all the crypted keys with a valid checksum and verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.

(we force a complete ckeys re-write if we find any missing crypted key checksum during the wallet loading process)
"Missing checksum in one ckey" test: Verifies that loading up a wallet with, at least one, 'ckey' with no checksum triggers a complete re-write of the crypted keys.
"Invalid ckey checksum error" test: Verifies that loading up a ckey with an invalid checksum stops the wallet loading process with a corruption error.
"Invalid ckey pubkey error" test: Verifies that loading up a ckey with an invalid pubkey stops the wallet loading process with a corruption error.

Extra note: The two commits in the middle are cleanups so I can use the DuplicateMockDatabase function without duplicating code.

Going to update the PR description with this too.

furszy force-pushed on Nov 19, 2022

bitcoin deleted a comment on Nov 19, 2022

in src/Makefile.test_util.include:22 in cd62eddbcf outdated

  18 | @@ -19,7 +19,7 @@ TEST_UTIL_H = \
  19 |    test/util/transaction_utils.h \
  20 |    test/util/txmempool.h \
  21 |    test/util/validation.h \
  22 | -  test/util/wallet.h
  23 | +  wallet/test/util.h

achow101 commented at 7:43 PM on November 21, 2022:

In cd62eddbcfba652a238374d62f70181cb3ad9075 "refactor: unify test/util/wallet.h with wallet/test/util.h"

nit: ISTM these should be guarded by ENABLE_WALLET? They aren't used by non-wallet benchmarks or tests, so it doesn't break anything.

This would also remove the need to have #ifdef ENABLE_WALLET in those files too.

furszy commented at 8:26 PM on November 21, 2022:

hmm yeah, could also remove them from BITCOIN_TEST_SUITE as TEST_UTIL_H is already included there.

in src/wallet/test/util.cpp:16 in cd62eddbcf outdated

  10 | @@ -11,10 +11,11 @@
  11 |  #include <wallet/wallet.h>
  12 |  #include <wallet/walletdb.h>
  13 |  
  14 | -#include <boost/test/unit_test.hpp>
  15 | -
  16 |  #include <memory>
  17 |  
  18 | +const std::string ADDRESS_BCRT1_UNSPENDABLE = "bcrt1qqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq3xueyj";

achow101 commented at 7:46 PM on November 21, 2022:

In cd62eddbcfba652a238374d62f70181cb3ad9075 "refactor: unify test/util/wallet.h with wallet/test/util.h"

nit: Although this were originally in src/test/util/wallet.h, it doesn't really make sense to be in the wallet's utils since it is not a wallet specific address. Could probably go in src/test/util/script.h or src/test/util/transaction_utils.h

furszy commented at 8:27 PM on November 21, 2022:

Could also just move it to wallet_balance.cpp. It's not used anywhere else.

achow101 commented at 7:53 PM on November 21, 2022: member

ACK 4db80c90b02b70a71a3ac1d614d779d222561525

furszy force-pushed on Nov 21, 2022

refactor: unify test/util/wallet.h with wallet/test/util.h

files share the same purpose, and we shouldn't have wallet code
inside the test directory.

This later is needed to use wallet util functions in the bench
and test binaries without be forced to duplicate them.

ee7a984f85

refactor: move DuplicateMockDatabase to wallet/test/util.h 373c99633e

test: load wallet, coverage for crypted keys

Adds test coverage for the wallet's crypted key loading from db process.
The following scenarios are covered:

1) "All ckeys checksums valid" test:
   Loads an encrypted wallet with all the crypted keys with a valid checksum and
   verifies that 'CWallet::Unlock' doesn't force an entire crypted keys re-write.

   (we force a complete ckeys re-write if we find any missing crypted key checksum
    during the wallet loading process)

2) "Missing checksum in one ckey" test:
   Verifies that loading up a wallet with, at least one, 'ckey' with no checksum
   triggers a complete re-write of the crypted keys.

3) "Invalid ckey checksum error" test:
   Verifies that loading up a ckey with an invalid checksum stops the wallet loading
   process with a corruption error.

4) "Invalid ckey pubkey error" test:
   Verifies that loading up a ckey with an invalid pubkey stops the wallet loading
   process with a corruption error.

13d9760829

furszy force-pushed on Nov 21, 2022

furszy commented at 8:34 PM on November 21, 2022: member

Updated per feedback, thanks achow101.

Small Diff:

Added ENABLE_WALLET guard for the wallet/test/util.h and util.cpp files at the test_util makefile level.
Moved ADDRESS_BCRT1_UNSPENDABLE from wallet/test/util.h to wallet_balance.cpp (it's only used there).

in src/wallet/test/walletload_tests.cpp:54 in 13d9760829

  50 | @@ -50,5 +51,139 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_unknown_descriptor, TestingSetup)
  51 |      }
  52 |  }
  53 |  
  54 | +bool HasAnyRecordOfType(WalletDatabase& db, const std::string& key)

aureleoules commented at 2:48 PM on November 22, 2022:

13d97608297bd56ed033d0e754d2e50447b02af0 nit:

bool HasAnyRecordOfType(WalletDatabase& db, std::string_view key)

furszy commented at 9:03 PM on November 23, 2022:

thanks, will do if have to retouch

in src/wallet/test/walletload_tests.cpp:85 in 13d9760829

  80 | +        return db;
  81 | +    };
  82 | +
  83 | +    {   // Context setup.
  84 | +        // Create and encrypt legacy wallet
  85 | +        std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", m_args, CreateMockWalletDatabase()));

aureleoules commented at 2:55 PM on November 22, 2022:

13d97608297bd56ed033d0e754d2e50447b02af0 nit: (same for other instances)

        auto wallet{std::make_shared<CWallet>(m_node.chain.get(), "", m_args, CreateMockWalletDatabase())};

furszy commented at 8:59 PM on November 23, 2022:

k thanks, will do if have to retouch.

aureleoules approved

aureleoules commented at 3:19 PM on November 22, 2022: member

ACK 13d97608297bd56ed033d0e754d2e50447b02af0

Cherry-picked the last three commits on master and verified the tests do not pass. I also verified that the tests correctly test the behavior.

achow101 commented at 8:52 PM on November 23, 2022: member

ACK 13d97608297bd56ed033d0e754d2e50447b02af0

fanquake requested review from Sjors on Nov 23, 2022

luke-jr approved

luke-jr commented at 1:25 AM on November 29, 2022: member

utACK cc5a5e81217506ec6f9fff34056290f8f40a7396 by itself (did not review refactors or new test)

achow101 merged this on Nov 29, 2022

achow101 closed this on Nov 29, 2022

sidhujag referenced this in commit f2e4361419 on Dec 1, 2022

furszy deleted the branch on May 27, 2023

UdjinM6 referenced this in commit 17b8dd6c61 on Dec 10, 2023

UdjinM6 referenced this in commit d7bae33f57 on Dec 10, 2023

PastaPastaPasta referenced this in commit 985da9ab93 on Dec 11, 2023

ogabrielides referenced this in commit 8364e4b001 on Dec 22, 2023

PastaPastaPasta referenced this in commit f0feb36ce3 on Dec 24, 2023

bitcoin locked this on May 26, 2024

wallet: bugfix, invalid crypted key "checksum_valid" set #26532

Reviews

Conflicts

Covered the following scenarios