crypto: cleanse CSHA512 state in its destructor #36550

pull fametrano wants to merge 1 commits into bitcoin:master from fametrano:crypter-cleanse-sha512 changing 2 files +8 −0

fametrano commented at 6:16 AM on October 16, 2026: contributor

CSHA512::Finalize() leaves the digest in the hasher's state and the last block in its buffer, and the class never wipes them. Every CSHA512 outside tests and benchmarks hashes secret data: in CCrypter::BytesToKeySHA512AES the final state holds the key and IV derived from the passphrase, the two hashers in CHMAC_SHA512 hold BIP32 seed and key material, and the RNG hashers in random.cpp hold seed material.

I add a destructor that cleanses both, as ChaCha20 already does, so each hasher is wiped when it goes out of scope instead of by each caller. Following review, this replaces the one-line cleanse in crypter.cpp this PR first proposed. It also makes #36551 unnecessary. A test is impractical, since the object no longer exists once destroyed. I built the tree and ran test_bitcoin and ctest; both pass, and Core's warning flags report nothing new.
DrahtBot added the label Wallet on Oct 16, 2026
DrahtBot commented at 6:17 AM on October 16, 2026: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



External sites
- Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36550
- Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36550.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36550#author-ralph


Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.
in src/wallet/crypter.cpp:38 in 771f4a4536
```
  34 | @@ -35,6 +35,7 @@ int CCrypter::BytesToKeySHA512AES(const std::span<const unsigned char> salt, con
  35 |      memcpy(key, buf, WALLET_CRYPTO_KEY_SIZE);
  36 |      memcpy(iv, buf + WALLET_CRYPTO_KEY_SIZE, WALLET_CRYPTO_IV_SIZE);
  37 |      memory_cleanse(buf, sizeof(buf));
  38 | +    memory_cleanse(&di, sizeof(di));
```
vicjuma commented at 6:40 PM on October 16, 2026:

Any motivation to this other than:

...as src/bip324.cpp does for its hkdf object and as #36537 proposes for the HMAC hashers.

I can understand why it was done for hkdf , but are there any risks posed here? Even if there were, creating a destructor at the class level should be the way to go, imo; otherwise, we will be going into a cleaning spree for every finalized digest

fametrano commented at 8:07 PM on October 16, 2026:

Agreed, done: CSHA512 now cleanses its state and buffer in its destructor, as ChaCha20 does, and the line in crypter.cpp is gone. The risk is small: after the function returns, the key and IV derived from the passphrase stay on the stack until overwritten, where a crash dump or another memory bug could expose them. Every CSHA512 outside tests and benchmarks hashes secret data, so the class is the right place for the wipe. This also covers the RNG hashers, so I'm closing #36551.

crypto: cleanse CSHA512 state in its destructor

Finalize() leaves the digest in the hasher's state, and the last block
in its buffer. Every CSHA512 outside tests and benchmarks hashes secret
data: in BytesToKeySHA512AES the final state is the wallet encryption
key and IV, and the RNG hashers hold seed material.

Cleanse both in a destructor, as ChaCha20 does, so that each hasher is
wiped when it goes out of scope instead of each caller cleansing it.

2c27910f33

fametrano force-pushed on Oct 16, 2026
fametrano renamed this:
~~wallet: cleanse the hasher in BytesToKeySHA512AES~~
crypto: cleanse CSHA512 state in its destructor
on Oct 16, 2026

Contributors

Labels

Linked (view graph)

#36551 random: wipe hashers in MixExtract and Strengthen

crypto: cleanse CSHA512 state in its destructor #36550

External sites

Reviews