Skip to content

fix(cggmp24): don't panic on out-of-range signer index in validate_consistency - #236

Open
Joshuaisikah wants to merge 1 commit into
LFDT-Lockness:cggmp24/mfrom
Joshuaisikah:fix/key-share-index-out-of-bounds
Open

fix(cggmp24): don't panic on out-of-range signer index in validate_consistency#236
Joshuaisikah wants to merge 1 commit into
LFDT-Lockness:cggmp24/mfrom
Joshuaisikah:fix/key-share-index-out-of-bounds

Conversation

@Joshuaisikah

Copy link
Copy Markdown

Fixes #201

DirtyKeyShare::validate_consistency (in cggmp24/src/key_share.rs) indexed aux.N[usize::from(core.i)] directly. Both current callers only ever pass a core that's already been through is_valid() (or is a Valid<_> by type), so core.i is in range in practice today. But validate_consistency itself has no way to enforce that on its own — it's relying entirely on caller discipline, and a future caller (or a refactor that reorders the existing checks) could trivially reintroduce a panic here.

Changes

  • Replace the direct index with .get(usize::from(core.i)).ok_or(InvalidKeyShareReason::PartyIndexOutOfBounds)?
  • Add the PartyIndexOutOfBounds error variant
  • Add a regression test (validate_consistency_rejects_out_of_range_signer_index) that calls validate_consistency directly with a deliberately out-of-range core.i, bypassing the public API's guards, to prove the function is safe on its own rather than relying on the invariant its callers happen to uphold today. Verified locally that this test panics against the old code (index out of bounds: the len is 2 but the index is 5) and passes cleanly against the fix.

I also checked key-share/src/valid.rs, per the issue's hedge that "the similar problem may be present" there too — I didn't find any unguarded indexing in that file, so no changes were needed there.

Testing

cargo test -p cggmp24 --features all-curves,hd-wallet,spof
cargo clippy -p cggmp24 --features all-curves,hd-wallet,spof --lib -- --no-deps -D clippy::all -D clippy::unwrap_used -D clippy::expect_used
cargo fmt --check -p cggmp24

All pass.

…nsistency

`DirtyKeyShare::validate_consistency` indexed `aux.N[usize::from(core.i)]`
directly. Both current callers only ever pass a `core` that's already been
through `is_valid()` (or is a `Valid<_>` by type), so `core.i` is in range
in practice today. But `validate_consistency` itself has no way to enforce
that, and a future caller (or a refactor that reorders the checks) could
trivially reintroduce a panic here.

Replace the direct index with `.get(...).ok_or(PartyIndexOutOfBounds)`, and
add a regression test that calls `validate_consistency` directly with a
deliberately out-of-range `core.i`, bypassing the public API's guards, to
prove the function is safe on its own rather than relying on caller
discipline.

Also checked key-share/src/valid.rs per the issue's hedge ("may be present
there too") — found no unguarded indexing to fix there.

Fixes LFDT-Lockness#201

Signed-off-by: Joshua Isika <joshuaiska@gmail.com>
@Joshuaisikah
Joshuaisikah force-pushed the fix/key-share-index-out-of-bounds branch from fbe77fa to 554623d Compare August 6, 2026 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Potential panics in key share validation

1 participant