crypto: Use secure_allocator for `AES256_ctx` #31774

davidgumberg · 2025-01-31T20:10:56Z

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.

DrahtBot · 2025-01-31T20:10:59Z

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/31774.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	sipa, theStack, furszy

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34004 (Implementation of SwiftSync by rustaceanrob)
#28690 (build: Introduce internal kernel library by sedited)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2025-01-31T21:32:45Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/36501336640}

Hints

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.

theStack · 2025-02-04T15:51:03Z

Concept ACK on clearing out the ctx/iv members

I'm wondering if a minimum-diff fix which simply replaces the memset calls in the dtors with memory_cleanse would be largely sufficient here? In #31166 (comment) one argument for not needing secure allocators was the short-lived nature of the secrets. Looking at the only usage in the wallet, this would imho apply here too (en/decrypting might take a while for larger inputs, but I still wouldn't classify that as long-lived):

bitcoin/src/wallet/crypter.cpp

Lines 85 to 91 in 94ca99a

    
           AES256CBCEncrypt enc(vchKey.data(), vchIV.data(), true); 
        
           size_t nLen = enc.Encrypt(vchPlaintext.data(), vchPlaintext.size(), vchCiphertext.data()); 
        
           if(nLen < vchPlaintext.size()) 
        
               return false; 
        
           vchCiphertext.resize(nLen); 
        
           return true;

bitcoin/src/wallet/crypter.cpp

Lines 102 to 108 in 94ca99a

    
           AES256CBCDecrypt dec(vchKey.data(), vchIV.data(), true); 
        
           int len = dec.Decrypt(ciphertext.data(), ciphertext.size(), plaintext.data()); 
        
           if (len == 0) { 
        
               return false; 
        
           } 
        
           plaintext.resize(len); 
        
           return true;

The reasoning might be a bit loose as there might be other uses of these classes in the future where the situation is different. Curious about other opinions.

sipa · 2025-02-04T16:35:20Z

I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

It may be better to:

Somehow make the AES256CBC classes members of CCrypter, surviving an individual encryption/decryption (e.g. by adding a reset function that can get called for each encryption/decryption, resetting the CBC state, but letting the key material survive).
Follow @theStack's suggestion of not locking the memory, but just securely wiping it after each operation.

DrahtBot · 2025-02-08T00:50:43Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/36882999841}

Hints

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.

davidgumberg · 2025-02-08T01:49:49Z

I'm wondering if a minimum-diff fix which simply replaces the memset calls in the dtors with memory_cleanse would be largely sufficient here? In #31166 (comment) one argument for not needing secure allocators was the short-lived nature of the secrets.

It seems right to me that this structure is always short lived, but I also felt in #31166 secure_allocator should have been used over just memory_cleanse(). I feel that the alloc/dealloc strategy for secrets in memory should be reused and applied universally unless there is a good reason not to.

I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

I added a benchmark for WalletEncrypt() which I ran a few times and it appears that the performance impact is neglible on my machine (Ryzen 7900x, Fedora 40), if the benchmark I wrote is actually representative:

wallet type	branch	EncryptWallet() (ns/key)	benchmark overhead (ns/key)	normalized value (total - overhead)	flamegraph
Descriptor	`15d8500`	42,106.55	36,235.03	5,871.53	link
Descriptor	master	42,050.82	36,362.40	5,688.42	link
Legacy	`15d8500`	17,067.82	2,779.16	14,288.6	link
Legacy	master	16,812.63	2,757.26	14,055.57	link

But if the short-lived nature of the key material makes just using memory_cleanse() a good-enough solution over having to reason about whether the benchmark is sufficiently correct, and whether the larger / more complicated diff required to reuse secure_allocator is worth the review effort / risk, I'm happy to change it.

laanwj · 2025-03-27T13:27:45Z

I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.

This may still be true, though FWIW the idea behind the LockedPool is that it reduces the amount of locking/unlocking operations by mapping and locking memory in blocks, not every time it's requested.

But agree that for short-lived key material, it's less important, there is little to no chance of it ending up in swap anyway.

DrahtBot · 2025-05-01T05:42:13Z

🚧 At least one of the CI tasks failed.
_{Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/41468173464}
_{LLM reason (✨ experimental): The CI failure is caused by an assertion failure in wallet creation during the bench_sanity_check test.}

Hints

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.

davidgumberg · 2025-05-01T05:44:18Z

Rebased to address merge conflict, dropped legacy wallet encryption benchmark, and reduced number of keys since the benchmark was taking an unreasonably long time.

sipa

utACK 591764f6170d6f74d7eebb5fec1cbf5b912098a4, just coding style nits

src/wallet/wallet.cpp

src/bench/wallet_encrypt.cpp

DrahtBot · 2025-05-06T04:03:17Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/41695411207}
_{LLM reason (✨ experimental): The CI failure is due to issues identified by lint-includes.py and the all_python_linters check.}

Hints

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.

davidgumberg · 2025-05-06T04:26:42Z

Refactored to address @furszy feedback to avoid adding a test-only parameter to CWallet, and added a somewhat opinionated refactor (7176b26) of CWallet::EncryptMasterKey() since I was touching the iteration measuring stuff anyways.

I don't like the idea of using a for loop that iterates once with 0 and once with 1, but it's shorter, I believe it is easier to understand how the weighted average calculation is working, and this version generalizes to any number of measurement runs.

sipa

utACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd

theStack

Code-review ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd

furszy

ACK 8bdcd12d3bcbeaf922fa10dc2a261848e3900cfd

No need to tackle the nano nits I left.

src/wallet/wallet.cpp

furszy · 2025-11-10T13:38:41Z

src/wallet/wallet.cpp

    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.


In 7176b26:
Pedantic ultra-nano nit:
We finish comments with a dot only if they span more than one line.

Really? I couldn't find this in doc/ and I find a lot of counter-examples when doing:

git grep -n -C 2 "// .*\." '*.cpp' '*.h'

furszy · 2025-11-10T13:47:33Z

src/wallet/wallet.cpp

-    master_key.nDeriveIterations = (master_key.nDeriveIterations + static_cast<unsigned int>(master_key.nDeriveIterations * target / (SteadyClock::now() - start))) / 2;
+        // 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.


In 7176b26:
nano nit: you are already mentioning this above the for-loop line.

Fair enough, I think this is adding a little more information than the comment above the for loop, but I'll remove when/if retouching.

achow101 · 2025-11-10T16:46:43Z

src/wallet/wallet.cpp

-    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;
+        // target_iterations : elapsed_iterations :: target_time : elapsed_time


In 7176b26cde7cbaffdd92af9c25f85f8e5233e78a "refactor: Generalize derivation target calculation"

This comment does not seem helpful, I'm not sure what information it is trying to convey.

https://en.wikipedia.org/wiki/Ratio#Notation_and_terminology (archive link)

This comment explains why the assignment below is correct. This can be read as "target_iterations is to elapsed_iterations as target_time is to elapsed_time", this is mostly equivalent to: $\frac{\text{targetiterations}}{\text{elapsediterations}} = \frac{\text{targettime}}{\text{elapsedtime}}$

src/bench/wallet_encrypt.cpp

achow101 · 2025-11-10T16:54:24Z

src/bench/wallet_encrypt.cpp

+        // Skip actually encrypting wallet on the overhead measuring run, so we
+        // can subtract the overhead from the full benchmark results to get
+        // encryption performance.
+        if (!measure_overhead) {


In 26442fec572da263765c576c080186c84cee1615 "bench: Add wallet encryption benchmark"

We've generally not had "measure overhead" in the benchmarks. If this is because encryption is actually a very small amount of time, I would suggest generating significantly more keys that need to actually be encrypted.

Unfortunately, even after generating more keys, there is still significant overhead, and this overhead is not fixed, it scales with the number of keys...

Even though total - overhead isn't something we've measured in any existing benchmarks, it's probably a good idea for some benchmarks. It won't make a difference for catching extreme regressions, but for smaller performance regressions / improvements, the setup overhead in each run adds noise that makes changes harder to measure. Ideally there would be some way to achieve this with nanobench, but for now this hasn't been implemented and the maintainer of nanobench has suggested this approach: martinus/nanobench#86 (comment)

glozow · 2025-12-09T15:53:45Z

IIRC we're waiting on @davidgumberg to address the comments from @achow101 's reviews #31774 (comment) here?

DrahtBot · 2025-12-09T22:36:10Z

🚧 At least one of the CI tasks failed.
_{Task Windows-cross to x86_64, msvcrt: https://github.com/bitcoin/bitcoin/actions/runs/20080403030/job/57605821707}
_{LLM reason (✨ experimental): Linker failure from multiple definitions in lockedpool.cpp (duplicate symbols across modules).}

Hints

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.

Adds a special case where if the elapsed time during measurement of DKF performance is 0, the default derive iterations are used so that behavior is stable for testing and benchmarks.

Allows `crypto` functions and classes to use `secure_allocator`.

davidgumberg · 2025-12-10T00:12:20Z

git range-diff 68ac9f1..8bdcd12 9890058..e54690a

Rebased against master to fix a silent merge conflict with lockedpool.cpp being moved from util -> kernel in aed38ea, and addressed @achow101's comment about the previous benchmark only measuring encryption of a single key, I've updated the benchmarks to test encryption of 2000 keys, the performance impact is still negligible:

branch	EncryptWallet (total - overhead)	total ns/key	overhead ns/key
master + benchmarks (`8abee2b`)	192,652.62 ns	293,553.20 ns	104,682.68 ns
branch (`e54690a`)	192,772.32 ns	294,794.46 ns	104,329.09 ns

Benchmark details

To set up, I cleaned my build dir and used pyperf. ```bash rm -rf ./build pip install pyperf && sudo pyperf system tune ```

Master + benchmarks (`8abee2b`)

git checkout 8abee2b
cmake -B build -DBUILD_BENCH=ON && cmake --build build -j $(nproc)
./build/bin/bench_bitcoin --filter="WalletEncrypt.*" -min-time=12000

ns/key	key/s	err%	ins/key	cyc/key	IPC	bra/key	miss%	total	benchmark
293,553.20	3,406.54	0.1%	6,848,134.51	1,258,326.47	5.442	151,429.70	0.9%	12.91	`WalletEncryptDescriptors`
104,682.68	9,552.68	0.1%	2,329,994.28	448,813.08	5.191	74,084.65	0.9%	13.02	`WalletEncryptDescriptorsBenchOverhead`

Branch (e54690a)

git checkout e54690a
cmake -B build -DBUILD_BENCH=ON && cmake --build build -j $(nproc)
./build/bin/bench_bitcoin --filter="WalletEncrypt.*" -min-time=12000

ns/key	key/s	err%	ins/key	cyc/key	IPC	bra/key	miss%	total	benchmark
294,794.46	3,392.19	0.3%	6,855,246.26	1,264,321.32	5.422	153,125.52	0.9%	12.97	`WalletEncryptDescriptors`
104,329.09	9,585.05	0.1%	2,330,778.18	447,414.99	5.209	74,203.06	0.8%	12.94	`WalletEncryptDescriptorsBenchOverhead`

DrahtBot added the Utils/log/libs label Jan 31, 2025

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch from ab21aa1 to 68f1370 Compare January 31, 2025 21:32

DrahtBot added the CI failed label Jan 31, 2025

davidgumberg mentioned this pull request Jan 31, 2025

crypto: secure erase memory #31744

Open

1 task

DrahtBot removed the CI failed label Jan 31, 2025

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch 2 times, most recently from 046c5e4 to a3be398 Compare February 8, 2025 00:50

DrahtBot added the CI failed label Feb 8, 2025

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch 2 times, most recently from fda2ec1 to 15d8500 Compare February 8, 2025 01:05

DrahtBot removed the CI failed label Feb 8, 2025

DrahtBot mentioned this pull request Feb 8, 2025

wallet: refactor: various master key encryption cleanups #31398

Merged

DrahtBot added the Needs rebase label Apr 30, 2025

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch from 15d8500 to 95d266f Compare May 1, 2025 04:42

DrahtBot added CI failed and removed Needs rebase labels May 1, 2025

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch from 95d266f to 591764f Compare May 1, 2025 05:43

DrahtBot removed the CI failed label May 1, 2025

sipa reviewed May 1, 2025

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/bench/wallet_encrypt.cpp Outdated Show resolved Hide resolved

DrahtBot requested a review from theStack May 1, 2025 12:56

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch from 591764f to 3d7d2c3 Compare May 2, 2025 01:15

DrahtBot added the CI failed label May 6, 2025

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch from 90528e7 to 8bdcd12 Compare May 6, 2025 04:13

DrahtBot removed the CI failed label May 6, 2025

DrahtBot added CI failed and removed CI failed labels May 16, 2025

achow101 self-requested a review October 22, 2025 14:45

sipa reviewed Nov 3, 2025

View reviewed changes

DrahtBot requested a review from theStack November 3, 2025 18:22

theStack approved these changes Nov 4, 2025

View reviewed changes

furszy reviewed Nov 10, 2025

View reviewed changes

achow101 reviewed Nov 10, 2025

View reviewed changes

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch 2 times, most recently from 07497f0 to b8048b1 Compare December 9, 2025 22:35

DrahtBot added the CI failed label Dec 9, 2025

davidgumberg added 6 commits December 9, 2025 14:43

refactor: Generalize derivation target calculation

e35f7b8

wallet: Make encryption derivation clock mockable

2c1a1bb

Adds a special case where if the elapsed time during measurement of DKF performance is 0, the default derive iterations are used so that behavior is stable for testing and benchmarks.

bench: Add wallet encryption benchmark

8abee2b

build: lockedpool.cpp kernel -> crypto

952e3ad

Allows `crypto` functions and classes to use `secure_allocator`.

crypto: Use secure_allocator for AES256_ctx

b1da85a

crypto: Use secure_allocator for AES256CBC*::iv

e54690a

davidgumberg force-pushed the 1-31-25-aes-secure-alloc branch from b8048b1 to e54690a Compare December 9, 2025 23:49

DrahtBot removed the CI failed label Dec 10, 2025

This was referenced Dec 10, 2025

Implementation of SwiftSync #34004

Draft

build: Introduce internal kernel library #28690

Open

sedited requested a review from sipa January 13, 2026 21:57

crypto: Use secure_allocator for AES256_ctx #31774

Are you sure you want to change the base?

crypto: Use secure_allocator for AES256_ctx #31774

Conversation

davidgumberg commented Jan 31, 2025

Uh oh!

DrahtBot commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Jan 31, 2025

Uh oh!

theStack commented Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Feb 4, 2025

Uh oh!

DrahtBot commented Feb 8, 2025

Uh oh!

davidgumberg commented Feb 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 1, 2025

Uh oh!

davidgumberg commented May 1, 2025

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented May 6, 2025

Uh oh!

davidgumberg commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

davidgumberg Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

davidgumberg Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

achow101 Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

davidgumberg Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

davidgumberg Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow commented Dec 9, 2025

Uh oh!

crypto: Use secure_allocator for `AES256_ctx` #31774

crypto: Use secure_allocator for `AES256_ctx` #31774

DrahtBot commented Jan 31, 2025 •

edited

Loading

theStack commented Feb 4, 2025 •

edited

Loading

davidgumberg commented Feb 8, 2025 •

edited

Loading

laanwj commented Mar 27, 2025 •

edited

Loading

davidgumberg commented May 6, 2025 •

edited

Loading

davidgumberg Nov 10, 2025 •

edited

Loading

davidgumberg Dec 10, 2025 •

edited

Loading

Master + benchmarks (`8abee2b`)