Skip to content

Enforce max_body_size on the bzip2 path - #235

Open
youdie006 wants to merge 1 commit into
libwww-perl:masterfrom
youdie006:bzip2-max-body-size
Open

youdie006 wants to merge 1 commit into
libwww-perl:masterfrom
youdie006:bzip2-max-body-size

Conversation

@youdie006

Copy link
Copy Markdown

Compress::Raw::Bunzip2->new takes $limitOutput as its fifth positional argument; the fourth is $verbosity, which Compress::Raw::Bzip2 documents as "This parameter is ignored". decoded_content passes four (Message.pm:398-403), so the limit lands in $verbosity and is discarded.

Measured on master, one bzip2 round, max_body_size(1024):

bzip2 bomb (79 B compressed)  ->  8,389,632 octets returned
gzip  bomb (8 KB compressed)  ->  stopped

Passing the limit in slot 4 is byte-for-byte identical to passing no limit at all; only slot 5 restricts the output.

Two more things fall out of the same ten lines:

  • The branch seeds $output = "\0" x Bufsize to buy the allowance, because Bunzip2 has no Bufsize option and takes it from what $output already holds. With appendOutput on, bzinflate appends after that padding, so every bzip2 body decoded with a limit set comes back prefixed with max_body_size NUL octets. On master, 'hello world' under a 1024 limit returns 1035 octets.
  • Unlike Zlib, Bunzip2 reports BZ_OK (not BZ_OUTBUFF_FULL) when it stops at its allowance, so the existing BZ_OUTBUFF_FULL check never fires. With only the slot fixed, the bomb returns silently truncated content instead of croaking.

I noticed this while reading #231 - its changelog describes the limit as one "that gzip and bzip2 already honoured", and the bzip2 half of that is not the case. #231 does not touch this branch (Message.pm:396-430 vs its deflate hunks), so the two should not conflict, but this wants a rebase if #231 lands first.

The new test rows go into the existing t/message-decode-bzipbomb.t. The file's current rows pass on master for the wrong reason: the payload is "\0" x 16M, so the NUL padding is indistinguishable from the body, and the triple-bzip2 header makes the second round die on the corrupted intermediate. Dropping just the verbosity argument from my fix makes two of those pre-existing rows fail, which is what shows they were never testing the limit.

Verification

Perl 5.38.2, Compress::Raw::Bzip2 2.204001.

prove -l: 672 tests on master, 674 with this change. Four files fail identically in both (t/headers.t wants Test::Differences; t/message-brotli.t, t/message-decode-brotlibomb.t, t/message-decode-zipbomb.t want Test::Needs) - optional deps I could not install, none touching this path.

perltidy --profile=.perltidyrc --assert-tidy --no-standard-output --outfile=/dev/null - the command precious.toml issues - exits 0 on both changed files. App::perlvars and omegasort I could not install; the diff adds no package variables and does not touch .gitignore.

mutation result
master + the new rows 2 fail: bomb not stopped, body NUL-prefixed
drop the verbosity argument 3 fail, including a pre-existing row
drop the padding strip 1 fail, body NUL-prefixed
strip one octet too many 1 fail, 'ello world'
also croak on BZ_STREAM_END 1 fail, croaks on a body that fits

The no-limit path is unchanged: $pad is empty, the new croak is gated on LimitOutput, and the existing max_body_size(undef) assertion still passes.

Brotli I could not exercise - IO::Uncompress::Brotli is not installable here, and t/message-decode-brotlibomb.t fails on master for that reason too. Reading it, brotli passes its limit as an explicit unbro() argument, so it is not affected.

Disclosure: prepared with AI assistance (Claude). I reviewed it, reproduced the bypass and the NUL prefix against decoded_content directly, and verified the red-green and mutation rows myself.

Compress::Raw::Bunzip2->new takes $limitOutput as its fifth positional
argument; the fourth is $verbosity, which the module documents as ignored.
decoded_content passed only four arguments, so the limit landed in $verbosity
and was discarded: an 8 MB bomb decoded in full against a 1024 octet limit.

The same branch also seeds $output with Bufsize NUL octets to buy the
allowance, which Bunzip2 then appends after, so every bzip2 body decoded with
a limit set came back prefixed with that padding.

And unlike Zlib, Bunzip2 reports BZ_OK rather than BZ_OUTBUFF_FULL when it
stops at its allowance, so the existing BZ_OUTBUFF_FULL check never fired.
Croak on an unfinished stream under LimitOutput instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant