From ceccb50a5ac54cdaaf56c7b9b57a9e4104fe08d3 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Mon, 31 Aug 2026 13:25:26 +0200 Subject: [PATCH] chilldkg: harden persisted-state bounds checks for noverify builds 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. --- src/modules/chilldkg/main_impl.h | 47 ++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/src/modules/chilldkg/main_impl.h b/src/modules/chilldkg/main_impl.h index 122911a4..d1002c5a 100644 --- a/src/modules/chilldkg/main_impl.h +++ b/src/modules/chilldkg/main_impl.h @@ -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);