You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "tomasilluminati (via GitHub)" <gi...@apache.org> on 2026/06/28 18:43:51 UTC
[PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
tomasilluminati opened a new pull request, #777:
URL: https://github.com/apache/commons-compress/pull/777
Adds output-counting guard BombGuardCompressorInputStream that caps decompressed size and an optional mean compression-ratio check with a configurable grace. Off by default, opt-in via CompressorStreamFactory setters. CompressorInputStream now implements InputStreamStatistics. Tests and a changes.xml entry are included.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
Posted by "tomasilluminati (via GitHub)" <gi...@apache.org>.
tomasilluminati commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-4827865784
@garydgregory
Yeah, I'm with you on the footgun. The ratio is genuinely hard to tune and easy to get wrong, exactly the "works for bar, blows up on foo" trap, so I'd rather not hand people a sharp tool they can't aim.
Nice pointer on setAfterRead, hadn't thought of it. That actually means neither piece needs a new class. The size cap is setMaxCount plus a throwing onMaxCount, and if the ratio ever comes back it can ride along as a setAfterRead consumer on the same BoundedInputStream.
So my lean is to keep it minimal, wire an absolute decompressed-size limit into CompressorStreamFactory via BoundedInputStream, drop the bomb class and the exception, and leave the ratio out for now. We can always document it as a recipe later.
Happy to let it simmer and hear what @ppkarwasz thinks before I change anything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add a decompressed size limit to CompressorStreamFactory [commons-compress]
Posted by "tomasilluminati (via GitHub)" <gi...@apache.org>.
tomasilluminati commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-5754887997
Pushed the second version. It is the shape we settled on: size limit only,
configured through `CompressorStreamFactory.builder().setMaxDecompressedSize(long)`,
composed over `BoundedInputStream` with `setMaxCount` and a throwing
`setOnMaxCount`. The guard class and its exception are gone; the limit throws
`CompressorException`. The factory got a builder rather than a fourth
constructor, following the review; the existing constructors are untouched.
One detail worth a look: the callback reads a single byte from the decompressor
before throwing, so a stream of exactly the limit reads to EOF and only real
excess fails. Without that, `IOUtils.toByteArray` on a legitimate file of
exactly N bytes would throw, and Piotr's idea of using a declared entry size
as the limit would not work.
`CompressorInputStream` now implements `InputStreamStatistics`; the base
`getCompressedCount()` returns -1 and keeps the interface's `throws
IOException` so subclasses that already declare it keep compiling.
Rebased on master; the branch name still says bomb-guard because the PR is
bound to it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-4827734106
@tomasilluminati
My initial reaction to the ratio check feature is that it's nice but I doubt folks will know how to use it properly without shooting themselves in the foot. "Our app blew up when we allowed users to import files of type foo but everything was fine with bar files" kind of thing. That said, there is a way to implement a ratio check I think using `org.apache.commons.io.input.ProxyInputStream.AbstractBuilder.setAfterRead(IOIntConsumer)` where you'd do the check in the consumer (`BoundedInputStream` extends `ProxyInputStream`).
This one can simmer... ;) maybe @ppkarwasz has some thoughts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
Posted by "tomasilluminati (via GitHub)" <gi...@apache.org>.
tomasilluminati commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-4827044231
@ppkarwasz COMPRESS-726 PR Opened
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-4863262678
> @ppkarwasz, given that, do you still want the mean-ratio guard and an on-by-default limit?
I guess, we can deliver a mean-ratio guard in a separate wrapper: only a limited number of users need it. Basically only if the compressed stream comes primarily from untrusted sources (like in a server), users might want to add a mean-ratio check as a *fail-fast* mechanism to prevent a DoS.
However, I think it would be nice if every `CompressorInputStream` inherited from `BoundedInputStream`: for archive formats that contain compressed data (ZIP, 7z), we usually have a (_declared_) decompressed size, so we could set that size as limit and prevent corrupted archives to fill the disk. Malicious archives, of course, would need the user to set a hard maximum decompressed size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-4863192879
> My initial reaction to the ratio check feature is that it's nice but I doubt folks will know how to use it properly without shooting themselves in the foot. "Our app blew up when we allowed users to import files of type foo but everything was fine with bar files" kind of thing.
As with most features, we probably need to provide sensible defaults, so users don't need to modify them. AI gives the following usual compression rates:
- Binaries: 2-3×
- Text/source code: 3-5×
- XML/JSON: 5-15×
- Log files: 10-30×
Higher compression rates are impossible, unless you compress zeros, so 100× looks like a safe default. When I chose the default ratio for apache/mina#52, I tried to copy what HTTP Server does: [`mod_deflate`](https://httpd.apache.org/docs/current/mod/mod_deflate.html), has these knobs:
- `DeflateInflateRatioLimit` defaults to 200,
- Rather than a grace size, `mod_deflate` only rejects a stream once the cumulative ratio stays above the limit for `DeflateInflateRatioBurst + 1` (default `4`) consecutive output buffers. With the default 8 KiB buffer, that corresponds to roughly 32 KiB of decompressed output.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
Posted by "tomasilluminati (via GitHub)" <gi...@apache.org>.
tomasilluminati commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-4862267941
@garydgregory , I've got the minimal version ready, absolute decompressed-size limit composed over IO's BoundedInputStream, dropping the extra class and the exception. One note, since the cap is on the real decompressed output, it already covers the faked-payload-size case Piotr raised (it ignores any declared size). @ppkarwasz, given that, do you still want the mean-ratio guard and an on-by-default limit? The ratio fires earlier and needs no tuned number, so I'm happy to fold it back as a second, opt-in check if you want it. Otherwise I'll push the size-only version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [COMPRESS-726] Add optional decompression bomb protection to CompressorStreamFactory [commons-compress]
Posted by "tomasilluminati (via GitHub)" <gi...@apache.org>.
tomasilluminati commented on PR #777:
URL: https://github.com/apache/commons-compress/pull/777#issuecomment-4827612979
@garydgregory
Thanks, this is really helpful, and honestly it points at a simpler shape than what I pushed.
If all we want is a decompressed-size limit, IO's BoundedInputStream already does it (setMaxCount plus an onMaxCount that throws). So we don't really need a compress-specific class or a new exception. I'm happy to drop BombGuardCompressorInputStream and DecompressionBombException and just have CompressorStreamFactory wrap the stream in a BoundedInputStream when a max size is set. That's the single place you're describing, it reuses IO, and the bomb wording goes away. The builder point sorts itself out too, since there's no class left to build.
The only bit that doesn't fit there is the compression-ratio check, since BoundedInputStream has no idea about ratios and it needs getCompressedCount() from the compressor side. So really the only open question is scope. Just the size limit, or the ratio guard too?
- Size only, I drop the class and the exception and wire BoundedInputStream into the factory. Smallest change.
- Ratio too, I keep it but move it onto CompressorInputStream where both counts already live, still exposed through the factory.
My hunch from your note is size-only, but just say the word and I'll redo it that way. Either way the InputStreamStatistics change stays, the compressed count is handy on its own.
Thanks again for the steer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org