wallet: reject KDF iteration counts that overflow the int count #35859

benthecarman commented at 5:04 AM on August 1, 2026: contributor

CMasterKey::nDeriveIterations values are deserialized from wallet files without an upper bound. Values above INT_MAX cannot be represented by the signed count used by BytesToKeySHA512AES. Narrowing such a value previously made key derivation run billions of SHA-512 rounds, hanging wallet unlock.

Reject out-of-range counts before the signed conversion. This prevents the long-running derivation and avoids sanitizer errors. Add unit tests for invalid boundary values and a normal iteration count.

Found and created with kimi k3.

DrahtBot added the label Wallet on Aug 1, 2026

DrahtBot commented at 5:04 AM on August 1, 2026: 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/35859.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	151henry151
Concept ACK	l0rinc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

// Some corner cases the came up while testing -> // Some corner cases that came up while testing [“the came” is a grammatical typo that breaks the sentence]

2026-08-02 04:22:18

benthecarman force-pushed on Aug 1, 2026

DrahtBot added the label CI failed on Aug 1, 2026

DrahtBot commented at 5:31 AM on August 1, 2026: contributor

🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/30685144257/job/91329306668 LLM reason (✨ experimental): CI failed due to an UndefinedBehaviorSanitizer error (implicit integer sign change) in wallet/crypter.cpp triggered by wallet_crypto_tests (passphrase_rounds_limit).

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Aug 1, 2026

151henry151 commented at 5:54 PM on August 1, 2026: contributor

ACK fa833d949b786e58c598b997054ec15de682c825

Reviewed the diff and tested (wallet_crypto_tests; SetKeyFromPassphrase with UINT_MAX hangs past 45s on master and returns immediately here). count is -1 there, so the loop is i != -2 with i counting up and can't exit on its own. Rejecting rounds > INT_MAX before the signed conversion is the right shape — widening count to unsigned just makes UINT_MAX a well-defined multi-hour hang, and switching the loop to i < count - 1 alone would treat count == -1 as a successful 1-iteration derive.

One gap that I think belongs in this PR: EncryptMasterKey ignores the return value of its two calibration SetKeyFromPassphrase calls. A near-zero first sample (coarse clock, VM glitch) can already push nDeriveIterations above INT_MAX; with this change the second call then returns in microseconds, so its elapsed is ~0 as well and the average is another garbage count (the double→unsigned conversion is also UB once the value exceeds UINT_MAX). The unchecked returns and that cast predate the PR — what's new is the second call failing silently instead of hanging. Check the return value and don't update nDeriveIterations on failure.

Then, a few more things that could be addressed as follow-ups: INT_MAX is still accepted (~600ns/round at -O2 on an i7-7700HQ, so ~21 minutes; only reachable via the wallet file, so local DoS), and a MAX_DERIVE_ITERATIONS should be chosen against calibrations on fast hardware with a plan for wallets that already exceed it. Rejection also looks like a wrong passphrase with no log — a WalletLogPrintf of the rejected count would help.

in src/wallet/crypter.cpp:23 in fa833d949b

  19 | @@ -19,7 +20,7 @@ int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, con
  20 |      // greater than the aes256 block size (16b) + aes256 key size (32b),
  21 |      // there's no need to process more than once (D_0).
  22 |  
  23 | -    if(!count || !key || !iv)
  24 | +    if(count < 1 || !key || !iv)

l0rinc commented at 7:18 PM on August 1, 2026:

fa833d9 Wallet: Reject oversized KDF iteration counts:

nit: since this private KDF has one caller (which now validates the signed range before conversion) a negative count is unreachable. We could minimize the diff by leaving this line unchanged.

in src/wallet/crypter.cpp:45 in fa833d949b

  40 | @@ -40,7 +41,8 @@ int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, con
  41 |  
  42 |  bool CCrypter::SetKeyFromPassphrase(const SecureString& key_data, const std::span<const unsigned char> salt, const unsigned int rounds, const unsigned int derivation_method)
  43 |  {
  44 | -    if (rounds < 1 || salt.size() != WALLET_CRYPTO_SALT_SIZE) {
  45 | +    // Reject counts that cannot be represented by the KDF's signed count parameter.
  46 | +    if (rounds < 1 || rounds > static_cast<unsigned int>(std::numeric_limits<int>::max()) || salt.size() != WALLET_CRYPTO_SALT_SIZE) {

l0rinc commented at 7:19 PM on August 1, 2026:

fa833d9 Wallet: Reject oversized KDF iteration counts:

We could simplify this slightly and also be more in line with BytesToKeySHA512AES:

    if (!rounds || !std::in_range<int>(rounds) || salt.size() != WALLET_CRYPTO_SALT_SIZE) {

in src/wallet/test/wallet_crypto_tests.cpp:101 in fa833d949b

  96 | @@ -96,6 +97,19 @@ BOOST_AUTO_TEST_CASE(passphrase) {
  97 |      TestCrypter::TestPassphrase(vchSalt, SecureString(hash.begin(), hash.end()), rounds);
  98 |  }
  99 |  
 100 | +BOOST_AUTO_TEST_CASE(passphrase_rounds_limit) {
 101 | +    constexpr std::array<uint8_t, WALLET_CRYPTO_SALT_SIZE> salt{"0000deadbeef0000"_hex_u8};

l0rinc commented at 7:20 PM on August 1, 2026:

fa833d9 Wallet: Reject oversized KDF iteration counts:

nit:

    constexpr auto salt{"0000deadbeef0000"_hex_u8};

in src/wallet/test/wallet_crypto_tests.cpp:108 in fa833d949b

 103 | +    // Iteration counts above INT_MAX (e.g. from a crafted or corrupted wallet file) narrow to a
 104 | +    // negative count in the KDF and must be rejected instead of making key derivation loop for an
 105 | +    // unbounded amount of time.
 106 | +    BOOST_CHECK(!crypt.SetKeyFromPassphrase("passphrase", salt, 0, 0));
 107 | +    BOOST_CHECK(!crypt.SetKeyFromPassphrase("passphrase", salt, static_cast<unsigned int>(std::numeric_limits<int>::max()) + 1, 0));
 108 | +    BOOST_CHECK(!crypt.SetKeyFromPassphrase("passphrase", salt, std::numeric_limits<unsigned int>::max(), 0));

l0rinc commented at 7:23 PM on August 1, 2026:

fa833d9 Wallet: Reject oversized KDF iteration counts:

nit:

    for (unsigned int rounds : {0u, std::numeric_limits<int>::max() + 1u}) {
        BOOST_CHECK(!crypt.SetKeyFromPassphrase("passphrase", salt, rounds, /*derivation_method=*/0));
    }

in src/wallet/crypter.cpp:42 in fa833d949b outdated

  40 | @@ -40,7 +41,8 @@ int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, con
  41 |  
  42 |  bool CCrypter::SetKeyFromPassphrase(const SecureString& key_data, const std::span<const unsigned char> salt, const unsigned int rounds, const unsigned int derivation_method)

l0rinc commented at 9:40 PM on August 1, 2026:

fa833d9 Wallet: Reject oversized KDF iteration counts:

Can we make the methods in CCrypter all [[nodiscard]] and make sure the call sites are not ignored?

benthecarman commented at 4:22 AM on August 2, 2026:

Added in a second commit

l0rinc approved

l0rinc commented at 9:48 PM on August 1, 2026: contributor

Concept ACK fa833d949b786e58c598b997054ec15de682c825, thanks a lot for checking!

I left a few non-blocking suggestions. Could you also mark the fallible CCrypter methods [[nodiscard]] and handle their results at every call site - or at least for SetKeyFromPassphrase()?

Wallet: Reject oversized KDF iteration counts

CMasterKey::nDeriveIterations values are deserialized from wallet files
without an upper bound. Values above INT_MAX cannot be represented by
the signed count used by BytesToKeySHA512AES. Narrowing such a value
previously made key derivation run billions of SHA-512 rounds, hanging
wallet unlock.

Reject out-of-range counts before the signed conversion. This prevents
the long-running derivation and avoids sanitizer errors. Add unit tests
for invalid boundary values and a normal iteration count.

ec97238f45

Wallet: Check crypter return values

Mark CCrypter's fallible methods as nodiscard and handle the
key-derivation calibration call in EncryptMasterKey. This prevents a
failed calibration derive from feeding invalid iteration counts into the
final master-key encryption step.

7b18a0c88c

benthecarman force-pushed on Aug 2, 2026

benthecarman requested review from l0rinc on Aug 2, 2026

151henry151 commented at 2:00 PM on August 2, 2026: contributor

re-ACK 7b18a0c88c

Reviewed the range-diff since fa833d9. Failing closed in EncryptMasterKey removes the garbage-average path from a near-zero calibration sample — checked that it aborts rather than averaging a second bogus count. MAX_DERIVE_ITERATIONS and logging the rejected count remain follow-ups, not blockers.

Tested with wallet_crypto_tests,wallet_tests,walletload_tests and a full build with -DSANITIZERS=address,float-divide-by-zero,integer,undefined — passed.

in src/wallet/crypter.cpp:50 in ec97238f45

  47 |      }
  48 |  
  49 |      int i = 0;
  50 |      if (derivation_method == 0) {
  51 | -        i = BytesToKeySHA512AES(salt, key_data, rounds, vchKey.data(), vchIV.data());
  52 | +        i = BytesToKeySHA512AES(salt, key_data, static_cast<int>(rounds), vchKey.data(), vchIV.data());

l0rinc commented at 3:25 AM on August 3, 2026:

ec97238 Wallet: Reject oversized KDF iteration counts:

Unless there's a good reason for adding this, given that the assumptions only become stricter after this change, I'd revert this line.

in src/wallet/wallet.cpp:576 in 7b18a0c88c

 569 | @@ -570,8 +570,11 @@ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyin
 570 |      // Get the weighted average of iterations we can do in 100ms over 2 runs.
 571 |      for (int i = 0; i < 2; i++){
 572 |          auto start_time{NodeClock::now()};
 573 | -        crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
 574 | +        const bool key_set{crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)};
 575 |          auto elapsed_time{NodeClock::now() - start_time};
 576 | +        if (!key_set) {
 577 | +            return false;

l0rinc commented at 3:40 AM on August 3, 2026:

7b18a0c Wallet: Check crypter return values:

Now that we return early on failure, we shouldn't modify the output parameter: a failed calibration can leave nDeriveIterations changed while vchCryptedKey still contains the old ciphertext.

Could we copy master_key and only move it to the output after encryption succeeds?

<details><summary>preserve master key on failure</summary>

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index bbb930864d..b76c5da070 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -566,11 +566,12 @@ 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()};
-        const bool key_set{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;
@@ -578,27 +579,28 @@ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyin
 
         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;
+        unsigned int target_iterations = updated_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);
+        updated_master_key.nDeriveIterations = (i * updated_master_key.nDeriveIterations + 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;
 }

</details>

in src/wallet/test/wallet_crypto_tests.cpp:110 in ec97238f45

 105 | +    // unbounded amount of time.
 106 | +    for (unsigned int rounds : {0u, std::numeric_limits<int>::max() + 1u}) {
 107 | +        BOOST_CHECK(!crypt.SetKeyFromPassphrase("passphrase", salt, rounds, /*derivation_method=*/0));
 108 | +    }
 109 | +    // Sane values still work.
 110 | +    BOOST_CHECK(crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, /*derivation_method=*/0));

l0rinc commented at 3:44 AM on August 3, 2026:

ec97238 Wallet: Reject oversized KDF iteration counts:

Do we really need 25,000 rounds to check a "sane" value?

<details><summary>trim KDF rounds test</summary>

diff --git a/src/wallet/test/wallet_crypto_tests.cpp b/src/wallet/test/wallet_crypto_tests.cpp
index 39c0626697..cc6bdedc21 100644
--- a/src/wallet/test/wallet_crypto_tests.cpp
+++ b/src/wallet/test/wallet_crypto_tests.cpp
@@ -107,8 +107,8 @@ BOOST_AUTO_TEST_CASE(passphrase_rounds_limit) {
     for (unsigned int rounds : {0u, std::numeric_limits<int>::max() + 1u}) {
         BOOST_CHECK(!crypt.SetKeyFromPassphrase("passphrase", salt, rounds, /*derivation_method=*/0));
     }
-    // Sane values still work.
-    BOOST_CHECK(crypt.SetKeyFromPassphrase("passphrase", salt, CMasterKey::DEFAULT_DERIVE_ITERATIONS, /*derivation_method=*/0));
+    // In-range values still work.
+    BOOST_CHECK(crypt.SetKeyFromPassphrase("passphrase", salt, /*rounds=*/1, /*derivation_method=*/0));
 }
 
 BOOST_AUTO_TEST_CASE(encrypt) {

</details>

in src/wallet/crypter.h:109 in 7b18a0c88c

l0rinc commented at 3:59 AM on August 3, 2026:

7b18a0c Wallet: Check crypter return values:

Could you please also make these [[nodiscard]] (no call-site adjustments needed as far as I can see)?

[[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);

in src/wallet/wallet.cpp:575 in 7b18a0c88c

 569 | @@ -570,8 +570,11 @@ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyin
 570 |      // Get the weighted average of iterations we can do in 100ms over 2 runs.
 571 |      for (int i = 0; i < 2; i++){
 572 |          auto start_time{NodeClock::now()};
 573 | -        crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
 574 | +        const bool key_set{crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)};
 575 |          auto elapsed_time{NodeClock::now() - start_time};
 576 | +        if (!key_set) {

l0rinc commented at 6:26 PM on August 3, 2026:

7b18a0c Wallet: Check crypter return values:

Optional hardening since this predates the PR: target_iterations is converted from double to unsigned int before any range check, so a sufficiently small positive elapsed_time can make the conversion undefined. Would it make sense to reject values outside [1, INT_MAX] before converting, i.e. store the double calculation first, check whether we can safely cast it, and only do so when safe? (the preceding elapsed_time <= 0s should eliminate NaN already)

<details><summary>validate calibrated KDF rounds</summary>

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index b76c5da070..aa383e0893 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -76,6 +76,7 @@
 #include <cassert>
 #include <condition_variable>
 #include <exception>
+#include <limits>
 #include <optional>
 #include <stdexcept>
 #include <thread>
@@ -584,9 +585,10 @@ static bool EncryptMasterKey(const SecureString& wallet_passphrase, const CKeyin
         }

         // target_iterations : elapsed_iterations :: target_time : elapsed_time
-        unsigned int target_iterations = updated_master_key.nDeriveIterations * target_time / elapsed_time;
+        double target_iterations{updated_master_key.nDeriveIterations * target_time / elapsed_time};
+        if (target_iterations < 1 || target_iterations > std::numeric_limits<int>::max()) return false;
         // Get the weighted average with previous runs.
-        updated_master_key.nDeriveIterations = (i * updated_master_key.nDeriveIterations + target_iterations) / (i + 1);
+        updated_master_key.nDeriveIterations = (i * updated_master_key.nDeriveIterations + static_cast<unsigned int>(target_iterations)) / (i + 1);
     }

     if (updated_master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) {

</details>

l0rinc approved

l0rinc commented at 6:39 PM on August 3, 2026: contributor

I left a few comments.

Found and created with kimi k3.

AI attribution is unnecessary - authors and reviewers fully bear responsibility for contributions and reviews.

DrahtBot requested review from l0rinc on Aug 3, 2026