bug: Vec deserializer preallocates full capacity from untrusted varint before reading data

[mpguerra]

Credit: @dingledropper for reporting this issue

This issue was created with help from Claude.ai

What happened?

I expected to see this happen:
zcash_deserialize_external_count should allocate memory proportionally to data actually read, as zcashd does via chunked resize() interleaved with reads (serialize.h:761–777).

Instead, this happened:
zcash_deserialize_external_count calls Vec::with_capacity(external_count) in a single upfront allocation based on the peer-supplied varint, before reading any element data. A crafted message with an inflated count forces the full allocation regardless of how much data follows. For block::Hash (32 bytes × 65,535 = ~2 MiB), this is reachable via read_getblocks and read_getheaders from any peer that has completed the version/verack handshake.

This is the same defect class fixed for AddrV1/AddrV2 in PR #10494 (GHSA-xr93-pcq3-pxf8), but those fixes capped the per-type max_allocation() rather than addressing the root cause in the deserializer.

---

What were you doing when the issue happened?

Code audit of TrustedPreallocate impls following the AddrV1/V2 fix in PR #10494.

The root cause is in zebra-chain/src/serialization/zcash_deserialize.rs:96:

let mut vec = Vec::with_capacity(external_count);  // full upfront allocation
for _ in 0..external_count {
    vec.push(T::zcash_deserialize(&mut reader)?);   // data read happens after
}

zcashd's equivalent (serialize.h:761–777) avoids this by growing the vector in chunks interleaved with reads, so memory tracks data actually received rather than the claimed count:

while (nMid < nSize) {
    nMid += 5000000 / sizeof(T);
    if (nMid > nSize) nMid = nSize;
    v.resize(nMid);
    for (; i < nMid; i++)
        Unserialize(is, v[i]);
}

A minimal Rust equivalent is to cap the initial allocation:

let mut vec = Vec::with_capacity(external_count.min(1024));
for _ in 0..external_count {
    vec.push(T::zcash_deserialize(&mut reader)?);
}

This fixes the root cause once in the core deserializer rather than requiring each TrustedPreallocate impl to be individually audited and capped. The Vec grows naturally via push() as real data arrives — the performance cost is negligible (a handful of amortized reallocation-copies vs. per-element deserialization cost).

TrustedPreallocate and max_allocation() remain as defense-in-depth to reject clearly bogus counts early.

---

Zebra Version

zebrad 4.4.1 (commit 1ec1078)

---

Additional information

• GHSA-xr93-pcq3-pxf8 — the AddrV1/AddrV2 sibling, fixed in PR fix(zebrad): apply combined security fixes #10494
• PR fix(network): Enforce 160-entry cap in read_headers #10528 — MAX_HEADERS_PER_MESSAGE cap for read_headers (v4.4.0)
• zcashd serialize.h:761–777 — chunked deserialization pattern that prevents this class of issue

[zmanian]

Adding one sibling P2P preallocation shape from our local audit: this issue covers zcash_deserialize_external_count() allocating from a peer-supplied element count, but zebra-network also reserves frame-buffer capacity from the 24-byte P2P message header before any body bytes arrive.

Specifically, after parsing a valid external message header, zebra-network/src/protocol/external/codec.rs rejects body lengths above MAX_PROTOCOL_MESSAGE_LEN, then calls src.reserve(body_len + HEADER_LEN) before the body is present. A peer can send a valid header with body_len = MAX_PROTOCOL_MESSAGE_LEN and then stall; Codec::decode() returns Ok(None) while the framed-read buffer has grown toward the declared 2 MiB body.

I added a local current-behavior proof named header_only_max_body_len_reserves_full_body_capacity_today and reran it today:

cargo test -p zebra-network header_only_max_body_len_reserves_full_body_capacity_today --lib
...
test protocol::external::codec::tests::vectors::header_only_max_body_len_reserves_full_body_capacity_today ... ok

This is bounded by connection limits and timeouts, so I would treat it as public availability hardening rather than private disclosure. The likely fix is similar in spirit to this issue: avoid reserving the whole peer-declared size up front; grow incrementally or reserve only a bounded chunk while waiting for body progress.

[dingledropper]

Thanks for the sibling find @zmanian — agreed the codec-level reserve is the same shape and worth a separate fix. I'll keep my PR scoped to zcash_deserialize_external_count per Pili's framing in this issue. Happy to coordinate on the codec.rs side if you'd like — feel free to file that as its own PR (your local test + audit work makes you the natural owner there) or tag me if you'd prefer I take it.

Thanks much, dingledropper

[zmanian]

I'll provide a PR for this.