Skip to content

fix: prevent zip-slip path traversal when extracting project archives - #44

Open
Waynting wants to merge 1 commit into
aloth:mainfrom
Waynting:fix/zip-slip-path-traversal
Open

fix: prevent zip-slip path traversal when extracting project archives#44
Waynting wants to merge 1 commit into
aloth:mainfrom
Waynting:fix/zip-slip-path-traversal

Conversation

@Waynting

Copy link
Copy Markdown

Summary

olcli pull and olcli sync extract project zip archives by joining each
entry name directly onto the target directory:

const filePath = join(targetDir, entry.entryName);
// ...
writeFileSync(filePath, entry.getData());

Entry names are taken from the archive as-is, with no check for ..
segments, absolute paths, or Windows drive letters. A malicious or
compromised server can therefore craft an archive containing entries like
../../../../home/user/.bashrc and write files outside the target
directory on the user's machine (zip-slip, CWE-22).

This matters in practice because olcli explicitly supports self-hosted
Overleaf/ShareLaTeX instances, so the server is not always overleaf.com.
Note that adm-zip's built-in extractAllTo() has its own traversal
protection, but olcli extracts manually via entry.getData() +
writeFileSync(), which bypasses it.

Fix

  • Add a resolveWithin(baseDir, relativePath) helper in src/paths.ts.
    It resolves the candidate path against the base directory and returns
    null unless the result is strictly inside it. This rejects ..
    escapes, absolute paths, Windows drive letters, and sibling-prefix
    edge cases (/tmp/project-evil string-prefixed by /tmp/project).
  • Pull: unsafe entries are skipped with a warning (first 5 names
    listed) instead of being written. They are also excluded from the
    remoteManifest written to .olcli.json, so they can't pollute the
    deletion-propagation logic on subsequent syncs.
  • Sync: unsafe entries are filtered out when building the
    remoteFiles map, with a spinner warning. The write loop additionally
    re-checks via resolveWithin as defense in depth.

Legitimate archives are unaffected — safe entry names resolve to exactly
the same paths as before.

Testing

  • Added test/paths.test.ts (6 cases) using Node's built-in node:test,
    runnable with the existing tsx dev dependency — no new dependencies.
  • Verified against a real malicious archive. Since adm-zip normalizes
    names on creation, I crafted the zip with Python's zipfile (which
    stores arcnames verbatim) and ran the extraction filter over it:
  EXTRACT  main.tex -> /tmp/extract-target/main.tex
  BLOCKED  ../../../../tmp/pwned.sh
  BLOCKED  /etc/evil
  BLOCKED  figures/../../escape.txt
  • npx tsc --noEmit passes with no errors.

Side change

Added a "test": "tsx --test test/*.test.ts" script to package.json.
The publish workflow already runs npm test --if-present, which was
previously a no-op — with this script it now acts as a real verification
gate before publishing. Happy to drop this from the PR if you'd prefer to
keep it strictly scoped to the fix.

Checklist

  • npm test — 6/6 passing
  • npx tsc --noEmit — clean
  • No new runtime or dev dependencies
  • No behavior change for well-formed archives

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant