Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions Lib/test/test_zipfile/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -4874,6 +4874,48 @@ def tearDown(self):
unlink(TESTFN2)


class AbstractBoundedDecompressTests:
# ZipExtFile._read1() bounds the output of each decompress() call so that a
# small member declaring a large uncompressed size cannot expand into one
# unbounded read.
def test_read1_output_is_bounded(self):
buf = io.BytesIO()
with zipfile.ZipFile(buf, "w", compression=self.compression) as zf:
zf.writestr("big", b"\0" * (4 * 1024 * 1024))
with zipfile.ZipFile(io.BytesIO(buf.getvalue())) as zf:
with zf.open("big") as f:
self.assertLessEqual(len(f._read1(100)), f.MIN_READ_SIZE)


class StoredBoundedDecompressTests(AbstractBoundedDecompressTests,
unittest.TestCase):
compression = zipfile.ZIP_STORED


@requires_zlib()
class DeflateBoundedDecompressTests(AbstractBoundedDecompressTests,
unittest.TestCase):
compression = zipfile.ZIP_DEFLATED


@requires_bz2()
class Bzip2BoundedDecompressTests(AbstractBoundedDecompressTests,
unittest.TestCase):
compression = zipfile.ZIP_BZIP2


@requires_lzma()
class LzmaBoundedDecompressTests(AbstractBoundedDecompressTests,
unittest.TestCase):
compression = zipfile.ZIP_LZMA


@requires_zstd()
class ZstdBoundedDecompressTests(AbstractBoundedDecompressTests,
unittest.TestCase):
compression = zipfile.ZIP_ZSTANDARD


class AbstractBadCrcTests:
def test_testzip_with_bad_crc(self):
"""Tests that files with bad CRCs return their name from testzip."""
Expand Down
38 changes: 33 additions & 5 deletions Lib/zipfile/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -801,7 +801,16 @@ def unused_data(self):
except AttributeError:
return b''

def decompress(self, data):
@property
def _needs_input(self):
# While the LZMA properties header is still being buffered, more input
# is required; afterwards defer to the wrapped decompressor so a bounded
# decompress() call can be drained across reads.
if self._decomp is None:
return True
return self._decomp.needs_input

def decompress(self, data, max_length=-1):
if self._decomp is None:
self._unconsumed += data
if len(self._unconsumed) <= 4:
Expand All @@ -817,7 +826,7 @@ def decompress(self, data):
data = self._unconsumed[4 + psize:]
del self._unconsumed

result = self._decomp.decompress(data)
result = self._decomp.decompress(data, max_length)
self.eof = self._decomp.eof
return result

Expand Down Expand Up @@ -884,6 +893,13 @@ def _get_compressor(compress_type, compresslevel=None):
return None


def _decompressor_needs_input(decompressor):
# bz2/zstd expose the stdlib decompressor's public needs_input; the LZMA

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# wrapper keeps it private (_needs_input) to avoid adding public API.
needs_input = getattr(decompressor, "needs_input", None)
return decompressor._needs_input if needs_input is None else needs_input


def _get_decompressor(compress_type):
_check_compression(compress_type)
if compress_type == ZIP_STORED:
Expand Down Expand Up @@ -1186,8 +1202,15 @@ def _read1(self, n):
data = self._decompressor.unconsumed_tail
if n > len(data):
data += self._read2(n - len(data))
else:
elif self._compress_type == ZIP_STORED:
data = self._read2(n)
else:
# bzip2/lzma/zstd: a bounded decompress() call may leave input
# buffered inside the decompressor; drain that before reading more.
if _decompressor_needs_input(self._decompressor):
data = self._read2(n)
else:
data = b''

if self._compress_type == ZIP_STORED:
self._eof = self._compress_left <= 0
Expand All @@ -1200,8 +1223,13 @@ def _read1(self, n):
if self._eof:
data += self._decompressor.flush()
else:
data = self._decompressor.decompress(data)
self._eof = self._decompressor.eof or self._compress_left <= 0
# Bound the output of a single decompress() call (mirroring the
# DEFLATE path above) so that a small compressed member cannot
# expand into one unbounded read.
data = self._decompressor.decompress(data, max(n, self.MIN_READ_SIZE))
self._eof = (self._decompressor.eof or
self._compress_left <= 0 and
_decompressor_needs_input(self._decompressor))

data = data[:self._left]
self._left -= len(data)
Expand Down

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in Security.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Bound the amount of data :mod:`zipfile` decompresses per read for members
compressed with bzip2, LZMA, or Zstandard, matching the existing limit for
deflate. A small archive member could previously expand into an unbounded
allocation even when read in small chunks.
Loading