Skip to content

Commit 8981a1b

Browse files
fix(chain): cap block::Hash and CountedHeader preallocation at protocol-level constants (#10570)
`block::Hash::max_allocation` previously returned 65,535 (derived from `MAX_PROTOCOL_MESSAGE_LEN / 32`), and `CountedHeader::max_allocation` returned ~1,409 (same message-size-derived shape). Both are reachable via `Vec::zcash_deserialize` from any post-handshake peer through `getblocks`, `getheaders`, and `headers` messages. A peer can claim up to those counts and force `Vec::with_capacity` preallocation before any payload bytes are read. Cap at protocol-level constants: - `MAX_BLOCK_LOCATOR_LENGTH = 101`, a new `block`-module constant that matches Bitcoin Core's `MAX_LOCATOR_SZ` in `net_processing.cpp` (which zcashd inherits). - The existing `MAX_HEADERS_PER_MESSAGE = 160` constant from `crate::serialization::zcash_serialize`, already enforced on the sending side and at the codec level for `read_headers` (PR #10528). Same fix shape applied to `AddrV1`/`AddrV2` in PR #10494 for GHSA-xr93-pcq3-pxf8. CWE-770 (Allocation of Resources Without Limits or Throttling). Tests: - `block_hash_max_allocation` proptest updated to assert `Hash::max_allocation() == MAX_BLOCK_LOCATOR_LENGTH`. - `counted_header_max_allocation` proptest updated to assert `CountedHeader::max_allocation() == MAX_HEADERS_PER_MESSAGE as u64`. - Test-only constants `BLOCK_HEADER_LENGTH` and `MIN_COUNTED_HEADER_LEN`, which were used by the old message-size-derived formula in `header.rs`, moved into the `preallocate` test module where they are the only consumers. Co-authored-by: dingledropper <dingledropper@users.noreply.github.com>
1 parent b46207b commit 8981a1b

4 files changed

Lines changed: 83 additions & 38 deletions

File tree

CHANGELOG.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,16 @@ and this project adheres to [Semantic Versioning](https://semver.org).
1616
the per-type `max_allocation()` caps from PR #10494
1717
([GHSA-xr93-pcq3-pxf8](https://github.com/ZcashFoundation/zebra/security/advisories/GHSA-xr93-pcq3-pxf8)).
1818
CWE-770.
19+
- Cap `block::Hash::max_allocation` at `MAX_BLOCK_LOCATOR_LENGTH = 101`
20+
(matching Bitcoin Core's `MAX_LOCATOR_SZ` in `net_processing.cpp`) and
21+
`CountedHeader::max_allocation` at the existing
22+
`MAX_HEADERS_PER_MESSAGE = 160` constant (already enforced on the
23+
sending side and at the codec level for `read_headers`). The previous
24+
values were derived from `MAX_PROTOCOL_MESSAGE_LEN` and returned 65,535
25+
and ~1,409 respectively, allowing a post-handshake peer to force ~2 MiB
26+
of upfront `Vec` preallocation per `getblocks`/`getheaders` message
27+
before any payload bytes were read. Same fix shape as
28+
GHSA-xr93-pcq3-pxf8 for `AddrV1`/`AddrV2` (PR #10494). CWE-770.
1929

2030
### Added
2131

zebra-chain/src/block.rs

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use crate::{
1111
orchard,
1212
parameters::{Network, NetworkUpgrade},
1313
sapling,
14-
serialization::{TrustedPreallocate, MAX_PROTOCOL_MESSAGE_LEN},
14+
serialization::TrustedPreallocate,
1515
sprout,
1616
transaction::Transaction,
1717
transparent,
@@ -258,14 +258,30 @@ impl<'a> From<&'a Block> for Hash {
258258
}
259259
}
260260

261-
/// A serialized Block hash takes 32 bytes
262-
const BLOCK_HASH_SIZE: u64 = 32;
261+
/// The maximum number of `block::Hash` entries Zebra will preallocate for in
262+
/// a single peer-deserialized vector.
263+
///
264+
/// In the P2P protocol, `Vec<block::Hash>` appears as the `known_blocks` block
265+
/// locator in `getblocks` and `getheaders` messages. The Bitcoin/Zcash
266+
/// convention encodes locators with exponentially-spaced heights (1, 2, 3, …,
267+
/// 10, 20, 40, …, genesis), giving `~log2(N) + 10` entries for chain length N.
268+
/// For current Zcash chain heights (~3M blocks) a legitimate locator has ~32
269+
/// entries.
270+
///
271+
/// We cap at 101 to match Bitcoin Core's `MAX_LOCATOR_SZ` constant
272+
/// (`net_processing.cpp`), which zcashd inherits. This avoids any risk of
273+
/// rejecting legitimate locators sent by compatible nodes that follow the
274+
/// existing Bitcoin/Zcash protocol convention.
275+
///
276+
/// Without this cap, `Hash::max_allocation` was previously derived from
277+
/// `MAX_PROTOCOL_MESSAGE_LEN / 32 = 65,535`, which allowed a remote peer to
278+
/// force ~2 MiB heap preallocation per crafted `getblocks`/`getheaders` message
279+
/// before any payload was read. This is the same class as
280+
/// GHSA-xr93-pcq3-pxf8 (`addr_limit`), fixed for AddrV1/V2 in PR #10494.
281+
pub const MAX_BLOCK_LOCATOR_LENGTH: u64 = 101;
263282

264-
/// The maximum number of hashes in a valid Zcash protocol message.
265283
impl TrustedPreallocate for Hash {
266284
fn max_allocation() -> u64 {
267-
// Every vector type requires a length field of at least one byte for de/serialization.
268-
// Since a block::Hash takes 32 bytes, we can never receive more than (MAX_PROTOCOL_MESSAGE_LEN - 1) / 32 hashes in a single message
269-
((MAX_PROTOCOL_MESSAGE_LEN - 1) as u64) / BLOCK_HASH_SIZE
285+
MAX_BLOCK_LOCATOR_LENGTH
270286
}
271287
}

zebra-chain/src/block/header.rs

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use thiserror::Error;
88
use crate::{
99
fmt::HexDebug,
1010
parameters::Network,
11-
serialization::{TrustedPreallocate, MAX_PROTOCOL_MESSAGE_LEN},
11+
serialization::{TrustedPreallocate, MAX_HEADERS_PER_MESSAGE},
1212
work::{difficulty::CompactDifficulty, equihash::Solution},
1313
};
1414

@@ -152,27 +152,21 @@ pub struct CountedHeader {
152152
pub header: Arc<Header>,
153153
}
154154

155-
/// The serialized size of a Zcash block header.
156-
///
157-
/// Includes the equihash input, 32-byte nonce, 3-byte equihash length field, and equihash solution.
158-
const BLOCK_HEADER_LENGTH: usize =
159-
crate::work::equihash::Solution::INPUT_LENGTH + 32 + 3 + crate::work::equihash::SOLUTION_SIZE;
160-
161-
/// The minimum size for a serialized CountedHeader.
162-
///
163-
/// A CountedHeader has BLOCK_HEADER_LENGTH bytes + 1 or more bytes for the transaction count
164-
pub(crate) const MIN_COUNTED_HEADER_LEN: usize = BLOCK_HEADER_LENGTH + 1;
165-
166155
/// The Zcash accepted block version.
167156
///
168157
/// The consensus rules do not force the block version to be this value but just equal or greater than it.
169158
/// However, it is suggested that submitted block versions to be of this exact value.
170159
pub const ZCASH_BLOCK_VERSION: u32 = 4;
171160

172161
impl TrustedPreallocate for CountedHeader {
162+
/// Cap `CountedHeader` preallocation at the existing protocol-level
163+
/// constant `MAX_HEADERS_PER_MESSAGE = 160`. The previous return value was
164+
/// derived from `MAX_PROTOCOL_MESSAGE_LEN`, allowing peer-controlled
165+
/// preallocation amplification — same shape as GHSA-xr93-pcq3-pxf8 for
166+
/// `AddrV1`/`AddrV2` (PR #10494).
173167
fn max_allocation() -> u64 {
174-
// Every vector type requires a length field of at least one byte for de/serialization.
175-
// Therefore, we can never receive more than (MAX_PROTOCOL_MESSAGE_LEN - 1) / MIN_COUNTED_HEADER_LEN counted headers in a single message
176-
((MAX_PROTOCOL_MESSAGE_LEN - 1) / MIN_COUNTED_HEADER_LEN) as u64
168+
// Cast safe: MAX_HEADERS_PER_MESSAGE is the constant 160, which fits
169+
// trivially in u64 on every platform.
170+
MAX_HEADERS_PER_MESSAGE as u64
177171
}
178172
}

zebra-chain/src/block/tests/preallocate.rs

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,41 +5,58 @@ use std::sync::Arc;
55
use proptest::prelude::*;
66

77
use crate::{
8-
block::{
9-
header::MIN_COUNTED_HEADER_LEN, CountedHeader, Hash, Header, BLOCK_HASH_SIZE,
10-
MAX_PROTOCOL_MESSAGE_LEN,
8+
block::{CountedHeader, Hash, Header, MAX_BLOCK_LOCATOR_LENGTH},
9+
serialization::{
10+
arbitrary::max_allocation_is_big_enough, TrustedPreallocate, ZcashSerialize,
11+
MAX_HEADERS_PER_MESSAGE, MAX_PROTOCOL_MESSAGE_LEN,
1112
},
12-
serialization::{arbitrary::max_allocation_is_big_enough, TrustedPreallocate, ZcashSerialize},
1313
};
1414

15+
/// The serialized size of a Zcash block header.
16+
///
17+
/// Equihash input + 32-byte nonce + 3-byte equihash solution-length field +
18+
/// equihash solution. Used as a serialized-size lower bound by the
19+
/// `counted_header_min_length` proptest.
20+
const BLOCK_HEADER_LENGTH: usize =
21+
crate::work::equihash::Solution::INPUT_LENGTH + 32 + 3 + crate::work::equihash::SOLUTION_SIZE;
22+
23+
/// The minimum size for a serialized `CountedHeader`: header bytes plus at
24+
/// least one byte for the transaction count CompactSize.
25+
const MIN_COUNTED_HEADER_LEN: usize = BLOCK_HEADER_LENGTH + 1;
26+
1527
proptest! {
16-
/// Verify that the serialized size of a block hash used to calculate the allocation limit is correct
28+
/// Verify that the serialized size of a block hash is 32 bytes, matching the protocol spec.
1729
#[test]
1830
fn block_hash_size_is_correct(hash in Hash::arbitrary()) {
1931
let serialized = hash.zcash_serialize_to_vec().expect("Serialization to vec must succeed");
20-
prop_assert!(serialized.len() as u64 == BLOCK_HASH_SIZE);
32+
prop_assert!(serialized.len() as u64 == 32);
2133
}
2234

2335
/// Verify that...
24-
/// 1. The smallest disallowed vector of `Hash`s is too large to send via the Zcash Wire Protocol
25-
/// 2. The largest allowed vector is small enough to fit in a legal Zcash Wire Protocol message
36+
/// 1. `Hash::max_allocation` is exactly `MAX_BLOCK_LOCATOR_LENGTH`. The cap is intentionally
37+
/// far smaller than the message-size limit, to prevent peer-controlled preallocation
38+
/// amplification (sibling of GHSA-xr93-pcq3-pxf8).
39+
/// 2. The largest allowed vector still fits in a legal Zcash Wire Protocol message.
2640
#[test]
2741
fn block_hash_max_allocation(hash in Hash::arbitrary_with(())) {
2842
let (
2943
smallest_disallowed_vec_len,
30-
smallest_disallowed_serialized_len,
44+
_smallest_disallowed_serialized_len,
3145
largest_allowed_vec_len,
3246
largest_allowed_serialized_len,
3347
) = max_allocation_is_big_enough(hash);
3448

49+
// The cap is exactly the locator-protocol cap, not derived from message size.
50+
prop_assert!(Hash::max_allocation() == MAX_BLOCK_LOCATOR_LENGTH);
51+
3552
// Check that our smallest_disallowed_vec is only one item larger than the limit
3653
prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == Hash::max_allocation());
37-
// Check that our smallest_disallowed_vec is too big to send as a protocol message
38-
prop_assert!(smallest_disallowed_serialized_len > MAX_PROTOCOL_MESSAGE_LEN);
3954

4055
// Check that our largest_allowed_vec contains the maximum number of hashes
4156
prop_assert!((largest_allowed_vec_len as u64) == Hash::max_allocation());
57+
4258
// Check that our largest_allowed_vec is small enough to send as a protocol message
59+
// (this is now slack: the locator cap is much smaller than the message-size limit).
4360
prop_assert!(largest_allowed_serialized_len <= MAX_PROTOCOL_MESSAGE_LEN);
4461
}
4562

@@ -59,8 +76,12 @@ proptest! {
5976
#![proptest_config(ProptestConfig::with_cases(128))]
6077

6178
/// Verify that...
62-
/// 1. The smallest disallowed vector of `CountedHeaders`s is too large to send via the Zcash Wire Protocol
63-
/// 2. The largest allowed vector is small enough to fit in a legal Zcash Wire Protocol message
79+
/// 1. `CountedHeader::max_allocation` is exactly `MAX_HEADERS_PER_MESSAGE`. The cap is a
80+
/// protocol-level constant (160 headers per `headers` message — the Zcash convention
81+
/// Zebra already enforces on the sending side and at the codec level for `read_headers`)
82+
/// rather than a message-size-derived value, to prevent peer-controlled preallocation
83+
/// amplification (sibling of GHSA-xr93-pcq3-pxf8).
84+
/// 2. The largest allowed vector still fits in a legal Zcash Wire Protocol message.
6485
#[test]
6586
fn counted_header_max_allocation(header in any::<Arc<Header>>()) {
6687
let header = CountedHeader {
@@ -69,19 +90,23 @@ proptest! {
6990

7091
let (
7192
smallest_disallowed_vec_len,
72-
smallest_disallowed_serialized_len,
93+
_smallest_disallowed_serialized_len,
7394
largest_allowed_vec_len,
7495
largest_allowed_serialized_len,
7596
) = max_allocation_is_big_enough(header);
7697

98+
// The cap is exactly the headers-protocol cap, not derived from message size.
99+
// Cast safe: MAX_HEADERS_PER_MESSAGE is the constant 160 (well under u64::MAX).
100+
prop_assert!(CountedHeader::max_allocation() == MAX_HEADERS_PER_MESSAGE as u64);
101+
77102
// Check that our smallest_disallowed_vec is only one item larger than the limit
78103
prop_assert!(((smallest_disallowed_vec_len - 1) as u64) == CountedHeader::max_allocation());
79-
// Check that our smallest_disallowed_vec is too big to send as a protocol message
80-
prop_assert!(smallest_disallowed_serialized_len > MAX_PROTOCOL_MESSAGE_LEN);
81104

82105
// Check that our largest_allowed_vec contains the maximum number of CountedHeaders
83106
prop_assert!((largest_allowed_vec_len as u64) == CountedHeader::max_allocation());
107+
84108
// Check that our largest_allowed_vec is small enough to send as a protocol message
109+
// (this is now slack: the headers cap is much smaller than the message-size limit).
85110
prop_assert!(largest_allowed_serialized_len <= MAX_PROTOCOL_MESSAGE_LEN);
86111
}
87112
}

0 commit comments

Comments
 (0)