Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR adds a security fix for the zip extraction process to prevent path traversal (zip slip) attacks, changes the unzip method to return the count of extracted files instead of void, and bumps the library version to 10.0.alpha12.
Changes:
- Modified the
unzipmethod to usenormalize()instead oftoRealPath()for path validation, preventing path traversal attacks while avoiding issues with non-existent paths - Changed the
unzipmethod return type fromvoidtointto return the count of successfully extracted files - Added comprehensive test coverage for the security fix with multiple path traversal attack vectors
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| rlib-io/src/main/java/javasabr/rlib/io/util/FileUtils.java | Implements security fix for path traversal in unzip method and adds file count return value |
| rlib-io/src/test/java/javasabr/rlib/io/FileUtilsTest.java | Adds new test case with malicious zip entries to verify path traversal protection |
| build.gradle | Increments project version from 10.0.alpha11 to 10.0.alpha12 |
| README.md | Updates version reference in documentation from 10.0.alpha11 to 10.0.alpha12 |
|
|
| Path normalizedDestination = destination.normalize(); | ||
| int count = 0; | ||
| try (var zin = new ZipInputStream(Files.newInputStream(zipFile))) { | ||
| for (var entry = zin.getNextEntry(); entry != null; entry = zin.getNextEntry()) { | ||
|
|
||
| String entryName = entry.getName(); | ||
| Path targetFile = destination | ||
| .resolve(entryName) | ||
| .toRealPath(LinkOption.NOFOLLOW_LINKS); | ||
|
|
||
| if (!targetFile.startsWith(destination)) { | ||
| .normalize(); | ||
| if (!targetFile.startsWith(normalizedDestination)) { | ||
| LOGGER.warning(entryName, "Unexpected entry name:[%s] which is outside"::formatted); | ||
| continue; | ||
| } | ||
|
|
||
| if (entry.isDirectory()) { | ||
| Files.createDirectories(targetFile); | ||
| } else { | ||
| Files.createDirectories(targetFile.getParent()); | ||
| Files.copy(zin, targetFile, StandardCopyOption.REPLACE_EXISTING); | ||
| count++; |
There was a problem hiding this comment.
The zip-slip protection based on Path.normalize()+startsWith() does not protect against traversal via pre-existing symlinks inside the destination (e.g., destination contains a symlinked subdir pointing outside). To make this a robust security fix, validate against destination.toRealPath(NOFOLLOW_LINKS) and ensure the resolved target’s real parent path stays within the destination before writing.
| public static int unzip(Path destination, Path zipFile) { | ||
| if (!Files.exists(destination)) { | ||
| throw new IllegalArgumentException("The folder " + destination + " doesn't exist."); | ||
| } |
There was a problem hiding this comment.
unzip() only checks Files.exists(destination), but if destination exists and is a file (not a directory) extraction will fail later with an IOException. Consider validating Files.isDirectory(destination) up front and throwing an IllegalArgumentException with a clear message.
| } | |
| } | |
| if (!Files.isDirectory(destination)) { | |
| throw new IllegalArgumentException("The path " + destination + " is not a directory."); | |
| } |
| assertThat(unpackedFiles).isEqualTo(3); | ||
| assertThat(outputDir | ||
| .resolve("fileA.txt")) | ||
| .exists(); | ||
| assertThat(outputDir | ||
| .resolve("dir_a") | ||
| .resolve("fileC.txt")) | ||
| .exists(); |
There was a problem hiding this comment.
This test expects 3 unpacked files, which implies the entry "dir_a/../fileD.txt" is accepted and extracted, but it never asserts that fileD actually exists. Adding an assertion for outputDir.resolve("fileD.txt") (and/or ensuring the directory entry behavior) would make the test fully validate the intended unzip behavior.
|
|
|
1 participant
Add security fix for unzip process