From 62c209f50d9c33fde5062ebca317b9a4233aff62 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Sun, 1 Dec 2024 00:30:05 +0100 Subject: [PATCH 1/5] 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. --- src/wallet/crypter.h | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index 944858fb3f8..0dd7da65545 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -23,7 +23,7 @@ const unsigned int WALLET_CRYPTO_IV_SIZE = 16; * derived using derivation method nDerivationMethod * (0 == EVP_sha512()) and derivation iterations nDeriveIterations. * vchOtherDerivationParameters is provided for alternative algorithms - * which may require more parameters (such as scrypt). + * which may require more parameters. * * Wallet Private Keys are then encrypted using AES-256-CBC * with the double-sha256 of the public key as the IV, and the @@ -37,11 +37,9 @@ public: std::vector vchCryptedKey; std::vector vchSalt; //! 0 = EVP_sha512() - //! 1 = scrypt() unsigned int nDerivationMethod; unsigned int nDeriveIterations; - //! Use this for more parameters to key derivation, - //! such as the various parameters to scrypt + //! Use this for more parameters to key derivation (currently unused) std::vector vchOtherDerivationParameters; SERIALIZE_METHODS(CMasterKey, obj) From a6d9b415aa3afcfe463887d0fde00c3d2d32672a Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Sun, 1 Dec 2024 00:47:45 +0100 Subject: [PATCH 2/5] 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. --- src/wallet/crypter.h | 9 ++++++--- src/wallet/test/fuzz/crypter.cpp | 2 +- src/wallet/test/wallet_crypto_tests.cpp | 6 +++--- src/wallet/wallet.cpp | 12 ++++++------ 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index 0dd7da65545..fa15f02c8c6 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -42,6 +42,11 @@ public: //! Use this for more parameters to key derivation (currently unused) std::vector vchOtherDerivationParameters; + //! Default/minimum number of key derivation rounds + // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M + // ie slightly lower than the lowest hardware we need bother supporting + static constexpr unsigned int DEFAULT_DERIVE_ITERATIONS = 25000; + SERIALIZE_METHODS(CMasterKey, obj) { READWRITE(obj.vchCryptedKey, obj.vchSalt, obj.nDerivationMethod, obj.nDeriveIterations, obj.vchOtherDerivationParameters); @@ -49,9 +54,7 @@ public: CMasterKey() { - // 25000 rounds is just under 0.1 seconds on a 1.86 GHz Pentium M - // ie slightly lower than the lowest hardware we need bother supporting - nDeriveIterations = 25000; + nDeriveIterations = DEFAULT_DERIVE_ITERATIONS; nDerivationMethod = 0; vchOtherDerivationParameters = std::vector(0); } diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp index 7869f5f39c8..c1454c6e059 100644 --- a/src/wallet/test/fuzz/crypter.cpp +++ b/src/wallet/test/fuzz/crypter.cpp @@ -38,7 +38,7 @@ FUZZ_TARGET(crypter, .init = initialize_crypter) // Limiting the value of rounds since it is otherwise uselessly expensive and causes a timeout when fuzzing. crypt.SetKeyFromPassphrase(/*key_data=*/secure_string, /*salt=*/ConsumeFixedLengthByteVector(fuzzed_data_provider, WALLET_CRYPTO_SALT_SIZE), - /*rounds=*/fuzzed_data_provider.ConsumeIntegralInRange(0, 25000), + /*rounds=*/fuzzed_data_provider.ConsumeIntegralInRange(0, CMasterKey::DEFAULT_DERIVE_ITERATIONS), /*derivation_method=*/derivation_method); } diff --git a/src/wallet/test/wallet_crypto_tests.cpp b/src/wallet/test/wallet_crypto_tests.cpp index 7c74c313088..e01c7a4e5cf 100644 --- a/src/wallet/test/wallet_crypto_tests.cpp +++ b/src/wallet/test/wallet_crypto_tests.cpp @@ -83,7 +83,7 @@ static void TestEncrypt(const CCrypter& crypt, const std::span salt{"0000deadbeef0000"_hex_u8}; CCrypter crypt; - crypt.SetKeyFromPassphrase("passphrase", salt, 25000, 0); + crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0); TestCrypter::TestEncrypt(crypt, "22bcade09ac03ff6386914359cfe885cfeb5f77ff0d670f102f619687453b29d"_hex_u8); for (int i = 0; i != 100; i++) @@ -113,7 +113,7 @@ BOOST_AUTO_TEST_CASE(encrypt) { BOOST_AUTO_TEST_CASE(decrypt) { constexpr std::array salt{"0000deadbeef0000"_hex_u8}; CCrypter crypt; - crypt.SetKeyFromPassphrase("passphrase", salt, 25000, 0); + crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0); // Some corner cases the came up while testing TestCrypter::TestDecrypt(crypt,"795643ce39d736088367822cdc50535ec6f103715e3e48f4f3b1a60a08ef59ca"_hex_u8); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dd9dfcd516a..1f34bea4415 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -628,8 +628,8 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod); pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; - if (pMasterKey.second.nDeriveIterations < 25000) - pMasterKey.second.nDeriveIterations = 25000; + if (pMasterKey.second.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) + pMasterKey.second.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", pMasterKey.second.nDeriveIterations); @@ -825,15 +825,15 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) CCrypter crypter; constexpr MillisecondsDouble target{100}; auto start{SteadyClock::now()}; - crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, 25000, kMasterKey.nDerivationMethod); - kMasterKey.nDeriveIterations = static_cast(25000 * target / (SteadyClock::now() - start)); + crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, kMasterKey.nDerivationMethod); + kMasterKey.nDeriveIterations = static_cast(CMasterKey::DEFAULT_DERIVE_ITERATIONS * target / (SteadyClock::now() - start)); start = SteadyClock::now(); crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast(kMasterKey.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; - if (kMasterKey.nDeriveIterations < 25000) - kMasterKey.nDeriveIterations = 25000; + if (kMasterKey.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) + kMasterKey.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; WalletLogPrintf("Encrypting Wallet with an nDeriveIterations of %i\n", kMasterKey.nDeriveIterations); From 846545947cd3b993c40362b9d0afcd7b4f5f05bd Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Sun, 1 Dec 2024 01:38:20 +0100 Subject: [PATCH 3/5] wallet: refactor: dedup master key encryption / derivation rounds setting --- src/wallet/wallet.cpp | 69 ++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1f34bea4415..7794034d0ca 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -576,6 +576,35 @@ void CWallet::UpgradeDescriptorCache() SetWalletFlag(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED); } +/* Given a wallet passphrase string and an unencrypted master key, determine the proper key + * derivation parameters (should take at least 100ms) and encrypt the master key. */ +static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyingMaterial& plain_master_key, CMasterKey& master_key) +{ + constexpr MillisecondsDouble target{100}; + auto start{SteadyClock::now()}; + CCrypter crypter; + + crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod); + master_key.nDeriveIterations = static_cast(master_key.nDeriveIterations * target / (SteadyClock::now() - start)); + + start = SteadyClock::now(); + crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod); + master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; + + if (master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) { + master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; + } + + if (!crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)) { + return false; + } + if (!crypter.Encrypt(plain_master_key, master_key.vchCryptedKey)) { + return false; + } + + return true; +} + bool CWallet::Unlock(const SecureString& strWalletPassphrase) { CCrypter crypter; @@ -619,24 +648,11 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, return false; if (Unlock(_vMasterKey)) { - constexpr MillisecondsDouble target{100}; - auto start{SteadyClock::now()}; - crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod); - pMasterKey.second.nDeriveIterations = static_cast(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start)); - - start = SteadyClock::now(); - crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod); - pMasterKey.second.nDeriveIterations = (pMasterKey.second.nDeriveIterations + static_cast(pMasterKey.second.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; - - if (pMasterKey.second.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) - pMasterKey.second.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; - + if (!EncryptMasterKey(strNewWalletPassphrase, _vMasterKey, pMasterKey.second)) { + return false; + } WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", pMasterKey.second.nDeriveIterations); - if (!crypter.SetKeyFromPassphrase(strNewWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod)) - return false; - if (!crypter.Encrypt(_vMasterKey, pMasterKey.second.vchCryptedKey)) - return false; WalletBatch(GetDatabase()).WriteMasterKey(pMasterKey.first, pMasterKey.second); if (fWasLocked) Lock(); @@ -822,26 +838,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) kMasterKey.vchSalt.resize(WALLET_CRYPTO_SALT_SIZE); GetStrongRandBytes(kMasterKey.vchSalt); - CCrypter crypter; - constexpr MillisecondsDouble target{100}; - auto start{SteadyClock::now()}; - crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, kMasterKey.nDerivationMethod); - kMasterKey.nDeriveIterations = static_cast(CMasterKey::DEFAULT_DERIVE_ITERATIONS * target / (SteadyClock::now() - start)); - - start = SteadyClock::now(); - crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod); - kMasterKey.nDeriveIterations = (kMasterKey.nDeriveIterations + static_cast(kMasterKey.nDeriveIterations * target / (SteadyClock::now() - start))) / 2; - - if (kMasterKey.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) - kMasterKey.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; - + if (!EncryptMasterKey(strWalletPassphrase, _vMasterKey, kMasterKey)) { + return false; + } WalletLogPrintf("Encrypting Wallet with an nDeriveIterations of %i\n", kMasterKey.nDeriveIterations); - if (!crypter.SetKeyFromPassphrase(strWalletPassphrase, kMasterKey.vchSalt, kMasterKey.nDeriveIterations, kMasterKey.nDerivationMethod)) - return false; - if (!crypter.Encrypt(_vMasterKey, kMasterKey.vchCryptedKey)) - return false; - { LOCK2(m_relock_mutex, cs_wallet); mapMasterKeys[++nMasterKeyMaxID] = kMasterKey; From 5a92077fd5317f936da2fa0aa45e0173248f765b Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Sun, 1 Dec 2024 03:57:02 +0100 Subject: [PATCH 4/5] wallet: refactor: dedup master key decryption --- src/wallet/wallet.cpp | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7794034d0ca..cf01b49951e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -605,19 +605,30 @@ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyin return true; } -bool CWallet::Unlock(const SecureString& strWalletPassphrase) +static bool DecryptMasterKey(const SecureString& wallet_passphrase, const CMasterKey& master_key, CKeyingMaterial& plain_master_key) { CCrypter crypter; + if (!crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)) { + return false; + } + if (!crypter.Decrypt(master_key.vchCryptedKey, plain_master_key)) { + return false; + } + + return true; +} + +bool CWallet::Unlock(const SecureString& strWalletPassphrase) +{ CKeyingMaterial _vMasterKey; { LOCK(cs_wallet); for (const MasterKeyMap::value_type& pMasterKey : mapMasterKeys) { - if(!crypter.SetKeyFromPassphrase(strWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod)) - return false; - if (!crypter.Decrypt(pMasterKey.second.vchCryptedKey, _vMasterKey)) + if (!DecryptMasterKey(strWalletPassphrase, pMasterKey.second, _vMasterKey)) { continue; // try another master key + } if (Unlock(_vMasterKey)) { // Now that we've unlocked, upgrade the key metadata UpgradeKeyMetadata(); @@ -638,14 +649,12 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, LOCK2(m_relock_mutex, cs_wallet); Lock(); - CCrypter crypter; CKeyingMaterial _vMasterKey; for (MasterKeyMap::value_type& pMasterKey : mapMasterKeys) { - if(!crypter.SetKeyFromPassphrase(strOldWalletPassphrase, pMasterKey.second.vchSalt, pMasterKey.second.nDeriveIterations, pMasterKey.second.nDerivationMethod)) - return false; - if (!crypter.Decrypt(pMasterKey.second.vchCryptedKey, _vMasterKey)) + if (!DecryptMasterKey(strOldWalletPassphrase, pMasterKey.second, _vMasterKey)) { return false; + } if (Unlock(_vMasterKey)) { if (!EncryptMasterKey(strNewWalletPassphrase, _vMasterKey, pMasterKey.second)) { From a8333fc9ff9adaa97a1f9024f5783cc071777150 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Sun, 1 Dec 2024 04:33:25 +0100 Subject: [PATCH 5/5] 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- --- src/wallet/wallet.cpp | 44 +++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cf01b49951e..7c409eb4bc8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -620,16 +620,16 @@ static bool DecryptMasterKey(const SecureString& wallet_passphrase, const CMaste bool CWallet::Unlock(const SecureString& strWalletPassphrase) { - CKeyingMaterial _vMasterKey; + CKeyingMaterial plain_master_key; { LOCK(cs_wallet); - for (const MasterKeyMap::value_type& pMasterKey : mapMasterKeys) + for (const auto& [_, master_key] : mapMasterKeys) { - if (!DecryptMasterKey(strWalletPassphrase, pMasterKey.second, _vMasterKey)) { + if (!DecryptMasterKey(strWalletPassphrase, master_key, plain_master_key)) { continue; // try another master key } - if (Unlock(_vMasterKey)) { + if (Unlock(plain_master_key)) { // Now that we've unlocked, upgrade the key metadata UpgradeKeyMetadata(); // Now that we've unlocked, upgrade the descriptor cache @@ -649,20 +649,20 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, LOCK2(m_relock_mutex, cs_wallet); Lock(); - CKeyingMaterial _vMasterKey; - for (MasterKeyMap::value_type& pMasterKey : mapMasterKeys) + CKeyingMaterial plain_master_key; + for (auto& [master_key_id, master_key] : mapMasterKeys) { - if (!DecryptMasterKey(strOldWalletPassphrase, pMasterKey.second, _vMasterKey)) { + if (!DecryptMasterKey(strOldWalletPassphrase, master_key, plain_master_key)) { return false; } - if (Unlock(_vMasterKey)) + if (Unlock(plain_master_key)) { - if (!EncryptMasterKey(strNewWalletPassphrase, _vMasterKey, pMasterKey.second)) { + if (!EncryptMasterKey(strNewWalletPassphrase, plain_master_key, master_key)) { return false; } - WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", pMasterKey.second.nDeriveIterations); + WalletLogPrintf("Wallet passphrase changed to an nDeriveIterations of %i\n", master_key.nDeriveIterations); - WalletBatch(GetDatabase()).WriteMasterKey(pMasterKey.first, pMasterKey.second); + WalletBatch(GetDatabase()).WriteMasterKey(master_key_id, master_key); if (fWasLocked) Lock(); return true; @@ -837,35 +837,35 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) if (IsCrypted()) return false; - CKeyingMaterial _vMasterKey; + CKeyingMaterial plain_master_key; - _vMasterKey.resize(WALLET_CRYPTO_KEY_SIZE); - GetStrongRandBytes(_vMasterKey); + plain_master_key.resize(WALLET_CRYPTO_KEY_SIZE); + GetStrongRandBytes(plain_master_key); - CMasterKey kMasterKey; + CMasterKey master_key; - kMasterKey.vchSalt.resize(WALLET_CRYPTO_SALT_SIZE); - GetStrongRandBytes(kMasterKey.vchSalt); + master_key.vchSalt.resize(WALLET_CRYPTO_SALT_SIZE); + GetStrongRandBytes(master_key.vchSalt); - if (!EncryptMasterKey(strWalletPassphrase, _vMasterKey, kMasterKey)) { + if (!EncryptMasterKey(strWalletPassphrase, plain_master_key, master_key)) { return false; } - WalletLogPrintf("Encrypting Wallet with an nDeriveIterations of %i\n", kMasterKey.nDeriveIterations); + WalletLogPrintf("Encrypting Wallet with an nDeriveIterations of %i\n", master_key.nDeriveIterations); { LOCK2(m_relock_mutex, cs_wallet); - mapMasterKeys[++nMasterKeyMaxID] = kMasterKey; + mapMasterKeys[++nMasterKeyMaxID] = master_key; WalletBatch* encrypted_batch = new WalletBatch(GetDatabase()); if (!encrypted_batch->TxnBegin()) { delete encrypted_batch; encrypted_batch = nullptr; return false; } - encrypted_batch->WriteMasterKey(nMasterKeyMaxID, kMasterKey); + encrypted_batch->WriteMasterKey(nMasterKeyMaxID, master_key); for (const auto& spk_man_pair : m_spk_managers) { auto spk_man = spk_man_pair.second.get(); - if (!spk_man->Encrypt(_vMasterKey, encrypted_batch)) { + if (!spk_man->Encrypt(plain_master_key, encrypted_batch)) { encrypted_batch->TxnAbort(); delete encrypted_batch; encrypted_batch = nullptr;