🐛 grader: `safe_extract` zip-slip check uses string prefix and can be bypassed

[howervoo]

TL;DR

The path-traversal guard in safe_extract (grader/app/main.py:61) compares resolved paths with str.startswith(...). A prefix match is not a path-boundary match, so an archive entry can escape the destination directory while still passing the check.

Details

def safe_extract(zip_path: Path, dest: Path) -> Path:
    with zipfile.ZipFile(zip_path) as archive:
        for member in archive.infolist():
            target = (dest / member.filename).resolve()
            if not str(target).startswith(str(dest.resolve())):
                raise ValueError(f"Unsafe archive path: {member.filename}")
        archive.extractall(dest)
    return normalize_root(dest)

The intent is correct — reject any member that resolves outside dest. The problem is the comparison. startswith treats dest as a plain string prefix, not as a directory boundary. So if dest resolves to /tmp/out, a target that resolves to /tmp/out-evil/payload.py passes the check, because:

"/tmp/out-evil/payload.py".startswith("/tmp/out")  # True

even though /tmp/out-evil is a sibling of the destination, not inside it. Since this runs on candidate-submitted zip files in the grader, that's untrusted input writing to an attacker-influenced path.

There's a secondary subtlety: the loop validates every member first, then calls archive.extractall(dest). extractall re-derives its own paths, so the function is trusting that its validation and extractall's behavior stay in lockstep — fine today, but it's the kind of thing that drifts.

References

• grader/app/main.py:61-68 — safe_extract
• The normalize_root helper it returns into is at grader/app/main.py:71

Suggested Fix

Use a real path-boundary check instead of a string prefix. Python 3.9+ has Path.is_relative_to:

root = dest.resolve()
target = (dest / member.filename).resolve()
if not target.is_relative_to(root):
    raise ValueError(f"Unsafe archive path: {member.filename}")

(or, pre-3.9, wrap target.relative_to(root) in a try/except). This rejects the out-evil sibling case correctly while still allowing legitimate nested paths.

Severity: Medium — zip-slip on untrusted submissions; impact depends on what the grader does with the extracted tree, but the guard is clearly weaker than intended.