gh-156002: Bound zipfile decompression for bzip2/LZMA/Zstandard - #156003
Open
encukou wants to merge 8 commits into
Open
gh-156002: Bound zipfile decompression for bzip2/LZMA/Zstandard#156003encukou wants to merge 8 commits into
encukou wants to merge 8 commits into
Conversation
…dard zipfile.ZipExtFile._read1() bounds the output of each decompress() call for DEFLATE members by passing a max_length to zlib, but for bzip2, LZMA, and Zstandard members it called decompress() with no bound. A whole compressed chunk was therefore expanded into a single allocation before the data[:self._left] clip ran, so a consumer that deliberately reads in small chunks to limit memory (for example zf.open(name).read(8192)) was silently unprotected for non-DEFLATE members. A small, spec-conformant archive member declaring a large uncompressed size could drive multi-GB peak memory. _read1() now passes a per-call bound to the non-DEFLATE decompress() (mirroring the DEFLATE branch) and drains the decompressor's internal buffer across calls by checking needs_input before reading more compressed input. zipfile's LZMADecompressor wrapper forwards max_length and exposes needs_input so the bound also holds for LZMA members.
Replace the Linux-only subprocess RSS test with a cross-platform check that _read1() output is bounded by MIN_READ_SIZE for bzip2/LZMA/Zstandard.
|
|
||
|
|
||
| def _decompressor_needs_input(decompressor): | ||
| # bz2/zstd expose the stdlib decompressor's public needs_input; the LZMA |
Member
There was a problem hiding this comment.
The LZMA wrapper is private, it's not in __all__ or documented, why not just make it a "public" property and avoid this little dance?
Member
Author
There was a problem hiding this comment.
That's one definition of “private” :)
For the backports, I think it's best to be extra careful. Testing on 3.14.7 and having things break with 3.14.6 is not fun.
Let's make it public (& more maintainable) in 3.16 afterwards.
Member
There was a problem hiding this comment.
This should be in Security.
Contributor
|
Agreed — this bounds a decompression-bomb DoS, so |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Patch by @tonghuaroot