[SECURITY] Zip Slip Vulnerability: Unchecked zip extraction in input_handler.py allows arbitrary file write #109

Activity

1. wernerkasselman-au commented on Jun 22, 2026

   

   wernerkasselman-au

   on Jun 22, 2026

   Contributor

   More actions

   I went looking at this before adding more hardening, and I think the premise is mostly already handled by the standard library, so it is worth pinning down before more work goes into it.

   I tested the #109 PoC on CPython 3.13 (3.13.13) and the ../../../../tmp/evil.py member does not land in /tmp. zipfile.ZipFile.extractall sanitises every member before writing: _extract_member drops the drive/UNC prefix and strips . and .. components (zipfile/__init__.py:1831-1836, the invalid_path_parts = ('', os.path.curdir, os.path.pardir) filter), so ../../../../tmp/evil.py is flattened to tmp/evil.py inside extract_dir. A symlink member does not escape either: extraction always goes through self.open(...) then open(targetpath, "wb") and shutil.copyfileobj (:1861-1863), so a symlink-marked entry comes out as a regular file whose content is the link-target text, not a working symlink. I ran this against a crafted archive (../ members plus an S_IFLNK external_attr entry) and nothing landed outside the target dir.

   The reason that matters here is that the only extraction site in the tree is input_handler.py:182 zf.extractall(extract_dir). There is no manual os.path.join(name) + open(..., "wb") loop anywhere (which is the pattern the classic Zip Slip CVE actually exploits), and there is no tarfile use at all, so on any supported Python (the project pins >=3.12, and the stripping has been in extractall far longer than that) the zip path is not write-anywhere.

   Two concrete follow-ons for the cluster:

   • fix(security): prevent Zip Slip path traversal vulnerability in input… #116 as written would break the scanner, not fix it. The diff changes extractall(extract_dir) to extractall(extract_dir, filter="data"), but zipfile.ZipFile.extractall has no filter= parameter, that is a tarfile feature (PEP 706). On 3.12 and 3.13 it raises TypeError: ZipFile.extractall() got an unexpected keyword argument 'filter', so every zip scan would crash. I confirmed the TypeError on 3.13.13. The standard-library page the PR points at is the tarfile one, not zipfile.
   • fix(input-handler): validate URLs against SSRF and add zip-slip protection #159's added namelist() prefix check is harmless but redundant for the same reason, and the sibling-prefix edge in it (a bare startswith with no separator) is not reachable through extractall since the .. is already stripped. Fine to keep as belt-and-suspenders, just not load-bearing.

   One caveat so this is not overstated: this is specific to zipfile. If SkillSpector ever ingests tar archives, tarfile does recreate symlinks and device nodes and does need filter="data" (that is where that advice is right), and a git clone preserves symlinks too, so the symlink concern still applies to those ingest paths (#21). For the zip path on the current code though, I would close #109 and #116 rather than carry them, unless someone can show a member that escapes extract_dir on a supported Python, happy to be proven wrong with a PoC that actually reproduces.
2. HetCreep commented on Jun 23, 2026

   

   HetCreep

   on Jun 23, 2026

   Author

   More actions

   Thanks for the careful write-up — you're right, and I appreciate it. I went back and confirmed it against the CPython source rather than the generic Zip-Slip premise my report assumed:

   • On the supported range (requires-python = >=3.12,<3.14), ZipFile._extract_member sanitises every member before writing: invalid_path_parts = ('', os.path.curdir, os.path.pardir) strips ./.., and os.path.splitroot(arcname) strips the drive/root, so ../../../../tmp/evil.py collapses to tmp/evil.py inside extract_dir — no escape. (CPython 3.13 Lib/zipfile/__init__.py.)
   • input_handler.py:182 is the only extraction site, and it's extractall(extract_dir) — not the manual os.path.join(name) + open(..., "wb") loop that the classic Zip-Slip CVE actually needs. No tarfile use either.
   • And you're right that fix(security): prevent Zip Slip path traversal vulnerability in input… #116 would break the scanner rather than fix it: ZipFile.extractall(self, path=None, members=None, pwd=None) has no filter= parameter (that's a tarfile / PEP 706 feature), so extractall(extract_dir, filter="data") raises TypeError on 3.12/3.13.

   So my report over-generalised the Zip-Slip pattern to extractall, which already sanitises — that's on me. Happy to see #109 (and #116) closed as not-applicable on supported Python.

   The one place the concern is genuinely real is the caveat you raised in #21: tarfile does recreate symlinks/devices, and a git clone preserves symlinks, so if SkillSpector ever ingests tar archives or clones, that's where filter="data" / symlink-containment actually belongs — that feels like the right home for the hardening.
3. rng1995 commented on Aug 6, 2026

   

   rng1995

   on Aug 6, 2026

   Collaborator

   More actions

   Thanks for the report and the careful follow-up. As confirmed in the discussion, supported CPython versions already sanitize ZipFile.extractall() member paths, so the original write-outside-root PoC is not reproducible. In addition, merged PR #159 adds an explicit extraction-containment check and regression coverage as defense in depth. Closing as resolved/not applicable on the supported Python range.
4. rng1995

   closed this as not plannedon Aug 6, 2026