chilldkg: fix out-of-bounds write in recovery data parsing
secp256k1_chilldkg_deserialize_recovery_data read the threshold t from the first four bytes of the recovery blob and used it directly as the loop bound writing into the fixed-size sum_coms[SECP256K1_CHILLDKG_MAX_PARTICIPANTS] array. params_validate would reject an out-of-range t, but it only runs after the parse -- the overflow happens first. The reference implementation is safe only because Python lists grow dynamically; the C port lost that implicit bound. Reachable with attacker-supplied input from both public entry points, secp256k1_chilldkg_participant_recover and secp256k1_chilldkg_coordinator_recover (recovery data is untrusted by design: participants who never received cmsg2 are expected to accept recovery data from third parties). Confirmed with AddressSanitizer: t = 129 with a 4261-byte blob writes one group element past the array; t = 30000 with a ~1 MB blob writes ~1.2 MB of attacker- controlled group elements past it. The spec's own "invalid threshold" recovery vector cannot catch this because the memory-safe reference never exercises a t large enough to overflow a fixed C array. Fix: reject t < 1 and t > SECP256K1_CHILLDKG_MAX_PARTICIPANTS right after reading t, before any use as a loop bound, and reject recovery_len > SECP256K1_CHILLDKG_MAX_RECOVERY_LEN up front for consistency with the recovery-ack paths (the macro moves next to the parser; with t and n bounded the length cap is redundant, but it documents and enforces the invariant at the single choke point). Invalid recovery data is a plain input error (RecoveryDataError in the reference), so this maps to SECP256K1_CHILLDKG_INVALID_INPUT with outputs zeroed, like any other malformed blob.
This commit is contained in:
@@ -703,6 +703,10 @@ size_t secp256k1_chilldkg_investigation_msg_len(size_t n_participants) {
|
||||
return 65 * n_participants;
|
||||
}
|
||||
|
||||
/* Maximum length of the recovery data: 4 + 33*t + 162*n bytes with
|
||||
* t, n <= SECP256K1_CHILLDKG_MAX_PARTICIPANTS. */
|
||||
#define SECP256K1_CHILLDKG_MAX_RECOVERY_LEN (4 + 195 * SECP256K1_CHILLDKG_MAX_PARTICIPANTS)
|
||||
|
||||
/* Parse the self-delimiting recovery data (deserialize_recovery_data in
|
||||
* chilldkg_ref/chilldkg.py):
|
||||
* u32be(t) || sum_coms (33*t) || hostpubkeys (33*n) || pubnonces (33*n) ||
|
||||
@@ -717,10 +721,16 @@ static int secp256k1_chilldkg_deserialize_recovery_data(uint32_t *t_out, size_t
|
||||
const unsigned char *ptr;
|
||||
int overflow;
|
||||
|
||||
if (recovery_len < 4) {
|
||||
if (recovery_len < 4 || recovery_len > SECP256K1_CHILLDKG_MAX_RECOVERY_LEN) {
|
||||
return 0;
|
||||
}
|
||||
t = secp256k1_read_be32(recovery);
|
||||
/* Bound t before using it as a loop bound over the fixed-size sum_coms
|
||||
* array: the recovery data is untrusted input. (The reference gets this
|
||||
* implicitly from Python's dynamically-sized lists.) */
|
||||
if (t < 1 || t > SECP256K1_CHILLDKG_MAX_PARTICIPANTS) {
|
||||
return 0;
|
||||
}
|
||||
rest_len = recovery_len - 4;
|
||||
if (rest_len < 33 * t || (rest_len - 33 * t) % 162 != 0) {
|
||||
return 0;
|
||||
@@ -920,10 +930,6 @@ static void secp256k1_chilldkg_recovery_ack_message(unsigned char *msg, const un
|
||||
memcpy(msg + 37, recovery, recovery_len);
|
||||
}
|
||||
|
||||
/* The maximum length of recovery data: 4 + 33*t + 162*n with t, n <=
|
||||
* SECP256K1_CHILLDKG_MAX_PARTICIPANTS. */
|
||||
#define SECP256K1_CHILLDKG_MAX_RECOVERY_LEN (4 + 195 * SECP256K1_CHILLDKG_MAX_PARTICIPANTS)
|
||||
|
||||
/* Check that the recovery data parses and matches the given session
|
||||
* parameters (part of participant_recovery_ack_sign and
|
||||
* participant_recovery_acks_verify in the reference). Returns 1 on match, 0
|
||||
|
||||
Reference in New Issue
Block a user