From d35594b56560f6ca006b57f021a171c38a0af5e2 Mon Sep 17 00:00:00 2001 From: Shaurya K Sharma Date: Thu, 8 Oct 2026 23:40:30 +0100 Subject: [PATCH 1/6] fix(security): bound ZIP metadata and stream TAR traversal (#542) --- apps/backend/app/storage/local.py | 74 +++++++++++++++---- .../tests/test_archive_metadata_limits.py | 47 ++++++++++++ docs/DEVELOPMENT.md | 7 ++ 3 files changed, 115 insertions(+), 13 deletions(-) create mode 100644 apps/backend/tests/test_archive_metadata_limits.py diff --git a/apps/backend/app/storage/local.py b/apps/backend/app/storage/local.py index b8e571fa..1e7a5681 100644 --- a/apps/backend/app/storage/local.py +++ b/apps/backend/app/storage/local.py @@ -1,4 +1,5 @@ import shutil +import struct import tarfile import zipfile from pathlib import Path @@ -81,13 +82,14 @@ 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: + with tarfile.open(archive_path, mode="r|*") as archive: self._safe_extract_tar(archive, destination) self._strip_macos_artifacts(destination) return self._normalise_single_root(destination) @@ -96,6 +98,51 @@ def extract_archive(self, archive_path: Path, repository_id: str) -> Path: 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 = zipfile._EndRecData(source) + if footer is None: + raise zipfile.BadZipFile("Missing end record") + size = footer[zipfile._ECD_SIZE] + if size > 16 * 1024 * 1024 or footer[zipfile._ECD_ENTRIES_TOTAL] > self.max_extracted_entries: + raise ValidationServiceError("Archive central-directory metadata exceeds configured limits.") + start = footer[zipfile._ECD_LOCATION] - size + if footer[zipfile._ECD_SIGNATURE] == zipfile.stringEndArchive64: + start -= zipfile.sizeEndCentDir64 + 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(zipfile.sizeCentralDir) + if len(raw) != zipfile.sizeCentralDir: + raise zipfile.BadZipFile("Truncated central-directory header") + header = struct.unpack(zipfile.structCentralDir, raw) + if header[0] != 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 ( + zipfile._CD_FILENAME_LENGTH, + zipfile._CD_EXTRA_FIELD_LENGTH, + 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 +167,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 +186,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") + 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..42f4084e --- /dev/null +++ b/apps/backend/tests/test_archive_metadata_limits.py @@ -0,0 +1,47 @@ +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="metadata exceeds"): + 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" diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index b15aac09..563e1784 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -169,3 +169,10 @@ 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. These bounds do not isolate parsing into an OS-limited subprocess; hostile extended TAR headers +and parser CPU/RSS still require deployment-level resource isolation. No exhaustive DoS guarantee is claimed. From 5f24804665a9d8294cb8c840161312187427c4b3 Mon Sep 17 00:00:00 2001 From: Shaurya K Sharma Date: Thu, 8 Oct 2026 23:59:28 +0100 Subject: [PATCH 2/6] fix(storage): type dynamic CPython archive parser access --- apps/backend/app/storage/local.py | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/apps/backend/app/storage/local.py b/apps/backend/app/storage/local.py index 1e7a5681..8d56f54f 100644 --- a/apps/backend/app/storage/local.py +++ b/apps/backend/app/storage/local.py @@ -107,26 +107,26 @@ def _check_zip_metadata(self, path: Path) -> None: ZipFile implementation; regression fixtures exercise this boundary. """ with path.open("rb") as source: - footer = zipfile._EndRecData(source) + footer = getattr(zipfile, "_EndRecData")(source) if footer is None: raise zipfile.BadZipFile("Missing end record") - size = footer[zipfile._ECD_SIZE] - if size > 16 * 1024 * 1024 or footer[zipfile._ECD_ENTRIES_TOTAL] > self.max_extracted_entries: + size = footer[getattr(zipfile, "_ECD_SIZE")] + if size > 16 * 1024 * 1024 or footer[getattr(zipfile, "_ECD_ENTRIES_TOTAL")] > self.max_extracted_entries: raise ValidationServiceError("Archive central-directory metadata exceeds configured limits.") - start = footer[zipfile._ECD_LOCATION] - size - if footer[zipfile._ECD_SIGNATURE] == zipfile.stringEndArchive64: - start -= zipfile.sizeEndCentDir64 + zipfile.sizeEndCentDir64Locator + 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(zipfile.sizeCentralDir) - if len(raw) != zipfile.sizeCentralDir: + raw = source.read(getattr(zipfile, "sizeCentralDir")) + if len(raw) != getattr(zipfile, "sizeCentralDir"): raise zipfile.BadZipFile("Truncated central-directory header") - header = struct.unpack(zipfile.structCentralDir, raw) - if header[0] != zipfile.stringCentralDir: + 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: @@ -134,9 +134,9 @@ def _check_zip_metadata(self, path: Path) -> None: length = sum( header[index] for index in ( - zipfile._CD_FILENAME_LENGTH, - zipfile._CD_EXTRA_FIELD_LENGTH, - zipfile._CD_COMMENT_LENGTH, + getattr(zipfile, "_CD_FILENAME_LENGTH"), + getattr(zipfile, "_CD_EXTRA_FIELD_LENGTH"), + getattr(zipfile, "_CD_COMMENT_LENGTH"), ) ) if source.tell() + length > end: @@ -197,7 +197,7 @@ def _safe_extract_tar(self, archive: tarfile.TarFile, destination: Path) -> None # 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") - archive.members.clear() + getattr(archive, "members").clear() def _strip_macos_artifacts(self, destination: Path) -> None: """Delete Finder/Archive Utility artifacts an archive may carry (#397). From a320ba130b22bc97923bea589a02fd8c2a447825 Mon Sep 17 00:00:00 2001 From: Shaurya K Sharma Date: Fri, 9 Oct 2026 00:16:19 +0100 Subject: [PATCH 3/6] fix(storage): bound extended TAR metadata before format detection --- apps/backend/app/storage/local.py | 31 ++++++++++++++++--- .../tests/test_archive_metadata_limits.py | 29 +++++++++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/apps/backend/app/storage/local.py b/apps/backend/app/storage/local.py index 8d56f54f..9708ef28 100644 --- a/apps/backend/app/storage/local.py +++ b/apps/backend/app/storage/local.py @@ -3,6 +3,7 @@ import tarfile import zipfile from pathlib import Path +from typing import cast from fastapi import UploadFile @@ -11,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 @@ -88,11 +104,16 @@ def extract_archive(self, archive_path: Path, repository_id: str) -> Path: self._strip_macos_artifacts(destination) return self._normalise_single_root(destination) - if tarfile.is_tarfile(archive_path): - with tarfile.open(archive_path, mode="r|*") 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 diff --git a/apps/backend/tests/test_archive_metadata_limits.py b/apps/backend/tests/test_archive_metadata_limits.py index 42f4084e..19f0cfab 100644 --- a/apps/backend/tests/test_archive_metadata_limits.py +++ b/apps/backend/tests/test_archive_metadata_limits.py @@ -45,3 +45,32 @@ def test_small_force_zip64_archive_still_extracts(tmp_path): 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() From 2cd904873525c706b31c28013611e696b45c2f87 Mon Sep 17 00:00:00 2001 From: Shaurya K Sharma Date: Fri, 9 Oct 2026 00:19:40 +0100 Subject: [PATCH 4/6] docs(storage): describe bounded extended TAR headers --- docs/DEVELOPMENT.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 563e1784..49a357b5 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -174,5 +174,4 @@ first. 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. These bounds do not isolate parsing into an OS-limited subprocess; hostile extended TAR headers -and parser CPU/RSS still require deployment-level resource isolation. No exhaustive DoS guarantee is claimed. +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. From 8930427e44d1b6f30e4f36566d9fee8457ff48af Mon Sep 17 00:00:00 2001 From: Shaurya K Sharma Date: Fri, 9 Oct 2026 00:37:56 +0100 Subject: [PATCH 5/6] Preserve entry-budget validation response --- apps/backend/app/storage/local.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/backend/app/storage/local.py b/apps/backend/app/storage/local.py index 9708ef28..0ae2d49c 100644 --- a/apps/backend/app/storage/local.py +++ b/apps/backend/app/storage/local.py @@ -132,7 +132,9 @@ def _check_zip_metadata(self, path: Path) -> None: if footer is None: raise zipfile.BadZipFile("Missing end record") size = footer[getattr(zipfile, "_ECD_SIZE")] - if size > 16 * 1024 * 1024 or footer[getattr(zipfile, "_ECD_ENTRIES_TOTAL")] > self.max_extracted_entries: + 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"): From b5af6648664dd297fd3e30669d7599fc9398fd21 Mon Sep 17 00:00:00 2001 From: Shaurya K Sharma Date: Fri, 9 Oct 2026 00:45:39 +0100 Subject: [PATCH 6/6] Align metadata regression with preserved entry-limit contract --- apps/backend/tests/test_archive_metadata_limits.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/backend/tests/test_archive_metadata_limits.py b/apps/backend/tests/test_archive_metadata_limits.py index 19f0cfab..93792e8e 100644 --- a/apps/backend/tests/test_archive_metadata_limits.py +++ b/apps/backend/tests/test_archive_metadata_limits.py @@ -18,7 +18,7 @@ def forbidden(*args, **kwargs): raise AssertionError("ZipInfo materialized before metadata limit") monkeypatch.setattr(zipfile, "ZipFile", forbidden) - with pytest.raises(ValidationServiceError, match="metadata exceeds"): + with pytest.raises(ValidationServiceError, match="Archive contains more entries than the configured maximum"): storage.extract_archive(path, "bounded")