crypto: accept empty HMAC keys #35688

l0rinc commented at 10:46 PM on July 8, 2026: contributor

Problem: CHMAC_SHA256 and CHMAC_SHA512 use memcpy to copy key bytes. Empty vectors can provide a null data() pointer, which triggers a UBSan null-pointer warning even for zero-length copies and forces the crypto fuzz target to avoid empty inputs.

Fix: Use std::copy, which has defined semantics for empty ranges, add empty-key HMAC test vectors, and let the crypto fuzz target pass empty byte vectors directly. Remove the stale eval_script empty-input guard in a separate commit because ConsumeRemainingBytes() already returns before copying when no bytes remain.

DrahtBot added the label Utils/log/libs on Jul 8, 2026

DrahtBot commented at 10:46 PM on July 8, 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/35688.

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

DrahtBot added the label CI failed on Jul 8, 2026

l0rinc closed this on Jul 8, 2026

l0rinc reopened this on Jul 8, 2026

DrahtBot removed the label CI failed on Jul 9, 2026

in src/crypto/hmac_sha256.cpp:16 in ac1ddae4f6

  12 | @@ -13,7 +13,7 @@ CHMAC_SHA256::CHMAC_SHA256(const unsigned char* key, size_t keylen)
  13 |  {
  14 |      unsigned char rkey[64];
  15 |      if (keylen <= 64) {
  16 | -        memcpy(rkey, key, keylen);
  17 | +        if (keylen) memcpy(rkey, key, keylen);

maflcko commented at 8:56 AM on July 9, 2026:

Why not use std::copy? It has several benefits:

The args are checked for types and any casts are explicit (the src/span.h header exists, if any casts are really needed. Though, I don't think they are here, because it compiles fine for me without them?)
The code won't have to be touched again if C29 is applied to C++ (in the future, or retroactively)
I haven't confirmed this exhaustively, but I presume compilers will create the same output with std::copy anyway? At least, it would be odd if the performance was worse?
copy has defined semantics for empty source ranges in all versions of C++.

l0rinc commented at 4:15 PM on July 9, 2026:

It claims that #35010 is related to null-annotations

It's where @ryanofsky suggested std::copy: #35010 (comment)

Why not use std::copy

I presume compilers will create the same output with std::copy anyway

That's why, besides the godbolt showing the behavior difference between the two (which may or may not apply here, just seemed simpler to reason about it as such)

https://godbolt.org/z/jMPP8EhEr, which also has a type-check includes. However, the types here are the same: both are uint8_t for src and dst

Not sure what you mean, the third pane uses -DBYTE_SOURCE -DUSE_STD_COPY -std=c++20 -fsanitize=undefined -fno-sanitize-recover=undefined which fails with: 'cannot convert 'std::byte' to 'unsigned char' in assignment'

It mixes in the eval_script fuzz target change, which could make sense to split up into a separate commit

Sure, I can do that

copy has defined semantics for empty source ranges in all versions of C++

In the examples I found the empty case was always mentioned as an exception, so the failures in godbolt were used as source of truth

maflcko commented at 4:40 PM on July 9, 2026:

However, the types here are the same: both are uint8_t for src and dst

Not sure what you mean

I just mean that the type of key and rkey is the same: uint8_t. So the conversion issue seems unrelated, but not important.

copy has defined semantics for empty source ranges in all versions of C++

In the examples I found the empty case was always mentioned as an exception, so the failures in godbolt were used as source of truth

I am pretty sure those examples claiming that std::copy can't handle empty data are wrong. The stdlib copy really should be fine, I've used it in the past to fix this issue: fa4b52bd16189d40761c5976b8427e30779aba23. See also https://en.cppreference.com/cpp/algorithm/copy and the C++ spec. Up to you, feel free to ignore.

maflcko commented at 6:12 PM on July 9, 2026:

It claims that #35010 is related to null-annotations

It's where @ryanofsky suggested std::copy: #35010 (comment)

Yes, but I don't think they are related.

There is one issue about type-data.h (https://github.com/bitcoin/bitcoin/pull/35118#issuecomment-4859660038)

and pull #35010 is some ai slop in a different file: src/ipc/capnp/common-types.h.

Let's try to keep separate topics in separate threads. In any case, both are unrelated to this pull request and should not be in the commit message here?

l0rinc commented at 7:48 PM on July 9, 2026:

Thanks, @maflcko, I switched both HMAC copies to std::copy, removed the unrelated references, and shortened the commit messages and PR description. I also added a Godbolt comparison (https://godbolt.org/z/fzsP1jf17) confirming identical assembly for the fixed 64/128-byte copies with current GCC and Clang at both -O2 and -O3, and split the eval_script cleanup into a separate commit.

maflcko commented at 9:59 AM on July 9, 2026: member

Seems fine to change this, but at least for me it is really hard to follow the commit description, as it seems to mix several different broad topics:

It claims that #35010 is related to null-annotations where both the src/dst are zero-size. This is not true, because #35010 is about size mismatches, where the dst is too small to fit the src.
It claims that #35010 is related to the discussion (and was picked up in the discussion) in #35118 (review), however I fail to see the link.
Then it links to https://godbolt.org/z/jMPP8EhEr, which also has a type-check includes. However, the types here are the same: both are uint8_t for src and dst.
It mixes in the eval_script fuzz target change, which could make sense to split up into a separate commit.

Again, seems fine to change, but the commit message can be a bit shorter and basically just say something like:

crypto: accept empty HMAC keys

This avoids an Ubsan nullptr warning on memcpy, when passing empty HMAC keys.

With the warning gone, workarounds in the tests can be removed.

Though, feel free to ignore/adjust. I think that commit messages and pull descriptions should be clean and concise, and opinions and extended discussions are better left to GH review or issue comments.

crypto: accept empty HMAC keys

Replace `memcpy` with `std::copy` so empty HMAC keys do not trigger a UBSan null-pointer warning.
Add empty-key test vectors and let the crypto fuzz target exercise empty byte vectors directly.

Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

b80907909c

fuzz: remove stale eval_script empty-input guard

`ConsumeRemainingBytes()` already returns before copying when no bytes remain, so call it directly.

dc67c4cc39

l0rinc force-pushed on Jul 9, 2026