From c9952bd10acdce3220f4b4ac3f4f2340de87973c Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 1 Sep 2026 23:37:35 +0200 Subject: [PATCH] chilldkg: enforce the tag length bounds in noverify builds 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 --- src/modules/chilldkg/util_impl.h | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/modules/chilldkg/util_impl.h b/src/modules/chilldkg/util_impl.h index f10512da..63bfcc4e 100644 --- a/src/modules/chilldkg/util_impl.h +++ b/src/modules/chilldkg/util_impl.h @@ -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);