fix: bound decompressed size to guard against decompression bombs#582
fix: bound decompressed size to guard against decompression bombs#582iemejia wants to merge 4 commits into
Conversation
`Codec::decompress` grew the output buffer without limit, so a small compressed block could inflate to an enormous buffer and exhaust memory (a "decompression bomb"): deflate/zstd/bzip2/xz decompressed the whole stream into a `Vec`, and snappy allocated `vec![0; decompressed_size]` from the (untrusted) block header. Bound every codec's output by the configured `max_allocation_bytes`: - deflate uses `decompress_to_vec_with_limit`, mapping the resulting `HasMoreOutput` status to a memory-allocation error; - snappy validates the header-declared size with `safe_len` before allocating; - zstd/bzip2/xz read through `Read::take(max + 1)` and reject an output that exceeds the budget. The limit stays configurable through `max_allocation_bytes`, consistent with the existing byte-length guards. Legitimate blocks within the budget still round-trip. Assisted-by: GitHub Copilot:claude-opus-4.8
There was a problem hiding this comment.
Pull request overview
This PR hardens Codec::decompress against decompression bombs by bounding decompressed output to the existing global max_allocation_bytes budget, and adds an integration test that exercises the new bounds across supported codecs.
Changes:
- Enforce
max_allocation_bytesas a hard cap for decompressed output across deflate, snappy, zstd, bzip2, and xz. - Map “would exceed limit” conditions to
Details::MemoryAllocationerrors (including deflate’sHasMoreOutput). - Add an integration test that lowers the global allocation budget and verifies oversized outputs are rejected while small payloads still round-trip.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| avro/src/codec.rs | Adds decompression output limits per codec to prevent unbounded allocations during decode. |
| avro/tests/decompression_bomb.rs | New integration test covering decompression-bomb rejection and a within-budget round-trip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BzDecoder::new(&stream[..]) | ||
| .take((max_bytes as u64).saturating_add(1)) | ||
| .read_to_end(&mut decoded) | ||
| .unwrap_or_else(|_| unreachable!("No I/O errors possible with Vec<u8>")); |
There was a problem hiding this comment.
Fixed in f1b049e: added a dedicated Details::Bzip2Decompress(std::io::Error) variant and replaced unwrap_or_else(unreachable!) with .map_err(Details::Bzip2Decompress)?, so corrupt bzip2 input returns an AvroResult error instead of aborting the process.
| XzDecoder::new(&stream[..]) | ||
| .take((max_bytes as u64).saturating_add(1)) | ||
| .read_to_end(&mut decoded) | ||
| .unwrap_or_else(|_| unreachable!("No I/O errors possible with Vec<u8>")); |
There was a problem hiding this comment.
Fixed in f1b049e: same treatment for xz — added Details::XzDecompress(std::io::Error) and map read_to_end errors to it via .map_err(...)? rather than panicking.
| let result = codec.decompress(&mut bomb); | ||
| assert!( | ||
| result.is_err(), | ||
| "a decompressed output exceeding the budget must be rejected, got {result:?}" | ||
| ); |
There was a problem hiding this comment.
Fixed in f1b049e: the test now matches on e.details() and asserts Details::MemoryAllocation { .. } specifically, so a failure for any other reason (codec bug/regression) no longer passes.
- bzip2/xz: propagate a decode error instead of panicking. read_to_end can fail on malformed/corrupt input, so replace unwrap_or_else(unreachable!) with dedicated Details::Bzip2Decompress / Details::XzDecompress errors, mapped via map_err, so a bad block returns an AvroResult error rather than aborting. - decompression_bomb test: assert the failure is specifically Details::MemoryAllocation, so a regression that fails for another reason cannot mask the missing bound. Assisted-by: GitHub Copilot:claude-opus-4.8
| match e.status { | ||
| FailedCannotMakeProgress => Error::from(ErrorKind::UnexpectedEof), | ||
| BadParam => Error::other("Unexpected error: miniz_oxide reported invalid output buffer size. Please report this to avro-rs developers."), // not possible for _to_vec() | ||
| Adler32Mismatch => Error::from(ErrorKind::InvalidData), | ||
| Failed => Error::from(ErrorKind::InvalidData), | ||
| Done => Error::other("Unexpected error: miniz_oxide reported an error with a success status. Please report this to avro-rs developers."), | ||
| NeedsMoreInput => Error::from(ErrorKind::UnexpectedEof), | ||
| HasMoreOutput => Error::other("Unexpected error: miniz_oxide has more data than the output buffer can hold. Please report this to avro-rs developers."), // not possible for _to_vec() | ||
| other => Error::other(format!("Unexpected error: {other:?}")) | ||
| FailedCannotMakeProgress => IoError::from(ErrorKind::UnexpectedEof), | ||
| BadParam => IoError::other("Unexpected error: miniz_oxide reported invalid output buffer size. Please report this to avro-rs developers."), // not possible for _to_vec() | ||
| Adler32Mismatch => IoError::from(ErrorKind::InvalidData), |
There was a problem hiding this comment.
Fixed in c682b65: updated the deflate BadParam message to a generic 'invalid parameter while decompressing' and dropped the stale // not possible for _to_vec() comment, since this path now uses decompress_to_vec_with_limit.
Addresses review:
- Feature-gate Details::Bzip2Decompress and Details::XzDecompress behind the
`bzip` and `xz` features (like the existing Snappy variants), so adding them
is not a breaking change for downstream crates that exhaustively match on
Details without those features enabled.
- Update the deflate BadParam message: it no longer references decompress_to_vec
("invalid output buffer size" / "not possible for _to_vec()") now that the
code uses decompress_to_vec_with_limit.
Assisted-by: GitHub Copilot:claude-opus-4.8
| let decompressed_size = snap::raw::decompress_len(&stream[..stream.len() - 4]) | ||
| .map_err(Details::GetSnappyDecompressLen)?; | ||
| // The decompressed size is taken from the (untrusted) block | ||
| // header, so bound it before allocating for it. | ||
| let decompressed_size = crate::util::safe_len(decompressed_size)?; |
There was a problem hiding this comment.
Fixed in ea34450: a Snappy block shorter than the trailing 4-byte CRC now computes data_end = stream.len().checked_sub(4) and returns a new (snappy-gated) Details::BadSnappyLength error, slicing via the checked offset — so a short/corrupt block returns an AvroResult error instead of underflowing stream.len() - 4 and panicking. Added a regression test over 0..4-byte inputs.
A Snappy block ends with a 4-byte CRC32. Decompression sliced the block with stream.len() - 4, which underflows (usize) and panics if a truncated or corrupt block is shorter than 4 bytes -- a decode failure turned into a process abort. Compute the data end with checked_sub(4) and return a new (snappy-gated) Details::BadSnappyLength error for a too-short block, slicing via the checked offset. Adds a regression test over 0..4 byte inputs. Assisted-by: GitHub Copilot:claude-opus-4.8
What
Codec::decompressgrew the output buffer without any limit, so a small compressed block could inflate to an enormous buffer and exhaust memory (a "decompression bomb"):miniz_oxide::inflate::decompress_to_vec, which grows the outputVecunbounded;vec![0; decompressed_size]wheredecompressed_sizecomes from the (untrusted) block header;Vecviaio::copy/read_to_end.A block byte buffer is capped at the allocation budget (
safe_lenin the container reader), but its decompressed output was not — so a ~few-KB block could expand to many GB.How
Bound every codec's decompressed output by the existing configurable
max_allocation_bytesbudget:decompress_to_vec_with_limit(stream, max_bytes)and map the resultingHasMoreOutputstatus to aMemoryAllocationerror.decompressed_sizewithsafe_lenbefore allocatingvec![0; …].Read::take(max_bytes + 1)and reject when the output exceeds the budget (memory stays bounded tomax_bytes + 1).The limit stays configurable through
max_allocation_bytes, consistent with the existing byte-length guards, and legitimate blocks within the budget still round-trip unchanged.Tests
avro/tests/decompression_bomb.rs(its own process so the write-once allocation limit can be lowered): for deflate, snappy, zstandard, bzip2 and xz, an 8 MiB-plaintext bomb that compresses to a few KB is rejected on decompress, while a small payload still round-trips.cargo fmt --checkandcargo clippy --tests(all features) are clean.Notes
This is a companion to #581 (bounding array/map element allocation); together they cap the two remaining unbounded-allocation paths in the decoder.