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, [