wallet: refactor: various master key encryption cleanups

theStack commented at 3:54 am on December 1, 2024: contributor

This PR contains various cleanups around the wallet’s master key encryption logic. The default/minimum key derivation rounds magic number of 25000 is hoisted into a constant (member of CMasterKey) and two new functions EncryptMasterKey/DecryptMasterKey are introduced in order to deduplicate code for the derivation round determination and master key en/decryption. Also, mentions of the never-implemented derivation method scrypt are removed from the wallet crypter header and both plain and encrypted master key instances are renamed to adapt to moderning coding style (hopefully improving readability).

DrahtBot commented at 3:54 am on December 1, 2024: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31398.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg
Concept ACK	furszy

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:

#31774 (crypto: Use secure_allocator for AES256_ctx by davidgumberg)

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 Dec 1, 2024

furszy commented at 6:04 pm on December 27, 2024: member

Concept ACK, nice code dedup.

in src/wallet/crypter.h:48 in 22e9869524 outdated

41@@ -42,16 +42,19 @@ class CMasterKey
42     //! Use this for more parameters to key derivation (currently unused)
43     std::vector<unsigned char> vchOtherDerivationParameters;
44 
45+    //! Default/minimum number of key derivation rounds
46+    // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
47+    // ie slightly lower than the lowest hardware we need bother supporting
48+    static constexpr int DEFAULT_DERIVE_ITERATIONS = 25000;

davidgumberg commented at 8:20 am on January 25, 2025:

nit: this could be used in src/wallet/test/wallet_crypto_tests.cpp and src/wallet/test/fuzz/crypter.cpp also, although maybe the fuzzing range should actually go higher than this

davidgumberg commented at 8:49 am on January 25, 2025:

0    static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000;

theStack commented at 5:20 pm on January 27, 2025:

Good catch, done. Kept the fuzzing range as-is though since this PR is not meant to change any behaviour.

theStack commented at 5:21 pm on January 27, 2025:

Makes sense to match the type of the constant with the one of the variable that it gets assigned to (nDeriveIterations), done.

in src/wallet/crypter.h:37 in a6af5b45a1 outdated

36@@ -37,11 +37,9 @@ class CMasterKey
37     std::vector<unsigned char> vchCryptedKey;

davidgumberg commented at 8:33 am on January 25, 2025:

nit: could also remove the mention of scrypt above on L26

theStack commented at 5:20 pm on January 27, 2025:

Done.

in src/wallet/wallet.cpp:592 in de9b54780b outdated

581+    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
582+    master_key.nDeriveIterations = static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start));
583+
584+    start = SteadyClock::now();
585+    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
586+    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;

davidgumberg commented at 9:03 am on January 25, 2025:

just a remark feel-free-to-ignore: I tried to think of a way to deduplicate this adjustment logic using a lambda here, but it turned out worse :)

 0    CCrypter crypter;
 1
 2    auto adjust_iterations = [&](unsigned int current_iterations) -> unsigned int {
 3        auto start{SteadyClock::now()};
 4
 5        if (!crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, current_iterations, master_key.nDerivationMethod)) {
 6            return false;
 7        }
 8
 9        return current_iterations * target / (SteadyClock::now() - start);
10    };
11
12    master_key.nDeriveIterations = adjust_iterations(master_key.nDeriveIterations);
13    master_key.nDeriveIterations = (master_key.nDeriveIterations + adjust_iterations(master_key.nDeriveIterations)) / 2;

in src/wallet/wallet.cpp:587 in de9b54780b outdated

576+{
577+    constexpr MillisecondsDouble target{100};
578+    auto start{SteadyClock::now()};
579+    CCrypter crypter;
580+
581+    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);

davidgumberg commented at 9:08 am on January 25, 2025:

Should the return value of the ‘benchmark’ runs of SetKeyFromPassphrase be checked and an early return if false, or does it not matter until the real SetKeyFromPassphrase call below?

theStack commented at 5:22 pm on January 27, 2025:

Good question. Left it as-is for now, since this is a refactoring PR, but maybe something to consider for a follow-up PR.

in src/wallet/wallet.cpp:596 in de9b54780b outdated

585+    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
586+    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
587+
588+    if (master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) {
589+        master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS;
590+    }

davidgumberg commented at 9:12 am on January 25, 2025:

nit, maybe:

0master_key.nDeriveIterations = std::max(CMasterKey::DEFAULT_DERIVE_ITERATIONS, master_key.nDeriveIterations);

theStack commented at 5:27 pm on January 27, 2025:

Left this as-is for ease of review. Even though the suggestion would deduplicate the use of the constant, I find the std::max construct slightly harder to grasp than the two lines now (but maybe that’s me, happy to change).

davidgumberg commented at 5:54 pm on January 27, 2025:

Agreed, it is confusing to use std::max when we are really interested in setting a minimum value.

davidgumberg commented at 9:12 am on January 25, 2025: contributor

Concept ACK, left a few non-blocking nits

wallet: doc: remove mentions of unavailable scrypt derivation method

These comments are there since wallet encryption was first introduced
(see commit 4e87d341f75f13bbd7d108c31c03886fbc4df56f, PR #352), but scrypt
was actually never implemented as a derivation method.

62c209f50d

wallet: refactor: introduce `CMasterKey::DEFAULT_DERIVE_ITERATIONS` constant

This gets rid of the magic number used in both the `CMasterKey` ctor
and the wallet encryption / passphrase change methods.

a6d9b415aa

wallet: refactor: dedup master key encryption / derivation rounds setting 846545947c

wallet: refactor: dedup master key decryption 5a92077fd5

scripted-diff: wallet: rename plain and encrypted master key variables

-BEGIN VERIFY SCRIPT-
sed -i s/_vMasterKey/plain_master_key/g ./src/wallet/wallet.cpp
sed -i s/kMasterKey/master_key/g ./src/wallet/wallet.cpp
sed -i "s/const MasterKeyMap::value_type& pMasterKey/const auto\& \[_, master_key\]/g" ./src/wallet/wallet.cpp
sed -i s/pMasterKey\.second/master_key/g ./src/wallet/wallet.cpp
sed -i "s/MasterKeyMap::value_type& pMasterKey/auto\& \[master_key_id, master_key\]/g" ./src/wallet/wallet.cpp
sed -i s/pMasterKey\.first/master_key_id/g ./src/wallet/wallet.cpp
-END VERIFY SCRIPT-

a8333fc9ff

theStack force-pushed on Jan 27, 2025

theStack commented at 5:27 pm on January 27, 2025: contributor

@davidgumberg: Thanks for the detailed review! I took most of your suggestions (see above) and rebased on master.

davidgumberg commented at 6:31 pm on January 27, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/31398/commits/a8333fc9ff9adaa97a1f9024f5783cc071777150

0$ git range-diff dbc8ba1..bda3a48  0a931a9..a8333fc | sed 's/^ \{4\}//' # first four spaces removed to improve github diff highlighting

 01:  a6af5b45a1 ! 1:  62c209f50d wallet: doc: remove mentions of unavailable scrypt derivation method
 1@@ Commit message
 2     was actually never implemented as a derivation method.
 3 
 4  ## src/wallet/crypter.h ##
 5+@@ src/wallet/crypter.h: const unsigned int WALLET_CRYPTO_IV_SIZE = 16;
 6+  * derived using derivation method nDerivationMethod
 7+  * (0 == EVP_sha512()) and derivation iterations nDeriveIterations.
 8+  * vchOtherDerivationParameters is provided for alternative algorithms
 9+- * which may require more parameters (such as scrypt).
10++ * which may require more parameters.
11+  *
12+  * Wallet Private Keys are then encrypted using AES-256-CBC
13+  * with the double-sha256 of the public key as the IV, and the
14 @@ src/wallet/crypter.h: public:
15      std::vector<unsigned char> vchCryptedKey;
16      std::vector<unsigned char> vchSalt;
172:  22e9869524 ! 2:  a6d9b415aa wallet: refactor: introduce `CMasterKey::DEFAULT_DERIVE_ITERATIONS` constant
18@@ src/wallet/crypter.h: public:
19 +    //! Default/minimum number of key derivation rounds
20 +    // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
21 +    // ie slightly lower than the lowest hardware we need bother supporting
22-+    static constexpr int DEFAULT_DERIVE_ITERATIONS = 25000;
23++    static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000;
24 +
25      SERIALIZE_METHODS(CMasterKey, obj)
26      {
27@@ src/wallet/crypter.h: public:
28          vchOtherDerivationParameters = std::vector<unsigned char>(0);
29      }
30 
31+ ## src/wallet/test/fuzz/crypter.cpp ##
32+@@ src/wallet/test/fuzz/crypter.cpp: FUZZ_TARGET(crypter, .init = initialize_crypter)
33+         // Limiting the value of rounds since it is otherwise uselessly expensive and causes a timeout when fuzzing.
34+         crypt.SetKeyFromPassphrase(/*key_data=*/secure_string,
35+                                    /*salt=*/ConsumeFixedLengthByteVector(fuzzed_data_provider, WALLET_CRYPTO_SALT_SIZE),
36+-                                   /*rounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, 25000),
37++                                   /*rounds=*/fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, CMasterKey::DEFAULT_DERIVE_ITERATIONS),
38+                                    /*derivation_method=*/derivation_method);
39+     }
40+ 
41+
42+ ## src/wallet/test/wallet_crypto_tests.cpp ##
43+@@ src/wallet/test/wallet_crypto_tests.cpp: static void TestEncrypt(const CCrypter& crypt, const std::span<const unsigned ch
44+ BOOST_AUTO_TEST_CASE(passphrase) {
45+     // These are expensive.
46+ 
47+-    TestCrypter::TestPassphrase("0000deadbeef0000"_hex_u8, "test", 25000,
48++    TestCrypter::TestPassphrase("0000deadbeef0000"_hex_u8, "test", CMasterKey::DEFAULT_DERIVE_ITERATIONS,
49+                                 "fc7aba077ad5f4c3a0988d8daa4810d0d4a0e3bcb53af662998898f33df0556a"_hex_u8,
50+                                 "cf2f2691526dd1aa220896fb8bf7c369"_hex_u8);
51+ 
52+@@ src/wallet/test/wallet_crypto_tests.cpp: BOOST_AUTO_TEST_CASE(passphrase) {
53+ BOOST_AUTO_TEST_CASE(encrypt) {
54+     constexpr std::array<uint8_t, WALLET_CRYPTO_SALT_SIZE> salt{"0000deadbeef0000"_hex_u8};
55+     CCrypter crypt;
56+-    crypt.SetKeyFromPassphrase("passphrase", salt, 25000, 0);
57++    crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0);
58+     TestCrypter::TestEncrypt(crypt, "22bcade09ac03ff6386914359cfe885cfeb5f77ff0d670f102f619687453b29d"_hex_u8);
59+ 
60+     for (int i = 0; i != 100; i++)
61+@@ src/wallet/test/wallet_crypto_tests.cpp: BOOST_AUTO_TEST_CASE(encrypt) {
62+ BOOST_AUTO_TEST_CASE(decrypt) {
63+     constexpr std::array<uint8_t, WALLET_CRYPTO_SALT_SIZE> salt{"0000deadbeef0000"_hex_u8};
64+     CCrypter crypt;
65+-    crypt.SetKeyFromPassphrase("passphrase", salt, 25000, 0);
66++    crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0);
67+ 
68+     // Some corner cases the came up while testing
69+     TestCrypter::TestDecrypt(crypt,"795643ce39d736088367822cdc50535ec6f103715e3e48f4f3b1a60a08ef59ca"_hex_u8);
70+
71  ## src/wallet/wallet.cpp ##
72 @@ src/wallet/wallet.cpp: bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
73                  crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod);
743:  de9b54780b = 3:  846545947c wallet: refactor: dedup master key encryption / derivation rounds setting
754:  ba9f582332 = 4:  5a92077fd5 wallet: refactor: dedup master key decryption
765:  bda3a48190 = 5:  a8333fc9ff scripted-diff: wallet: rename plain and encrypted master key variables

DrahtBot requested review from furszy on Jan 27, 2025

in src/wallet/crypter.h:60 in a6d9b415aa outdated

58-        // ie slightly lower than the lowest hardware we need bother supporting
59-        nDeriveIterations = 25000;
60+        nDeriveIterations = DEFAULT_DERIVE_ITERATIONS;
61         nDerivationMethod = 0;
62         vchOtherDerivationParameters = std::vector<unsigned char>(0);
63     }

davidgumberg commented at 6:36 pm on January 27, 2025:

nit: this constructor should probably be replaced with default initializers

 0-    unsigned int nDerivationMethod;
 1-    unsigned int nDeriveIterations;
 2+    unsigned int nDerivationMethod{0};
 3+    unsigned int nDeriveIterations{DEFAULT_DERIVE_ITERATIONS};
 4     //! Use this for more parameters to key derivation (currently unused)
 5-    std::vector<unsigned char> vchOtherDerivationParameters;
 6+    std::vector<unsigned char> vchOtherDerivationParameters{0};
 7
 8     SERIALIZE_METHODS(CMasterKey, obj)
 9     {
10         READWRITE(obj.vchCryptedKey, obj.vchSalt, obj.nDerivationMethod, obj.nDeriveIterations, obj.vchOtherDerivationParameters);
11     }
12-
13-    CMasterKey()
14-    {
15-        nDeriveIterations = DEFAULT_DERIVE_ITERATIONS;
16-        nDerivationMethod = 0;
17-        vchOtherDerivationParameters = std::vector<unsigned char>(0);
18-    }
19 };

(https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#c-data-structures)

wallet: refactor: various master key encryption cleanups #31398

Code Coverage & Benchmarks

Reviews

Conflicts