From cf36df070b4dfa954df78bb59c687de54b277a5a Mon Sep 17 00:00:00 2001 From: benthecarman Date: Sat, 1 Aug 2026 23:17:41 -0500 Subject: [PATCH] Wallet: Check crypter return values Mark CCrypter's fallible methods and crypto helpers as nodiscard, and handle key-derivation calibration failures. Keep the output master key unchanged until encryption succeeds. Validate calibrated iteration counts before integer conversion. This prevents a failed calibration from leaving mismatched parameters and ciphertext or triggering undefined conversion behavior. --- src/wallet/crypter.h | 16 +++++++------- src/wallet/test/fuzz/crypter.cpp | 8 +++---- src/wallet/test/wallet_crypto_tests.cpp | 13 ++++++----- src/wallet/wallet.cpp | 29 +++++++++++++++++-------- 4 files changed, 39 insertions(+), 27 deletions(-) diff --git a/src/wallet/crypter.h b/src/wallet/crypter.h index 58ba65081aa..0d1293c6cfe 100644 --- a/src/wallet/crypter.h +++ b/src/wallet/crypter.h @@ -76,13 +76,13 @@ private: std::vector> vchIV; bool fKeySet; - int BytesToKeySHA512AES(std::span salt, const SecureString& key_data, unsigned int count, unsigned char* key, unsigned char* iv) const; + [[nodiscard]] int BytesToKeySHA512AES(std::span salt, const SecureString& key_data, unsigned int count, unsigned char* key, unsigned char* iv) const; public: - bool SetKeyFromPassphrase(const SecureString& key_data, std::span salt, unsigned int rounds, unsigned int derivation_method); - bool Encrypt(const CKeyingMaterial& vchPlaintext, std::vector &vchCiphertext) const; - bool Decrypt(std::span ciphertext, CKeyingMaterial& plaintext) const; - bool SetKey(const CKeyingMaterial& new_key, std::span new_iv); + [[nodiscard]] bool SetKeyFromPassphrase(const SecureString& key_data, std::span salt, unsigned int rounds, unsigned int derivation_method); + [[nodiscard]] bool Encrypt(const CKeyingMaterial& vchPlaintext, std::vector &vchCiphertext) const; + [[nodiscard]] bool Decrypt(std::span ciphertext, CKeyingMaterial& plaintext) const; + [[nodiscard]] bool SetKey(const CKeyingMaterial& new_key, std::span new_iv); void CleanKey() { @@ -104,9 +104,9 @@ public: } }; -bool EncryptSecret(const CKeyingMaterial& vMasterKey, const CKeyingMaterial &vchPlaintext, const uint256& nIV, std::vector &vchCiphertext); -bool DecryptSecret(const CKeyingMaterial& master_key, std::span ciphertext, const uint256& iv, CKeyingMaterial& plaintext); -bool DecryptKey(const CKeyingMaterial& master_key, std::span crypted_secret, const CPubKey& pub_key, CKey& key); +[[nodiscard]] bool EncryptSecret(const CKeyingMaterial& vMasterKey, const CKeyingMaterial &vchPlaintext, const uint256& nIV, std::vector &vchCiphertext); +[[nodiscard]] bool DecryptSecret(const CKeyingMaterial& master_key, std::span ciphertext, const uint256& iv, CKeyingMaterial& plaintext); +[[nodiscard]] bool DecryptKey(const CKeyingMaterial& master_key, std::span crypted_secret, const CPubKey& pub_key, CKey& key); } // namespace wallet #endif // BITCOIN_WALLET_CRYPTER_H diff --git a/src/wallet/test/fuzz/crypter.cpp b/src/wallet/test/fuzz/crypter.cpp index f0a6a36e8e3..621a890ce64 100644 --- a/src/wallet/test/fuzz/crypter.cpp +++ b/src/wallet/test/fuzz/crypter.cpp @@ -35,10 +35,10 @@ FUZZ_TARGET(crypter, .init = initialize_crypter) const unsigned int derivation_method = fuzzed_data_provider.ConsumeBool() ? 0 : fuzzed_data_provider.ConsumeIntegral(); // 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, CMasterKey::DEFAULT_DERIVE_ITERATIONS), - /*derivation_method=*/derivation_method); + (void)crypt.SetKeyFromPassphrase(/*key_data=*/secure_string, + /*salt=*/ConsumeFixedLengthByteVector(fuzzed_data_provider, WALLET_CRYPTO_SALT_SIZE), + /*rounds=*/fuzzed_data_provider.ConsumeIntegralInRange(0, CMasterKey::DEFAULT_DERIVE_ITERATIONS), + /*derivation_method=*/derivation_method); } CKey random_ckey; diff --git a/src/wallet/test/wallet_crypto_tests.cpp b/src/wallet/test/wallet_crypto_tests.cpp index 353eb46f97d..d7b91330ede 100644 --- a/src/wallet/test/wallet_crypto_tests.cpp +++ b/src/wallet/test/wallet_crypto_tests.cpp @@ -24,7 +24,7 @@ static void TestPassphraseSingle(const std::span salt, cons const std::span correct_iv = {}) { CCrypter crypt; - crypt.SetKeyFromPassphrase(passphrase, salt, rounds, 0); + BOOST_REQUIRE(crypt.SetKeyFromPassphrase(passphrase, salt, rounds, /*derivation_method=*/0)); if (!correct_key.empty()) { BOOST_CHECK_MESSAGE(memcmp(crypt.vchKey.data(), correct_key.data(), crypt.vchKey.size()) == 0, @@ -50,8 +50,9 @@ static void TestDecrypt(const CCrypter& crypt, const std::span correct_plaintext = {}) { CKeyingMaterial decrypted; - crypt.Decrypt(ciphertext, decrypted); + const bool decrypt_ok{crypt.Decrypt(ciphertext, decrypted)}; if (!correct_plaintext.empty()) { + BOOST_REQUIRE(decrypt_ok); BOOST_CHECK_EQUAL_COLLECTIONS(decrypted.begin(), decrypted.end(), correct_plaintext.begin(), correct_plaintext.end()); } } @@ -60,7 +61,7 @@ static void TestEncryptSingle(const CCrypter& crypt, const CKeyingMaterial& plai const std::span correct_ciphertext = {}) { std::vector ciphertext; - crypt.Encrypt(plaintext, ciphertext); + BOOST_REQUIRE(crypt.Encrypt(plaintext, ciphertext)); if (!correct_ciphertext.empty()) { BOOST_CHECK_EQUAL_COLLECTIONS(ciphertext.begin(), ciphertext.end(), correct_ciphertext.begin(), correct_ciphertext.end()); @@ -106,7 +107,7 @@ BOOST_AUTO_TEST_CASE(passphrase_zero_rounds) { BOOST_AUTO_TEST_CASE(encrypt) { constexpr std::array salt{"0000deadbeef0000"_hex_u8}; CCrypter crypt; - crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0); + BOOST_REQUIRE(crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, /*derivation_method=*/0)); TestCrypter::TestEncrypt(crypt, "22bcade09ac03ff6386914359cfe885cfeb5f77ff0d670f102f619687453b29d"_hex_u8); for (int i = 0; i != 100; i++) @@ -120,9 +121,9 @@ BOOST_AUTO_TEST_CASE(encrypt) { BOOST_AUTO_TEST_CASE(decrypt) { constexpr std::array salt{"0000deadbeef0000"_hex_u8}; CCrypter crypt; - crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, 0); + BOOST_REQUIRE(crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, /*derivation_method=*/0)); - // Some corner cases the came up while testing + // Some corner cases that came up while testing TestCrypter::TestDecrypt(crypt,"795643ce39d736088367822cdc50535ec6f103715e3e48f4f3b1a60a08ef59ca"_hex_u8); TestCrypter::TestDecrypt(crypt,"de096f4a8f9bd97db012aa9d90d74de8cdea779c3ee8bc7633d8b5d6da703486"_hex_u8); TestCrypter::TestDecrypt(crypt,"32d0a8974e3afd9c6c3ebf4d66aa4e6419f8c173de25947f98cf8b7ace49449c"_hex_u8); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c36472b2680..8e89953e03e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -76,10 +76,12 @@ #include #include #include +#include #include #include #include #include +#include #include struct KeyOriginInfo; @@ -566,36 +568,45 @@ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyin { constexpr MillisecondsDouble target_time{100}; CCrypter crypter; + CMasterKey updated_master_key{master_key}; // Get the weighted average of iterations we can do in 100ms over 2 runs. for (int i = 0; i < 2; i++){ auto start_time{NodeClock::now()}; - crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod); + const bool key_set{crypter.SetKeyFromPassphrase(wallet_passphrase, updated_master_key.vchSalt, updated_master_key.nDeriveIterations, updated_master_key.nDerivationMethod)}; auto elapsed_time{NodeClock::now() - start_time}; + if (!key_set) { + return false; + } if (elapsed_time <= 0s) { // We are probably in a test with a mocked clock. - master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; + updated_master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; break; } // target_iterations : elapsed_iterations :: target_time : elapsed_time - unsigned int target_iterations = master_key.nDeriveIterations * target_time / elapsed_time; - // Get the weighted average with previous runs. - master_key.nDeriveIterations = (i * master_key.nDeriveIterations + target_iterations) / (i + 1); + const double target_iterations{updated_master_key.nDeriveIterations * target_time / elapsed_time}; + if (target_iterations < 1 || target_iterations > std::numeric_limits::max()) { + return false; + } + // Get the weighted average with previous runs. Use 64-bit math so the + // sum cannot wrap; the average of two unsigned int values fits in one. + updated_master_key.nDeriveIterations = (uint64_t{updated_master_key.nDeriveIterations} * i + static_cast(target_iterations)) / (i + 1); } - if (master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) { - master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; + if (updated_master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) { + updated_master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS; } - if (!crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)) { + if (!crypter.SetKeyFromPassphrase(wallet_passphrase, updated_master_key.vchSalt, updated_master_key.nDeriveIterations, updated_master_key.nDerivationMethod)) { return false; } - if (!crypter.Encrypt(plain_master_key, master_key.vchCryptedKey)) { + if (!crypter.Encrypt(plain_master_key, updated_master_key.vchCryptedKey)) { return false; } + master_key = std::move(updated_master_key); return true; }