Fix Zip Slip vulnerability in archive extraction (#296)#304
Fix Zip Slip vulnerability in archive extraction (#296)#304slachiewicz merged 1 commit intoplexus-utils-3.xfrom
Conversation
--------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: slachiewicz <6705942+slachiewicz@users.noreply.github.com> (cherry picked from commit 6d780b3)
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical Zip Slip vulnerability in the archive extraction functionality. The Zip Slip vulnerability allows attackers to write files outside the intended extraction directory through malicious archive entries containing path traversal sequences (e.g., ../../evil.txt).
Changes:
- Implemented path validation using canonical paths with proper separator checks to prevent directory traversal attacks
- Added comprehensive test coverage for various Zip Slip attack vectors including parent directory traversal, absolute paths, and similar directory name exploits
- Enhanced error handling for path verification failures
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/java/org/codehaus/plexus/util/Expand.java | Implements canonical path validation with File.separator-based prefix checking to prevent path traversal attacks during archive extraction |
| src/test/java/org/codehaus/plexus/util/ExpandTest.java | Adds comprehensive test coverage for Zip Slip vulnerability scenarios and verifies normal extraction still works correctly |
Comments suppressed due to low confidence (1)
src/main/java/org/codehaus/plexus/util/Expand.java:151
- Method extractFile ignores exceptional return value of File.setLastModified.
f.setLastModified(entryDate.getTime());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } catch (IOException e) { | ||
| throw new IOException("Failed to verify entry path for '" + entryName + "'", e); |
There was a problem hiding this comment.
The catch block will catch and re-wrap the IOException thrown by the security check at line 123, changing the error message from "Entry 'X' outside the target directory" to "Failed to verify entry path for 'X'". Consider catching the IOException from getCanonicalPath separately, or rethrowing the security exception without wrapping it to preserve the clearer error message.
| targetDir.mkdirs(); | ||
|
|
||
| // Create a malicious zip with absolute path | ||
| File evilTarget = new File("/tmp/evil-absolute.txt"); |
There was a problem hiding this comment.
The test uses a hardcoded absolute path '/tmp/evil-absolute.txt' which may not work correctly on all platforms (e.g., Windows systems). Consider using a path within the test directory or constructing a platform-appropriate absolute path for better cross-platform compatibility.
| File evilTarget = new File("/tmp/evil-absolute.txt"); | |
| File evilTarget = new File(tempDir, "evil-absolute.txt").getAbsoluteFile(); |
|
Is this a fix for CVE-2025-67030? Is there any plans to create a release tag and publish the version? Seems like a high vulnerability being reporting. |
Co-authored-by: copilot-swe-agent[bot] 198982749+Copilot@users.noreply.github.com
Co-authored-by: slachiewicz 6705942+slachiewicz@users.noreply.github.com
(cherry picked from commit 6d780b3)