fix: bound MerkleProof path length on deserialize#3902
Conversation
MerkleProof::read trusted path_len from the peer/API and called Vec::with_capacity without a cap, so a huge path_len could panic with capacity overflow or force a large allocation. Reject path lengths above 64 (well beyond any legitimate O(log n) proof) and cap preallocation. Also apply the same 100k single-read limit to StreamingReader that BinReader already enforces. Related to segment decode bounds (mimblewimble#3827 / mimblewimble#3850).
| return Err(ser::Error::TooLargeReadErr); | ||
| } | ||
| // Cap preallocation even when path_len is within the allowed range. | ||
| let mut path = Vec::with_capacity(min(path_len, MERKLE_PROOF_PATH_PREALLOC) as usize); |
There was a problem hiding this comment.
With path_len capped at 64, the full allocation is at most 2 KiB. Could we allocate path_len directly and drop the second limit and the min import?
| /// Read a fixed number of bytes. | ||
| fn read_fixed_bytes(&mut self, len: usize) -> Result<Vec<u8>, Error> { | ||
| // Match BinReader / BufReader: refuse huge single reads that would OOM or panic. | ||
| if len > 100_000 { |
There was a problem hiding this comment.
This is now the third copy of the 100k reader limit. Could we use one shared constant and add a direct StreamingReader boundary test here?
| } | ||
|
|
||
| #[test] | ||
| fn merkle_proof_read_rejects_huge_path_len() { |
There was a problem hiding this comment.
Could we also cover the accepted side of the boundary: 64 hashes accepted and 65 rejected? The current tests only exercise rejection.
|
|
||
| #[test] | ||
| fn merkle_proof_read_rejects_capacity_overflow_path_len() { | ||
| // Historical bug: path_len near u64::MAX caused Vec::with_capacity to panic. |
There was a problem hiding this comment.
Could we trim these tests a little? The names already describe the cases, and the historical context is covered by the PR description. A compact bounds test covering 64, 65, and u64::MAX would avoid the duplicated setup.
2 participants
Summary
MerkleProof::readused the untrustedpath_lendirectly inVec::with_capacity. A crafted proof with a huge path length could panic (capacity overflow) or attempt a massive allocation — the same class of issue fixed for PMMR segments in #3850 / reported in #3827.Changes
TooLargeReadErrfor oversizedpath_lenbefore allocatingStreamingReader::read_fixed_byteswith BinReader’s existing 100k single-read limitu64::MAXpath lengthsTest plan
cargo test -p grin_core --test merkle_proofcargo test -p grin_core --test segment