From c6520f8e54a859281bc9ee66ada47b11adf39701 Mon Sep 17 00:00:00 2001 From: Transaction Science Date: Sat, 17 Oct 2026 04:36:53 -0400 Subject: [PATCH] jwp: a zstd payload over the cap is rejected, not truncated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `CompressionStrategy` trait documents its third argument as "`max_output` caps the decompressed size to prevent decompression bombs". `ZstdCompression::decompress` implemented that as decoder.take(max_output as u64).read_to_end(&mut output)?; Ok(output) `Read::take(n)` plus `read_to_end` does not detect the cap being hit. It yields exactly `n` bytes and reports success, discarding the rest, so the bomb case — the one condition `max_output` exists for — came back as a value rather than a failure, and the caller could not tell a complete payload from a truncated prefix of an arbitrarily larger one. The consequence is not confined to the strategy. `AdaptiveCodec::decode` calls `decompress(&raw_payload, MAX_PAYLOAD_LEN)` with a 16 MiB cap, and on `Ok` goes on to rewrite the header — `payload_length: payload.len() as u32`, commented "Normalize payload_length to decompressed size" — and to fold that length into the connection statistics via `observe_frame`. A peer frame whose zstd payload expands to 20 MiB was therefore accepted as a well-formed, complete 16 MiB frame, with the truncation recorded nowhere. `Lz4Compression::decompress`, behind the same trait, has always returned `CompressionError` for exactly this condition, so the two implementations had opposite semantics. Only the lz4 side was tested (`lz4_rejects_oversized_decompression`), which is why the divergence was invisible. Read one byte past the cap and reject when it arrives. That distinguishes "exactly at the cap", which is valid and still returns the whole payload, from "over the cap", which is now an error — and brings zstd into line with lz4. The limit is computed with `saturating_add` so a `max_output` of `usize::MAX` cannot wrap it. Tests: `zstd_rejects_oversized_decompression` mirrors the lz4 test and asserts on the fixture actually exceeding the cap, so it cannot pass vacuously; `zstd_accepts_output_exactly_at_the_cap` pins the boundary the extra byte must not break. Co-Authored-By: Claude Opus 5 --- .../crates/jwp/src/compression.rs | 54 ++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/joule-code/joule-code-rs/crates/jwp/src/compression.rs b/joule-code/joule-code-rs/crates/jwp/src/compression.rs index 2ee717436..9b5ef859f 100644 --- a/joule-code/joule-code-rs/crates/jwp/src/compression.rs +++ b/joule-code/joule-code-rs/crates/jwp/src/compression.rs @@ -114,11 +114,27 @@ impl CompressionStrategy for ZstdCompression { let mut output = Vec::with_capacity(data.len().min(max_output)); use std::io::Read; + // One byte past the cap, so the cap being hit is detectable. + // + // `take(max_output)` alone yields exactly `max_output` bytes and + // reports success, discarding the rest: the bomb case — the one + // condition `max_output` exists for — came back as a value, and the + // caller could not tell a complete payload from a truncated prefix of + // an arbitrarily larger one. Reading one extra byte distinguishes + // "exactly at the cap" (valid) from "over it" (rejected), and matches + // `Lz4Compression::decompress`, which has always rejected this. + let limit = (max_output as u64).saturating_add(1); decoder - .take(max_output as u64) + .take(limit) .read_to_end(&mut output) .map_err(|e| JwpError::CompressionError(e.to_string()))?; + if output.len() > max_output { + return Err(JwpError::CompressionError(format!( + "decompressed size exceeds max {max_output}" + ))); + } + Ok(output) } @@ -312,4 +328,40 @@ mod tests { let result = c.decompress(&compressed, 10); assert!(result.is_err()); } + + /// The zstd half of the trait contract — "`max_output` caps the + /// decompressed size to prevent decompression bombs". + /// + /// This used to return `Ok` with the first `max_output` bytes, so a + /// payload that expands past the cap was accepted as a complete one and + /// `AdaptiveCodec::decode` went on to rewrite the frame header to the + /// truncated length. Only the lz4 strategy was tested, which is why the + /// two implementations behind one trait had opposite semantics. + #[test] + fn zstd_rejects_oversized_decompression() { + let c = ZstdCompression::default(); + let compressed = c.compress(TEST_DATA).unwrap(); + assert!( + TEST_DATA.len() > 10, + "the fixture has to exceed the cap for this to test anything" + ); + let result = c.decompress(&compressed, 10); + assert!( + result.is_err(), + "a payload expanding past the cap must be rejected, not truncated: {:?}", + result.map(|v| v.len()) + ); + } + + /// The boundary the extra byte must not break: output exactly at the cap + /// is valid and comes back whole. + #[test] + fn zstd_accepts_output_exactly_at_the_cap() { + let c = ZstdCompression::default(); + let compressed = c.compress(TEST_DATA).unwrap(); + let out = c + .decompress(&compressed, TEST_DATA.len()) + .expect("exactly at the cap is within the cap"); + assert_eq!(out, TEST_DATA); + } } -- 2.54.0 (Apple Git-157)