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

[HetCreep]

Description

The _extract_zip method in src/skillspector/input_handler.py extracts uploaded zip files using Python's zipfile.ZipFile.extractall() without verifying or sanitizing the path names of the archived files. This permits a malicious zip file containing path traversal sequences (e.g. ../../evil.py) to write files outside the target extraction directory on the host system.

Vulnerable Code

In input_handler.py:181–182:

with zipfile.ZipFile(zip_path, "r") as zf:
    zf.extractall(extract_dir)  # No path or size verification

Proof of Concept

A malicious client can package a zip archive where file paths are constructed with traversal strings:

import zipfile
with zipfile.ZipFile("payload.zip", "w") as zf:
    zf.writestr("../../../../tmp/evil.py", "print('exploited')")

When scanned:

skillspector scan payload.zip

The file will be extracted outside extract_dir (e.g. into /tmp/evil.py).

Remediation

1. Explicitly sanitize each member path before extraction by checking that its resolved path starts with the extraction directory.
2. For Python 3.12+, utilize the filter="data" argument in extractall().
3. Reject extraction if any member contains directory traversal sequences (.. or Windows equivalent \..).

[wernerkasselman-au]

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.

[HetCreep]

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.