[java] arbitrary directory creation during archive extraction to a pathname to a restricted directory

[odaysec]

User description

selenium/java/src/org/openqa/selenium/io/Zip.java

Line 103 in 3744929

File file = new File(outputDir, entry.getName());

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.

fix the issue, we need to validate the paths of directories being created in the unzip method. Specifically:

• Before calling FileHandler.createDir for a directory entry, ensure that the canonical path of the directory is within the canonical path of the output directory.
• Use File.getCanonicalPath() to normalize paths and compare them using String.startsWith() to ensure the directory is within the intended output directory.

This fix will prevent directory traversal attacks by ensuring that no directories or files are created outside the intended output directory.

---

PR Type

Bug fix

---

Description

• Fix directory traversal vulnerability in zip extraction

• Add canonical path validation for directory entries

• Prevent malicious archives from creating files outside target directory

---

Diagram Walkthrough

flowchart LR
  A["Zip Entry"] --> B["Get Canonical Path"]
  B --> C["Validate Path"]
  C --> D{"Path within target?"}
  D -->|Yes| E["Create Directory"]
  D -->|No| F["Throw IOException"]

Loading

File Walkthrough

	Relevant files
Security	Zip.java Add directory traversal protection                                              java/src/org/openqa/selenium/io/Zip.java Add canonical path validation before directory creation Check if directory path is within target output directory Throw IOException for paths outside target directory +5/-0

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 Security concerns Incomplete security fix: The current implementation only protects against directory traversal for directory entries, but regular file entries remain vulnerable to the same attack. An attacker could still use path traversal sequences like "../../../etc/passwd" in file names to write files outside the intended directory. The validation logic should be applied to all entries before any file operations.

⚡ Recommended focus areas for review Incomplete Fix The fix only validates directory entries but not regular file entries. Malicious archives can still create files outside the target directory using path traversal in file names. The same canonical path validation should be applied to all entries, not just directories. if (entry.isDirectory()) { String canonicalOutputDirPath = outputDir.getCanonicalPath(); String canonicalDirPath = file.getCanonicalPath(); if (!canonicalDirPath.startsWith(canonicalOutputDirPath + File.separator)) { throw new IOException("Directory entry is outside of the target dir: " + entry.getName()); } FileHandler.createDir(file); continue; } Logic Error The path validation logic has a flaw - it only checks if the path starts with outputDir + File.separator, but this doesn't handle the case where the canonical path equals the output directory path exactly. A malicious entry could potentially bypass this check. if (!canonicalDirPath.startsWith(canonicalOutputDirPath + File.separator)) { throw new IOException("Directory entry is outside of the target dir: " + entry.getName()); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Protect files from zip slip The validation only applies to directories but file entries also need protection against zip slip attacks. Move this validation logic outside the directory-only block to protect both files and directories. java/src/org/openqa/selenium/io/Zip.java [105-109] String canonicalOutputDirPath = outputDir.getCanonicalPath(); -String canonicalDirPath = file.getCanonicalPath(); -if (!canonicalDirPath.startsWith(canonicalOutputDirPath + File.separator)) { - throw new IOException("Directory entry is outside of the target dir: " + entry.getName()); +String canonicalFilePath = file.getCanonicalPath(); +if (!canonicalFilePath.startsWith(canonicalOutputDirPath + File.separator) && + !canonicalFilePath.equals(canonicalOutputDirPath)) { + throw new IOException("Entry is outside of the target dir: " + entry.getName()); } +if (entry.isDirectory()) { [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 10 __ Why: This suggestion addresses a critical security issue by pointing out that the PR's zip slip validation only covers directories, leaving file entries vulnerable. Applying the check to all entries is essential for security.	High
	Fix path validation edge case The path validation logic has a flaw when the output directory is the root directory or when paths are equal. Add a check for exact path equality to handle edge cases where canonicalDirPath.equals(canonicalOutputDirPath) should be allowed. java/src/org/openqa/selenium/io/Zip.java [107-109] -if (!canonicalDirPath.startsWith(canonicalOutputDirPath + File.separator)) { +if (!canonicalDirPath.startsWith(canonicalOutputDirPath + File.separator) && + !canonicalDirPath.equals(canonicalOutputDirPath)) { throw new IOException("Directory entry is outside of the target dir: " + entry.getName()); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a logical flaw in the new validation logic that would cause an exception when the entry path is the same as the output directory, thus improving the correctness of the PR.	Medium
More

[cgoldberg]

whether valid or not, I just wanted to point out that @odaysec's commit history is basically all just drive-by AI-generated slop.

[joerg1985]

In general this change does make sense, but the impact is mutch lower than the title did tell.
So i have update the title to match the actual issue the PR does fix.

@odaysec please run the format script, so this could be merged.