jwp: a zstd payload over the cap is rejected, not truncated #288
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/jwp-zstd-decompression-cap"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
The defect
CompressionStrategydocuments its third argument as "max_outputcaps the decompressed size to prevent decompression bombs".ZstdCompression::decompressimplemented that as:Read::take(n)plusread_to_enddoes not detect the cap being hit. It yields exactlynbytes and reports success, discarding the rest — so the bomb case, the one conditionmax_outputexists 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.It is not confined to the strategy
AdaptiveCodec::decodecallsdecompress(&raw_payload, MAX_PAYLOAD_LEN)with a 16 MiB cap, and onOkgoes on to:payload_length: payload.len() as u32, commented "Normalize payload_length to decompressed size", andobserve_frame(payload.len(), ...).So a peer frame whose zstd payload expands to 20 MiB was accepted as a well-formed, complete 16 MiB frame, with the truncation recorded nowhere and the truncated length counted as the real payload in the energy/size profile.
The same trait, opposite semantics
Lz4Compression::decompresshas always returnedCompressionErrorfor exactly this condition:Only the lz4 side was tested (
lz4_rejects_oversized_decompression), with no zstd equivalent — which is why two implementations behind one trait diverged on the contract the trait exists to state.The fix
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 uses
saturating_addso amax_outputofusize::MAXcannot wrap it.Tests
zstd_rejects_oversized_decompression— mirrors the lz4 test. It also asserts the fixture actually exceeds the cap, so it cannot pass vacuously if the fixture ever shrinks.zstd_accepts_output_exactly_at_the_cap— pins the boundary the extra byte must not break.Falsification
The fix has two halves and each is load-bearing:
take(max_output)), keep the checkzstd_rejects_oversized_decompressionFAILED — the check can never firezstd_rejects_oversized_decompressionFAILED — returnsmax+1bytes asOkIn both,
zstd_accepts_output_exactly_at_the_cap,zstd_roundtripand the lz4 tests still passed, so the mutations are isolated to the behaviour under test.Verification
On the build host (aarch64), the CI-equivalent gate for
joule-code-rs:cargo check --workspace --all-targets— exit 0cargo test --workspace— exit 0, 0 failed suites, 22 suites with passing testscargo test -p jwp— 154 passed, 0 failed🤖 Generated with Claude Code
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 <[email protected]>