🔒 Fix Zip Slip vulnerability in ZipRODirectory#33
Conversation
The `ZipRODirectory` class iterates over entries of a zip file but failed to comprehensively ensure that the `name` of the `ZipEntry` did not contain malicious path traversal sequences (like `../` or `..\`) or absolute paths (like `/etc/passwd` or `C:\Windows\System32\cmd.exe`), relying instead on a weak `.contains(".." + separator)` check. This could result in an arbitrary file write upon extraction.
This change adds comprehensive validation to explicitly reject Zip entries containing:
- Path traversal elements (`../`, `..\`)
- Absolute UNIX paths (starting with `/`)
- Absolute Windows paths (starting with `\`, or matching a drive letter regex `^[a-zA-Z]:[\\\\/].*`)
Also added a unit test `ZipRODirectoryZipSlipTest` to verify that `ZipRODirectory` correctly rejects such entries.
Co-authored-by: jslawson865 <145846954+jslawson865@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
The `ZipRODirectory` class iterates over entries of a zip file but failed to comprehensively ensure that the `name` of the `ZipEntry` did not contain malicious path traversal sequences (like `../` or `..\`) or absolute paths (like `/etc/passwd` or `C:\Windows\System32\cmd.exe`), relying instead on a weak `.contains(".." + separator)` check. This could result in an arbitrary file write upon extraction.
This change adds comprehensive validation to explicitly reject Zip entries containing:
- Path traversal elements (`../`, `..\`)
- Absolute UNIX paths (starting with `/`)
- Absolute Windows paths (starting with `\`, or matching a drive letter regex `^[a-zA-Z]:[\\\\/].*`)
Also added a unit test `ZipRODirectoryZipSlipTest` to verify that `ZipRODirectory` correctly rejects such entries.
Fixed CI failure due to windows-latest deprecation by switching to windows-2025 in build.yml.
Co-authored-by: jslawson865 <145846954+jslawson865@users.noreply.github.com>
1 participant
🎯 What: Fixed a Zip Slip vulnerability in
⚠️ Risk: The vulnerability could allow a malicious ZIP file to write arbitrary files outside the intended extraction directory due to path traversal strings (e.g.
ZipRODirectory.../) or absolute paths.🛡️ Solution: Implemented robust validation to filter out
ZipEntrys containing../,..\,/,\, and Windows drive letter absolute paths, and added unit tests to verify the fix.PR created automatically by Jules for task 731810063137283838 started by @jslawson865