crypto: Use secure_allocator for `AES256

davidgumberg commented at 8:10 pm on January 31, 2025: contributor

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 commented at 8:10 pm on January 31, 2025: 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/31774.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31398 (wallet: refactor: various master key encryption cleanups by theStack)

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 added the label Utils/log/libs on Jan 31, 2025

davidgumberg force-pushed on Jan 31, 2025

DrahtBot added the label CI failed on Jan 31, 2025

DrahtBot commented at 9:32 pm on January 31, 2025: contributor

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

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.

DrahtBot removed the label CI failed on Jan 31, 2025

theStack commented at 3:51 pm on February 4, 2025: contributor

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):

https://github.com/bitcoin/bitcoin/blob/94ca99ac51dddbee79d6409ebcc43b1119b0aca9/src/wallet/crypter.cpp#L85-L91 https://github.com/bitcoin/bitcoin/blob/94ca99ac51dddbee79d6409ebcc43b1119b0aca9/src/wallet/crypter.cpp#L102-L108

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 commented at 4:35 pm on February 4, 2025: member

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.

wallet: EncryptWallet forced derivations

Forcing a derivation count is useful for benchmarks, since otherwise
`EncryptWallet` trying to normalize itself interferes with measurement.

b7e6995380

davidgumberg force-pushed on Feb 8, 2025

DrahtBot commented at 0:50 am on February 8, 2025: contributor

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

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.

DrahtBot added the label CI failed on Feb 8, 2025

davidgumberg force-pushed on Feb 8, 2025

bench: Add wallet encryption benchmark 15fa9efd3c

build: Move `lockedpool.cpp` from util -> crypto

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

721c9240fb

crypto: Use `secure_allocator` for `AES256_ctx` 28d15152f5

crypto: Use `secure_allocator` for `AES256CBC*::iv` 15d8500f99

davidgumberg force-pushed on Feb 8, 2025

davidgumberg commented at 1:49 am on February 8, 2025: contributor

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	https://github.com/bitcoin/bitcoin/pull/31774/commits/15d8500f99012422be495b8e85e4e25e6a4419d8	42,106.55	36,235.03	5,871.53	link
Descriptor	master	42,050.82	36,362.40	5,688.42	link
Legacy	https://github.com/bitcoin/bitcoin/pull/31774/commits/15d8500f99012422be495b8e85e4e25e6a4419d8	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.

DrahtBot removed the label CI failed on Feb 8, 2025

crypto: Use secure_allocator for AES256_ctx #31774

Code Coverage & Benchmarks

Reviews

Conflicts

crypto: Use secure_allocator for `AES256_ctx` #31774