← Watch feed
Merge bitcoin/bitcoin#35859: wallet: use unsigned KDF iteration count
Public commit record
91/100 · Strong
What the developer wrote
Authored by Ava Chow
Merge bitcoin/bitcoin#35859: wallet: use unsigned KDF iteration count
cf36df070b4dfa954df78bb59c687de54b277a5a Wallet: Check crypter return values (benthecarman)
b76afff27490686cd35c9a8cf004f543f7d65a42 Wallet: Use unsigned KDF iteration count (benthecarman)
Pull request description:
CMasterKey::nDeriveIterations values are deserialized from wallet files
as unsigned 32-bit integers, but key derivation narrowed the count to a
signed int. A count above INT_MAX became negative in the conversion, and
the derivation loop counter then overflowed, which is undefined
behavior.
Keep the count unsigned through the derivation path to match the
serialized type, and add tests for zero and normal counts.
Also check key-derivation calibration failures and validate calculated
iteration counts before conversion. Keep the output master key unchanged
until derivation and encryption succeed, and mark fallible crypter
methods as [[nodiscard]].
ACKs for top commit:
l0rinc:
code review ACK cf36df070b4dfa954df78bb59c687de54b277a5a
achow101:
ACK cf36df070b4dfa954df78bb59c687de54b277a5a
Tree-SHA512: 95d5db2655fef8ca499af7da0f0258b4bee90975468286cb88e424257c4c5bf36407d2b2816b538e2ac3227e2a3a75c8d775211c65fe3ec6d12189da0b05fba1
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference
What changed, and why it matters
This Bitcoin Core update fixes a bug in how wallet encryption counts the number of times it scrambles a passphrase. The count was being treated as a signed integer, so a value larger than about 2 billion could flip to negative and cause undefined behavior. The patch keeps the count unsigned throughout, checks for failures, and adds tests. It is a defensive hardening fix rather than a demonstrated remote exploit.
Recommended action
Apply the patch. It is a low-risk hardening fix for wallet key derivation. Wallet operators should upgrade normally; no immediate emergency response is indicated because exploitation requires a malformed wallet file or unusual local conditions.
Security signals we found
01
Signed/unsigned integer type mismatch in KDF iteration count
02
Potential undefined behavior from loop counter overflow on maliciously large iteration counts
03
Unchecked return values from wallet crypter methods now addressed with [[nodiscard]] and explicit checks
04
Calibration result validated before narrowing to unsigned int
05
Output master key mutation deferred until encryption succeeds
Evidence from the diff
CMasterKey::nDeriveIterations is serialized as uint32_t, but CCrypter::BytesToKeySHA512AES accepted an int count. A wallet file with nDeriveIterations > INT_MAX would become negative when passed in, making the loop counter overflow and producing undefined behavior. The patch changes the derivation path to unsigned int, rejects zero rounds, validates calibration results before conversion, uses 64-bit math for averaging, defers mutation of the output master_key until success, and marks fallible crypter methods [[nodiscard]].
Changed components
src/wallet/crypter.cppsrc/wallet/crypter.hsrc/wallet/wallet.cppwallet encryption / passphrase-based key derivationCMasterKey / CCrypterInspect captured patch +49 / −30
### src/wallet/crypter.cpp
@@ -12,7 +12,7 @@
#include <vector>
namespace wallet {
-int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, const SecureString& key_data, int count, unsigned char* key, unsigned char* iv) const
+int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, const SecureString& key_data, unsigned int count, unsigned char* key, unsigned char* iv) const
{
// This mimics the behavior of openssl's EVP_BytesToKey with an aes256cbc
// cipher and sha512 message digest. Because sha512's output size (64b) is
@@ -29,7 +29,7 @@ int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, con
di.Write(salt.data(), salt.size());
di.Finalize(buf);
- for(int i = 0; i != count - 1; i++)
+ for (unsigned int i = 0; i != count - 1; ++i)
di.Reset().Write(buf, sizeof(buf)).Finalize(buf);
memcpy(key, buf, WALLET_CRYPTO_KEY_SIZE);
@@ -40,7 +40,7 @@ int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, con
bool CCrypter::SetKeyFromPassphrase(const SecureString& key_data, const std::span<const unsigned char> salt, const unsigned int rounds, const unsigned int derivation_method)
{
- if (rounds < 1 || salt.size() != WALLET_CRYPTO_SALT_SIZE) {
+ if (!rounds || salt.size() != WALLET_CRYPTO_SALT_SIZE) {
return false;
}
### src/wallet/crypter.h
@@ -76,13 +76,13 @@ friend class wallet_crypto_tests::TestCrypter; // for test access to chKey/chIV
std::vector<unsigned char, secure_allocator<unsigned char>> vchIV;
bool fKeySet;
- int BytesToKeySHA512AES(std::span<const unsigned char> salt, const SecureString& key_data, int count, unsigned char* key, unsigned char* iv) const;
+ [[nodiscard]] int BytesToKeySHA512AES(std::span<const unsigned char> salt, const SecureString& key_data, unsigned int count, unsigned char* key, unsigned char* iv) const;
public:
- bool SetKeyFromPassphrase(const SecureString& key_data, std::span<const unsigned char> salt, unsigned int rounds, unsigned int derivation_method);
- bool Encrypt(const CKeyingMaterial& vchPlaintext, std::vector<unsigned char> &vchCiphertext) const;
- bool Decrypt(std::span<const unsigned char> ciphertext, CKeyingMaterial& plaintext) const;
- bool SetKey(const CKeyingMaterial& new_key, std::span<const unsigned char> new_iv);
+ [[nodiscard]] bool SetKeyFromPassphrase(const SecureString& key_data, std::span<const unsigned char> salt, unsigned int rounds, unsigned int derivation_method);
+ [[nodiscard]] bool Encrypt(const CKeyingMaterial& vchPlaintext, std::vector<unsigned char> &vchCiphertext) const;
+ [[nodiscard]] bool Decrypt(std::span<const unsigned char> ciphertext, CKeyingMaterial& plaintext) const;
+ [[nodiscard]] bool SetKey(const CKeyingMaterial& new_key, std::span<const unsigned char> new_iv);
void CleanKey()
{
@@ -104,9 +104,9 @@ friend class wallet_crypto_tests::TestCrypter; // for test access to chKey/chIV
}
};
-bool EncryptSecret(const CKeyingMaterial& vMasterKey, const CKeyingMaterial &vchPlaintext, const uint256& nIV, std::vector<unsigned char> &vchCiphertext);
-bool DecryptSecret(const CKeyingMaterial& master_key, std::span<const unsigned char> ciphertext, const uint256& iv, CKeyingMaterial& plaintext);
-bool DecryptKey(const CKeyingMaterial& master_key, std::span<const unsigned char> crypted_secret, const CPubKey& pub_key, CKey& key);
+[[nodiscard]] bool EncryptSecret(const CKeyingMaterial& vMasterKey, const CKeyingMaterial &vchPlaintext, const uint256& nIV, std::vector<unsigned char> &vchCiphertext);
+[[nodiscard]] bool DecryptSecret(const CKeyingMaterial& master_key, std::span<const unsigned char> ciphertext, const uint256& iv, CKeyingMaterial& plaintext);
+[[nodiscard]] bool DecryptKey(const CKeyingMaterial& master_key, std::span<const unsigned char> crypted_secret, const CPubKey& pub_key, CKey& key);
} // namespace wallet
#endif // BITCOIN_WALLET_CRYPTER_H
### 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<unsigned int>();
// 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<unsigned int>(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<unsigned int>(0, CMasterKey::DEFAULT_DERIVE_ITERATIONS),
+ /*derivation_method=*/derivation_method);
}
CKey random_ckey;
### src/wallet/test/wallet_crypto_tests.cpp
@@ -24,7 +24,7 @@ static void TestPassphraseSingle(const std::span<const unsigned char> salt, cons
const std::span<const unsigned char> 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<const unsigned ch
const std::span<const unsigned char> 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<const unsigned char> correct_ciphertext = {})
{
std::vector<unsigned char> 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());
@@ -96,10 +97,17 @@ BOOST_AUTO_TEST_CASE(passphrase) {
TestCrypter::TestPassphrase(vchSalt, SecureString(hash.begin(), hash.end()), rounds);
}
+BOOST_AUTO_TEST_CASE(passphrase_zero_rounds) {
+ constexpr auto salt{"0000deadbeef0000"_hex_u8};
+ CCrypter crypt;
+ BOOST_CHECK(!crypt.SetKeyFromPassphrase("passphrase", salt, /*rounds=*/0, /*derivation_method=*/0));
+ BOOST_CHECK(crypt.SetKeyFromPassphrase("passphrase", salt, /*rounds=*/1, /*derivation_method=*/0));
+}
+
BOOST_AUTO_TEST_CASE(encrypt) {
constexpr std::array<uint8_t, WALLET_CRYPTO_SALT_SIZE> 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++)
@@ -113,9 +121,9 @@ BOOST_AUTO_TEST_CASE(encrypt) {
BOOST_AUTO_TEST_CASE(decrypt) {
constexpr std::array<uint8_t, WALLET_CRYPTO_SALT_SIZE> 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);
### src/wallet/wallet.cpp
@@ -76,10 +76,12 @@
#include <cassert>
#include <condition_variable>
#include <exception>
+#include <limits>
#include <optional>
#include <stdexcept>
#include <thread>
#include <tuple>
+#include <utility>
#include <variant>
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<unsigned int>::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<unsigned int>(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;
}
Risk score
Our methodology →Why this scored 58/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.