Commit c04c7da
committed
fix(project): close two path-escape holes in _safe_extract
The containment check was `str(target).startswith(str(dest))`, a string
prefix rather than a path boundary. With a destination basename of
`out`, a member named `../outsider.txt` resolves to a sibling whose path
begins with the destination's, and passed. So did `../out-evil/x.txt`.
`../escape.txt` was correctly blocked, which is why the hole survived.
The loop also read `member.name` and never `member.linkname`. A symlink
`escape_dir -> ../outside_target` followed by a member
`escape_dir/payload.txt` has two innocuous names, passed both checks,
and wrote through the symlink to a directory outside the destination.
Hardlinks and absolute-target symlinks were equally unguarded.
Containment now uses `Path.is_relative_to`, available across the
supported range, and link members have their target resolved and
boundary-checked -- symlinks against the link's own directory,
hardlinks against the archive root.
`extractall(filter="data")` blocks all of this and is the reason none of
it was exploitable on a current interpreter. The comment claiming it as
3.12+ understated its availability: it is present from 3.10.12 and
3.11.4, the PEP 706 backports, and absent in 3.10.11 and 3.11.3.
`requires-python = ">=3.10"` admits those, where the manual check is the
only defence.1 parent 8ab1a29 commit c04c7da
1 file changed
Lines changed: 29 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -81,16 +81,41 @@ def _sha256(path: Path) -> str: | |||
| 81 | 81 | ||
| 82 | 82 | ||
| 83 | 83 | def _safe_extract(tar_path: Path, dest: Path) -> None: | |
| 84 | - """Extract a tarball, refusing entries that escape ``dest``.""" | ||
| 84 | + """Extract a tarball, refusing entries that escape ``dest``. | ||
| 85 | + | ||
| 86 | + The manual checks below cover path escapes, link escapes, and special | ||
| 87 | + files. They are not a ``data_filter`` equivalent: notably they do not | ||
| 88 | + sanitize modes, so on the fallback path setuid/setgid bits and a | ||
| 89 | + directory member's permissions are applied as the archive states them. | ||
| 90 | + That is accepted here because the asset is pinned by SHA-256 and its | ||
| 91 | + ``build/utils.sh`` has to stay executable. | ||
| 92 | + """ | ||
| 85 | 93 | dest = dest.resolve() | |
| 86 | 94 | with tarfile.open(tar_path, "r:gz") as tar: | |
| 87 | 95 | members = tar.getmembers() | |
| 88 | 96 | for member in members: | |
| 89 | 97 | target = (dest / member.name).resolve() | |
| 90 | - if not str(target).startswith(str(dest)): | ||
| 98 | + # ``is_relative_to`` compares path components. A string prefix | ||
| 99 | + # test would accept a sibling whose name merely starts with | ||
| 100 | + # ``dest``, such as ``../out-evil/x.txt`` beside ``out/``. | ||
| 101 | + if not target.is_relative_to(dest): | ||
| 91 | 102 | raise RuntimeError(f"Refusing to extract unsafe path: {member.name}") | |
| 92 | - # ``filter='data'`` (3.12+) blocks unsafe members; older Pythons | ||
| 93 | - # fall back to the manual check above. | ||
| 103 | + if member.issym() or member.islnk(): | ||
| 104 | + # A link's name can be innocuous while its target escapes. | ||
| 105 | + # Symlink targets resolve against the link's own directory; | ||
| 106 | + # hardlink targets are relative to the archive root. | ||
| 107 | + base = target.parent if member.issym() else dest | ||
| 108 | + link_target = (base / member.linkname).resolve() | ||
| 109 | + if not link_target.is_relative_to(dest): | ||
| 110 | + raise RuntimeError(f"Refusing to extract unsafe link: {member.name} -> {member.linkname}") | ||
| 111 | + if member.isdev(): | ||
| 112 | + # FIFOs and device nodes. ``filter="data"`` rejects these; | ||
| 113 | + # the fallback would otherwise mknod them. Every member of | ||
| 114 | + # the pinned archives is a file, directory, or symlink. | ||
| 115 | + raise RuntimeError(f"Refusing to extract special file: {member.name}") | ||
| 116 | + # ``filter='data'`` blocks unsafe members. It landed in 3.12 and was | ||
| 117 | + # backported to 3.10.12 and 3.11.4 (PEP 706), so earlier patch | ||
| 118 | + # releases in the supported range fall back to the checks above. | ||
| 94 | 119 | try: | |
| 95 | 120 | tar.extractall(dest, filter="data") | |
| 96 | 121 | except TypeError: | |
0 commit comments