From bf31d42486ddea9f2d9d2b55621a2e016c0411bb Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 1 Sep 2026 23:36:43 +0200 Subject: [PATCH 1/7] frost: fix stack buffer overflow in frost_nonce_test frost_nonce_test_internal declares extra_in as 16 bytes and filled it with testrand256, which writes 32. Three iterations of the loop each overflowed the array by 16 bytes. AddressSanitizer is unambiguous about it: ERROR: AddressSanitizer: stack-buffer-overflow WRITE of size 1 ... in testrand256 src/testrand_impl.h:90 #1 frost_nonce_test_internal src/modules/frost/tests_impl.h:533 [4336, 4352) 'extra_in' (line 510) <== Memory access at offset 4352 overflows this variable The plain test suite passes regardless, which is why this survived: under the current stack layout the 16 stray bytes land on secshare, which is re-derived into pubshare_tmp immediately afterwards, so the two stay consistent and nothing downstream notices. That is luck, not correctness -- the target is whatever the compiler happens to place next, and msglen, extra_in_len or i are equally plausible. This should have been failing CI already: the sanitizers_debian job in .github/workflows/ci.yml sets FROST: 'yes' alongside CFLAGS: '-fsanitize=undefined,address -g'. Use testrand_bytes_test with an explicit length, matching how the musig tests fill their own extra_input (src/modules/musig/tests_impl.h:578). extra_in_len is already drawn from testrand_int(sizeof(extra_in) + 1), so the buffer stays 16 bytes and the value range is unchanged. Scanned all three new test files for other fixed-width writers aimed at undersized arrays; this was the only one. With the fix, the frost, chilldkg and iceberg suites run clean under -fsanitize=address,undefined. Co-Authored-By: Claude Opus 5 --- src/modules/frost/tests_impl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/modules/frost/tests_impl.h b/src/modules/frost/tests_impl.h index 16a7beb2..776f96b9 100644 --- a/src/modules/frost/tests_impl.h +++ b/src/modules/frost/tests_impl.h @@ -530,7 +530,7 @@ static void frost_nonce_test_internal(void) { msglen = testrand_int(sizeof(msg) + 1); testrand256(msg); extra_in_len = testrand_int(sizeof(extra_in) + 1); - testrand256(extra_in); + testrand_bytes_test(extra_in, sizeof(extra_in)); if (testrand_bits(1)) { secp256k1_pubkey pubshare_tmp; CHECK(secp256k1_ec_pubkey_create(CTX, &pubshare_tmp, secshare) == 1); From 34fa8e0b2e9c83b2d6f793238f390b5c198731e6 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 1 Sep 2026 23:36:54 +0200 Subject: [PATCH 2/7] frost: zero the trusted dealer outputs when key generation fails secp256k1_frost_trusted_dealer_keygen clears secshares32, thresh_pk and pubshares up front, under a comment promising that "the outputs are unusable if this function fails". The per-participant loop then fills them in one participant at a time, and every failure after that point leaves the shares written so far in the caller's buffer while returning 0. The reachable failure is the zero-share check inside the loop, which has negligible probability, so this is hygiene rather than a live leak. It still contradicts the stated contract, and the buffer it leaves populated holds real secret shares for participants 0..i-1 of a setup the caller has been told to discard. Re-zero all three outputs on the failure path. secshares32 goes through secp256k1_memzero_explicit rather than memset because it is secret and the buffer is dead afterwards. n_participants is validated before any path that can reach the cleanup label, so the lengths are safe to use there. Co-Authored-By: Claude Opus 5 --- src/modules/frost/keygen_impl.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/modules/frost/keygen_impl.h b/src/modules/frost/keygen_impl.h index 65954dd1..f83a8ebc 100644 --- a/src/modules/frost/keygen_impl.h +++ b/src/modules/frost/keygen_impl.h @@ -293,6 +293,14 @@ int secp256k1_frost_trusted_dealer_keygen(const secp256k1_context *ctx, unsigned ret = 1; cleanup: + if (!ret) { + /* The loop above may have written real secret shares for the first + * few participants before failing. Zero the outputs again so that a + * failed call leaves nothing usable behind, as promised above. */ + secp256k1_memzero_explicit(secshares32, n_participants * 32); + memset(thresh_pk, 0, sizeof(*thresh_pk)); + memset(pubshares, 0, n_participants * sizeof(*pubshares)); + } secp256k1_scalar_clear(&secret); secp256k1_scalar_clear(&share); secp256k1_scalar_clear(&x); From 3765a8288615b764d096eebe17a3f531f7ec97e2 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 1 Sep 2026 23:37:22 +0200 Subject: [PATCH 3/7] frost: document deterministic_sign's nonce derivation domain BIP 445's det_nonce_hash commits to the secret share, my_id, u, the sorted ids, the aggothernonce, the x-only tweaked threshold public key and the message. It does not commit to the pubshares, to the untweaked threshold public key, or to the accumulated tweaks. Because Q and -Q share an x-coordinate, two tweak caches can agree on everything the derivation hashes and still disagree on the sign g*gacc that multiplies the secret share -- a cache initialized from thresh_pk and one initialized from its negation being the smallest example. Two calls differing only in that emit the same pubnonce and partial signatures s = k + e*lambda*d and s' = k - e*lambda*d over the identical k = k1 + b*k2, so subtracting them yields the secret share. Demonstrated on a sole signer (u = 1, ids = {0}, pubshares = NULL) with thresh_sk = 0x11.. and msg = 0x42..: tweaked pk (cache A) 4f355bdc...075871aa tweaked pk (cache B) 4f355bdc...075871aa same x-only key pubnonce A == pubnonce B nonce reused sA - sB 0d7d9c4e...aa748ffa -2*e*d 0d7d9c4e...aa748ffa d recovered Nothing inside a single call can catch this. The self-verification that Sign performs passes in both cases, because each partial signature is individually valid under the cache it was produced with; validate_session_params likewise only ties the pubshares to the cache's own Q0, which both caches satisfy by construction. Note also that pubshares is optional, so there need not be a second value to disagree with. No code change: this is the specified derivation, and committing to Q0 or to gacc here would diverge from BIP 445 and invalidate the det_sign test vectors. The obligation is the caller's, so state it where the caller will meet it -- in the function's own documentation and alongside the existing secnonce and session_secrand32 rules in frost.md. The rule is that the tweak cache and the pubshares are fixed key material settled at key generation, never per-session parameters taken from a coordinator or a peer; under that discipline a repeated call is byte-identical and harmless, which is the point of a deterministic nonce. Worth raising against the BIP: the spec could close this by hashing the untweaked threshold public key, at the cost of new test vectors. Co-Authored-By: Claude Opus 5 --- include/secp256k1_frost.h | 19 +++++++++++++++++++ src/modules/frost/frost.md | 23 +++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/include/secp256k1_frost.h b/include/secp256k1_frost.h index c4c7c18a..5f05f713 100644 --- a/include/secp256k1_frost.h +++ b/include/secp256k1_frost.h @@ -520,6 +520,25 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_sign( * through NonceAgg as a pubnonce contribution, and a pubnonce's components * are never the point at infinity); if it does, this function fails. * + * WARNING: the derivation above is the whole of what the nonce depends on. It + * does NOT commit to the pubshares, to the untweaked threshold public key, or + * to which tweaks the cache accumulated -- only to the x-only encoding of the + * _tweaked_ threshold public key (this is BIP 445's det_nonce_hash, not a + * deviation). Two tweak caches can therefore agree on that x-only key and + * still disagree on the sign g*gacc that multiplies the secret share, because + * Q and -Q have the same x-coordinate: a cache initialized from the threshold + * public key and one initialized from its negation are the simplest example. + * Two calls that differ only in that way emit the SAME pubnonce and two + * partial signatures that differ only in the sign of the secret-share term, + * which is two equations in the nonce and the secret share -- the secret + * share falls out of the pair. + * + * The caller must therefore treat the tweak cache and the pubshares as fixed + * key material belonging to the group, established once at key generation, + * and never as per-session parameters accepted from a coordinator or any + * other peer. Given that, repeating a call reproduces a byte-identical result + * and is harmless, which is the point of a deterministic nonce. + * * Returns: 0 if the arguments are invalid or signing fails, 1 otherwise * Args: ctx: pointer to a context object * Out: partial_sig: pointer to a partial_sig object diff --git a/src/modules/frost/frost.md b/src/modules/frost/frost.md index e634c573..7fc31085 100644 --- a/src/modules/frost/frost.md +++ b/src/modules/frost/frost.md @@ -96,6 +96,29 @@ Security notes unique for every call to `secp256k1_frost_nonce_gen`. Passing the secret share to `nonce_gen` is recommended as defense-in-depth against bad randomness. +- `secp256k1_frost_deterministic_sign` has no `session_secrand32` to keep + fresh; its safety rests instead on what the nonce derivation commits to. Per + BIP 445's `det_nonce_hash` that is the secret share, `my_id`, `u`, the sorted + ids, the aggothernonce, the **x-only** tweaked threshold public key, and the + message — and nothing else. In particular it does not commit to the + pubshares, to the untweaked threshold public key, or to the accumulated + tweaks. Since `Q` and `-Q` share an x-coordinate, a tweak cache initialized + from the threshold public key and one initialized from its negation present + the same x-only key to the derivation while disagreeing on the sign `g*gacc` + that multiplies the secret share. Two calls differing only in that produce + the same pubnonce and partial signatures `s = k + e*lambda*d` and + `s' = k - e*lambda*d`, where `k` is the identical `k1 + b*k2`; subtracting + them yields `d` directly. The same holds for any two caches that agree on + the tweaked x-only key but not on `g*gacc`. + + This is a property of the specified derivation, not of this implementation, + and it is not detectable from inside a single call: the self-verification in + `Sign` passes in both cases, because each signature is individually valid + under its own cache. The caller carries the obligation. Treat the tweak + cache and the pubshares as fixed key material established once at key + generation, and never accept either as a per-session parameter from the + coordinator or another peer. Under that discipline a repeated call is + byte-identical and harmless, which is what the deterministic nonce is for. - Final signatures produced by `secp256k1_frost_partial_sig_agg` are ordinary BIP340 signatures; they are verified with `secp256k1_schnorrsig_verify` against the (tweaked) x-only threshold public key. From c9952bd10acdce3220f4b4ac3f4f2340de87973c Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 1 Sep 2026 23:37:35 +0200 Subject: [PATCH 4/7] 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); From 7afee05de877cadeddf22e81490891b15fab8f2b Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Tue, 1 Sep 2026 23:37:52 +0200 Subject: [PATCH 5/7] tools: add the iceberg test vector generator src/modules/iceberg/vectors.h opens with regeneration instructions that name ./tools/test_vectors_iceberg_generate.py, and that script was never committed. The same header calls the file "the one thing in this repository that cannot be rebuilt from what the repository contains", which the missing generator made true twice over: neither the reference nor the tool that reads it was reachable from a clone. Recovered from the tree the Iceberg C is vendored in, at sources/secp256k1-kmp/native/secp256k1 of the benchmark repository (bitcoin-core/secp256k1 branch iceberg-module, commit 96201552, per that repository's PINS.txt). Confirmed to be the generator that produced the checked-in vectors before committing it: - its HEADER template reproduces the header of vectors.h exactly, including the ICEBERG_VECTOR_MAX_PARTICIPANTS 9 and ICEBERG_VECTOR_MAX_SEEDS 126 it computes from the configuration list; - its seed rule, sha256("iceberg test vectors|