Harden DeBin against untrusted length prefixes (CWE-770 unbounded allocation, plus String overflow panic)

[li-jin-quan]

Harden DeBin deserialization against untrusted length prefixes (CWE-770 unbounded allocation + overflow panic)

Summary

DeBin (the binary format deserializer) reads a length prefix from the input and, for Vec<T>, HashSet<T> and HashMap<K, V>, uses it to preallocate capacity before a single element is read. A malformed 8-byte input can therefore force allocations of gigabytes — or abort the process outright — while contributing essentially no data. This PR also fixes an integer overflow in String's bounds check that turns a usize::MAX length into a panic.

Details

Lengths are deserialized via DeBin for usize, which reads an 8-byte LE u64 from the untrusted buffer. For collections:

// before (src/serde_bin.rs, DeBin for Vec<T>)
let len: usize = DeBin::de_bin(o, d)?;      // attacker-controlled, up to 2^64-1
let mut out = Vec::with_capacity(len);      // allocates len * size_of::<T>() up front
for _ in 0..len {
    out.push(DeBin::de_bin(o, d)?)          // truncation is only detected here,
}                                            // after the allocation already happened

The same unchecked pattern exists in HashSet::with_capacity(len) and HashMap::with_capacity(len). By contrast, the LinkedList, BTreeSet and BTreeMap impls in the same file already use ::new() and grow from actual data, and String bounds-checks the declared length — so this looks like an oversight in exactly the three impls that preallocate.

The overflow. String's check *o + len > d.len() can itself overflow on 64-bit targets (e.g. *o = 1, len = usize::MAX → the sum wraps to 8), after which &d[*o..(*o + len)] panics with "slice index starts at 9 but ends at 8". In release builds without overflow checks this is a reachable panic on malformed input.

Impact

Deserializing untrusted binary data is a normal use case for DeBin (network peers, user-supplied save files / documents, IPC payloads). With a counting global allocator, an 8-byte truncated buffer:

impl	before	after
Vec<u8> (declares 64 MiB)	67,108,864-byte single allocation	124 bytes, then Err
HashMap<u8, u8> (declares 64 MiB entries)	402,657,123-byte single allocation	544 bytes, then Err
HashSet<u8> (declares 64 MiB entries)	268,439,403-byte single allocation	544 bytes, then Err
String (declares usize::MAX at offset 1)	panic (slice index)	Err

Amplification is bounded only by u64::MAX * size_of::<T>(). Declaring more than physical memory aborts the process in handle_alloc_error — verified: an 8-byte input declaring 1 TiB for Vec<u8> exits with 0xC0000409 (Windows abort); catch_unwind cannot intercept it. A service deserializing peer-supplied payloads can be killed by a single small message.

Fix

No API change, no magic-number caps: the three collection impls now grow from the data actually present (::new() + push/insert), consistent with the existing LinkedList / BTreeSet / BTreeMap impls, and String uses checked_add for the end offset. Valid inputs produce identical results; only the preallocation is gone (a few geometric re-growths for large legitimate vectors).

Testing

New tests/untrusted_alloc.rs uses a counting #[global_allocator] to assert that peak allocation stays proportional to the actual input (< 8 KiB) and that the overflow case returns Err instead of panicking. All checks run in a single #[test] because the global allocator is process-global and parallel tests would pollute the counters. Verified bidirectionally: with the fix stashed, the test fails (peak 67,112,831 bytes); with the fix applied, the full cargo test --lib --tests suite passes.

Happy to adjust if you would prefer a bounded try_reserve instead of pure incremental growth.

Reporter: Li Jinquan — github.com/li-jin-quan