chilldkg: security review fixes

[LLFourn]

Summary

A security review of the chilldkg module in schnorr_fun (and adjacent code reached from its decoding paths). No critical vulnerabilities were found. The issues fall into two buckets:

1. Decoding panics — cases where malformed serialized input could crash the process rather than return an error.
2. Defensive hardening — places where the protocol was permissive in ways that didn't expose secrets but tolerated unintended behaviour (ghost contributions, x-only PoP replay, silently accepted unverified certs, redundant/confusing parameters, etc.).

Each commit is self-contained and independently reviewable.

Commit-by-commit

b41919d — Replace &'static str errors in chilldkg with typed error enums

Introduces simplepedpop::AddInputError, encpedpop::AddInputError, encpedpop::RecoverShareError, certpedpop::RecoverShareError. Replaces string-typed Result<_, &'static str> returns so callers can discriminate failures programmatically. Drops the unused CertifierError::DuplicateCertificate variant.

58d90d9 — Fix panics on malformed input in chilldkg and SharedKey decoding

Three panic paths reachable from untrusted input:

• encpedpop::Contributor::verify_agg_input indexed encryption_nonces[my_index] directly. A coordinator sending a shorter vec crashed the contributor; now returns ContributionDidntMatch.
• SharedKey::from_slice(&[]) and the bincode::Decode / serde::Deserialize impls panicked on empty polynomials via poly[0]. Now return None / a decode error.
• simplepedpop::Contributor::gen_keygen_input indexed secret_poly[0] when called with threshold == 0. Now asserts threshold > 0 matching Coordinator::new.

b638920 — Enforce contributor count across chilldkg layers

A malicious coordinator can craft an AggKeygenInput with more key_contrib slots than any honest contributor agreed to (a "ghost" contribution whose secret the coordinator knows). Doesn't compromise secrecy but gives the coordinator extra influence over the public key and breaks the protocol's implicit "every contributor signed off" contract.

• simplepedpop::Contributor now stores n_contributors at construction and verify_agg_input rejects mismatched key_contrib.len().
• encpedpop::Contributor::verify_agg_input additionally rejects mismatched encryption_nonces.len().
• certpedpop::Certifier::new becomes fallible and rejects an AggKeygenInput whose contributor count doesn't match contributor_keys.len(), with new variant CertifierError::ContributorCountMismatch.

4ff15a0 — Bind simplepedpop proof-of-possession to its slot index and parity

The PoP previously signed the empty message. Concrete replay primitives this allowed:

• (com, pop) from one slot replayed at a different slot.
• (−com, pop) accepted under BIP340 x-only verification even though the caller intended com.

The PoP message is now parity_byte || idx_be (5 bytes), carried under the BIP340-style 33-byte domain separator "BIP DKG/pop". Parity binding closes the x-only gap — (−A, pop_A) reconstructs a different message and fails verify. This diverges from the bip-frost-dkg Python reference's choice to bind only idx via BIP340's tag_prefix; we bind more and use the BIP340-recommended message-prefix domain separator. Regression test covers cross-slot replay and negated-key-same-slot replay.

21ce300 — Remove serialization from CertifiedKeygen

The only sound path to a CertifiedKeygen is Certifier::finish, where every stored certificate was verified as it came in. Deserialization bypassed that and could land attacker-chosen gammas in the struct; vrf_security_check would then hand back a beacon value the attacker controlled. Dropping the bincode::Encode/Decode and serde Serialize/Deserialize derives on CertifiedKeygen eliminates the unverified-state path. Apps that need to persist a certified keygen can serialize AggKeygenInput and the certificate map separately and rebuild on the receiving side.

b1e1c4e — Validate share index and encryption key in receive_secret_share

encpedpop::receive_secret_share silently tolerated two malformed inputs: a missing entry in encrypted_shares (treated as a zero ciphertext via unwrap_or_default) and a caller-supplied keypair that didn't match the registered encryption key. Both surfaced as the generic InvalidSecretShare, diverging from the sibling AggKeygenInput::recover_share path which already returned UnknownShareIndex / WrongEncryptionKey. Adds both variants to ReceiveShareError and checks them explicitly before decryption.

9c34719 — Always verify signatures in Certifier::receive_certificate

On a duplicate cert (same key already registered), the function returned Ok without calling verify_cert on the new signature. A debug_assert_eq! guarded the "sigs must match" case but vanished in release builds, so a mismatched second signature was silently discarded while the caller believed the cert was validated. For randomized signature / VRF schemes this was the always-taken path. Now every supplied signature is verified; a verified duplicate is idempotent (first stored wins), a failed verification returns InvalidSignature.

Conversation History

User: can you check for security issues in chilldkg's implementation in schnorr_fun. I am especially concerned with parties not checking their inputs and included in things.

Assistant: (Flagged 7 concerns: panic in encpedpop verify_agg_input; ghost contributor injection; receive_secret_share keypair validation; PoP not bound to context; Certifier silent duplicate-cert; vrf_security_check trusts unverified gammas; same-keypair-for-encryption-and-cert design note.)

User: cool fix #1 and then we'll discuss #2.

User: actually before moving on to #2 can you search for similar problems especially in decoding implementations that could cause panics.

Assistant: (Found SharedKey::from_slice(&[]) panic, bincode Decode / serde Deserialize empty-poly panics, gen_keygen_input threshold=0 panic.)

User: Do you think that #2 is a security vulnerability? Can you explain the attack.

Assistant: (Ghost injection: count defense missing means malicious coordinator can add slots with known-secret "ghost" contributions. Not a secrecy break but adaptive-bias on the public key; protocol doesn't enforce "N authorized contributors.")

User: right but the thing is that in practice the coordinator is always a contributor as well... But I agree it's sort of worth fixing since technically the coordinator role itself is not meant to have that ability.

User: why couldn't the attacker just echo back the signature shares contributions of the honest party too (except negated). They are homomorphic.

Assistant: (Walked through why the negation attack doesn't close into key compromise: encpedpop pads are H(dh_key, enc_key, dest) hash outputs, not linearly composable. The attack remains a targeted DoS; encryption-layer non-linearity keeps the downstream damage bounded.)

User: Ok can you look up the python referenmce for the FROST chilldkg and see what they throw in the PoP and we can see if we can follow it.

(— bip-frost-dkg signs idx.to_bytes(4, big) under tag "BIP DKG/pop message". schnorr_fun uses BIP340 message-prefix domain separation instead of tag_prefix. Bound idx here.)

User: @simplepedpop.rs I feel like [the x-only same-slot replay passing] is not a very desirable behaviour.

(— the test acknowledged that negated-key-same-slot passed PoP and was only caught by verify_agg_input's Point-equality self-check. User wanted the PoP itself to bind parity. Extended the PoP message to parity_byte || idx_be.)

User: maybe we just shouldn't allow [CertifiedKeygen] to be serialized?

(— chose the simplest fix for vrf_security_check's reliance on unverified gammas: drop the derive.)

User: A just drop it [the n_contributors param from receive_secret_share]. When you're receiving a secret share the only thing you care about is it's the correct secret share with respect to the join sharing polynomial.

(— decided receive_secret_share's n_contributors parameter was redundant with agg_input; receivers' defense against count tampering is the cert round or out-of-band comparison, not a per-receive assertion. Folded into b638920.)

User: What is the actual security implications of duplicate keys though in general... I am not really interested in adding more errors I am more interested in avoiding them when the protocol could tolerate duplicates.

(— audited for panics / spurious errors on duplicate keys across Certifier::required_keys, certificates, and related paths; found none; concluded the BTreeSet dedup is intentional and benign. No code changes.)

[nickfarrow]

Noticed that b638920 says

    - simplepedpop::receive_secret_share takes n_contributors and rejects
      length mismatches with a new ReceiveShareError::WrongContributorCount.

but that was not implemented.

I think this is ok since a malicious coordinator inflating the contributor count should achieve nothing given the security model. The dangerous situation would be if a coordinator were able to inflate the receiver count, in frostsnap we check this count in the keygen check.

[nickfarrow]

This function verifies certificates for the expected parties in cert_keys, but propagates the caller-supplied certificate map which could contain extra (cert_key, certificate)s, which is then included in the VRF beacon.

I don't think this is exploitable with how we are currently using it in Frostsnap, devices & coordinator independently build their own certificate and only include known participants. But it is a rough edge that could be solved by filtering on cert_keys or asserting length of cert keys matches certificate map length in finalize.

[LLFourn]

eh we don't need a "sanity check" polluting the API. We could turn this into simpler new function which just takes a list of certificiates.

[LLFourn]

feels like the n_contributors and n_encryption_nonces should already be equal here. This is checking the validity of agg_input implicitly but it should already be correct. agg_input should just have a method n_contributors() so you don't have to call inner().

In order to enforce the invariant correctly we'll have to make WireAggkeygenInput that is serializable and has an invariant checker that converts it to AggKeygenInput.

[LLFourn]

Seems to me that this error variant is only used in one of the functions and is no tused in receive_certificate even though it returns CertifierError. We should make the errors tight.

[LLFourn]

shouldn't we check that the certified keygen obtained from this is eq to the one above.

[LLFourn]

these are defined here but never used in this file. "EncryptionKey" should be a clue. They should be defined in encpedpop.

[LLFourn]

This should call simplepedpop's AggKeygenInput's n_contributors.

[LLFourn]

I think we should just put the is_event_y as the first param to avoid the impression we're rehashing the whole point.

[LLFourn]

@nickfarrow thanks for catching the stale commit message. I probably asked it to undo it because it wasn't doing anything. Effectively, trying to tighten security so that much more things are checked automatically was a bit of an odessy. The current implementation has a very loose API and relies on the checking symmetric view accross parties for security. Since security always rests on that check this is not really wrong but this API could do with a lot more defense in-depth. There were various and subtle ways of using it insecurely most of which we avoided in frostsnap but it would be much more confident if the API didn't have any.

I built #248 on top of this one but it might be better just to review #248 in its entireity rather than continueing tto review this one. The purpose of #248 is to create a less flexible but absoluitely impossible to fuck up API.