#35688 crypto: accept empty HMAC keys

full analysis

https://github.com/bitcoin/bitcoin/pull/35688 · l0rinc · +9/-22 in 5 files, 2 commits · labels: Utils/log/libs

Goal

  • Allow cryptographic HMAC functions to accept empty keys without triggering undefined behavior
  • Eliminates UBSan warnings and removes empty-input workarounds across fuzz targets

Replaces memcpy with std::copy in CHMAC_SHA256 and CHMAC_SHA512 so that zero-length HMAC keys do not pass a null pointer to memcpy and trigger UBSan warnings. It adds empty-key test vectors to crypto_tests and removes empty-input workarounds from the crypto and eval_script fuzz targets.

Problem: Calling memcpy with a null pointer even for zero bytes is undefined behavior in C++, triggering UBSan runtime warnings and forcing fuzz targets to add defensive checks against empty byte vectors.

Category: Utilities (logging, arguments, libraries) (#34 of 66)

P3 · bug fix

  • P3 because resolving undefined behavior on empty keys cleans up sanitizer runs
  • Enables fuzz testing to exercise empty keys directly without special-casing

Fixes technical undefined behavior in cryptographic primitives that triggers UBSan when hashing with empty keys. While memcpy(nullptr, ..., 0) rarely misbehaves in practice, fixing it ensures clean sanitizer runs and allows crypto fuzzing to cover empty inputs directly.

Membership: Modifies core cryptographic helper classes CHMAC_SHA256 and CHMAC_SHA512 in src/crypto/.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 1

Reviewability: Stale: Author silent

  • Author has been silent for over 60 days
  • Code is small, rebased, and CI is passing

The author has been inactive for 69 days, exceeding the project's 60-day silence threshold. The code itself is small, rebased, and CI is passing.

Author status: silent since 2026-07-09 after addressing review feedback

Resolved concerns:

  • maflcko suggested using std::copy, keeping types consistent, and cleaning up commit descriptions and references to unrelated PRs; l0rinc adopted all suggestions.

Agreement: Positive

  • General support after author resolved all implementation and cleanup comments
  • Suggested using standard copy and cleaning up commits (maflcko)
  • Approved the updated commits (sedited)

Positive; author addressed suggestions on std::copy from maflcko and received an ACK from sedited

All feedback on approach and commit structure was addressed by the author, and sedited subsequently approved the PR.

  • maflcko recommended std::copy and asked to shorten commit messages and split the eval_script cleanup
  • l0rinc implemented the suggestions and force-pushed
  • sedited approved the latest commits

Review verdicts (DrahtBot): 1

Files

25 lines under test/bench/ci.

  • src/test/fuzz/eval_script.cpp +1/-10
  • src/test/fuzz/crypto.cpp +0/-10
  • src/test/crypto_tests.cpp +4/-0
  • src/crypto/hmac_sha256.cpp +2/-1
  • src/crypto/hmac_sha512.cpp +2/-1

Card

This PR replaces memcpy with std::copy in CHMAC_SHA256 and CHMAC_SHA512 to avoid undefined behavior and UBSan null-pointer warnings when initializing HMAC with empty keys. It adds empty-key HMAC test vectors and removes unnecessary empty-input guards in the crypto and eval_script fuzz targets. The change improves the correctness of cryptographic primitives under sanitizers and enables cleaner fuzz coverage of edge cases. The code has an approval from sedited after addressing suggestions from maflcko, though the author has been inactive for over 60 days.

Data

dossier JSON · extract JSON · model openrouter/google/gemini-3.8-flash, generated 2026-09-17T15:54, confidence high, input hash 40c82492ba16725e