From 6dc1288dd104329b85af60fdbd187c35def00bb0 Mon Sep 17 00:00:00 2001 From: ethernet Date: Thu, 24 Sep 2026 13:55:15 -0400 Subject: [PATCH] refactor(pm): route .deb payloads through the one tar extractor DebPackage kept its own 116-line member walker next to pm.store.extract_tar. Both enforced the same containment rules, so the .deb data.tar now goes through extract_tar (which accepts an open stream) and FilterError maps to InstallError. Link fallbacks on hosts without symlink support come from tarfile's own copy fallback, which also resolves chained aliases. The Termux mode normalization stays in DebPackage, since it is .deb policy and not extraction. The per-member FilterError/OSError swallow in pm/packages.py was already removed; Git's MSYS /proc link exemption is unchanged. --- pm/package.py | 105 ++++++------------------------------ pm/store.py | 10 ++-- tests/pm/test_deb_safety.py | 4 +- 3 files changed, 24 insertions(+), 95 deletions(-) diff --git a/pm/package.py b/pm/package.py index aa61cd3391..9f49699a11 100644 --- a/pm/package.py +++ b/pm/package.py @@ -172,8 +172,8 @@ class DebPackage(Package): .deb (Package + Version) must match the pin. Digest verification of the downloaded bytes happens in the store, as for every package. - unpack() is the hardened extractor: traversal, symlink-target, and - member-type checks, shaped after the established safe-extract rules + unpack() pulls data.tar out of the ar container and hands it to + pm.store.extract_tar, the one containment policy for every PM tarball (symlinks allowed with in-root targets; devices/fifos refused). """ @@ -210,98 +210,21 @@ class DebPackage(Package): offset = start + size + (size % 2) if payload is None: raise InstallError(self.name, f"no data.tar member in {archive.name}") - self._safe_untar(payload, staged) + self._untar_payload(payload, staged) - def _safe_untar(self, payload: bytes, staged: Path) -> None: + def _untar_payload(self, payload: bytes, staged: Path) -> None: import io - import posixpath - import shutil import stat as stat_mod import tarfile + from pm.store import extract_tar + + try: + extract_tar(io.BytesIO(payload), staged) + except tarfile.FilterError as exc: + member = exc.tarinfo.name if exc.tarinfo is not None else "?" + raise InstallError(self.name, f"unsafe member {member!r}: {exc}") from exc real_staged = os.path.realpath(staged) - - def _contained(p: Path) -> bool: - """True when p's REAL location (following any planted symlink - ancestors) stays inside the staged tree.""" - real = os.path.realpath(p) - return real == real_staged or real.startswith(real_staged + os.sep) - - deferred_links = [] - with tarfile.open(fileobj=io.BytesIO(payload)) as tf: - for member in tf.getmembers(): - path = PurePosixPath(member.name) - parts = tuple(q for q in path.parts if q not in ("", ".")) - if path.is_absolute() or ".." in parts: - raise InstallError(self.name, f"unsafe member path {member.name!r}") - if not parts: - continue # the "./" root member - target = staged.joinpath(*parts) - if not _contained(target): - raise InstallError(self.name, f"member escapes staged tree: {member.name}") - if member.isdir(): - target.mkdir(parents=True, exist_ok=True) - continue - if member.issym(): - if PurePosixPath(member.linkname).is_absolute() or Path(member.linkname).is_absolute(): - raise InstallError( - self.name, - f"absolute symlink target {member.linkname!r} " - f"in {member.name}", - ) - link_dir = "/".join(parts[:-1]) - resolved = posixpath.normpath(posixpath.join(link_dir, member.linkname)) - if resolved == ".." or resolved.startswith("../"): - raise InstallError(self.name, f"symlink escapes root: {member.name}") - if not _contained(target.parent / member.linkname): - raise InstallError(self.name, f"symlink escapes staged tree: {member.name}") - target.parent.mkdir(parents=True, exist_ok=True) - if target.exists() or target.is_symlink(): - target.unlink() - try: - target.symlink_to(member.linkname) - except OSError: - resolved_path = staged.joinpath(*resolved.split("/")) - if resolved_path.is_file(): - shutil.copy2(resolved_path, target) - else: - deferred_links.append((member.linkname, target, resolved_path)) - continue - if not member.isfile(): - raise InstallError(self.name, f"unsupported member type: {member.name}") - target.parent.mkdir(parents=True, exist_ok=True) - if not _contained(target.parent): - raise InstallError( - self.name, - f"member {member.name} resolves outside the staged tree " - "through a symlinked ancestor", - ) - extracted = tf.extractfile(member) - if extracted is None: - raise InstallError(self.name, f"cannot read member {member.name}") - with extracted, open(target, "wb") as dst: - shutil.copyfileobj(extracted, dst) - try: - target.chmod(member.mode & 0o777) - except OSError: - pass - while deferred_links: - pending = [] - for linkname, target, resolved_path in deferred_links: - if not _contained(resolved_path): - raise InstallError( - self.name, - f"symlink {target.name} resolves outside the staged tree", - ) - if target.exists() or target.is_symlink(): - continue - if resolved_path.is_file(): - shutil.copy2(resolved_path, target) - else: - pending.append((linkname, target, resolved_path)) - if len(pending) == len(deferred_links): - break - deferred_links = pending # Termux debs carry owner-only modes across the whole tree (700 on # binaries n libs, 600 on stdlib .py files) -- postinst would # normalize on a real phone, but pm extracts without postinst, and @@ -321,13 +244,15 @@ class DebPackage(Package): continue if stat_mod.S_ISLNK(mode): continue - if not _contained(f): + if not os.path.realpath(f).startswith(real_staged + os.sep): continue wanted = 0o644 | (0o111 if mode & 0o111 else 0) try: f.chmod(wanted) except OSError: - pass + # Best effort by design: a file we cannot chmod keeps its + # extracted mode, and verify() still judges the tree. + continue def verify(self, entry: Path, target: str) -> str: """'' when the staged tree is plausible on target: the expected diff --git a/pm/store.py b/pm/store.py index cda20d5371..da9baba18e 100644 --- a/pm/store.py +++ b/pm/store.py @@ -12,6 +12,7 @@ import tempfile import time from contextlib import contextmanager from pathlib import Path +from typing import IO from pm.filesystem import is_junction @@ -157,8 +158,10 @@ def _tar_filter(member, dest: str): return member.replace(deep=False, uid=None, gid=None, uname=None, gname=None, mode=None) return tarfile.data_filter(member, dest) -def extract_tar(archive: Path, dest: Path, *, git_msys: bool = False) -> None: - """Extract a tarball with one containment policy for PM's tar consumers. +def extract_tar(archive: Path | IO[bytes], dest: Path, *, git_msys: bool = False) -> None: + """Extract a tarball (a path, or an open stream such as a .deb's data.tar) + with the one containment policy every PM tar consumer shares. Unsafe + members raise tarfile.FilterError. MSYS Git ships dev/fd links and etc/mtab into /proc; those aren't usable on Windows. Skip only those known links, never a filter error or failed file write. @@ -167,7 +170,8 @@ def extract_tar(archive: Path, dest: Path, *, git_msys: bool = False) -> None: dest.mkdir(parents=True, exist_ok=True) real_dest = os.path.realpath(dest) - with tarfile.open(archive) as tf: + opened = tarfile.open(archive) if isinstance(archive, (str, os.PathLike)) else tarfile.open(fileobj=archive) + with opened as tf: if git_msys: members = (m for m in tf if not (m.issym() and ( (m.name.lstrip("./").startswith("dev/") and m.linkname.startswith("/proc/")) diff --git a/tests/pm/test_deb_safety.py b/tests/pm/test_deb_safety.py index 769426778e..8e00d33d19 100644 --- a/tests/pm/test_deb_safety.py +++ b/tests/pm/test_deb_safety.py @@ -1,4 +1,4 @@ -"""DebPackage._safe_untar containment regressions. +"""DebPackage.unpack containment regressions (via pm.store.extract_tar). A safe extractor must forbid ANY write or chmod outside the staged tree. The malicious .deb fixtures are real ar+tars built in a temp sandbox and @@ -57,7 +57,7 @@ def test_chained_aliases_survive_without_symlink_support(tmp_path, monkeypatch): def unavailable(*args, **kwargs): raise OSError("symlinks unavailable") - monkeypatch.setattr(Path, "symlink_to", unavailable) + monkeypatch.setattr(os, "symlink", unavailable) deb = tmp_path / "aliases.deb" lib = "data/data/com.termux/files/usr/lib" _build_deb(deb, [