From 7afaab53dc0f06bd0ac0aa37311f48607d14c4a4 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Mon, 31 Aug 2026 01:19:28 +0200 Subject: [PATCH] frost: close remaining gaps against the BIP 445 reference Four small divergences from the reference implementation and its API contract, none of which changes any signature: the differential harness (240 signing + 120 deterministic-signing cases against the Python reference) produces byte-identical output before and after. Length prefixes that do not fit ------------------------------- secp256k1_frost_sha256_write_prefixed asserted, via VERIFY_CHECK, that the length fits into its prefix. VERIFY_CHECK compiles away in release builds, so a length that does not fit was silently truncated modulo 2^(8*prefix_size) instead of being rejected, yielding a nonce that does not follow the spec. The reference raises OverflowError instead. Only the 4-byte extra_in prefix of nonce_hash is affected, and only where size_t is wider than 32 bits, so this needs an extra_in of 4 GiB to trigger. It is nevertheless a silent deviation, so write_prefixed now returns 0 without writing anything, and the failure is propagated: secp256k1_frost_nonce_function and secp256k1_frost_det_nonce_function return 0, and secp256k1_frost_nonce_gen returns 0 after wiping session_secrand32 and the nonces. Checking the shifted-out bits (which the loop already computes) rather than comparing extra_in_len against a 32-bit bound avoids a comparison that is always true on 32-bit platforms. The bound is now documented on the extra_in_len parameter. Identifiers equal to UINT32_MAX ------------------------------- BIP 445 derive_interpolating_value accepts every identifier in 0 <= id < 2^32, but secp256k1_frost_ids_are_valid rejected UINT32_MAX because the mapping to the polynomial x-coordinate, id + 1, overflows in uint32_t arithmetic. The +1 is now added in scalar arithmetic, where it cannot overflow, and the identifier restriction is gone. The denominator never needed the +1 at all, since x_j - x_i = (id_j + 1) - (my_id + 1) = id_j - my_id so it is computed directly from the identifiers. This was unreachable through the public API -- validate_session_params already bounds identifiers by n_participants, which is at most SECP256K1_FROST_MAX_PARTICIPANTS = 128 -- but it made an internal helper diverge from the algorithm it implements. frost_large_id_test covers it by reconstructing the constant term of a random degree-2 polynomial from shares held by identifiers 0, UINT32_MAX - 1 and UINT32_MAX. Zero-length messages -------------------- secp256k1_frost_session_init and secp256k1_frost_deterministic_sign required a non-NULL msg, so an empty message -- which the reference represents as the byte string b"" -- could only be passed as a pointer that is never dereferenced. Both now accept NULL when msglen is 0, matching secp256k1_schnorrsig_sign_custom and the msg parameter of secp256k1_frost_nonce_gen. secp256k1_sha256_write guards both of its memcpy calls on a non-zero length, so it is never reached with a NULL pointer. NonceGen keeps its distinction between a NULL msg and a zero-length msg: there the BIP really does distinguish msg = None (hashed as the single byte 0x00) from msg = b"" (hashed as 0x01 followed by an eight-byte zero length), and the API expresses that as NULL versus non-NULL. frost_empty_msg_test runs a signing round over a zero-length message passed both ways and checks that the two session objects are identical. The two API tests that relied on a NULL msg always being rejected now pass an explicit non-zero msglen; previously they passed a random msglen that could be 0. Header documentation -------------------- The parameter tables of eleven doc comments had names that did not line up with their block's continuation column. All parameter tables are now aligned consistently, with wrapped text two columns past the colon. Verification ------------ - gcc and clang, -std=c89 -pedantic-errors -Werror, with and without -DVERIFY: clean - tests (multiple seeds), noverify_tests and frost_example: pass - ctime_tests under MemorySanitizer: exits 0 with halt_on_error=1 - vectors.h still reproduces exactly from the spec's JSON vectors - 240 signing + 120 deterministic-signing differential cases against the BIP 445 Python reference: byte-identical to the previous commit Co-Authored-By: Claude Opus 5 --- include/secp256k1_frost.h | 111 ++++++++++++++++--------------- src/modules/frost/keygen.h | 4 +- src/modules/frost/keygen_impl.h | 23 ++++--- src/modules/frost/session_impl.h | 60 +++++++++++------ src/modules/frost/tests_impl.h | 96 +++++++++++++++++++++++++- 5 files changed, 205 insertions(+), 89 deletions(-) diff --git a/include/secp256k1_frost.h b/include/secp256k1_frost.h index d0e89d24..c4c7c18a 100644 --- a/include/secp256k1_frost.h +++ b/include/secp256k1_frost.h @@ -225,15 +225,15 @@ SECP256K1_API int secp256k1_frost_partial_sig_serialize( * Out: secshares32: pointer to an n_participants*32-byte array to store * the secret shares; participant with id i receives * secshares32[i*32..(i+1)*32] - * thresh_pk: pointer to a pubkey object to store the threshold + * thresh_pk: pointer to a pubkey object to store the threshold * public key (full point, parity is meaningful) - * pubshares: pointer to an array of n_participants pubkey + * pubshares: pointer to an array of n_participants pubkey * objects to store the public shares; entry i belongs * to the participant with id i - * In: n_participants: total number of participants n. Must be between 1 - * and SECP256K1_FROST_MAX_PARTICIPANTS. - * threshold: threshold t. Must be between 1 and n_participants. - * threshold_seckey32: pointer to the 32-byte threshold secret key + * In: n_participants: total number of participants n. Must be between 1 + * and SECP256K1_FROST_MAX_PARTICIPANTS. + * threshold: threshold t. Must be between 1 and n_participants. + * threshold_seckey32: pointer to the 32-byte threshold secret key */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_trusted_dealer_keygen( const secp256k1_context *ctx, @@ -254,12 +254,12 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_trusted_dealer_ke * produced it. * * Returns: 1 if the key material is valid and consistent, 0 otherwise - * Args: ctx: pointer to a context object + * Args: ctx: pointer to a context object * In: thresh_pk: pointer to the threshold public key * pubshares: array of n_participants pubkeys; entry i is the * public share of the participant with id i * n_participants: total number of participants n - * threshold: threshold t + * threshold: threshold t */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_threshold_info_validate( const secp256k1_context *ctx, @@ -275,8 +275,8 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_threshold_info_va * `frost_session_init`, even if no tweaks are applied. * * Returns: 0 if the arguments are invalid, 1 otherwise - * Args: ctx: pointer to a context object - * Out: cache: pointer to the tweak cache to initialize + * Args: ctx: pointer to a context object + * Out: cache: pointer to the tweak cache to initialize * In: thresh_pk: pointer to the (untweaked) threshold public key */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_tweak_cache_init( @@ -294,7 +294,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_tweak_cache_init( * Args: ctx: pointer to a context object * Out: tweaked_pk: pointer to an xonly_pubkey object to store the tweaked * threshold public key - * In: cache: pointer to the tweak cache + * In: cache: pointer to the tweak cache */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_tweaked_pubkey_get( const secp256k1_context *ctx, @@ -314,7 +314,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_tweaked_pubkey_ge * Out: tweaked_pk: pointer to an xonly_pubkey object to store the tweaked * threshold public key. If you do not need it, this arg can * be NULL. - * In: cache: pointer to the tweak cache + * In: cache: pointer to the tweak cache * tweak32: pointer to the 32-byte tweak */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_pubkey_xonly_tweak_add( @@ -336,7 +336,7 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_pubkey_xonly_twea * Out: tweaked_pk: pointer to an xonly_pubkey object to store the tweaked * threshold public key. If you do not need it, this arg can * be NULL. - * In: cache: pointer to the tweak cache + * In: cache: pointer to the tweak cache * tweak32: pointer to the 32-byte tweak */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_pubkey_ec_tweak_add( @@ -368,17 +368,18 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_pubkey_ec_tweak_a * randomness is masked with the share, see * BIP 445 "Modifications to Nonce Generation"). * pubshare: pointer to the signer's public share, or NULL - * thresh_pk32: pointer to the 32-byte x-only encoding of the + * thresh_pk32: pointer to the 32-byte x-only encoding of the * threshold public key the signature will verify * against (i.e. after applying tweaks, if any), * or NULL * msg: pointer to the message to be signed, or NULL if * the message is not known yet * msglen: length of msg. Must be 0 if msg is NULL. - * extra_in: pointer to additional data to bind into the + * extra_in: pointer to additional data to bind into the * nonce derivation, or NULL - * extra_in_len: length of extra_in. Must be 0 if extra_in is - * NULL. + * extra_in_len: length of extra_in. Must be 0 if extra_in is + * NULL and at most 2^32 - 1 (the nonce hash + * commits to it in a 4-byte length prefix). */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_nonce_gen( const secp256k1_context *ctx, @@ -404,12 +405,12 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_nonce_gen( * malformed, 1 otherwise * Args: ctx: pointer to a context object * Out: aggnonce: pointer to an aggnonce object - * error_index: if non-NULL, set to the index of the offending - * pubnonce on failure + * error_index: if non-NULL, set to the index of the offending + * pubnonce on failure * In: pubnonces: input array of pointers to pubnonces. The pubnonce at * index i must belong to the signer with ids[i] passed * to `frost_session_init`. - * n_pubnonces: length of the pubnonces array. Must be greater than + * n_pubnonces: length of the pubnonces array. Must be greater than * 0. */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_nonce_agg( @@ -430,24 +431,25 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_nonce_agg( * * Returns: 0 if the arguments are invalid (including inconsistent key * material or duplicate signer ids), 1 otherwise - * Args: ctx: pointer to a context object - * Out: session: pointer to a session object - * In: aggnonce: pointer to the aggregate nonce from - * `frost_nonce_agg` + * Args: ctx: pointer to a context object + * Out: session: pointer to a session object + * In: aggnonce: pointer to the aggregate nonce from + * `frost_nonce_agg` * ids: array of the u signer identifiers. Every id must * be unique and smaller than n_participants. * pubshares: array of u pubkeys with the public shares of the * signers (entry i belongs to ids[i]), or NULL if the * pubshares are unknown. If provided, they are * validated against the threshold public key. - * n_signers: number of signers u. Must be between threshold and + * n_signers: number of signers u. Must be between threshold and * n_participants. * n_participants: total number of participants n. Must be at most * SECP256K1_FROST_MAX_PARTICIPANTS. * threshold: threshold t. Must be between 1 and n_participants. - * tweak_cache: pointer to the tweak cache holding the threshold + * tweak_cache: pointer to the tweak cache holding the threshold * public key and all tweaks applied to it - * msg: pointer to the message to sign + * msg: pointer to the message to sign, or NULL if + * msglen is 0 * msglen: length of msg */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_session_init( @@ -477,18 +479,18 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_session_init( * here. * * 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 - * In: secnonce: pointer to the signer's secnonce from - * `frost_nonce_gen` + * Args: ctx: pointer to a context object + * Out: partial_sig: pointer to a partial_sig object + * In: secnonce: pointer to the signer's secnonce from + * `frost_nonce_gen` * secshare32: pointer to the signer's 32-byte secret share - * session: pointer to the session + * session: pointer to the session * ids: array of the u signer identifiers (identical to * session_init) * pubshares: array of u pubkeys with the signers' public shares * (identical to session_init), or NULL - * n_signers: number of signers u - * my_id: this signer's identifier + * n_signers: number of signers u + * my_id: this signer's identifier */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_sign( const secp256k1_context *ctx, @@ -521,22 +523,23 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_sign( * 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 - * pubnonce: pointer to a pubnonce object holding this signer's + * pubnonce: pointer to a pubnonce object holding this signer's * public nonce, to be sent to the coordinator * In: secshare32: pointer to the signer's 32-byte secret share - * my_id: this signer's identifier - * aggothernonce: pointer to the aggregate of the other signers' + * my_id: this signer's identifier + * aggothernonce: pointer to the aggregate of the other signers' * public nonces, or NULL for a sole signer - * ids: array of the u signer identifiers - * pubshares: array of u pubkeys with the signers' public + * ids: array of the u signer identifiers + * pubshares: array of u pubkeys with the signers' public * shares, or NULL - * n_signers: number of signers u - * n_participants: total number of participants n - * threshold: threshold t - * tweak_cache: pointer to the tweak cache - * msg: pointer to the message to sign - * msglen: length of msg - * aux_rand32: pointer to 32 bytes of auxiliary randomness mixed + * n_signers: number of signers u + * n_participants: total number of participants n + * threshold: threshold t + * tweak_cache: pointer to the tweak cache + * msg: pointer to the message to sign, or NULL if + * msglen is 0 + * msglen: length of msg + * aux_rand32: pointer to 32 bytes of auxiliary randomness mixed * into the nonce derivation, or NULL */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_deterministic_sign( @@ -565,15 +568,15 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_deterministic_sig * verified. * * Returns: 1 if the partial signature is valid, 0 otherwise - * Args: ctx: pointer to a context object - * In: partial_sig: pointer to the partial signature + * Args: ctx: pointer to a context object + * In: partial_sig: pointer to the partial signature * pubnonce: pointer to the signer's public nonce * pubshare: pointer to the signer's public share - * session: pointer to the session + * session: pointer to the session * ids: array of the u signer identifiers (identical to * session_init) - * n_signers: number of signers u - * signer_index: index of the signer in the ids array + * n_signers: number of signers u + * signer_index: index of the signer in the ids array */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_partial_sig_verify( const secp256k1_context *ctx, @@ -599,11 +602,11 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_partial_sig_verif * Args: ctx: pointer to a context object * Out: sig64: pointer to a 64-byte array to store the final * BIP340 signature - * error_index: if non-NULL, set to the index of the offending - * partial signature on failure + * error_index: if non-NULL, set to the index of the offending + * partial signature on failure * In: session: pointer to the session * partial_sigs: input array of pointers to partial signatures - * n_sigs: length of the partial_sigs array. Must equal + * n_sigs: length of the partial_sigs array. Must equal * n_signers from `frost_session_init`. */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_partial_sig_agg( diff --git a/src/modules/frost/keygen.h b/src/modules/frost/keygen.h index 4ac798ca..4e40a923 100644 --- a/src/modules/frost/keygen.h +++ b/src/modules/frost/keygen.h @@ -36,8 +36,8 @@ static int secp256k1_frost_tweak_cache_load(const secp256k1_context* ctx, secp25 * BIP 445 derive_interpolating_value algorithm: with x-coordinate of an * identifier id being id + 1, the value is * lambda = product_{j != i} x_j / (x_j - x_i) (mod n). - * Returns 1 on success and 0 if my_id is not in ids, if ids contains - * duplicates, or if an id equals UINT32_MAX (for which id + 1 overflows). */ + * Returns 1 on success and 0 if my_id is not in ids or if ids contains + * duplicates. Every uint32_t value is a valid identifier. */ static int secp256k1_frost_derive_interpolating_value(secp256k1_scalar *out, const uint32_t *ids, size_t n_ids, uint32_t my_id); /* Evaluates the polynomial given by the pubshares at the identifier-space diff --git a/src/modules/frost/keygen_impl.h b/src/modules/frost/keygen_impl.h index fb410be9..65954dd1 100644 --- a/src/modules/frost/keygen_impl.h +++ b/src/modules/frost/keygen_impl.h @@ -86,15 +86,11 @@ static int secp256k1_frost_tweak_cache_load(const secp256k1_context* ctx, secp25 return 1; } -/* Returns 1 if the ids array contains no duplicates and no id equals - * UINT32_MAX (whose x-coordinate id + 1 would overflow), 0 otherwise. */ +/* Returns 1 if the ids array contains no duplicates, 0 otherwise. */ static int secp256k1_frost_ids_are_valid(const uint32_t *ids, size_t n_ids) { size_t i, j; for (i = 0; i < n_ids; i++) { - if (ids[i] == UINT32_MAX) { - return 0; - } for (j = i + 1; j < n_ids; j++) { if (ids[i] == ids[j]) { return 0; @@ -105,7 +101,7 @@ static int secp256k1_frost_ids_are_valid(const uint32_t *ids, size_t n_ids) { } static int secp256k1_frost_derive_interpolating_value(secp256k1_scalar *out, const uint32_t *ids, size_t n_ids, uint32_t my_id) { - secp256k1_scalar num, deno; + secp256k1_scalar num, deno, one; size_t i; int found = 0; @@ -123,20 +119,25 @@ static int secp256k1_frost_derive_interpolating_value(secp256k1_scalar *out, con } /* lambda = product_{j != i} x_j / (x_j - x_i), where the x-coordinate of - * an identifier id is id + 1. */ + * an identifier id is id + 1. The +1 is added in scalar arithmetic because + * it would overflow for id == UINT32_MAX, which the BIP 445 + * derive_interpolating_value algorithm permits (0 <= id < 2^32). */ secp256k1_scalar_set_int(&num, 1); secp256k1_scalar_set_int(&deno, 1); + secp256k1_scalar_set_int(&one, 1); for (i = 0; i < n_ids; i++) { - secp256k1_scalar x_j, term; + secp256k1_scalar id_j, x_j, term; if (ids[i] == my_id) { continue; } - secp256k1_scalar_set_int(&x_j, ids[i] + 1); + secp256k1_scalar_set_int(&id_j, ids[i]); + /* x_j = id_j + 1 */ + secp256k1_scalar_add(&x_j, &id_j, &one); secp256k1_scalar_mul(&num, &num, &x_j); /* x_j - x_i = (id_j + 1) - (my_id + 1) = id_j - my_id */ - secp256k1_scalar_set_int(&term, my_id + 1); + secp256k1_scalar_set_int(&term, my_id); secp256k1_scalar_negate(&term, &term); - secp256k1_scalar_add(&term, &term, &x_j); + secp256k1_scalar_add(&term, &term, &id_j); secp256k1_scalar_mul(&deno, &deno, &term); } /* deno != 0 because the ids are distinct */ diff --git a/src/modules/frost/session_impl.h b/src/modules/frost/session_impl.h index c274590f..31580e4b 100644 --- a/src/modules/frost/session_impl.h +++ b/src/modules/frost/session_impl.h @@ -41,8 +41,10 @@ static void secp256k1_frost_sha256_tagged_deterministic_nonce(const secp256k1_ha /* Writes data into the hash, framed with a big-endian length prefix of * prefix_size bytes, matching the framing of optional and variable-length * inputs in the BIP 445 nonce_hash and det_nonce_hash algorithms. If data is - * NULL, only the (zero) length prefix is written. */ -static void secp256k1_frost_sha256_write_prefixed(const secp256k1_hash_ctx *hash_ctx, secp256k1_sha256 *sha, unsigned int prefix_size, const unsigned char *data, size_t data_len) { + * NULL, only the (zero) length prefix is written. Returns 0 without writing + * anything if data_len does not fit into prefix_size bytes, which corresponds + * to the OverflowError raised by the reference implementation. */ +static int secp256k1_frost_sha256_write_prefixed(const secp256k1_hash_ctx *hash_ctx, secp256k1_sha256 *sha, unsigned int prefix_size, const unsigned char *data, size_t data_len) { unsigned char prefix[8]; size_t len = data_len; unsigned int i; @@ -55,12 +57,17 @@ static void secp256k1_frost_sha256_write_prefixed(const secp256k1_hash_ctx *hash prefix[i] = len & 0xFF; len >>= 8; } - /* The length must fit into the prefix */ - VERIFY_CHECK(len == 0); + if (len != 0) { + /* The length does not fit into the prefix. Only reachable for the + * 4-byte extra_in prefix, and only on platforms whose size_t is wider + * than 32 bits. */ + return 0; + } secp256k1_sha256_write(hash_ctx, sha, prefix, prefix_size); if (data != NULL) { secp256k1_sha256_write(hash_ctx, sha, data, data_len); } + return 1; } static const unsigned char secp256k1_frost_secnonce_magic[4] = { 0x5c, 0xcf, 0xb9, 0x99 }; @@ -325,14 +332,16 @@ int secp256k1_frost_partial_sig_serialize(const secp256k1_context* ctx, unsigned } /* Nonce derivation function of NonceGen (BIP 445). Computes the secret nonces - * k[0], k[1] from the session randomness and the optional inputs. All secret - * intermediates (the masked randomness, the hash states, the hash output) are - * cleansed before this function returns. */ -static void secp256k1_frost_nonce_function(const secp256k1_hash_ctx *hash_ctx, secp256k1_scalar *k, const unsigned char *session_secrand, const unsigned char *secshare32, const unsigned char *pubshare33, const unsigned char *thresh_pk32, const unsigned char *msg, size_t msglen, const unsigned char *extra_in, size_t extra_in_len) { + * k[0], k[1] from the session randomness and the optional inputs. Returns 0 if + * one of the variable-length inputs is too long for its length prefix, 1 + * otherwise. All secret intermediates (the masked randomness, the hash states, + * the hash output) are cleansed before this function returns. */ +static int secp256k1_frost_nonce_function(const secp256k1_hash_ctx *hash_ctx, secp256k1_scalar *k, const unsigned char *session_secrand, const unsigned char *secshare32, const unsigned char *pubshare33, const unsigned char *thresh_pk32, const unsigned char *msg, size_t msglen, const unsigned char *extra_in, size_t extra_in_len) { secp256k1_sha256 sha; unsigned char rand[32]; unsigned char i; unsigned char msg_present; + int ret = 1; if (secshare32 != NULL) { secp256k1_frost_sha256_tagged_aux(hash_ctx, &sha); @@ -347,14 +356,14 @@ static void secp256k1_frost_nonce_function(const secp256k1_hash_ctx *hash_ctx, s secp256k1_frost_sha256_tagged_nonce(hash_ctx, &sha); secp256k1_sha256_write(hash_ctx, &sha, rand, sizeof(rand)); - secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 1, pubshare33, pubshare33 != NULL ? 33 : 0); - secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 1, thresh_pk32, thresh_pk32 != NULL ? 32 : 0); + ret &= secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 1, pubshare33, pubshare33 != NULL ? 33 : 0); + ret &= secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 1, thresh_pk32, thresh_pk32 != NULL ? 32 : 0); msg_present = msg != NULL; secp256k1_sha256_write(hash_ctx, &sha, &msg_present, 1); if (msg_present) { - secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 8, msg, msglen); + ret &= secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 8, msg, msglen); } - secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 4, extra_in, extra_in_len); + ret &= secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 4, extra_in, extra_in_len); for (i = 0; i < 2; i++) { unsigned char buf[32]; @@ -369,6 +378,7 @@ static void secp256k1_frost_nonce_function(const secp256k1_hash_ctx *hash_ctx, s } secp256k1_memclear_explicit(rand, sizeof(rand)); secp256k1_sha256_clear(&sha); + return ret; } int secp256k1_frost_nonce_gen(const secp256k1_context* ctx, secp256k1_frost_secnonce *secnonce, secp256k1_frost_pubnonce *pubnonce, unsigned char *session_secrand32, const unsigned char *secshare32, const secp256k1_pubkey *pubshare, const unsigned char *thresh_pk32, const unsigned char *msg, size_t msglen, const unsigned char *extra_in, size_t extra_in_len) { @@ -413,7 +423,13 @@ int secp256k1_frost_nonce_gen(const secp256k1_context* ctx, secp256k1_frost_secn pubshare33_ptr = pubshare33; } - secp256k1_frost_nonce_function(secp256k1_get_hash_context(ctx), k, session_secrand32, secshare32, pubshare33_ptr, thresh_pk32, msg, msglen, extra_in, extra_in_len); + if (!secp256k1_frost_nonce_function(secp256k1_get_hash_context(ctx), k, session_secrand32, secshare32, pubshare33_ptr, thresh_pk32, msg, msglen, extra_in, extra_in_len)) { + /* extra_in is too long to be committed to in a 4-byte length prefix. */ + secp256k1_scalar_clear(&k[0]); + secp256k1_scalar_clear(&k[1]); + secp256k1_memzero_explicit(session_secrand32, 32); + return 0; + } { /* k_i == 0 has negligible probability, so its occurrence is not * secret. Fail cleanly instead of asserting like the reference @@ -536,7 +552,7 @@ static int secp256k1_frost_validate_session_params(const secp256k1_context *ctx, return 0; } } - /* Rejects duplicate ids (and UINT32_MAX, already covered above). */ + /* Rejects duplicate ids; the range check above covers the rest. */ if (!secp256k1_frost_ids_are_valid(ids, n_signers)) { return 0; } @@ -661,7 +677,7 @@ int secp256k1_frost_session_init(const secp256k1_context *ctx, secp256k1_frost_s ARG_CHECK(aggnonce != NULL); ARG_CHECK(ids != NULL); ARG_CHECK(tweak_cache != NULL); - ARG_CHECK(msg != NULL); + ARG_CHECK(msg != NULL || msglen == 0); if (!secp256k1_frost_tweak_cache_load(ctx, &cache_i, tweak_cache)) { return 0; @@ -949,9 +965,10 @@ int secp256k1_frost_partial_sig_agg(const secp256k1_context *ctx, unsigned char * || ser32(u) || sorted ser32 ids || aggothernonce || * tweaked_thresh_pk_xonly || len8(msg) || msg || byte(i)) * where aggothernonce is its 66-byte serialization or the empty string for a - * sole signer. Returns 0 if one of the nonces is zero (which can only happen - * with negligible probability), 1 otherwise. All secret intermediates are - * cleansed before this function returns. */ + * sole signer. Returns 0 if the message is too long for its length prefix, or + * if one of the nonces is zero (which can only happen with negligible + * probability), 1 otherwise. All secret intermediates are cleansed before this + * function returns. */ static int secp256k1_frost_det_nonce_function(const secp256k1_hash_ctx *hash_ctx, secp256k1_scalar *k, const unsigned char *secshare32, uint32_t my_id, const uint32_t *sorted_ids, size_t n_signers, const unsigned char *aggothernonce66, const unsigned char *tweaked_pk32, const unsigned char *msg, size_t msglen) { secp256k1_sha256 sha; unsigned char buf[4]; @@ -972,7 +989,10 @@ static int secp256k1_frost_det_nonce_function(const secp256k1_hash_ctx *hash_ctx secp256k1_sha256_write(hash_ctx, &sha, aggothernonce66, 66); } secp256k1_sha256_write(hash_ctx, &sha, tweaked_pk32, 32); - secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 8, msg, msglen); + if (!secp256k1_frost_sha256_write_prefixed(hash_ctx, &sha, 8, msg, msglen)) { + secp256k1_sha256_clear(&sha); + return 0; + } for (i = 0; i < 2; i++) { unsigned char hash32[32]; @@ -1019,7 +1039,7 @@ int secp256k1_frost_deterministic_sign(const secp256k1_context *ctx, secp256k1_f ARG_CHECK(secshare32 != NULL); ARG_CHECK(ids != NULL); ARG_CHECK(tweak_cache != NULL); - ARG_CHECK(msg != NULL); + ARG_CHECK(msg != NULL || msglen == 0); ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)); hash_ctx = secp256k1_get_hash_context(ctx); diff --git a/src/modules/frost/tests_impl.h b/src/modules/frost/tests_impl.h index 409ef6fb..16a7beb2 100644 --- a/src/modules/frost/tests_impl.h +++ b/src/modules/frost/tests_impl.h @@ -1281,7 +1281,9 @@ static void frost_sign_test_internal(void) { CHECK_ILLEGAL(CTX, secp256k1_frost_session_init(CTX, &session_tmp, &aggnonce, NULL, NULL, u, n, t, &cache, msg, msglen)); CHECK_ILLEGAL(CTX, secp256k1_frost_session_init(CTX, &session_tmp, &aggnonce, ids, NULL, u, n, t, NULL, msg, msglen)); CHECK_ILLEGAL(CTX, secp256k1_frost_session_init(CTX, &session_tmp, &aggnonce, ids, NULL, u, n, t, &invalid_cache, msg, msglen)); - CHECK_ILLEGAL(CTX, secp256k1_frost_session_init(CTX, &session_tmp, &aggnonce, ids, NULL, u, n, t, &cache, NULL, msglen)); + /* A NULL message is only legal with msglen == 0. */ + CHECK_ILLEGAL(CTX, secp256k1_frost_session_init(CTX, &session_tmp, &aggnonce, ids, NULL, u, n, t, &cache, NULL, 1)); + CHECK(secp256k1_frost_session_init(CTX, &session_tmp, &aggnonce, ids, NULL, u, n, t, &cache, NULL, 0) == 1); /* sign: every call that reaches the secnonce load wipes the secnonce, * so a fresh one is generated before each call. */ @@ -1326,7 +1328,7 @@ static void frost_sign_test_internal(void) { CHECK_ILLEGAL(CTX, secp256k1_frost_deterministic_sign(CTX, &psig_tmp, &pubnonce_tmp, &secshares[32 * ids[0]], ids[0], NULL, NULL, NULL, u, n, t, &cache, msg, msglen, NULL)); CHECK_ILLEGAL(CTX, secp256k1_frost_deterministic_sign(CTX, &psig_tmp, &pubnonce_tmp, &secshares[32 * ids[0]], ids[0], NULL, ids, NULL, u, n, t, NULL, msg, msglen, NULL)); CHECK_ILLEGAL(CTX, secp256k1_frost_deterministic_sign(CTX, &psig_tmp, &pubnonce_tmp, &secshares[32 * ids[0]], ids[0], NULL, ids, NULL, u, n, t, &invalid_cache, msg, msglen, NULL)); - CHECK_ILLEGAL(CTX, secp256k1_frost_deterministic_sign(CTX, &psig_tmp, &pubnonce_tmp, &secshares[32 * ids[0]], ids[0], NULL, ids, NULL, u, n, t, &cache, NULL, msglen, NULL)); + CHECK_ILLEGAL(CTX, secp256k1_frost_deterministic_sign(CTX, &psig_tmp, &pubnonce_tmp, &secshares[32 * ids[0]], ids[0], NULL, ids, NULL, u, n, t, &cache, NULL, 1, NULL)); } } REPEAT_TEST(frost_sign_test) @@ -1500,6 +1502,94 @@ static void frost_api_test(void) { } } +/* The BIP 445 derive_interpolating_value algorithm accepts every identifier in + * 0 <= id < 2^32, so the +1 that maps an identifier to its polynomial + * x-coordinate must not be computed in uint32_t arithmetic. Reconstructs the + * constant term of a random degree-(t-1) polynomial from t shares whose + * identifiers include UINT32_MAX. */ +static void frost_large_id_test(void) { + static const uint32_t ids[3] = { 0, UINT32_MAX - 1, UINT32_MAX }; + secp256k1_scalar coeffs[3]; + secp256k1_scalar acc, one; + size_t i, j; + + for (i = 0; i < 3; i++) { + testutil_random_scalar_order(&coeffs[i]); + } + secp256k1_scalar_set_int(&one, 1); + secp256k1_scalar_set_int(&acc, 0); + for (i = 0; i < 3; i++) { + secp256k1_scalar x, share, lambda; + + /* x = id + 1, computed in scalar arithmetic so that it does not + * overflow for id == UINT32_MAX. */ + secp256k1_scalar_set_int(&x, ids[i]); + secp256k1_scalar_add(&x, &x, &one); + /* share = p(x) with p(0) = coeffs[2], evaluated with Horner's method. */ + secp256k1_scalar_set_int(&share, 0); + for (j = 0; j < 3; j++) { + secp256k1_scalar_mul(&share, &share, &x); + secp256k1_scalar_add(&share, &share, &coeffs[j]); + } + CHECK(secp256k1_frost_derive_interpolating_value(&lambda, ids, 3, ids[i]) == 1); + secp256k1_scalar_mul(&share, &share, &lambda); + secp256k1_scalar_add(&acc, &acc, &share); + } + /* sum_i lambda_i * p(x_i) == p(0) == coeffs[2] */ + CHECK(secp256k1_scalar_eq(&acc, &coeffs[2])); + + /* Duplicate identifiers are still rejected, and an identifier that is not + * in the set has no interpolating value. */ + { + static const uint32_t dup[2] = { UINT32_MAX, UINT32_MAX }; + secp256k1_scalar lambda; + CHECK(secp256k1_frost_derive_interpolating_value(&lambda, dup, 2, UINT32_MAX) == 0); + CHECK(secp256k1_frost_derive_interpolating_value(&lambda, ids, 3, 7) == 0); + } +} + +/* A zero-length message is signed like any other message, and may be passed as + * a NULL pointer: the reference's msg is a byte string, so an empty message is + * not distinguishable from a NULL pointer of length 0. (NonceGen is the sole + * exception: there the BIP distinguishes msg = None from msg = b"", which the + * API expresses as a NULL versus a non-NULL msg.) */ +static void frost_empty_msg_test(void) { + static const uint32_t ids[2] = { 0, 1 }; + unsigned char thresh_sk[32]; + unsigned char secshares[3 * 32]; + secp256k1_pubkey thresh_pk; + secp256k1_pubkey pubshares[3]; + secp256k1_frost_tweak_cache cache; + secp256k1_frost_secnonce secnonce[2]; + secp256k1_frost_pubnonce pubnonce[2]; + const secp256k1_frost_pubnonce *pubnonce_ptrs[2]; + secp256k1_frost_aggnonce aggnonce; + secp256k1_frost_session session_null, session_ptr; + unsigned char msg[1]; + unsigned char rand[32]; + size_t i; + + testutil_random_scalar_order_b32(thresh_sk); + CHECK(secp256k1_frost_trusted_dealer_keygen(CTX, secshares, &thresh_pk, pubshares, 3, 2, thresh_sk) == 1); + CHECK(secp256k1_frost_tweak_cache_init(CTX, &cache, &thresh_pk) == 1); + + /* A full signing round over a zero-length message, passed both ways. */ + frost_run_signing_round(secshares, pubshares, &cache, 3, 2, 2, NULL, 0); + frost_run_signing_round(secshares, pubshares, &cache, 3, 2, 2, msg, 0); + + /* A NULL message of length 0 and a non-NULL pointer of length 0 yield the + * same session. */ + for (i = 0; i < 2; i++) { + testrand256(rand); + CHECK(secp256k1_frost_nonce_gen(CTX, &secnonce[i], &pubnonce[i], rand, &secshares[32 * ids[i]], &pubshares[ids[i]], NULL, NULL, 0, NULL, 0) == 1); + pubnonce_ptrs[i] = &pubnonce[i]; + } + CHECK(secp256k1_frost_nonce_agg(CTX, &aggnonce, NULL, pubnonce_ptrs, 2) == 1); + CHECK(secp256k1_frost_session_init(CTX, &session_null, &aggnonce, ids, pubshares, 2, 3, 2, &cache, NULL, 0) == 1); + CHECK(secp256k1_frost_session_init(CTX, &session_ptr, &aggnonce, ids, pubshares, 2, 3, 2, &cache, msg, 0) == 1); + CHECK(secp256k1_memcmp_var(&session_null, &session_ptr, sizeof(session_null)) == 0); +} + /* Boundary test: keygen and a full signing round with n = * SECP256K1_FROST_MAX_PARTICIPANTS (128), the rejection of n = 129, and the * threshold extremes t = 1 and t = n. */ @@ -2171,6 +2261,8 @@ static const struct tf_test_entry tests_frost[] = { CASE1(frost_sign_test), CASE1(frost_api_test), CASE1(frost_boundary_test), + CASE1(frost_large_id_test), + CASE1(frost_empty_msg_test), CASE1(frost_tweak_edge_test), CASE1(frost_infinity_test), CASE1(frost_mismatch_test),