diff --git a/include/secp256k1_frost_enrollment.h b/include/secp256k1_frost_enrollment.h index 3d203b72..8cee13d9 100644 --- a/include/secp256k1_frost_enrollment.h +++ b/include/secp256k1_frost_enrollment.h @@ -106,19 +106,31 @@ extern "C" { * * This function operates on public data only. * - * Returns: 0 if the arguments are invalid (duplicate ids, n_ids == 0, n_ids - * greater than SECP256K1_FROST_MAX_PARTICIPANTS, unparseable - * thresh_pk), 1 otherwise + * This is also the natural place to pre-validate a parameter tuple: it + * enforces exactly the same constraints as the four functions below, and + * nothing else. Note that an unusable `thresh_pk` object is API MISUSE, not + * an invalid parameter: like every other entry point in the library, this + * function reports it through the illegal-argument callback rather than by + * returning 0. + * + * Returns: 0 if the parameters are invalid, 1 otherwise * Args: ctx: pointer to a context object * Out: out32: pointer to a 32-byte array for the hash. Set to zero * if this function returns 0. * In: thresh_pk: pointer to the threshold public key of the group * ids: array of the u helper identifiers. Every id must be - * unique; the order is irrelevant. - * n_ids: number of helpers u - * new_id: identifier of the participant receiving the share - * n_participants: total number of participants n - * threshold: threshold t + * unique, smaller than n_participants and different + * from new_id; the order is irrelevant. + * n_ids: number of helpers u. Must be between threshold and + * n_participants. + * new_id: identifier of the participant receiving the share. + * Must equal n_participants (enrollment) or be smaller + * than it (repair). + * n_participants: total number of participants n. Must be at most + * SECP256K1_FROST_MAX_PARTICIPANTS, and strictly + * smaller in enrollment mode. + * threshold: threshold t. Must be at least 2 (see + * frost_enrollment.md) and at most n_participants. */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_params_hash( const secp256k1_context *ctx, @@ -160,8 +172,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_params * Returns: 0 if the arguments are invalid, 1 otherwise * Args: ctx: pointer to a context object * Out: shares32_out: pointer to an array of u*32 bytes for the enrollment - * shares, aligned with `ids`. Set to zero if this - * function returns 0. + * shares, aligned with `ids`. Zeroed if this function + * returns 0 -- except when n_ids itself is out of + * range, in which case the buffer is not written at + * all, since its size is not known to be u*32. * params_hash32_out: pointer to a 32-byte array for the parameters hash, * identical to what * `secp256k1_frost_enrollment_params_hash` returns for @@ -208,11 +222,27 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_shares * * The function recomputes its own parameters hash from `thresh_pk` and the * parameter tuple it is given, and compares every entry of - * `received_params_hashes32` against it. On the first disagreement it returns - * 0 and, if `mismatch_id` is not NULL, stores the IDENTIFIER of the - * disagreeing helper there (not an array index, which would be ambiguous - * because identifiers need not be 0..u-1). `mismatch_id` is set to - * UINT32_MAX when the failure has another cause. + * `received_params_hashes32` against it. It then sums the shares. + * + * `mismatch_id` reports fault attribution for BOTH ways a specific helper's + * contribution can be at fault: + * + * - its parameters hash disagrees with the recomputed one, meaning that + * helper ran round 1.1 on a different parameter tuple or in a different + * group; + * - its entry in `all_shares32` is not a valid scalar (it is not smaller + * than the group order), meaning the value was corrupted in transit or + * fabricated. + * + * In both cases the function returns 0 and, if `mismatch_id` is not NULL, + * stores the IDENTIFIER of the responsible helper there -- not an array + * index, which would be ambiguous because identifiers need not be 0..u-1. + * The two causes are not distinguished, so a caller should not report one of + * them specifically. Note that the second cause can name the CALLER'S OWN + * identifier, since the share kept locally is summed along with the rest. + * + * `mismatch_id` is set to UINT32_MAX when the failure has neither cause, + * which covers every invalid-parameter and API-misuse case. * * Note the deliberately OPPOSITE own-slot conventions of the two u*32 input * buffers, both of which are aligned with `ids`: @@ -349,7 +379,10 @@ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_pubsha * from the helpers, or NULL to skip the comparison * expected_pubshare: pointer to the expected public share, from * `secp256k1_frost_enrollment_pubshare_derive`, or NULL - * to skip the verification (not recommended) + * to skip the verification (not recommended). When it + * is non-NULL, ctx must have been initialized for + * signing; when it is NULL, no context capability + * beyond the default is required. */ SECP256K1_API SECP256K1_WARN_UNUSED_RESULT int secp256k1_frost_enrollment_secshare_gen( const secp256k1_context *ctx, diff --git a/src/modules/frost_enrollment/enrollment_impl.h b/src/modules/frost_enrollment/enrollment_impl.h index edf77f2a..55168229 100644 --- a/src/modules/frost_enrollment/enrollment_impl.h +++ b/src/modules/frost_enrollment/enrollment_impl.h @@ -215,7 +215,9 @@ static void secp256k1_frost_enrollment_lagrange_at(secp256k1_scalar *out, const secp256k1_scalar_add(&term, &term, &id_i); secp256k1_scalar_mul(&deno, &deno, &term); } - /* deno != 0 because the ids are distinct and new_id is not among them */ + /* deno != 0 because the ids are distinct. (new_id not being among them + * is what keeps num nonzero, which is not required for correctness -- + * lambda == 0 would simply mean this helper contributes nothing.) */ VERIFY_CHECK(!secp256k1_scalar_is_zero(&deno)); secp256k1_scalar_inverse_var(&deno, &deno); secp256k1_scalar_mul(out, &num, &deno); @@ -289,13 +291,30 @@ int secp256k1_frost_enrollment_shares_gen(const secp256k1_context *ctx, unsigned ARG_CHECK(secshare32 != NULL); ARG_CHECK(thresh_pk != NULL); ARG_CHECK(ids != NULL); - /* By contract shares32_out holds n_ids entries, whatever n_ids is; the - * value itself is bounded by params_are_valid below. */ + + /* Validate BEFORE touching shares32_out, which is the only output in this + * module whose size is caller-supplied. A caller that passes a fixed + * buffer and an out-of-range n_ids -- relying on this API's "invalid + * ranges return 0" convention -- would otherwise have memory past the + * buffer zeroed before the call reported failure. The frost module + * validates counts first for the same reason + * (secp256k1_frost_trusted_dealer_keygen, keygen_impl.h:228). */ + if (!secp256k1_frost_enrollment_params_are_valid(ids, n_ids, new_id, n_participants, threshold)) { + /* The seed is consumed on this path too, so that "a failed call + * cannot be retried with the same randomness" holds without + * exception. params_hash32_out was zeroed above; shares32_out is + * deliberately left untouched, which is the whole point of + * validating here. */ + secp256k1_memzero_explicit(session_secrand32, 32); + return 0; + } memset(shares32_out, 0, n_ids * 32); hash_ctx = secp256k1_get_hash_context(ctx); memset(rand32, 0, sizeof(rand32)); + /* Re-validates, which is cheap and keeps this the single place the hash + * encoding is produced. */ if (!secp256k1_frost_enrollment_params_hash_checked(ctx, params_hash32_out, thresh_pk, ids, n_ids, new_id, n_participants, threshold)) { goto cleanup; } @@ -379,9 +398,10 @@ int secp256k1_frost_enrollment_share_agg(const secp256k1_context *ctx, unsigned /* set_int, not scalar_clear: this accumulator is READ on the first add, * and secp256k1_memclear_explicit marks its target undefined in VERIFY - * builds (src/util.h:295) precisely to catch reads of cleared memory. */ + * builds (src/util.h:295) precisely to catch reads of cleared memory. + * `term` is always written by set_b32 before it is read, so it needs no + * initializer -- but it is cleared on the way out. */ secp256k1_scalar_set_int(&sigma, 0); - secp256k1_scalar_set_int(&term, 0); memset(params_hash32, 0, sizeof(params_hash32)); /* The full parameter tuple is present here, so the mode and bounds are @@ -487,11 +507,18 @@ int secp256k1_frost_enrollment_secshare_gen(const secp256k1_context *ctx, unsign ARG_CHECK(sigmas32 != NULL); ARG_CHECK(thresh_pk != NULL); ARG_CHECK(ids != NULL); - ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)); + /* The ecmult_gen context is needed only for the public-share check, so it + * is only required when that check will run: a caller passing + * expected_pubshare == NULL never reaches a generator multiplication and + * should not need a context built for one. The check stays here at the + * top, where returning early cannot skip the cleanup that wipes the + * secrets below. */ + ARG_CHECK(expected_pubshare == NULL + || secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx)); - /* set_int for the same reason as in share_agg above. */ + /* set_int for the same reason as in share_agg above; `term` likewise + * needs no initializer. */ secp256k1_scalar_set_int(&secshare, 0); - secp256k1_scalar_set_int(&term, 0); memset(params_hash32, 0, sizeof(params_hash32)); /* Recomputed from the parameters the target believes and the group key it diff --git a/src/modules/frost_enrollment/tests_impl.h b/src/modules/frost_enrollment/tests_impl.h index a92d6d83..ce5cb480 100644 --- a/src/modules/frost_enrollment/tests_impl.h +++ b/src/modules/frost_enrollment/tests_impl.h @@ -814,6 +814,143 @@ static void run_frost_enrollment_random_test(void) { } } +/* Contract details that are easy to regress and that a caller can reasonably + * depend on. */ +static void run_frost_enrollment_contract_test(void) { + frost_enrollment_test_run r; + unsigned char guarded[4 * 32]; + unsigned char hash32[32]; + unsigned char secrand[32]; + unsigned char sigma[32]; + unsigned char out[32]; + unsigned char all_shares[2 * 32]; + unsigned char received[2 * 32]; + uint32_t mismatch_id; + size_t i; + + /* An out-of-range n_ids must be rejected WITHOUT writing shares32_out, + * whose size is only u*32 by contract: a caller passing a fixed buffer + * and a bad count would otherwise have memory past it zeroed. The seed is + * still consumed, so a failed call cannot be retried on it. */ + frost_enrollment_test_deal(&r, 3, 2, 2, 3); + memset(guarded, 0xa5, sizeof(guarded)); + testrand256(secrand); + memset(hash32, 0xff, sizeof(hash32)); + CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, r.secshares[0], &r.thresh_pk, r.ids, SECP256K1_FROST_MAX_PARTICIPANTS + 1, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0); + for (i = 0; i < sizeof(guarded); i++) { + CHECK(guarded[i] == 0xa5); + } + CHECK(secp256k1_is_zero_array(hash32, sizeof(hash32))); + CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand))); + + /* The same holds for an n_ids that is merely inconsistent with the rest + * of the tuple rather than out of the absolute range. */ + memset(guarded, 0xa5, sizeof(guarded)); + testrand256(secrand); + CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, r.secshares[0], &r.thresh_pk, r.ids, 1, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0); + for (i = 0; i < sizeof(guarded); i++) { + CHECK(guarded[i] == 0xa5); + } + CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand))); + + /* A valid call does zero the buffer it is allowed to write, and only + * that part of it. */ + memset(guarded, 0xa5, sizeof(guarded)); + testrand256(secrand); + CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, r.secshares[0], &r.thresh_pk, r.ids, r.u, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 1); + for (i = r.u * 32; i < sizeof(guarded); i++) { + CHECK(guarded[i] == 0xa5); + } + + /* secshare_gen needs a signing-capable context only when it is going to + * check the public share. With expected_pubshare == NULL it must work on + * the static context; with a public share it is API misuse there. */ + frost_enrollment_test_full_run(&r, 3, 2, 2, 3); + memset(out, 0xff, sizeof(out)); + CHECK(secp256k1_frost_enrollment_secshare_gen(STATIC_CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, r.params_hashes[0], NULL) == 1); + CHECK(secp256k1_memcmp_var(out, r.new_secshare, 32) == 0); + CHECK_ILLEGAL(STATIC_CTX, secp256k1_frost_enrollment_secshare_gen(STATIC_CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, r.params_hashes[0], &r.new_pubshare)); + + /* Both optional checks are genuinely optional: skipping either, or both, + * still produces the same share on an honest run. */ + memset(out, 0xff, sizeof(out)); + CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, NULL, &r.new_pubshare) == 1); + CHECK(secp256k1_memcmp_var(out, r.new_secshare, 32) == 0); + memset(out, 0xff, sizeof(out)); + CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, r.sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, NULL, NULL) == 1); + CHECK(secp256k1_memcmp_var(out, r.new_secshare, 32) == 0); + + /* share_agg reports a share that is not a valid scalar the same way it + * reports a parameters disagreement: by naming the responsible helper. + * The helper set here is {0, 2}, so an implementation returning the array + * index rather than the identifier would be caught. */ + frost_enrollment_test_deal(&r, 3, 2, 2, 1); + CHECK(r.ids[0] == 0 && r.ids[1] == 2); + frost_enrollment_test_round1_gen(&r); + frost_enrollment_test_collect(&r, 1, all_shares, received); + /* All-ones is larger than the group order. */ + memset(&all_shares[0], 0xff, 32); + mismatch_id = 0; + memset(sigma, 0xff, sizeof(sigma)); + CHECK(secp256k1_frost_enrollment_share_agg(CTX, sigma, &mismatch_id, all_shares, received, &r.thresh_pk, r.ids, r.u, r.ids[1], r.new_id, r.n, (uint32_t)r.t) == 0); + CHECK(mismatch_id == r.ids[0]); + CHECK(mismatch_id == 0); + CHECK(secp256k1_is_zero_array(sigma, sizeof(sigma))); + + /* And at the caller's OWN slot, which the header calls out: the kept + * share is summed along with the rest, so the caller can be named. */ + frost_enrollment_test_collect(&r, 1, all_shares, received); + memset(&all_shares[32], 0xff, 32); + mismatch_id = 0; + CHECK(secp256k1_frost_enrollment_share_agg(CTX, sigma, &mismatch_id, all_shares, received, &r.thresh_pk, r.ids, r.u, r.ids[1], r.new_id, r.n, (uint32_t)r.t) == 0); + CHECK(mismatch_id == r.ids[1]); + CHECK(mismatch_id == 2); + + /* An out-of-range sigma is rejected by secshare_gen too, which has no + * attribution to offer. */ + frost_enrollment_test_full_run(&r, 3, 2, 2, 3); + { + unsigned char sigmas[2 * 32]; + memcpy(sigmas, r.sigmas, sizeof(sigmas)); + memset(&sigmas[32], 0xff, 32); + memset(out, 0xff, sizeof(out)); + CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, r.params_hashes[0], &r.new_pubshare) == 0); + CHECK(secp256k1_is_zero_array(out, sizeof(out))); + } + + /* Sigmas summing to zero mod the group order are rejected: a zero share + * is not a usable secret key. secshare = 1 + (order - 1). */ + { + unsigned char sigmas[2 * 32]; + static const unsigned char order_minus_one[32] = { + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE, + 0xBA, 0xAE, 0xDC, 0xE6, 0xAF, 0x48, 0xA0, 0x3B, + 0xBF, 0xD2, 0x5E, 0x8C, 0xD0, 0x36, 0x41, 0x40 + }; + memset(sigmas, 0, sizeof(sigmas)); + sigmas[31] = 1; + memcpy(&sigmas[32], order_minus_one, 32); + memset(out, 0xff, sizeof(out)); + CHECK(secp256k1_frost_enrollment_secshare_gen(CTX, out, sigmas, &r.thresh_pk, r.ids, r.u, r.new_id, r.n, (uint32_t)r.t, NULL, NULL) == 0); + CHECK(secp256k1_is_zero_array(out, sizeof(out))); + } + + /* shares_gen rejects a secret share that is not a valid secret key. */ + { + unsigned char bad_secshare[32]; + memset(bad_secshare, 0, sizeof(bad_secshare)); + testrand256(secrand); + CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, bad_secshare, &r.thresh_pk, r.ids, r.u, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0); + CHECK(secp256k1_is_zero_array(guarded, r.u * 32)); + CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand))); + memset(bad_secshare, 0xff, sizeof(bad_secshare)); + testrand256(secrand); + CHECK(secp256k1_frost_enrollment_shares_gen(CTX, guarded, hash32, secrand, bad_secshare, &r.thresh_pk, r.ids, r.u, r.ids[0], r.new_id, r.n, (uint32_t)r.t) == 0); + CHECK(secp256k1_is_zero_array(secrand, sizeof(secrand))); + } +} + static const struct tf_test_entry tests_frost_enrollment[] = { CASE1(run_frost_enrollment_vectors_test), CASE1(run_frost_enrollment_reconstruction_test), @@ -823,6 +960,7 @@ static const struct tf_test_entry tests_frost_enrollment[] = { CASE1(run_frost_enrollment_fault_injection_test), CASE1(run_frost_enrollment_mismatch_test), CASE1(run_frost_enrollment_own_slot_test), + CASE1(run_frost_enrollment_contract_test), CASE1(run_frost_enrollment_invalid_params_test), CASE1(run_frost_enrollment_rejects_empty_set_test), CASE1(run_frost_enrollment_pubshare_derive_test),