From 83e9ef71202cb7c552b4ae99b514ff5396f00bde Mon Sep 17 00:00:00 2001 From: parthrohit22 Date: Sat, 8 Aug 2026 20:46:07 +0100 Subject: [PATCH] security(core): harden tar extraction and cover archive path guards Uploaded archives are untrusted input, and the tar path called extractall without a filter. The explicit member loop already rejects traversal, links and devices, but nothing enforced that at write time, and Python 3.14 will change the default silently. Pass filter=data so CPython applies its own extraction hardening as defence in depth: absolute paths and .. traversal stripped, links, devices and setuid metadata refused as members are written. Available since 3.12 and the backend requires >=3.12,<3.14. The traversal and link guards had no test coverage at all, so a regression would have been silent. Add four tests covering tar traversal, tar absolute path, tar symlink and the equivalent zip traversal, verified to reject the attack with no file written outside the destination. Also set path_separator=os in alembic.ini. Alembic warns when it is unset and falls back to legacy splitting on spaces, commas and colons; os is the correct reading of the single . entry and is stable when the fallback goes. --- apps/backend/alembic.ini | 5 ++ apps/backend/app/storage/local.py | 12 +++- .../tests/test_ingestion_resource_budgets.py | 67 +++++++++++++++++++ 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/apps/backend/alembic.ini b/apps/backend/alembic.ini index 87070c56..ebf9135b 100644 --- a/apps/backend/alembic.ini +++ b/apps/backend/alembic.ini @@ -1,6 +1,11 @@ [alembic] script_location = alembic prepend_sys_path = . +# Alembic 1.16+ warns when this is unset and falls back to legacy splitting on +# spaces, commas and colons. `os` uses the platform path separator, which is +# the correct reading of the single "." entry above and keeps the behaviour +# stable when the legacy fallback is eventually removed. +path_separator = os sqlalchemy.url = sqlite:///./.local/partha.db [loggers] diff --git a/apps/backend/app/storage/local.py b/apps/backend/app/storage/local.py index 0b121990..5fde18ab 100644 --- a/apps/backend/app/storage/local.py +++ b/apps/backend/app/storage/local.py @@ -136,7 +136,17 @@ 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}, ) - archive.extractall(destination) + # `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") def _normalise_single_root(self, destination: Path) -> Path: children = [child for child in destination.iterdir() if child.name != "__MACOSX"] diff --git a/apps/backend/tests/test_ingestion_resource_budgets.py b/apps/backend/tests/test_ingestion_resource_budgets.py index 5821d759..00bba43f 100644 --- a/apps/backend/tests/test_ingestion_resource_budgets.py +++ b/apps/backend/tests/test_ingestion_resource_budgets.py @@ -232,6 +232,73 @@ def fake_clone(_: GitHubClient, __: str, destination: Path, ___: str | None = No assert _repositories_dir_is_empty(file_count_limited_client.storage_path) # type: ignore[attr-defined] +# --- archive path safety ------------------------------------------------------ +# +# These cover the guards in ``LocalStorage._safe_extract_tar`` / +# ``_safe_extract_zip`` that reject traversal and link members. They were +# previously untested, so a regression would have been silent — and the tar +# path additionally relies on ``extractall(filter="data")`` as defence in depth. + + +def _malicious_tar_gz_bytes(members: list[tarfile.TarInfo], payload: bytes = b"pwned") -> bytes: + """A tar built from raw ``TarInfo`` objects, so unsafe members can be forged.""" + buffer = io.BytesIO() + with tarfile.open(fileobj=buffer, mode="w:gz") as archive: + for info in members: + if info.isreg(): + info.size = len(payload) + archive.addfile(info, io.BytesIO(payload)) + else: + archive.addfile(info) + return buffer.getvalue() + + +def test_tar_archive_with_parent_traversal_path_is_rejected(auth_client): + """A member escaping the destination via ``..`` must never be written.""" + escaping = tarfile.TarInfo("../escaped.txt") + escaping.type = tarfile.REGTYPE + + response = _upload(auth_client, "evil.tar.gz", _malicious_tar_gz_bytes([escaping])) + + error = assert_error_response(response, 422, "validation_error") + assert error.message == "Archive contains unsafe paths." + + +def test_tar_archive_with_absolute_path_is_rejected(auth_client): + """An absolute member path must not be able to write outside the sandbox.""" + absolute = tarfile.TarInfo("/tmp/partha-escaped.txt") + absolute.type = tarfile.REGTYPE + + response = _upload(auth_client, "evil.tar.gz", _malicious_tar_gz_bytes([absolute])) + + error = assert_error_response(response, 422, "validation_error") + assert error.message == "Archive contains unsafe paths." + + +def test_tar_archive_with_symlink_member_is_rejected(auth_client): + """Symlinks are refused outright: they are the classic extraction escape.""" + link = tarfile.TarInfo("sample/link") + link.type = tarfile.SYMTYPE + link.linkname = "/etc/passwd" + + response = _upload(auth_client, "evil.tar.gz", _malicious_tar_gz_bytes([link])) + + error = assert_error_response(response, 422, "validation_error") + assert error.message == "Archive contains unsupported link or device entries." + + +def test_zip_archive_with_parent_traversal_path_is_rejected(auth_client): + """The zip path enforces the same containment rule as the tar path.""" + buffer = io.BytesIO() + with zipfile.ZipFile(buffer, "w") as archive: + archive.writestr("../escaped.txt", "pwned") + + response = _upload(auth_client, "evil.zip", buffer.getvalue()) + + error = assert_error_response(response, 422, "validation_error") + assert error.message == "Archive contains unsafe paths." + + # --- regression: unaffected happy path --------------------------------------