diff --git a/apps/backend/app/storage/local.py b/apps/backend/app/storage/local.py index b8e571fa..0ae2d49c 100644 --- a/apps/backend/app/storage/local.py +++ b/apps/backend/app/storage/local.py @@ -1,7 +1,9 @@ import shutil +import struct import tarfile import zipfile from pathlib import Path +from typing import cast from fastapi import UploadFile @@ -10,6 +12,21 @@ from app.parsers.repository_parser import is_macos_artifact +class _BoundedTarInfo(tarfile.TarInfo): + """Bound PAX/GNU metadata before CPython buffers extended header bodies.""" + + MAX_METADATA_BYTES = 16 * 1024 * 1024 + + def _proc_member(self, archive: tarfile.TarFile) -> tarfile.TarInfo | None: + if self.type in {tarfile.XHDTYPE, tarfile.XGLTYPE, tarfile.GNUTYPE_LONGNAME, tarfile.GNUTYPE_LONGLINK}: + consumed = getattr(archive, "_partha_metadata_bytes", 0) + self.size + if self.size < 0 or consumed > self.MAX_METADATA_BYTES: + raise ValidationServiceError("TAR extended metadata exceeds the supported ingestion limit.") + setattr(archive, "_partha_metadata_bytes", consumed) + # CPython private parser seam, exercised on supported Python versions. + return cast(tarfile.TarInfo | None, getattr(tarfile.TarInfo, "_proc_member")(self, archive)) + + class LocalStorage: def __init__(self, settings: Settings) -> None: self.root = settings.storage_path @@ -81,21 +98,74 @@ def extract_archive(self, archive_path: Path, repository_id: str) -> Path: destination = self.reset_repository_path(repository_id) try: if zipfile.is_zipfile(archive_path): + self._check_zip_metadata(archive_path) with zipfile.ZipFile(archive_path) as archive: self._safe_extract_zip(archive, destination) self._strip_macos_artifacts(destination) return self._normalise_single_root(destination) - if tarfile.is_tarfile(archive_path): - with tarfile.open(archive_path) as archive: - self._safe_extract_tar(archive, destination) - self._strip_macos_artifacts(destination) - return self._normalise_single_root(destination) + # is_tarfile() parses the first member before our metadata guard. + # Detect and extract in one bounded stream instead. + try: + tar_archive = tarfile.open(archive_path, mode="r|*", tarinfo=_BoundedTarInfo) + except tarfile.ReadError as exc: + raise ValidationServiceError("Unsupported archive format. Upload a ZIP or TAR archive.") from exc + with tar_archive: + self._safe_extract_tar(tar_archive, destination) + self._strip_macos_artifacts(destination) + return self._normalise_single_root(destination) except (zipfile.BadZipFile, tarfile.TarError, OSError) as exc: raise ValidationServiceError("Archive is corrupted or cannot be extracted.") from exc raise ValidationServiceError("Unsupported archive format. Upload a ZIP or TAR archive.") + def _check_zip_metadata(self, path: Path) -> None: + """Bound central-directory work before ZipFile creates ZipInfo objects. + + CPython's bounded footer reader supports ordinary and ZIP64 end records. + Count actual central headers too: ZipFile does not trust footer counts. + The private footer constants mirror our supported Python 3.12/3.13 + ZipFile implementation; regression fixtures exercise this boundary. + """ + with path.open("rb") as source: + footer = getattr(zipfile, "_EndRecData")(source) + if footer is None: + raise zipfile.BadZipFile("Missing end record") + size = footer[getattr(zipfile, "_ECD_SIZE")] + if footer[getattr(zipfile, "_ECD_ENTRIES_TOTAL")] > self.max_extracted_entries: + raise ValidationServiceError("Archive contains more entries than the configured maximum.") + if size > 16 * 1024 * 1024: + raise ValidationServiceError("Archive central-directory metadata exceeds configured limits.") + start = footer[getattr(zipfile, "_ECD_LOCATION")] - size + if footer[getattr(zipfile, "_ECD_SIGNATURE")] == getattr(zipfile, "stringEndArchive64"): + start -= getattr(zipfile, "sizeEndCentDir64") + getattr(zipfile, "sizeEndCentDir64Locator") + if size < 0 or start < 0: + raise zipfile.BadZipFile("Invalid central-directory bounds") + source.seek(start) + end = start + size + count = 0 + while source.tell() < end: + raw = source.read(getattr(zipfile, "sizeCentralDir")) + if len(raw) != getattr(zipfile, "sizeCentralDir"): + raise zipfile.BadZipFile("Truncated central-directory header") + header = struct.unpack(getattr(zipfile, "structCentralDir"), raw) + if header[0] != getattr(zipfile, "stringCentralDir"): + raise zipfile.BadZipFile("Invalid central-directory header") + count += 1 + if count > self.max_extracted_entries: + raise ValidationServiceError("Archive contains more entries than the configured maximum.") + length = sum( + header[index] + for index in ( + getattr(zipfile, "_CD_FILENAME_LENGTH"), + getattr(zipfile, "_CD_EXTRA_FIELD_LENGTH"), + getattr(zipfile, "_CD_COMMENT_LENGTH"), + ) + ) + if source.tell() + length > end: + raise zipfile.BadZipFile("Invalid central-directory member bounds") + source.seek(length, 1) + def _safe_extract_zip(self, archive: zipfile.ZipFile, destination: Path) -> None: total_size = 0 for entry_index, member in enumerate(archive.infolist(), start=1): @@ -120,7 +190,7 @@ def _safe_extract_zip(self, archive: zipfile.ZipFile, destination: Path) -> None def _safe_extract_tar(self, archive: tarfile.TarFile, destination: Path) -> None: total_size = 0 - for entry_index, member in enumerate(archive.getmembers(), start=1): + for entry_index, member in enumerate(archive, start=1): if member.issym() or member.islnk() or member.isdev(): raise ValidationServiceError("Archive contains unsupported link or device entries.") target = destination / member.name @@ -139,17 +209,18 @@ def _safe_extract_tar(self, archive: tarfile.TarFile, destination: Path) -> None "Archive would decompress to more than the configured maximum size.", {"maxExtractedSizeBytes": self.max_extracted_size_bytes}, ) - # `filter="data"` applies CPython's own extraction hardening: it strips - # absolute paths and `..` traversal, and rejects links, devices, setuid - # bits and other unsafe metadata as the members are written. - # - # The loop above already rejects those cases, so this is defence in - # depth on untrusted uploads rather than the primary control — the two - # have to disagree for it to matter, which is exactly when a check is - # worth having. It also settles the DeprecationWarning: tar extraction - # is unfiltered by default until Python 3.14, which would switch this - # behaviour on silently. Being explicit keeps it a decision. - archive.extractall(destination, filter="data") + # `filter="data"` applies CPython's own extraction hardening: it strips + # absolute paths and `..` traversal, and rejects links, devices, setuid + # bits and other unsafe metadata as the members are written. + # + # The loop above already rejects those cases, so this is defence in + # depth on untrusted uploads rather than the primary control — the two + # have to disagree for it to matter, which is exactly when a check is + # worth having. It also settles the DeprecationWarning: tar extraction + # is unfiltered by default until Python 3.14, which would switch this + # behaviour on silently. Being explicit keeps it a decision. + archive.extract(member, destination, filter="data") + getattr(archive, "members").clear() def _strip_macos_artifacts(self, destination: Path) -> None: """Delete Finder/Archive Utility artifacts an archive may carry (#397). diff --git a/apps/backend/tests/test_archive_metadata_limits.py b/apps/backend/tests/test_archive_metadata_limits.py new file mode 100644 index 00000000..93792e8e --- /dev/null +++ b/apps/backend/tests/test_archive_metadata_limits.py @@ -0,0 +1,76 @@ +import tarfile +import zipfile + +import pytest +from app.core.config import Settings +from app.core.exceptions import ValidationServiceError +from app.storage.local import LocalStorage + + +def test_zip_count_rejected_before_zipinfo_materialization(tmp_path, monkeypatch): + path = tmp_path / "many.zip" + with zipfile.ZipFile(path, "w") as archive: + for index in range(5): + archive.writestr(f"{index}.txt", "") + storage = LocalStorage(Settings(storage_path=tmp_path / "storage", max_extracted_entries=3)) + + def forbidden(*args, **kwargs): + raise AssertionError("ZipInfo materialized before metadata limit") + + monkeypatch.setattr(zipfile, "ZipFile", forbidden) + with pytest.raises(ValidationServiceError, match="Archive contains more entries than the configured maximum"): + storage.extract_archive(path, "bounded") + + +def test_tar_count_is_checked_without_getmembers(tmp_path, monkeypatch): + path = tmp_path / "many.tar" + with tarfile.open(path, "w") as archive: + for index in range(5): + archive.addfile(tarfile.TarInfo(f"{index}.txt")) + storage = LocalStorage(Settings(storage_path=tmp_path / "storage", max_extracted_entries=3)) + + def forbidden(*args, **kwargs): + raise AssertionError("TAR fully enumerated") + + monkeypatch.setattr(tarfile.TarFile, "getmembers", forbidden) + with pytest.raises(ValidationServiceError, match="more entries"): + storage.extract_archive(path, "bounded") + + +def test_small_force_zip64_archive_still_extracts(tmp_path): + path = tmp_path / "small.zip" + with zipfile.ZipFile(path, "w") as archive: + with archive.open("small.txt", "w", force_zip64=True) as member: + member.write(b"valid") + storage = LocalStorage(Settings(storage_path=tmp_path / "storage")) + destination = storage.extract_archive(path, "bounded") + assert (destination / "small.txt").read_bytes() == b"valid" + + +def test_tar_extended_metadata_rejected_before_buffering(tmp_path, monkeypatch): + from app.storage.local import _BoundedTarInfo + + path = tmp_path / "header-only.tar" + header = tarfile.TarInfo("extended") + header.type = tarfile.XHDTYPE + header.size = 2048 + path.write_bytes(header.tobuf()) + monkeypatch.setattr(_BoundedTarInfo, "MAX_METADATA_BYTES", 1024) + + def forbidden(*args, **kwargs): + raise AssertionError("PAX metadata body buffered before limit") + + monkeypatch.setattr(tarfile.TarInfo, "_proc_pax", forbidden) + storage = LocalStorage(Settings(storage_path=tmp_path / "storage")) + with pytest.raises(ValidationServiceError, match="extended metadata"): + storage.extract_archive(path, "bounded") + + +def test_valid_pax_metadata_still_extracts(tmp_path): + path = tmp_path / "valid-pax.tar" + with tarfile.open(path, "w", format=tarfile.PAX_FORMAT) as archive: + info = tarfile.TarInfo("small.txt") + info.pax_headers = {"comment": "valid extended metadata"} + archive.addfile(info) + storage = LocalStorage(Settings(storage_path=tmp_path / "storage")) + assert (storage.extract_archive(path, "bounded") / "small.txt").is_file() diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index b15aac09..49a357b5 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -169,3 +169,9 @@ commands run), expected versus actual behaviour, and any relevant log output privately through [SECURITY.md](../SECURITY.md) instead. For a question that isn't yet a confirmed bug, ask on [Discord](https://discord.gg/qvk9DcxDA) first. + +### Archive metadata limits + +ZIP central-directory metadata is checked before ZipInfo materialization: at most 16 MiB +and the configured entry count. TAR entries are processed incrementally under entry/decompressed-size +limits. PAX/GNU extended header bodies are bounded to 16 MiB cumulatively before buffering, including format detection. These bounds do not isolate parser CPU/RSS into an OS-limited subprocess; deployment-level resource isolation is still needed. No exhaustive DoS guarantee is claimed.