crypto: cleanse HMAC stack buffers after use and ChainCode #35254

thomasbuilds commented at 11:04 AM on May 10, 2026: contributor

CHMAC_SHA256 and CHMAC_SHA512 leave two stack buffers populated on return: rkey[] holds K' ⊕ ipad after the constructor, and temp[] holds the inner-hash output after Finalize().

When the HMAC is keyed with sensitive material (chain code in BIP32Hash() in hash.cpp for BIP32 child key derivation; PRK in HKDF-Expand in hkdf_sha256_32.cpp, used for BIP324 transport keying), rkey is one constant XOR from that key, and temp is a one-way digest covering it.

This PR cleanses both buffers with memory_cleanse(), matching the convention already used in chacha20.cpp and chacha20poly1305.cpp. No observable change for callers.

Update: Cleansing the HMAC primitive's internal buffers still leaves a caller's ChainCode value populated in memory after use. The second commit promotes ChainCode from typedef uint256 to a base_blob<256> subclass with a memory_cleanse() destructor, so chain codes in CExtKey, CExtPubKey, and local variables are cleansed on scope exit. MUSIG_CHAINCODE is retyped from constexpr uint256 to const ChainCode to match its BIP328 semantic role; this also removes the GCC-14 consteval lambda workaround.

DrahtBot added the label Utils/log/libs on May 10, 2026

DrahtBot commented at 11:04 AM on May 10, 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/35254.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg
Concept ACK	sedited
Approach ACK	winterrdog
Stale ACK	optout21

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

crypto: cleanse HMAC stack buffers after use

CHMAC_SHA256 and CHMAC_SHA512 leave two stack buffers populated on
return: rkey[] holds K' XOR ipad after the constructor, and temp[]
holds the inner-hash output after Finalize().

When the HMAC is keyed with sensitive material (chain code in
BIP32Hash() in hash.cpp for BIP32 child key derivation; PRK in
HKDF-Expand in hkdf_sha256_32.cpp, used for BIP324 transport keying),
rkey is one constant XOR from that key, and temp is a one-way digest
covering it.

Cleanse both buffers with memory_cleanse(), matching the convention
in chacha20.cpp and chacha20poly1305.cpp. No observable change for
callers.

b3a3f88346

thomasbuilds force-pushed on May 10, 2026

sedited requested review from davidgumberg on May 10, 2026

optout21 commented at 11:38 AM on May 11, 2026: contributor

ConceptACK b3a3f88346dfd218a049acec6a77166f319c70e8

Seems like the right thing to do, done correctly.

While not trivial, it's possible to write a test for it (by calling a method with similar signature and layout of local variables, it's possible to read out the content of the local buffer, after the call). Do you think a test would be worthwhile here?

thomasbuilds commented at 1:06 PM on May 11, 2026: contributor

Thanks for the review. For the test I disagree, chacha20.cpp and chacha20poly1305.cpp use the same memory_cleanse pattern without equivalent coverage, not to mention the test would be fragile since stack layout isn't specified across our CI matrix.

optout21 commented at 1:30 PM on May 11, 2026: contributor

For the test I disagree, chacha20.cpp and chacha20poly1305.cpp use the same memory_cleanse pattern without equivalent coverage, not to mention the test would be fragile since stack layout isn't specified across our CI matrix.

I can accept that a test would be an overkill, and maybe not the best way to ensure no data is left on the stack. I've asked because it's an interesting challenge to write such a test. I think it would be fragile because the test would have to replicate the stack layout of the method-under-test, which may change. The variations in CI environments is less relevant for this, but there can be other issues, like implicit memory cleanup in some environments.

key: cleanse ChainCode on destruction

HMAC primitives cleanse their internal stack buffers, but a caller's
ChainCode remains populated in memory after use. Promote ChainCode
from `typedef uint256` to a `base_blob<256>` subclass with a
memory_cleanse() destructor, so chain codes in CExtKey, CExtPubKey,
and local variables are cleansed on scope exit.

Retype MUSIG_CHAINCODE from `constexpr uint256` to `const ChainCode`
to match its BIP328 semantic role. Dropping `constexpr` (ChainCode is
no longer a literal type) also removes the GCC-14 consteval lambda
workaround.

Remove the duplicate typedef in pubkey.h (which includes hash.h
transitively). Two fuzz-test call sites in test/fuzz/key.cpp now
construct the chain-code argument explicitly rather than relying on
the typedef.

21a1380c13

in src/crypto/hmac_sha512.cpp:31 in b3a3f88346 outdated

  26 | @@ -26,11 +27,14 @@ CHMAC_SHA512::CHMAC_SHA512(const unsigned char* key, size_t keylen)
  27 |      for (int n = 0; n < 128; n++)
  28 |          rkey[n] ^= 0x5c ^ 0x36;
  29 |      inner.Write(rkey, 128);
  30 | +
  31 | +    memory_cleanse(rkey, sizeof(rkey));

davidgumberg commented at 11:55 PM on May 11, 2026:

ChainCode is not cleansed in the calling code, maybe a destructor, i.e.:

+class ChainCode : public base_blob<256>  {
+public:
+    constexpr ChainCode() = default;
+    constexpr explicit ChainCode(std::span<const unsigned char> vch) : base_blob<256>(vch) {}
+
+    ~ChainCode() { memory_cleanse(data(), size()); }
+};
+

thomasbuilds commented at 1:10 PM on May 12, 2026:

Worth considering as a follow-up, but I'd keep it out of this PR since the chain code lives on the caller side while this PR only touches HMAC.

Btw the snippet doesn't quite work as-is since ChainCode is typedef uint256 in hash.h and pubkey.h, so replacing it with a class derived from base_blob<256> drops the uint8_t and string_view constructors uint256 provides, and breaks call sites that rely on the typedef. That's a wider refactor which would be another reason it belongs in its own PR. What do you think?

davidgumberg commented at 3:44 PM on May 12, 2026:

replacing it with a class derived from base_blob<256> drops the uint8_t and string_view constructors uint256 provides,

These are not used anywhere

breaks call sites that rely on the typedef.

Can you be more specific?

I tested my diff and there is only a single caller that (erroneously imo!) constructs a ChainCode from a uint256, and that would have to be fixed as well. It is like a 10 line diff, and without it, it helps little to be cleansing the chaincode in crypto code without cleansing it in the caller that invokes the crypto code.

thomasbuilds commented at 6:47 PM on May 12, 2026:

Indeed those constructors aren't used on ChainCode. Should have waited to post until I could actually verify sorry. There are three call sites: musig.cpp:77 + test/fuzz/key.cpp:88 and :278 where random_uint256 is passed as const ChainCode& to Derive.

I'll type MUSIG_CHAINCODE as ChainCode directly instead of uint256, so no cross-type construction is needed at the call site. This requires dropping constexpr since the destructor isn't constexpr but it isn't used in any constant-evaluated context. Also resolves the need for the GCC-14 consteval lambda workaround. Tested on GCC 13.3 and 14.2.

Will push the commit shortly.

sedited commented at 7:40 PM on May 12, 2026: contributor

Concept ACK

thomasbuilds renamed this:
~~crypto: cleanse HMAC stack buffers after use~~
crypto: cleanse HMAC stack buffers after use and ChainCode
on May 13, 2026

davidgumberg commented at 7:32 PM on May 14, 2026: contributor

crACK https://github.com/bitcoin/bitcoin/pull/35254/commits/21a1380c13470d28f53cc0b85d902693365d39d3

Re: the discussion about testing, I'm not sure how deterministic this is, but it seems that taintgrind / secret grind might be useful for writing tests like this: https://github.com/lmrs2/secretgrind/tree/stable-0.1.

Long-term in the project it might make sense to move toward all sensitive material to be allocated using secure_allocator, which does the cleansing but also mlock()'s secrets so that they are never swapped to disk, having more types maybe in the style of SecureString, but I think this approach is good enough as-is.

DrahtBot requested review from optout21 on May 14, 2026

DrahtBot requested review from sedited on May 14, 2026

winterrdog commented at 1:59 PM on May 17, 2026: none

Approach ACK

particularly like the automatic cleansing in the ChainCode destructor & dropping the GCC 14 consteval lambda workaround (was quite confusing), which makes the code intent much clearer now