-
Notifications
You must be signed in to change notification settings - Fork 18
GeneXus compression module #874
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Cherry pick to beta failed, 1 conflicted file in commit 594547c
|
Manual cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
Cherry pick to beta success |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the concerns that were raised have now been addressed and resolved
Cherry pick to beta success |
Cherry pick to beta success |
try (ZipInputStream zis = new ZipInputStream(Files.newInputStream(archive.toPath()))) { | ||
ZipEntry zipEntry; | ||
while ((zipEntry = zis.getNextEntry()) != null) { | ||
File newFile = new File(directory, zipEntry.getName()); |
Check failure
Code scanning / CodeQL
Arbitrary file access during archive extraction ("Zip Slip") High
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the issue, we need to validate the zipEntry.getName()
to ensure it does not contain directory traversal sequences or attempt to write files outside the intended extraction directory. This can be achieved by:
- Normalizing the path of the extracted file using
File.getCanonicalPath()
to resolve any..
or symbolic links. - Verifying that the normalized path starts with the canonical path of the intended extraction directory.
- Throwing an exception if the validation fails.
The fix will involve:
- Normalizing the
newFile
path and the target directory path. - Checking that the normalized
newFile
path starts with the normalized target directory path. - Rejecting any zip entries that fail this validation.
-
Copy modified lines R591-R596
@@ -590,2 +590,8 @@ | ||
File newFile = new File(directory, zipEntry.getName()); | ||
// Validate the new file path to prevent directory traversal | ||
String canonicalDirPath = new File(directory).getCanonicalPath(); | ||
String canonicalFilePath = newFile.getCanonicalPath(); | ||
if (!canonicalFilePath.startsWith(canonicalDirPath + File.separator)) { | ||
throw new IOException("Entry is outside of the target dir: " + zipEntry.getName()); | ||
} | ||
if (zipEntry.isDirectory()) { |
try (SevenZFile sevenZFile = new SevenZFile(archive)) { | ||
SevenZArchiveEntry entry; | ||
while ((entry = sevenZFile.getNextEntry()) != null) { | ||
File newFile = new File(directory, entry.getName()); |
Check failure
Code scanning / CodeQL
Arbitrary file access during archive extraction ("Zip Slip") High
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
try (TarArchiveInputStream tis = new TarArchiveInputStream(Files.newInputStream(archive.toPath()))) { | ||
TarArchiveEntry entry; | ||
while ((entry = tis.getNextEntry()) != null) { | ||
File newFile = new File(directory, entry.getName()); |
Check failure
Code scanning / CodeQL
Arbitrary file access during archive extraction ("Zip Slip") High
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Unsanitized archive entry, which may contain '..', is used in a
file system operation
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 months ago
To fix the issue, we need to validate the paths derived from entry.getName()
to ensure they remain within the intended extraction directory. This can be achieved by normalizing the constructed file path and verifying that it starts with the canonical path of the destination directory. If the validation fails, the code should throw an exception to prevent unsafe file operations.
Steps to implement the fix:
- Normalize the constructed file path using
File.getCanonicalFile()
orPath.normalize()
. - Compare the normalized path with the canonical path of the destination directory using
Path.startsWith()
to ensure it is within the intended directory. - Throw an exception if the validation fails.
-
Copy modified lines R642-R646
@@ -641,3 +641,7 @@ | ||
while ((entry = tis.getNextEntry()) != null) { | ||
File newFile = new File(directory, entry.getName()); | ||
File newFile = new File(directory, entry.getName()).getCanonicalFile(); | ||
File canonicalDir = new File(directory).getCanonicalFile(); | ||
if (!newFile.toPath().normalize().startsWith(canonicalDir.toPath())) { | ||
throw new IOException("Entry is outside the target directory: " + entry.getName()); | ||
} | ||
if (entry.isDirectory()) { |
No description provided.