From 7a35fde10f34430d408d86ef7f2490e0cd4a96df Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Fri, 4 Sep 2026 11:35:59 +0200 Subject: [PATCH] frost_enrollment: correct the zeroing contract and harden the CI guard The two findings from the second review round, plus the coverage gap and comment error it noted. Finding 8: the zeroing exception was narrower in the docs than in the code. The previous commit's fix leaves shares32_out untouched on EVERY params_are_valid rejection -- duplicate ids, threshold < 2, new_id among the helpers, an id >= n, the mode bounds -- but the header scoped the exception to "when n_ids itself is out of range", and frost_enrollment.md still said flatly that outputs are zeroed on every failure path. A caller who inspects the buffer after a failed call would, on a duplicate-id or t = 1 rejection, read whatever was there before. In this protocol that can be additive shares of a secret from an earlier call. Both now state the actual invariant: not written when the parameter tuple is rejected, because its extent is only known to be u*32 once n_ids has been validated against the rest of the tuple; zeroed on every failure found after that point (an unusable thresh_pk, my_id not among the helpers, an invalid secret share). The .md adds the sentence a defensive caller needs: treat a rejected tuple as leaving the buffer unmodified, not as leaving it zeroed. Documentation only -- the implementation and the tests already agreed with this reading. Finding 9: the new CMake CI step never checked that the modules landed. It configured, built and installed with all five module flags, then ran `ls -RlAh` -- which prints and does not assert -- and linked examples/ecdsa.c, which uses core symbols only. Deleting the add_compile_definitions or PUBLIC_HEADER line for a module during an upstream sync would keep the step green: no module code means no undefined references, and the install simply omits the header. The guard added one commit ago caught a MISORDERED block but not a MISSING one. The step now asserts both halves. `ls` of all five module headers fails the step under bash -e if any is absent, and examples/frost_enrollment.c is compiled and run against the installed library, which no header-only check would cover -- it needs the symbols to actually be in the library. Verified by running the step's exact command sequence under bash -e: it exits 0 as written, and against an install configured WITHOUT the modules the header assertion exits 2 and the example fails to compile on the missing include. Coverage: the params-hash attribution path was still only tested where identifier equals index. The previous commit pinned identifier semantics for the scalar-overflow path on helper set {0, 2}, but share_agg's other `*mismatch_id = ids[i]` -- the parameters-hash disagreement -- was exercised only on {0, 1}. It is now pinned on {1, 3}, corrupting each slot in turn so the mapping is fixed at both ends: index 0 must report 1, index 1 must report 3. Verified load-bearing by changing the assignment to `(uint32_t)i`, which fails the new case. Comment error: the {0, 2} overflow case carried the claim that it would catch an implementation returning array indices. It would not -- it corrupts slot 0, whose identifier is also 0. The discrimination comes from the paired own-slot case at slot 1 (identifier 2), and the comment now says so. Correcting the record rather than rewriting it: commit 69766dd3's message says the suite goes "from 12 cases to 16" with "six new tests". Both counts are wrong. The contract test landed in 266c6a7c, so the table went 13 -> 17, and that commit added four cases (api, infinity, no_side_effects, max_size) alongside the x = 0 addition to an existing test and the two de-confoundings. The commits are unpushed and could be amended, but the review cites them by hash, so the numbers are corrected here instead. With this commit the module has 17 tests. Verification: warning-free build; 17/17 at -i=16, -i=200 and -i=1000; ./tests, ./noverify_tests and ./exhaustive_tests exit 0; `make check` 12/12; the module clean under valgrind (0 errors from 0 contexts); ctime_tests clean under valgrind; CMake ctest 542/542; vectors.h still regenerates byte for byte, so nothing here touched a frozen encoding. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 10 ++++++ include/secp256k1_frost_enrollment.h | 9 ++--- .../frost_enrollment/frost_enrollment.md | 11 +++++++ src/modules/frost_enrollment/tests_impl.h | 33 +++++++++++++++++-- 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0fa84ce0..c88480ef 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -854,3 +854,13 @@ jobs: run: | cmake -B ${{ env.CI_BUILD }} -DCMAKE_INSTALL_PREFIX=${{ env.CI_INSTALL }} -DSECP256K1_EXPERIMENTAL=ON -DSECP256K1_ENABLE_MODULE_FROST=ON -DSECP256K1_ENABLE_MODULE_CHILLDKG=ON -DSECP256K1_ENABLE_MODULE_ICEBERG=ON -DSECP256K1_ENABLE_MODULE_PREFRACTAL=ON -DSECP256K1_ENABLE_MODULE_FROST_ENROLLMENT=ON && cmake --build ${{ env.CI_BUILD }} && cmake --install ${{ env.CI_BUILD }} && ls -RlAh ${{ env.CI_INSTALL }} gcc -o ecdsa examples/ecdsa.c -I ${{ env.CI_INSTALL }}/include -L ${{ env.CI_INSTALL }}/lib*/ -l secp256k1 -Wl,-rpath,"${{ env.CI_INSTALL }}/lib",-rpath,"${{ env.CI_INSTALL }}/lib64" && ./ecdsa + # Assert the modules actually landed. `ls -RlAh` above only prints, + # so without these a regression that silently drops a module from + # the CMake build -- a deleted add_compile_definitions or + # PUBLIC_HEADER line during an upstream sync -- would keep this step + # green: no module code means no undefined references, and the ecdsa + # link exercises core symbols only. + ls ${{ env.CI_INSTALL }}/include/secp256k1_frost.h ${{ env.CI_INSTALL }}/include/secp256k1_chilldkg.h ${{ env.CI_INSTALL }}/include/secp256k1_iceberg.h ${{ env.CI_INSTALL }}/include/secp256k1_prefractal.h ${{ env.CI_INSTALL }}/include/secp256k1_frost_enrollment.h + # And that the symbols are really in the library, not just the + # headers on disk. + gcc -o frost_enrollment examples/frost_enrollment.c -I ${{ env.CI_INSTALL }}/include -L ${{ env.CI_INSTALL }}/lib*/ -l secp256k1 -Wl,-rpath,"${{ env.CI_INSTALL }}/lib",-rpath,"${{ env.CI_INSTALL }}/lib64" && ./frost_enrollment diff --git a/include/secp256k1_frost_enrollment.h b/include/secp256k1_frost_enrollment.h index 8cee13d9..91344776 100644 --- a/include/secp256k1_frost_enrollment.h +++ b/include/secp256k1_frost_enrollment.h @@ -172,10 +172,11 @@ 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`. 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. + * shares, aligned with `ids`. Not written at all when + * the parameter tuple is rejected, since its extent is + * only known to be u*32 once n_ids has been validated + * against the rest of the tuple; zeroed on every + * failure detected after that point. * params_hash32_out: pointer to a 32-byte array for the parameters hash, * identical to what * `secp256k1_frost_enrollment_params_hash` returns for diff --git a/src/modules/frost_enrollment/frost_enrollment.md b/src/modules/frost_enrollment/frost_enrollment.md index 6e2f2568..b02f0210 100644 --- a/src/modules/frost_enrollment/frost_enrollment.md +++ b/src/modules/frost_enrollment/frost_enrollment.md @@ -324,3 +324,14 @@ operations. Every branch driven by identifiers, counts or hash comparisons operates on public values. Secret intermediates are cleansed with `secp256k1_memclear_explicit` before the functions return, and outputs are zeroed on every failure path. + +The one exception is `shares32_out` in +`secp256k1_frost_enrollment_shares_gen`, the only output in this module whose +size is caller-supplied. When the parameter tuple is rejected, that buffer is +left untouched rather than zeroed: its extent is only known to be `u*32` once +`n_ids` has been validated against the rest of the tuple, so zeroing first +would write past a buffer whose length the caller got wrong. Failures +detected after validation — an unusable `thresh_pk`, `my_id` not among the +helpers, an invalid secret share — do zero it. Callers that inspect an output +buffer after a failed call must therefore treat a rejected tuple as leaving it +unmodified, not as leaving it zeroed. diff --git a/src/modules/frost_enrollment/tests_impl.h b/src/modules/frost_enrollment/tests_impl.h index 56183a39..dd6485ca 100644 --- a/src/modules/frost_enrollment/tests_impl.h +++ b/src/modules/frost_enrollment/tests_impl.h @@ -911,8 +911,10 @@ static void run_frost_enrollment_contract_test(void) { /* 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. */ + * The helper set is {0, 2}; note that it is the SECOND case below, which + * corrupts slot 1 (identifier 2), that discriminates -- slot 0's + * identifier is also 0, so on its own it would pass for an + * implementation returning array indices. */ frost_enrollment_test_deal(&r, 3, 2, 2, 1); CHECK(r.ids[0] == 0 && r.ids[1] == 2); frost_enrollment_test_round1_gen(&r); @@ -935,6 +937,33 @@ static void run_frost_enrollment_contract_test(void) { CHECK(mismatch_id == r.ids[1]); CHECK(mismatch_id == 2); + /* The other attribution path -- a disagreeing parameters hash -- also has + * to name the identifier. Its own tests use the helper set {0, 1}, where + * every identifier equals its index, so it is pinned here instead on + * {1, 3}: the corrupted slot is index 0 and must be reported as 1. */ + frost_enrollment_test_deal(&r, 5, 2, 2, 4); + { + static const uint32_t skewed[2] = { 1, 3 }; + frost_enrollment_test_use_helpers(&r, skewed, 2); + frost_enrollment_test_round1_gen(&r); + frost_enrollment_test_collect(&r, 1, all_shares, received); + memset(&received[0], 0xa5, 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 == 1); + CHECK(secp256k1_is_zero_array(sigma, sizeof(sigma))); + /* And the same corruption one slot along, to fix the mapping at both + * ends: index 1, identifier 3. */ + frost_enrollment_test_collect(&r, 0, all_shares, received); + memset(&received[32], 0xa5, 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[0], r.new_id, r.n, (uint32_t)r.t) == 0); + CHECK(mismatch_id == r.ids[1]); + CHECK(mismatch_id == 3); + } + /* 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);