Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #231 +/- ##
==========================================
+ Coverage 94.44% 94.88% +0.44%
==========================================
Files 10 10
Lines 1260 1271 +11
Branches 351 358 +7
==========================================
+ Hits 1190 1206 +16
+ Misses 15 12 -3
+ Partials 55 53 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e429405 to
b1fa67e
Compare
| my $read = $z->read($output, $limit + 1); | ||
| return undef if !defined $read || $read < 0; | ||
| Carp::croak("Decoded content would be larger than $limit octets") | ||
| if $read > $limit; |
There was a problem hiding this comment.
I don't like the idea if reading an extra byte just to check if there is more data.
Would the following work instead?
my $read = $z->read( $output, $limit );
return undef if !defined $read || $read < 0;
Carp::croak("Decoded content would be larger than $limit octets") unless $z->eof;Question: would it make more sense to simple warn/carp?
There was a problem hiding this comment.
Thanks for looking at this. I dug into both points before replying.
On read($limit) + croak unless $z->eof instead of read($limit + 1):
On a modern perl the two are equivalent — I tested content sized exactly at the limit, one over, and well over, on both the zlib and raw-deflate paths (perl 5.42), and the eof check gave the same accept/croak verdict as +1 in every case.
The reason I kept +1 is the older perls this module still supports (MIN_PERL_VERSION is 5.008001). Checking $z->eof right after a fixed-count read depends on the decoder having detected end-of-stream, and that is the exact path the no-limit branch documents as unreliable:
Asking for a fixed number of octets [...] takes a different path through
IO::Uncompress::Base, and that path mis-reports the end of some raw deflate streams on perls older than 5.20.
So on a pre-5.20 perl an eof-based check risks a false "too large" croak on a raw-deflate body that is exactly at the limit. read($limit + 1) sidesteps eof entirely and decides purely on the byte count, which behaves the same across all supported perls. The cost is a single extra octet, so I'd lean towards keeping it — happy to reconsider if you'd rather drop pre-5.20 from the equation.
On warn/carp instead of croak:
I think croak is the right call here. max_body_size is a defence against decompression bombs, so once the content is known to exceed the limit we specifically do not want to finish decoding and hand back the full body — warning and continuing would still allocate the gigabytes the limit exists to prevent. It also keeps this branch consistent with the gzip and bzip2 branches, which already croak on overflow. Callers who want a non-fatal outcome can leave raise_error off, in which case decoded_content() traps the croak and returns undef with $@ set.
🤖 Review by Claude Code · Opus 4.8
decoded_content() did not apply max_body_size (or the global $HTTP::Message::MAXIMUM_BODY_SIZE) to "Content-Encoding: deflate", nor to its raw-deflate fallback, while gzip, x-gzip and bzip2 all honoured it. A hostile server could therefore bypass the limit entirely just by labelling a decompression bomb "deflate". Verified pre-patch: a 4GB bomb exhausts memory outright; post-patch it dies in 0.10s at 18MB RSS. Rather than reimplement inflate on Compress::Raw::Zlib, bound the existing IO::Uncompress decoder with a new _uncompress() helper. That keeps one decode path, so multi-member streams, trailing garbage and transparent pass-through all behave exactly as before and the cap is purely an added constraint. Confirmed byte-identical to the one-shot interface the unpatched code used, over eleven edge cases on perl 5.10, 5.12, 5.18 and 5.42. With a limit in force the decoder is asked for one octet more than the limit allows: enough to tell content that fits from content that does not, and never more memory than the caller has already agreed to hand over. With no limit the stream is read to its end a block at a time, the way the one-shot IO::Uncompress functions do. Asking for a fixed number of octets instead takes a different path through IO::Uncompress::Base, and that path mis-reports the end of some raw deflate streams on perls older than 5.20. Also reject a negative max_body_size. Each decoder reacted to one differently and some quietly returned no content at all, which is indistinguishable from a successful decode. max_body_size() and $HTTP::Message::MAXIMUM_BODY_SIZE were undocumented; they now have Pod covering the bomb rationale, the block-sized overshoot, which encodings enforce the limit, why base64 and quoted-printable are exempt, and that br cannot distinguish an over-limit body from a corrupt one. GH#206, reported by grr. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
b1fa67e to
6a4732d
Compare
Fixes #206. CVE-2026-65060.
decoded_content()did not applymax_body_size(or the global$HTTP::Message::MAXIMUM_BODY_SIZE) toContent-Encoding: deflate, nor to its raw-deflate fallback, whilegzip,x-gzipandbzip2all honoured it. A hostile or compromised server could bypass the limit entirely just by labelling a decompression bombdeflate.Measured, with a 1 MB limit set:
Out of memory in perl:util:safesysreallocApproach
The obvious fix is to reimplement inflate on top of
Compress::Raw::Zlib, matching what the gzip branch does. This does not do that. It keeps the existingIO::Uncompressdecoder and bounds it with a new_uncompress()helper.The reason is behavioural compatibility.
IO::Uncompress::InflateandCompress::Raw::Zlibdo not agree on multi-member streams, trailing garbage, or transparent pass-through of undecodable content. Swapping decoders would have changed all three as a side effect of a security fix. Bounding the existing one makes the cap a purely additional constraint.With a limit in force, the decoder is asked for exactly one octet more than the limit allows. That is enough to tell content that fits from content that does not, and it is never more memory than the caller has already agreed to hand over -- the same bound the gzip branch gets from
Bufsize => $content_limit.With no limit in force, the stream is read to its end a block at a time, which is what the one-shot
IO::Uncompressfunctions called by the previous code do internally. That split is not cosmetic: asking for a fixed number of octets takes a different path throughIO::Uncompress::Base, and that path mis-reports the end of some raw deflate streams on perls older than 5.20. The unlimited path was verified byte-identical to the one-shot interface across eleven edge cases (empty input, truncated streams, multi-member, trailing garbage, non-deflate content, high-ratio bombs, and so on) on perl 5.10, 5.12, 5.18 and 5.42, using theperldocker/perl-testerimages CI runs.Also in this change
A negative
max_body_sizeis now rejected. Each decoder reacted differently to one; the deflate path quietly returned no content at all, which is indistinguishable from a successful decode of an empty body. It now dies.max_body_size()and$HTTP::Message::MAXIMUM_BODY_SIZEare documented. They had no Pod at all. The new Pod covers the decompression-bomb rationale, the block-sized overshoot (so callers leave headroom), which encodings enforce the limit, whybase64andquoted-printableare exempt (neither can expand its input), and thatbrcannot distinguish an over-limit body from a corrupt one.Tests
t/message-decode-deflatebomb.tis new: 18 assertions covering both the zlib-deflate and raw-deflate forms.max_body_sizeparameter caps a body tooraise_error, an over-large body yieldsundefrather than a bombClient-WarningThat last one pins pre-existing behaviour rather than asserting what I first expected:
IO::Uncompress::RawInflatedefaults toTransparent => 1, so content that is not deflate at all comes back untouched. It is not amplified, so the cap has nothing to protect against there, and the test now documents that deliberately.Every cap assertion was confirmed to fail against the unpatched code before the fix went in. The suite passes on perl 5.10, 5.12, 5.18, 5.20 and 5.42.