From bc0ffc6391ad07ccb45af478ddd2e8775a1cc255 Mon Sep 17 00:00:00 2001 From: Matthias Wright Date: Wed, 17 Jun 2026 17:18:44 +0800 Subject: [PATCH 1/3] fix: bound decode allocation by decoded elements, not remaining bytes --- types/src/consensus_state.rs | 44 +++++++++++++++++++++++++++++------- types/src/dynamic_epocher.rs | 24 +++++++++++++++++++- types/src/withdrawal.rs | 29 ++++++++++++++++++++++-- 3 files changed, 86 insertions(+), 11 deletions(-) diff --git a/types/src/consensus_state.rs b/types/src/consensus_state.rs index 070d7045..10d6dddb 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() { @@ -1647,6 +1651,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 +1676,29 @@ 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 no + // deposit bodies present, decode must bail cheaply on EndOfBuffer. + 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 + // No deposit bodies follow. + + // DepositRequest::read_cfg rejects the empty body with an Insufficient + // bytes error rather than EndOfBuffer; 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()); + } + 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..85911b8c 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,21 @@ 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 + // No segment bodies follow. + + 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..6df8de61 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,23 @@ 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 + // No pubkey bodies follow. + + let result = WithdrawalQueue::read(&mut buf.as_ref()); + assert!(matches!(result, Err(Error::EndOfBuffer))); + } } From 56e5e32382ec4720b2137a69a5f513573f84f634 Mon Sep 17 00:00:00 2001 From: Matthias Wright Date: Tue, 23 Jun 2026 12:55:54 +0800 Subject: [PATCH 2/3] fix: bound captured_bytes decode allocation by remaining bytes --- types/src/consensus_state.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/types/src/consensus_state.rs b/types/src/consensus_state.rs index 10d6dddb..19121cea 100644 --- a/types/src/consensus_state.rs +++ b/types/src/consensus_state.rs @@ -1453,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)?; @@ -1699,6 +1706,25 @@ mod tests { 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 From d4873d923c68be40e7c1f37eeeb3a1b272bcbc3a Mon Sep 17 00:00:00 2001 From: Matthias Wright Date: Tue, 30 Jun 2026 21:03:30 +0800 Subject: [PATCH 3/3] test: feed partial bodies so decode-allocation guards exercise the bug --- types/src/consensus_state.rs | 18 +++++++++++------- types/src/dynamic_epocher.rs | 8 +++++++- types/src/withdrawal.rs | 7 ++++++- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/types/src/consensus_state.rs b/types/src/consensus_state.rs index 19121cea..7a90d489 100644 --- a/types/src/consensus_state.rs +++ b/types/src/consensus_state.rs @@ -1690,18 +1690,22 @@ mod tests { // 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 no - // deposit bodies present, decode must bail cheaply on EndOfBuffer. + // 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 - // No deposit bodies follow. - - // DepositRequest::read_cfg rejects the empty body with an Insufficient - // bytes error rather than EndOfBuffer; either way decode must fail - // cheaply rather than pre-allocate ~4 billion slots. + // 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()); } diff --git a/types/src/dynamic_epocher.rs b/types/src/dynamic_epocher.rs index 85911b8c..e3fe5a64 100644 --- a/types/src/dynamic_epocher.rs +++ b/types/src/dynamic_epocher.rs @@ -744,7 +744,13 @@ mod tests { let mut buf = BytesMut::new(); buf.put_u64(0); // current_epoch buf.put_u32(u32::MAX); // claims ~4 billion segments - // No segment bodies follow. + // 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 6df8de61..e28dd8c2 100644 --- a/types/src/withdrawal.rs +++ b/types/src/withdrawal.rs @@ -1640,7 +1640,12 @@ mod tests { buf.put_u32(1); // schedule_len = 1 buf.put_u64(0); // schedule entry epoch buf.put_u32(u32::MAX); // claims ~4 billion scheduled pubkeys - // No pubkey bodies follow. + // 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)));