Skip to content

Narrow ZipSlip sinks to file write operations, excluding read-only paths #21609

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.

Sign up for GitHub

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

Open
MarkLee131 wants to merge 1 commit into
base: main
Choose a base branch
from
+91 −11
Open

Narrow ZipSlip sinks to file write operations, excluding read-only paths #21609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions java/ql/lib/change-notes/2026-03-28-zipslip-narrow-sink.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The `java/zipslip` query now excludes read-only file operations from its sinks. Previously, it reused the full `path-injection` sink set, which includes read-only operations such as `ClassLoader.getResource()`, `FileInputStream`, and `File.exists()`. Since Zip Slip specifically targets file extraction (write) vulnerabilities, these read-only sinks are no longer considered, reducing false positives.
56 changes: 55 additions & 1 deletion java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,61 @@ module ZipSlipFlow = TaintTracking::Global<ZipSlipConfig>;

/**
* A sink that represents a file creation, such as a file write, copy or move operation.
*
* This excludes read-only operations (e.g., `ClassLoader.getResource`,
* `FileInputStream`, `File.exists`) that are not relevant to Zip Slip,
* which specifically targets file extraction (write) vulnerabilities.
*/
private class FileCreationSink extends DataFlow::Node {
FileCreationSink() { sinkNode(this, "path-injection") }
FileCreationSink() {
sinkNode(this, "path-injection") and
not this instanceof ReadOnlyPathSink
}
}

/**
* A path-injection sink that only performs read or inspection operations
* and is therefore not vulnerable to Zip Slip file extraction attacks.
*/
private class ReadOnlyPathSink extends DataFlow::Node {
ReadOnlyPathSink() {
sinkNode(this, "path-injection") and
(
// ClassLoader resource lookup methods
exists(MethodCall mc | this.asExpr() = mc.getAnArgument() |
mc.getMethod().getDeclaringType().hasQualifiedName("java.lang", ["ClassLoader", "Class"]) and
mc.getMethod()
.hasName(["getResource", "getResources", "getResourceAsStream",
"getSystemResource", "getSystemResources", "getSystemResourceAsStream"])
)
or
// File read-only inspection methods
exists(MethodCall mc | this.asExpr() = mc.getQualifier() |
mc.getMethod().getDeclaringType().hasQualifiedName("java.io", "File") and
mc.getMethod()
.hasName(["exists", "isFile", "isDirectory", "isHidden", "canRead", "canWrite",
"canExecute", "getName", "getPath", "getAbsolutePath", "getCanonicalPath",
"getCanonicalFile", "length", "lastModified"])
)
or
// FileInputStream (read-only file access)
exists(Call c | this.asExpr() = c.getAnArgument() |
c.(ConstructorCall).getConstructedType().hasQualifiedName("java.io", "FileInputStream")
or
c.(ConstructorCall).getConstructedType().hasQualifiedName("java.io", "FileReader")
)
or
// NIO read-only methods
exists(MethodCall mc | this.asExpr() = mc.getAnArgument() |
mc.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and
mc.getMethod()
.hasName(["exists", "notExists", "isDirectory", "isRegularFile", "isReadable",
"isWritable", "isExecutable", "isHidden", "isSameFile", "isSymbolicLink",
"probeContentType", "getFileStore", "getLastModifiedTime", "getOwner",
"getAttribute", "readAttributes", "size",
"readAllBytes", "readAllLines", "readString", "lines",
"newBufferedReader", "newInputStream"])
)
)
}
}
20 changes: 10 additions & 10 deletions java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
#select
| ZipTest.java:7:19:7:33 | getName(...) | ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:9:48:9:51 | file | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:9:48:9:51 | file | file system operation |
| ZipTest.java:7:19:7:33 | getName(...) | ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:10:49:10:52 | file | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:10:49:10:52 | file | file system operation |
| ZipTest.java:7:19:7:33 | getName(...) | ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:11:36:11:39 | file | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:11:36:11:39 | file | file system operation |
| ZipTest.java:8:19:8:33 | getName(...) | ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:10:48:10:51 | file | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:10:48:10:51 | file | file system operation |
| ZipTest.java:8:19:8:33 | getName(...) | ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:11:49:11:52 | file | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:11:49:11:52 | file | file system operation |
| ZipTest.java:8:19:8:33 | getName(...) | ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:12:36:12:39 | file | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:12:36:12:39 | file | file system operation |
edges
| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:9:48:9:51 | file | provenance | AdditionalTaintStep Sink:MaD:1 |
| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:10:49:10:52 | file | provenance | AdditionalTaintStep Sink:MaD:3 |
| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:11:36:11:39 | file | provenance | AdditionalTaintStep Sink:MaD:2 |
| ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:10:48:10:51 | file | provenance | AdditionalTaintStep Sink:MaD:1 |
| ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:11:49:11:52 | file | provenance | AdditionalTaintStep Sink:MaD:3 |
| ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:12:36:12:39 | file | provenance | AdditionalTaintStep Sink:MaD:2 |
models
| 1 | Sink: java.io; FileOutputStream; false; FileOutputStream; ; ; Argument[0]; path-injection; manual |
| 2 | Sink: java.io; FileWriter; false; FileWriter; ; ; Argument[0]; path-injection; manual |
| 3 | Sink: java.io; RandomAccessFile; false; RandomAccessFile; ; ; Argument[0]; path-injection; manual |
nodes
| ZipTest.java:7:19:7:33 | getName(...) : String | semmle.label | getName(...) : String |
| ZipTest.java:9:48:9:51 | file | semmle.label | file |
| ZipTest.java:10:49:10:52 | file | semmle.label | file |
| ZipTest.java:11:36:11:39 | file | semmle.label | file |
| ZipTest.java:8:19:8:33 | getName(...) : String | semmle.label | getName(...) : String |
| ZipTest.java:10:48:10:51 | file | semmle.label | file |
| ZipTest.java:11:49:11:52 | file | semmle.label | file |
| ZipTest.java:12:36:12:39 | file | semmle.label | file |
subpaths
22 changes: 22 additions & 0 deletions java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import java.io.*;
import java.nio.file.*;
import java.util.zip.*;
import java.util.*;
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import java.util.*; appears unused in this test file (no types from java.util are referenced). Consider removing it, or (if you intended to use the return type from ClassLoader.getSystemResources) declaring a variable and importing only the specific types needed (for example Enumeration).

Suggested change
import java.util.*;

Copilot uses AI. Check for mistakes.

public class ZipTest {
public void m1(ZipEntry entry, File dir) throws Exception {
Expand Down Expand Up @@ -60,4 +61,25 @@ public void m6(ZipEntry entry, Path dir) throws Exception {
throw new Exception();
OutputStream os = Files.newOutputStream(target); // OK
}

// GOOD: Entry name used for read-only operations, not file extraction
public void m7(ZipEntry entry) throws Exception {
String name = entry.getName();
// ClassLoader resource lookup is not a file write
ClassLoader.getSystemResources(name); // OK - read-only resource lookup
}

// GOOD: Entry name used for FileInputStream (read-only)
public void m8(ZipEntry entry, File dir) throws Exception {
String name = entry.getName();
File file = new File(dir, name);
FileInputStream fis = new FileInputStream(file); // OK - read-only
}

// GOOD: Entry name used for File.exists() check (read-only)
public void m9(ZipEntry entry, File dir) throws Exception {
String name = entry.getName();
File file = new File(dir, name);
boolean exists = file.exists(); // OK - read-only inspection
}
}
Loading