Skip to content

fix(OfficeUtils): strengthen ZipSlip path traversal protection in ZIP extraction#117

Open
maxm11 wants to merge 1 commit into
Euro-Office:mainfrom
maxm11:main
Open

fix(OfficeUtils): strengthen ZipSlip path traversal protection in ZIP extraction#117
maxm11 wants to merge 1 commit into
Euro-Office:mainfrom
maxm11:main

Conversation

@maxm11

@maxm11 maxm11 commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses Euro-Office/core#115 — Path Traversal in ZIP Extraction (ZipSlip).

Problem

The existing ZipSlip check in do_extract_currentfile() only rejected archive entries whose normalized path started with ../. This left several bypass vectors:

Payload Old Check
../../../etc/passwd Caught
foo/../../etc/passwd Bypassed — normalized to ../../etc/passwd but the check only looked at the filename, not the full output path
/etc/passwd (Unix) Bypassed — absolute paths were never rejected
`C:\Windows... (Windows) Bypassed — drive-letter absolute paths were never rejected

Fix

Replaced the weak check with proper prefix-based containment validation:

  1. Rejects empty normalized archive entry paths — catches degenerate entries like ./ or ..
  2. Rejects absolute paths in archive entries:
    • Unix-like: /etc/passwd → rejected immediately
    • Windows: C:\Windows\... or \\server\share\... → rejected immediately
  3. Prefix containment check — the core fix:
    • Normalizes both the output path and extraction directory via ShortenPath() to resolve all ../ and ./ components
    • Ensures the extraction directory ends with a separator for reliable prefix matching
    • Verifies normalized_output_path starts with normalized_extract_dir/
    • If not, returns UNZ_INTERNALERROR

Attack vectors now blocked

Payload Old Check New Check
../../../etc/passwd ✅ Caught ✅ Caught
foo/../../etc/passwd ❌ Bypassed ✅ Caught
/etc/passwd (Unix) ❌ Bypassed ✅ Caught
`C:\Windows... (Windows) ❌ Bypassed ✅ Caught
\\server\share\... (Windows) ❌ Bypassed ✅ Caught

Files changed

  • OfficeUtils/src/ZipUtilsCP.cpp — replaced weak check with prefix-based containment validation in do_extract_currentfile()

@maxm11 maxm11 requested a review from a team as a code owner July 4, 2026 15:05
@maxm11 maxm11 requested review from DmySyz and rikled and removed request for a team July 4, 2026 15:05
… extraction

Replace the weak check that only rejected paths starting with '../'
with proper prefix-based containment validation. The new logic:

- Rejects empty normalized archive entry paths
- Rejects absolute paths in archive entries (Unix / and Windows C:\, \UNC)
- Normalizes both output path and extraction directory via ShortenPath()
  to resolve all ../ and ./ components
- Verifies the resolved output path starts with the extraction directory
  prefix, rejecting any entry that escapes the target directory

Fixes Euro-Office#115

Agentic Model: Qwen3.6-35B-A3B (local)

Signed-off-by: Max Murphy <maxmrphy@gmail.com>
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