frost/secp256k1-zkp
3
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

chilldkg: harden persisted-state bounds checks for noverify builds

Browse Source 
The four opaque-state loaders (participant state1, state2,
coordinator state, inv_data) guarded the t/n/participant_id fields
read from the serialized object with VERIFY_CHECK, which is compiled
out in noverify (release) builds. chilldkg.md explicitly recommends
persisting state1 and the coordinator state between rounds, which
turns the persisted file into an attack surface: a state1 with n
altered to 100000 and the magic bytes left intact crashed
participant_step2 with out-of-bounds array indexing in a noverify
build.

Promote all four guards to ARG_CHECK, consistent with the magic-byte
check right above them: the illegal-argument callback fires, the load
returns 0, and the public entry points (participant_step2,
participant_finalize, coordinator_finalize, participant_investigate)
return SECP256K1_CHILLDKG_INVALID_INPUT before any state-derived size
is used -- verified in both verify and noverify builds.
This commit is contained in:
Kgothatso Ngako
2026-08-31 13:25:26 +02:00
parent c43f645f15
commit ceccb50a5a
1 changed files with 30 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

47
src/modules/chilldkg/main_impl.h
View File

@@ -102,10 +102,13 @@ static int secp256k1_chilldkg_participant_inv_data_load(const secp256k1_context
ptr += 4;
inv_data_i->simpl.participant_id = secp256k1_read_be32(ptr);
ptr += 4;
/* The remaining contents were written by inv_data_save. */
VERIFY_CHECK(inv_data_i->simpl.n >= 1
&& inv_data_i->simpl.n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS
&& inv_data_i->simpl.participant_id < inv_data_i->simpl.n);
/* The remaining contents were written by inv_data_save. The bounds check
* must not be a VERIFY_CHECK: a persisted inv_data object can be tampered
* with, and in noverify builds the check would be compiled out and the
* out-of-range n would index fixed-size arrays below and in callers. */
ARG_CHECK(inv_data_i->simpl.n >= 1
&& inv_data_i->simpl.n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS
&& inv_data_i->simpl.participant_id < inv_data_i->simpl.n);
secp256k1_scalar_set_b32(&inv_data_i->simpl.secshare, ptr, NULL);
ptr += 32;
/* This load always succeeds; call it unconditionally (it must not sit
@@ -163,11 +166,14 @@ static int secp256k1_chilldkg_participant_state1_load(const secp256k1_context *c
ptr += 4;
state_i->simpl_state.participant_id = secp256k1_read_be32(ptr);
ptr += 4;
/* The remaining contents were written by state1_save. */
VERIFY_CHECK(state_i->simpl_state.t >= 1
&& state_i->simpl_state.t <= state_i->simpl_state.n
&& state_i->simpl_state.n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS
&& state_i->simpl_state.participant_id < state_i->simpl_state.n);
/* The remaining contents were written by state1_save. The bounds check
* must not be a VERIFY_CHECK: a persisted state object can be tampered
* with, and in noverify builds the check would be compiled out and the
* out-of-range n would index fixed-size arrays in callers. */
ARG_CHECK(state_i->simpl_state.t >= 1
&& state_i->simpl_state.t <= state_i->simpl_state.n
&& state_i->simpl_state.n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS
&& state_i->simpl_state.participant_id < state_i->simpl_state.n);
/* This load always succeeds; call it unconditionally (it must not sit
* inside VERIFY_CHECK, which is compiled out in noverify builds). */
if (!secp256k1_chilldkg_point_load(&state_i->simpl_state.com_to_secret, ptr)) {
@@ -226,10 +232,13 @@ static int secp256k1_chilldkg_participant_state2_load(const secp256k1_context *c
ptr += 4;
state_i->n = secp256k1_read_be32(ptr);
ptr += 4;
/* The remaining contents were written by state2_save. */
VERIFY_CHECK(state_i->t >= 1
&& state_i->t <= state_i->n
&& state_i->n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS);
/* The remaining contents were written by state2_save. The bounds check
* must not be a VERIFY_CHECK: a persisted state object can be tampered
* with, and in noverify builds the check would be compiled out and the
* out-of-range t/n would index fixed-size arrays in callers. */
ARG_CHECK(state_i->t >= 1
&& state_i->t <= state_i->n
&& state_i->n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS);
memcpy(state_i->eq_input, ptr, sizeof(state_i->eq_input));
ptr += sizeof(state_i->eq_input);
memcpy(state_i->dkg_output.secshare32, ptr, 32);
@@ -569,10 +578,14 @@ static int secp256k1_chilldkg_coordinator_state_load(const secp256k1_context *ct
ptr += 4;
state_i->n = secp256k1_read_be32(ptr);
ptr += 4;
/* The remaining contents were written by coordinator_state_save. */
VERIFY_CHECK(state_i->t >= 1
&& state_i->t <= state_i->n
&& state_i->n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS);
/* The remaining contents were written by coordinator_state_save. The
* bounds check must not be a VERIFY_CHECK: a persisted state object can
* be tampered with, and in noverify builds the check would be compiled
* out and the out-of-range t/n would index fixed-size arrays in
* callers. */
ARG_CHECK(state_i->t >= 1
&& state_i->t <= state_i->n
&& state_i->n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS);
memcpy(state_i->eq_input, ptr, sizeof(state_i->eq_input));
ptr += sizeof(state_i->eq_input);
memcpy(state_i->thresh_pk33, ptr, 33);