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).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31398.
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.
Reviewers, this pull request conflicts with the following ones:
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.
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;
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
0 static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000;
nDeriveIterations), done.
36@@ -37,11 +37,9 @@ class CMasterKey
37 std::vector<unsigned char> vchCryptedKey;
scrypt above on L26
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;
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;
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);
SetKeyFromPassphrase be checked and an early return if false, or does it not matter until the real SetKeyFromPassphrase call below?
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+ }
nit, maybe:
0master_key.nDeriveIterations = std::max(CMasterKey::DEFAULT_DERIVE_ITERATIONS, master_key.nDeriveIterations);
std::max construct slightly harder to grasp than the two lines now (but maybe that’s me, happy to change).
std::max when we are really interested in setting a minimum value.
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.
This gets rid of the magic number used in both the `CMasterKey` ctor
and the wallet encryption / passphrase change methods.
-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-
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
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 }
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)
41 unsigned int nDerivationMethod;
42 unsigned int nDeriveIterations;
43- //! Use this for more parameters to key derivation,
44- //! such as the various parameters to scrypt
45+ //! Use this for more parameters to key derivation (currently unused)
46 std::vector<unsigned char> vchOtherDerivationParameters;
The memory consumption of an empty vector is negligible (24 bytes on my system [1]) so this is not a big deal, but happy to address this on the next push.
[1] according to
0 std::vector<unsigned char> empty_vec;
1 printf("%ld\n", sizeof(empty_vec) + memusage::DynamicUsage(empty_vec));
667 }
668- WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", pMasterKey.second.nDeriveIterations);
669+ WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", master_key.nDeriveIterations);
670
671- WalletBatch(GetDatabase()).WriteMasterKey(pMasterKey.first, pMasterKey.second);
672+ WalletBatch(GetDatabase()).WriteMasterKey(master_key_id, master_key);
WriteMasterKey() fails then this will still succeed when it shouldn’t.
869 delete encrypted_batch;
870 encrypted_batch = nullptr;
871 return false;
872 }
873- encrypted_batch->WriteMasterKey(nMasterKeyMaxID, kMasterKey);
874+ encrypted_batch->WriteMasterKey(nMasterKeyMaxID, master_key);
WriteMasterKey() fails then this will still succeed when it shouldn’t.
46 std::vector<unsigned char> vchOtherDerivationParameters;
47
48+ //! Default/minimum number of key derivation rounds
49+ // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
50+ // ie slightly lower than the lowest hardware we need bother supporting
51+ static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000;
0 static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25'000;
44- //! such as the various parameters to scrypt
45+ //! Use this for more parameters to key derivation (currently unused)
46 std::vector<unsigned char> vchOtherDerivationParameters;
47
48+ //! Default/minimum number of key derivation rounds
49+ // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M
0 // 25,000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M