diff --git a/types/src/consensus_state.rs b/types/src/consensus_state.rs index 070d7045..7a90d489 100644 --- a/types/src/consensus_state.rs +++ b/types/src/consensus_state.rs @@ -1248,7 +1248,14 @@ impl Read for ConsensusState { let latest_height = buf.try_get_u64().map_err(|_| Error::EndOfBuffer)?; let deposit_queue_len = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize; - let mut deposit_queue = VecDeque::with_capacity(deposit_queue_len.min(buf.remaining())); + // Never pre-size collections from the decoded length prefixes below: + // they are attacker-controlled u32s, and `buf.remaining()` is a byte + // count rather than an element count, so even a `min(buf.remaining())` + // hint over-allocates by `size_of::()` per slot before any element + // is validated. Growing on push is safe — each loop reads from `buf` + // and bails on the first `EndOfBuffer`, so only genuinely decoded + // elements are ever allocated. + let mut deposit_queue = VecDeque::new(); for _ in 0..deposit_queue_len { deposit_queue.push_back(DepositRequest::read_cfg(buf, &())?); } @@ -1257,8 +1264,7 @@ impl Read for ConsensusState { let protocol_param_changes_len = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize; - let mut protocol_param_changes = - Vec::with_capacity(protocol_param_changes_len.min(buf.remaining())); + let mut protocol_param_changes = Vec::new(); for _ in 0..protocol_param_changes_len { protocol_param_changes.push(crate::protocol_params::ProtocolParam::read_cfg(buf, &())?); } @@ -1287,7 +1293,7 @@ impl Read for ConsensusState { for _ in 0..added_validators_len { let key = buf.try_get_u64().map_err(|_| Error::EndOfBuffer)?; let validator_count = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize; - let mut validators = Vec::with_capacity(validator_count.min(buf.remaining())); + let mut validators = Vec::new(); for _ in 0..validator_count { let node_key = PublicKey::read_cfg(buf, &())?; let consensus_key = bls12381::PublicKey::read_cfg(buf, &())?; @@ -1301,8 +1307,7 @@ impl Read for ConsensusState { // Read removed_validators let removed_validators_len = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize; - let mut removed_validators = - Vec::with_capacity(removed_validators_len.min(buf.remaining())); + let mut removed_validators = Vec::new(); for _ in 0..removed_validators_len { removed_validators.push(PublicKey::read_cfg(buf, &())?); } @@ -1310,8 +1315,7 @@ impl Read for ConsensusState { // Read pending_execution_requests let pending_execution_requests_len = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize; - let mut pending_execution_requests = - Vec::with_capacity(pending_execution_requests_len.min(buf.remaining())); + let mut pending_execution_requests = Vec::new(); for _ in 0..pending_execution_requests_len { let len = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize; if len > buf.remaining() { @@ -1449,6 +1453,13 @@ impl Read for ConsensusState { let has_captured = buf.try_get_u8().map_err(|_| Error::EndOfBuffer)? != 0; let captured_bytes = if has_captured { let len = buf.try_get_u32().map_err(|_| Error::EndOfBuffer)? as usize; + // `len` is an attacker-controlled u32; reject it against the actual + // remaining bytes before allocating, so a malformed blob with a huge + // captured length fails cheaply instead of pre-allocating `len` + // bytes (mirrors the pending_execution_requests guard above). + if len > buf.remaining() { + return Err(Error::EndOfBuffer); + } let mut bytes = vec![0u8; len]; buf.try_copy_to_slice(&mut bytes) .map_err(|_| Error::EndOfBuffer)?; @@ -1647,6 +1658,7 @@ mod tests { use alloy_eips::eip4895::Withdrawal; use alloy_primitives::Address; + use bytes::BytesMut; use commonware_codec::{DecodeExt, Encode, ReadExt}; use commonware_consensus::types::{Epoch, Epocher, Height}; use commonware_cryptography::{Signer, bls12381, ed25519}; @@ -1671,6 +1683,52 @@ mod tests { } } + #[test] + fn test_decode_huge_deposit_queue_count_does_not_preallocate() { + // Regression guard for treating a length prefix as a safe element + // count. `deposit_queue_len` is an attacker-controlled u32; the decoder + // must reject a bogus count by running out of buffer, not by pre-sizing + // a VecDeque from it. (`buf.remaining()` is a byte count, so a + // count-derived capacity — even one capped by remaining bytes — + // over-allocates by size_of::() per slot.) With the + // count far exceeding the available bodies, decode must bail cheaply. + let mut buf = BytesMut::new(); + buf.put_u64(0); // epoch + buf.put_u64(0); // view + buf.put_u64(0); // latest_height + buf.put_u32(u32::MAX); // claims ~4 billion deposits + // Provide a partial deposit body, then truncate. This leaves a non-zero + // `buf.remaining()` at the allocation point, so the original + // `with_capacity(len.min(buf.remaining()))` would have over-allocated + // `remaining`-many slots here rather than the degenerate zero. + buf.put_slice(&[0u8; 48]); + + // DepositRequest::read_cfg runs out of buffer partway through the body; + // either way decode must fail cheaply rather than pre-allocate ~4 + // billion slots. + let result = ConsensusState::read(&mut buf.as_ref()); + assert!(result.is_err()); + } + + #[test] + fn test_decode_huge_captured_bytes_len_does_not_preallocate() { + // Regression guard for the captured-snapshot trailer: `captured_bytes` + // is a length-prefixed Vec, so a malformed blob with + // has_captured = true and a huge length must be rejected against the + // remaining bytes before the `vec![0u8; len]` allocation, not after. + let mut encoded = ConsensusState::default().encode().to_vec(); + // A default state has captured_bytes = None, so the encoding ends with + // the has_captured presence flag (0). Flip it to 1 and append a u32 + // length claiming ~4 GiB with no captured body following. + let last = encoded.len() - 1; + encoded[last] = 1; + encoded.extend_from_slice(&u32::MAX.to_be_bytes()); + + // Decode must bail cheaply on EndOfBuffer rather than pre-allocate ~4 GiB. + let result = ConsensusState::read(&mut encoded.as_slice()); + assert!(result.is_err()); + } + fn create_test_deposit_request(index: u64, amount: u64) -> DepositRequest { let mut withdrawal_credentials = [0u8; 32]; withdrawal_credentials[0] = 0x01; // Eth1 withdrawal prefix diff --git a/types/src/dynamic_epocher.rs b/types/src/dynamic_epocher.rs index b682cf47..e3fe5a64 100644 --- a/types/src/dynamic_epocher.rs +++ b/types/src/dynamic_epocher.rs @@ -244,7 +244,12 @@ impl Read for DynamicEpocher { if segments_len == 0 { return Err(Error::Invalid("DynamicEpocher", "no segments")); } - let mut segments = Vec::with_capacity(segments_len.min(buf.remaining())); + // Do not size the Vec from `segments_len`: it is an attacker-controlled + // u32 and `buf.remaining()` is a byte count, not an element count, so a + // bounded hint could still over-allocate by `size_of::()`. Let + // the Vec grow as elements are actually decoded; the loop bails on the + // first `EndOfBuffer`, so only genuine segments are ever allocated. + let mut segments = Vec::new(); for _ in 0..segments_len { let start_epoch = Epoch::new(buf.try_get_u64().map_err(|_| Error::EndOfBuffer)?); let start_height = Height::new(buf.try_get_u64().map_err(|_| Error::EndOfBuffer)?); @@ -727,4 +732,27 @@ mod tests { let result = DynamicEpocher::read(&mut buf.as_ref()); assert!(result.is_err()); } + + #[test] + fn test_decode_huge_segment_count_does_not_preallocate() { + // `segments_len` is an attacker-controlled u32; the decoder must reject + // a bogus count by running out of buffer, not by pre-sizing a Vec from + // it. (The buffer is a byte count, so a count-derived capacity — even + // one capped by remaining bytes — over-allocates by size_of::() + // per slot.) With the count far exceeding the available bodies, decode + // must bail cheaply on EndOfBuffer. + let mut buf = BytesMut::new(); + buf.put_u64(0); // current_epoch + buf.put_u32(u32::MAX); // claims ~4 billion segments + // Provide exactly one valid segment body, then truncate. This leaves a + // non-zero `buf.remaining()` at the allocation point, so the original + // `with_capacity(len.min(buf.remaining()))` would have over-allocated + // `remaining`-many slots here rather than the degenerate zero. + buf.put_u64(0); // segment[0] start_epoch + buf.put_u64(0); // segment[0] start_height + buf.put_u64(1); // segment[0] length (non-zero so it decodes, not the body that bails) + + let result = DynamicEpocher::read(&mut buf.as_ref()); + assert!(matches!(result, Err(Error::EndOfBuffer))); + } } diff --git a/types/src/withdrawal.rs b/types/src/withdrawal.rs index ac663f8e..e28dd8c2 100644 --- a/types/src/withdrawal.rs +++ b/types/src/withdrawal.rs @@ -624,7 +624,10 @@ impl Read for WithdrawalQueue { "scheduled epoch has no withdrawals", )); } - let mut pubkeys = VecDeque::with_capacity(pubkeys_len.min(buf.remaining())); + // `pubkeys_len` is an attacker-controlled u32 and `buf.remaining()` + // is a byte count, not an element count, so a bounded hint could + // still over-allocate. Grow as pubkeys are decoded instead. + let mut pubkeys = VecDeque::new(); for _ in 0..pubkeys_len { let mut pubkey = [0u8; 32]; buf.try_copy_to_slice(&mut pubkey) @@ -650,7 +653,10 @@ impl Read for WithdrawalQueue { "scheduled epoch has no withdrawals", )); } - let mut pubkeys = VecDeque::with_capacity(pubkeys_len.min(buf.remaining())); + // `pubkeys_len` is an attacker-controlled u32 and `buf.remaining()` + // is a byte count, not an element count, so a bounded hint could + // still over-allocate. Grow as pubkeys are decoded instead. + let mut pubkeys = VecDeque::new(); for _ in 0..pubkeys_len { let mut pubkey = [0u8; 32]; buf.try_copy_to_slice(&mut pubkey) @@ -1620,4 +1626,28 @@ mod tests { assert_eq!(queue.count_for_epoch(6), 1); assert_eq!(queue.get_for_epoch(6)[0].pubkey, [1u8; 32]); } + + #[test] + fn test_decode_huge_schedule_pubkey_count_does_not_preallocate() { + // A scheduled-pubkey count is an attacker-controlled u32; the decoder + // must reject a bogus count by exhausting the buffer, not by pre-sizing + // the VecDeque from it (the buffer is a byte count, so a count-derived + // capacity over-allocates by 32 bytes per slot). With the count far + // exceeding the available pubkey bodies, decode must bail cheaply. + let mut buf = BytesMut::new(); + buf.put_u64(0); // next_index + buf.put_u32(0); // withdrawals_len = 0 + buf.put_u32(1); // schedule_len = 1 + buf.put_u64(0); // schedule entry epoch + buf.put_u32(u32::MAX); // claims ~4 billion scheduled pubkeys + // Provide exactly one full pubkey body, then truncate. This leaves a + // non-zero `buf.remaining()` at the allocation point, so the original + // `with_capacity(pubkeys_len.min(buf.remaining()))` would have + // over-allocated `remaining`-many slots here rather than the + // degenerate zero; decode still bails on the second (missing) pubkey. + buf.put_slice(&[0u8; 32]); + + let result = WithdrawalQueue::read(&mut buf.as_ref()); + assert!(matches!(result, Err(Error::EndOfBuffer))); + } }