frost/secp256k1-zkp
3
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

chilldkg: enforce the tag length bounds in noverify builds

Browse Source 
secp256k1_chilldkg_pad33 and secp256k1_chilldkg_schnorrsig_sha256_tagged
each guarded a memcpy into a fixed-size stack buffer with VERIFY_CHECK,
which is compiled out in noverify (release) builds. In pad33 the
consequence is worse than the overflowing copy: the following
memset(out33 + len, 0, 33 - len) underflows its length to a huge value
when len exceeds 33.

Neither is reachable today. Every call site passes a string literal of
this module: "BIP DKG/certeq message" (22) and "BIP DKG/recovery
acknowledgment" (31) for pad33, and at most "BIP DKG/pop message" ||
"/challenge" (29 of 64) for the tagged-hash helper. This is the same
shape as the persisted-state guards promoted in ceccb50a, without the
attacker-controlled input path -- so the change is defence in depth, to
keep a future longer tag from smashing the stack in a release build
rather than failing a debug assertion.

Enforce both bounds outside VERIFY_CHECK and keep VERIFY_CHECK(0) inside
the branch as the debug-build diagnostic, matching the existing idiom in
this module (see the point_load fallbacks in the state loaders).

The tagged-hash helper clamps rather than returning early: an early
return would leave the caller's secp256k1_sha256 uninitialized and every
call site writes into it immediately, which is a worse failure than the
one being fixed. A clamped tag changes every hash the module computes,
so the chilldkg vectors would fail loudly rather than silently.

No behaviour change on any reachable input: the full test suite,
including the chilldkg vectors, is unaffected in both verify and
noverify builds.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
2026-09-01 23:37:35 +02:00
parent 3765a82886
commit c9952bd10a
1 changed files with 22 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
src/modules/chilldkg/util_impl.h
View File

@@ -104,7 +104,14 @@ static int secp256k1_chilldkg_xonly_load(secp256k1_ge *p, const unsigned char *i
static void secp256k1_chilldkg_pad33(unsigned char *out33, const char *str) {
size_t len = strlen(str);
VERIFY_CHECK(len <= 33);
/* Every call site passes a string literal of the module, so this cannot
* trigger. The clamp must not sit inside VERIFY_CHECK, which is compiled
* out in noverify builds: an over-long tag would overflow out33 and make
* the memset length below underflow to a huge value. */
if (len > 33) {
VERIFY_CHECK(0);
len = 33;
}
memcpy(out33, str, len);
memset(out33 + len, 0, 33 - len);
}
@@ -116,7 +123,20 @@ static void secp256k1_chilldkg_schnorrsig_sha256_tagged(const secp256k1_hash_ctx
size_t prefix_len = strlen(tag_prefix);
size_t subtag_len = strlen(subtag);
VERIFY_CHECK(prefix_len + subtag_len <= sizeof(tag));
/* The longest tag the module builds is "BIP DKG/pop message" ||
* "/challenge", 29 bytes. As in secp256k1_chilldkg_pad33, the bound is
* enforced outside VERIFY_CHECK so that a future over-long tag cannot
* overflow tag[] in a noverify build. Clamping rather than returning
* early keeps sha initialized for the caller; a truncated tag changes
* every hash the module computes, so the test vectors fail loudly. */
if (prefix_len > sizeof(tag)) {
VERIFY_CHECK(0);
prefix_len = sizeof(tag);
}
if (subtag_len > sizeof(tag) - prefix_len) {
VERIFY_CHECK(0);
subtag_len = sizeof(tag) - prefix_len;
}
memcpy(tag, tag_prefix, prefix_len);
memcpy(tag + prefix_len, subtag, subtag_len);
secp256k1_sha256_initialize_tagged(hash_ctx, sha, tag, prefix_len + subtag_len);