Merge bitcoin/bitcoin#31774: crypto: Use secure_allocator for AES256_ctx

af0da2fce2146a005f54b900a1fd545a6278173a crypto: Use `secure_allocator` for `AES256CBC*::iv` (David Gumberg)
d53852be316dd2322a7e621a81ee33e1b234c530 crypto: Use `secure_allocator` for `AES256_ctx` (David Gumberg)
8c6fedaa81882be1e2db4a9a2291205d774fa4c6 build: `lockedpool.cpp` kernel -> crypto (David Gumberg)
51ac1abf6fdb6340110397a959681d52e29f73f7 bench: Add wallet encryption benchmark (David Gumberg)
9a158725161f726eb6747811e9e9335eef83f616 wallet: Make encryption derivation clock mockable (David Gumberg)
ae5485fa0d258f060a22c399a4e257eaf96e67b5 refactor: Generalize derivation target calculation (David Gumberg)

Pull request description:

  Fixes #31744

  Reuse `secure_allocator` for `AES256_ctx` in the aes 256 encrypters and decrypters and the `iv` of `AES256CBC` encrypters and decrypters. These classes are relevant to `CCrypter`, used for encrypting wallets, and my understanding is that if an attacker knows some or all of the contents of these data structures (`AES256_ctx` & `iv`) they might be able to decrypt a user's wallet.

   Presently the `secure_allocator` tries to protect sensitive data with `mlock()` on POSIX systems and `VirtualLock()` on Windows to prevent memory being paged to disk, and by zero'ing out memory contents on deallocation with `memory_cleanse()` which is similar to `OPENSSL_cleanse()` by scaring compilers away from optimizing `memset` calls on non-Windows systems, and using `SecureZeroMemory()` on Windows.

ACKs for top commit:
  achow101:
    ACK af0da2fce2146a005f54b900a1fd545a6278173a
  furszy:
    utACK af0da2fce2146a005f54b900a1fd545a6278173a
  theStack:
    re-ACK af0da2fce2146a005f54b900a1fd545a6278173a

Tree-SHA512: 49067934fd2f2b285fc7b1a7c853fd2d4475431b3a811ae511f61074dc71a99a0826c3ab40ab4a5dfc84b2b9914a90c920d2484b38ac19502e3bd6170ad27622

src/bench/CMakeLists.txt

@@ -77,6 +77,7 @@ if(ENABLE_WALLET)
       wallet_balance.cpp
       wallet_create.cpp
       wallet_create_tx.cpp
+      wallet_encrypt.cpp
       wallet_loading.cpp
       wallet_ismine.cpp
       wallet_migration.cpp

src/bench/wallet_encrypt.cpp

@@ -0,0 +1,82 @@
+// Copyright (c) 2025-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <bench/bench.h>
+#include <key_io.h>
+#include <outputtype.h>
+#include <random.h>
+#include <support/allocators/secure.h>
+#include <test/util/setup_common.h>
+#include <util/time.h>
+#include <wallet/context.h>
+#include <wallet/test/util.h>
+#include <wallet/wallet.h>
+#include <wallet/walletutil.h>
+
+#include <cassert>
+
+namespace wallet {
+static void WalletEncrypt(benchmark::Bench& bench, unsigned int key_count)
+{
+    auto test_setup = MakeNoLogFileContext<TestingSetup>();
+    FastRandomContext rand{/*fDeterministic=*/true};
+
+    auto password{rand.randbytes(20)};
+    SecureString secure_pass{password.begin(), password.end()};
+
+    WalletContext context;
+    context.args = &test_setup->m_args;
+    context.chain = test_setup->m_node.chain.get();
+
+    uint64_t create_flags = WALLET_FLAG_DESCRIPTORS;
+    auto database = CreateMockableWalletDatabase();
+    auto wallet = TestCreateWallet(std::move(database), context, create_flags);
+
+    {
+        LOCK(wallet->cs_wallet);
+        for (unsigned int i = 0; i < key_count; i++) {
+            CKey key = GenerateRandomKey();
+            FlatSigningProvider keys;
+            std::string error;
+            std::vector<std::unique_ptr<Descriptor>> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /*require_checksum=*/false);
+            WalletDescriptor w_desc(std::move(desc.at(0)), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0);
+            Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false));
+        }
+    }
+
+    database = DuplicateMockDatabase(wallet->GetDatabase());
+
+    // reload the wallet for the actual benchmark
+    TestUnloadWallet(std::move(wallet));
+
+    // Setting a mock time is necessary to force default derive iteration count during
+    // wallet encryption.
+    SetMockTime(1);
+
+    // This benchmark has a lot of overhead, this should be good enough to catch
+    // any regressions, but for an accurate measurement of how long wallet
+    // encryption takes, this should be reworked after something like
+    // https://github.com/bitcoin/bitcoin/pull/34208 is merged.
+    bench.batch(key_count).unit("key").run([&] {
+        wallet = TestLoadWallet(std::move(database), context);
+
+        // Save a copy of the db before encrypting
+        database = DuplicateMockDatabase(wallet->GetDatabase());
+
+        wallet->EncryptWallet(secure_pass);
+
+        for (const auto& [_, key] : wallet->mapMasterKeys){
+            assert(key.nDeriveIterations == CMasterKey::DEFAULT_DERIVE_ITERATIONS);
+        }
+
+        TestUnloadWallet(std::move(wallet));
+    });
+}
+
+constexpr unsigned int KEY_COUNT = 2000;
+
+static void WalletEncryptDescriptors(benchmark::Bench& bench) { WalletEncrypt(bench, /*key_count=*/KEY_COUNT); }
+BENCHMARK(WalletEncryptDescriptors);
+
+} // namespace wallet

src/crypto/CMakeLists.txt

@@ -20,6 +20,7 @@ add_library(bitcoin_crypto STATIC EXCLUDE_FROM_ALL
   sha512.cpp
   siphash.cpp
   ../support/cleanse.cpp
+  ../support/lockedpool.cpp
 )
 
 target_link_libraries(bitcoin_crypto

src/crypto/aes.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <crypto/aes.h>
+#include <support/allocators/secure.h>
 
 #include <cstring>
 
@@ -12,32 +13,34 @@ extern "C" {
 
 AES256Encrypt::AES256Encrypt(const unsigned char key[32])
 {
-    AES256_init(&ctx, key);
+    ctx = allocator.allocate(1);
+    AES256_init(ctx, key);
 }
 
 AES256Encrypt::~AES256Encrypt()
 {
-    memset(&ctx, 0, sizeof(ctx));
+    allocator.deallocate(ctx, 1);
 }
 
 void AES256Encrypt::Encrypt(unsigned char ciphertext[16], const unsigned char plaintext[16]) const
 {
-    AES256_encrypt(&ctx, 1, ciphertext, plaintext);
+    AES256_encrypt(ctx, 1, ciphertext, plaintext);
 }
 
 AES256Decrypt::AES256Decrypt(const unsigned char key[32])
 {
-    AES256_init(&ctx, key);
+    ctx = allocator.allocate(1);
+    AES256_init(ctx, key);
 }
 
 AES256Decrypt::~AES256Decrypt()
 {
-    memset(&ctx, 0, sizeof(ctx));
+    allocator.deallocate(ctx, 1);
 }
 
 void AES256Decrypt::Decrypt(unsigned char plaintext[16], const unsigned char ciphertext[16]) const
 {
-    AES256_decrypt(&ctx, 1, plaintext, ciphertext);
+    AES256_decrypt(ctx, 1, plaintext, ciphertext);
 }
 
 
@@ -121,6 +124,7 @@ static int CBCDecrypt(const T& dec, const unsigned char iv[AES_BLOCKSIZE], const
 AES256CBCEncrypt::AES256CBCEncrypt(const unsigned char key[AES256_KEYSIZE], const unsigned char ivIn[AES_BLOCKSIZE], bool padIn)
     : enc(key), pad(padIn)
 {
+    iv = allocator.allocate(AES_BLOCKSIZE);
     memcpy(iv, ivIn, AES_BLOCKSIZE);
 }
 
@@ -131,12 +135,13 @@ int AES256CBCEncrypt::Encrypt(const unsigned char* data, int size, unsigned char
 
 AES256CBCEncrypt::~AES256CBCEncrypt()
 {
-    memset(iv, 0, sizeof(iv));
+    allocator.deallocate(iv, AES_BLOCKSIZE);
 }
 
 AES256CBCDecrypt::AES256CBCDecrypt(const unsigned char key[AES256_KEYSIZE], const unsigned char ivIn[AES_BLOCKSIZE], bool padIn)
     : dec(key), pad(padIn)
 {
+    iv = allocator.allocate(AES_BLOCKSIZE);
     memcpy(iv, ivIn, AES_BLOCKSIZE);
 }
 
@@ -148,5 +153,5 @@ int AES256CBCDecrypt::Decrypt(const unsigned char* data, int size, unsigned char
 
 AES256CBCDecrypt::~AES256CBCDecrypt()
 {
-    memset(iv, 0, sizeof(iv));
+    allocator.deallocate(iv, AES_BLOCKSIZE);
 }

src/crypto/aes.h

@@ -7,6 +7,7 @@
 #ifndef BITCOIN_CRYPTO_AES_H
 #define BITCOIN_CRYPTO_AES_H
 
+#include <support/allocators/secure.h>
 extern "C" {
 #include <crypto/ctaes/ctaes.h>
 }
@@ -18,7 +19,8 @@ static const int AES256_KEYSIZE = 32;
 class AES256Encrypt
 {
 private:
-    AES256_ctx ctx;
+    secure_allocator<AES256_ctx> allocator;
+    AES256_ctx *ctx;
 
 public:
     explicit AES256Encrypt(const unsigned char key[32]);
@@ -30,7 +32,8 @@ public:
 class AES256Decrypt
 {
 private:
-    AES256_ctx ctx;
+    secure_allocator<AES256_ctx> allocator;
+    AES256_ctx *ctx;
 
 public:
     explicit AES256Decrypt(const unsigned char key[32]);
@@ -46,9 +49,10 @@ public:
     int Encrypt(const unsigned char* data, int size, unsigned char* out) const;
 
 private:
+    secure_allocator<unsigned char> allocator;
     const AES256Encrypt enc;
     const bool pad;
-    unsigned char iv[AES_BLOCKSIZE];
+    unsigned char *iv;
 };
 
 class AES256CBCDecrypt
@@ -59,9 +63,10 @@ public:
     int Decrypt(const unsigned char* data, int size, unsigned char* out) const;
 
 private:
+    secure_allocator<unsigned char> allocator;
     const AES256Decrypt dec;
     const bool pad;
-    unsigned char iv[AES_BLOCKSIZE];
+    unsigned char *iv;
 };
 
 #endif // BITCOIN_CRYPTO_AES_H

src/kernel/CMakeLists.txt

@@ -54,7 +54,6 @@ add_library(bitcoinkernel
   ../script/solver.cpp
   ../signet.cpp
   ../streams.cpp
-  ../support/lockedpool.cpp
   ../sync.cpp
   ../txdb.cpp
   ../txgraph.cpp

src/util/CMakeLists.txt

@@ -33,7 +33,6 @@ add_library(bitcoin_util STATIC EXCLUDE_FROM_ALL
   ../random.cpp
   ../randomenv.cpp
   ../streams.cpp
-  ../support/lockedpool.cpp
   ../sync.cpp
 )
 

src/wallet/wallet.cpp

@@ -579,16 +579,26 @@ void CWallet::UpgradeDescriptorCache()
  * 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()};
+    constexpr MillisecondsDouble target_time{100};
     CCrypter crypter;
 
-    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
-    master_key.nDeriveIterations = static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start));
+    // 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);
+        auto elapsed_time{NodeClock::now() - start_time};
 
-    start = SteadyClock::now();
-    crypter.SetKeyFromPassphrase(wallet_passphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod);
-    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
+        if (elapsed_time <= 0s) {
+            // We are probably in a test with a mocked clock.
+            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);
+    }
 
     if (master_key.nDeriveIterations < CMasterKey::DEFAULT_DERIVE_ITERATIONS) {
         master_key.nDeriveIterations = CMasterKey::DEFAULT_DERIVE_ITERATIONS;