Skip to content
Merged
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
5 changes: 3 additions & 2 deletions machine/corpora/dbl_bundle_text_corpus.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from ..scripture import ENGLISH_VERSIFICATION
from ..scripture.verse_ref import Versification
from ..utils.typeshed import StrPath
from ..utils.zip_entry_utils import open_bounded_stream
from .scripture_text_corpus import ScriptureTextCorpus
from .usx_zip_text import UsxZipText

Expand All @@ -16,7 +17,7 @@ class DblBundleTextCorpus(ScriptureTextCorpus):

def __init__(self, filename: StrPath) -> None:
with ZipFile(filename, "r") as archive:
with archive.open("metadata.xml", "r") as stream:
with open_bounded_stream(archive, "metadata.xml") as stream:
doc = ElementTree.parse(stream)
version = doc.getroot().get("version", "2.0")
parts = version.split(".", maxsplit=3)
Expand All @@ -27,7 +28,7 @@ def __init__(self, filename: StrPath) -> None:
(zi for zi in archive.filelist if os.path.basename(zi.filename) == "versification.vrs"), None
)
if versification_entry is not None:
with archive.open(versification_entry, "r") as stream:
with open_bounded_stream(archive, versification_entry) as stream:
abbr = doc.getroot().findtext("./identification/abbreviation", "")
versification = Versification.parse(
TextIOWrapper(stream, encoding="utf-8-sig"), "versification.vrs", fallback_name=abbr
Expand Down
4 changes: 3 additions & 1 deletion machine/corpora/zip_entry_stream_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from zipfile import ZipFile

from ..utils.typeshed import StrPath
from ..utils.zip_entry_utils import open_bounded_stream
from .stream_container import StreamContainer


Expand All @@ -20,7 +21,8 @@ def __exit__(self, type: Any, value: Any, traceback: Any) -> None:
self.close()

def open_stream(self) -> BinaryIO:
return BytesIO(self._archive.read(self._entry))
with open_bounded_stream(self._archive, self._entry) as stream:
return BytesIO(stream.read())

def close(self) -> None:
self._archive.close()
4 changes: 3 additions & 1 deletion machine/corpora/zip_paratext_project_file_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from typing import BinaryIO, Optional, cast
from zipfile import ZipFile

from ..utils.zip_entry_utils import open_bounded_stream
from .paratext_project_file_handler import ParatextProjectFileHandler
from .usfm_stylesheet import UsfmStylesheet

Expand All @@ -27,7 +28,8 @@ def find(self, extension: str) -> Optional[str]:
def open(self, file_name: str) -> Optional[BinaryIO]:
for actual_entry_name in self._archive.namelist():
if actual_entry_name.lower() == file_name.lower():
return BytesIO(self._archive.read(actual_entry_name))
with open_bounded_stream(self._archive, actual_entry_name) as stream:
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
return BytesIO(stream.read())
return None

def create_stylesheet(self, file_name: str) -> UsfmStylesheet:
Expand Down
28 changes: 28 additions & 0 deletions machine/utils/zip_entry_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
from typing import IO, Union
from zipfile import BadZipFile, ZipFile, ZipInfo

DEFAULT_MAX_UNCOMPRESSED_SIZE = 100 * 1024 * 1024 # 100 MB
DEFAULT_MAX_COMPRESSION_RATIO = 100.0 # 100:1 ratio limit


def open_bounded_stream(
archive: ZipFile,
entry: Union[str, ZipInfo],
max_uncompressed_size: int = DEFAULT_MAX_UNCOMPRESSED_SIZE,
max_compression_ratio: float = DEFAULT_MAX_COMPRESSION_RATIO,
) -> IO[bytes]:
"""Opens a zip entry for reading, refusing entries that are too large or too highly compressed.

Raises `BadZipFile` if the entry's declared size or compression ratio exceeds the limits. `ZipFile` never yields
more than the declared size, so an entry whose header understates its size fails the CRC check with `BadZipFile`.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI: F4. The new docstring promises more than CPython delivers. ZipExtFile checks the CRC against the central-directory CRC over the bytes it has yielded, so if a crafted entry sets file_size and CRC to match a truncated prefix, the entry is silently cut short and no BadZipFile is raised. The size bound still holds. Saying "normally fails the CRC check" would be accurate. Unverified: I could not run Python here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

F4 still applies at bdce2ab: the docstring now says outright that an understated header "fails the CRC check with BadZipFile", and that claim is the reason BoundedStream was dropped. An entry whose CRC matches the truncated prefix is still cut short without any error. F5 covers the memory side of the same gap.

"""
if isinstance(entry, str):
entry = archive.getinfo(entry)

if entry.file_size > max_uncompressed_size:
raise BadZipFile("Entry uncompressed size exceeds maximum allowed limit.")

if entry.compress_size > 0 and entry.file_size / entry.compress_size > max_compression_ratio:
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
raise BadZipFile("Compression ratio exceeds safe threshold.")

return archive.open(entry, "r")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Minor: F5. With BoundedStream gone, a single read of a crafted entry no longer has a memory bound. ZipEntryStreamContainer.open_stream and ZipParatextProjectFileHandler.open call stream.read(), and ZipExtFile.read(-1) calls _read1(MAX_N). That decompresses up to 2 GiB in one call before cutting the output to file_size, so file_size=1000, compress_size=2MB passes both checks. Before bdce2ab, RawIOBase.readall read in 8 KB chunks. Reading in chunks at those two sites (e.g. shutil.copyfileobj) restores the bound. Unverified: I reasoned this from CPython zipfile and could not run it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI: F6. A bzip2 or LZMA entry is not bounded by these checks, even if the reads are chunked. For those types, ZipExtFile._read1 calls decompress(data) with no max_length, so one compressed chunk can expand without limit before the output is cut to file_size. This means F5's chunked-read fix covers only deflate. C# ZipArchive cannot read these types, so refusing anything except ZIP_STORED/ZIP_DEFLATED here would match C#. Unverified: I reasoned this from CPython zipfile and could not run Python here.

37 changes: 37 additions & 0 deletions tests/utils/test_zip_entry_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
from io import BytesIO
from zipfile import ZIP_DEFLATED, BadZipFile, ZipFile

from pytest import raises

from machine.utils.zip_entry_utils import open_bounded_stream


def create_zip(name: str, content: bytes) -> ZipFile:
buffer = BytesIO()
with ZipFile(buffer, "w", ZIP_DEFLATED) as archive:
archive.writestr(name, content)
return ZipFile(buffer, "r")


def test_open_bounded_stream_returns_readable_stream() -> None:
with create_zip("test.txt", b"Hello World") as archive:
with open_bounded_stream(archive, "test.txt", max_uncompressed_size=100) as stream:
assert stream.read() == b"Hello World"


def test_open_bounded_stream_empty_entry() -> None:
with create_zip("empty.txt", b"") as archive:
with open_bounded_stream(archive, "empty.txt") as stream:
assert stream.read() == b""


def test_open_bounded_stream_header_size_exceeded() -> None:
with create_zip("large.txt", bytes(200)) as archive:
with raises(BadZipFile):
open_bounded_stream(archive, "large.txt", max_uncompressed_size=100)


def test_open_bounded_stream_compression_ratio_exceeded() -> None:
with create_zip("bomb.txt", bytes(10_000)) as archive:
with raises(BadZipFile):
open_bounded_stream(archive, "bomb.txt", max_uncompressed_size=20_000, max_compression_ratio=2.0)
Loading