Arbitrary file access during archive extraction ("Zip Slip")#339
Merged
kevin-mcgoldrick merged 2 commits intomasterfrom Aug 12, 2024
Merged
Arbitrary file access during archive extraction ("Zip Slip")#339kevin-mcgoldrick merged 2 commits intomasterfrom
kevin-mcgoldrick merged 2 commits intomasterfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracting files from a malicious zip file, or similar type of archive, is at risk of directory traversal attacks if filenames from the archive are not properly validated.
Zip archives contain archive entries representing each file in the archive. These entries include a file path for the entry, but these file paths are not restricted and may contain unexpected special elements such as the directory traversal element (..). If these file paths are used to create a filesystem path, then a file operation may happen in an unexpected location. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files.
For example, if a zip file contains a file entry ..\sneaky-file, and the zip file is extracted to the directory c:\output, then naively combining the paths would result in an output file path of c:\output..\sneaky-file, which would cause the file to be written to c:\sneaky-file.
Recommendation
Ensure that output paths constructed from zip archive entries are validated to prevent writing files to unexpected locations.
The recommended way of writing an output file from a zip archive entry is to verify that the normalized full path of the output file starts with a prefix that matches the destination directory. Path normalization can be done with either java.io.File.getCanonicalFile() or java.nio.file.Path.normalize(). Prefix checking can be done with String.startsWith(..), but it is better to use java.nio.file.Path.startsWith(..), as the latter works on complete path segments.
Another alternative is to validate archive entries against a whitelist of expected files.
Example
In this example, a file path taken from a zip archive item entry is combined with a destination directory. The result is used as the destination file path without verifying that the result is within the destination directory. If provided with a zip file containing an archive path like ..\sneaky-file, then this file would be written outside the destination directory.
To fix this vulnerability, we need to verify that the normalized file still has destinationDir as its prefix, and throw an exception if this is not the case.
References
Snyk: Zip Slip Vulnerability.
OWASP: Path Traversal.
Common Weakness Enumeration: CWE-22.
Please make sure these check boxes are checked before submitting
mvn clean test -P default** PR review process **