Merge branch 'review-fixes'

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);

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.

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);

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);