Skip to content

Commit 9a69251

Browse files
MarkLee131MarkLee131
committed
Narrow ZipSlip sinks to file write operations, excluding read-only paths
The java/zipslip query previously reused the full path-injection sink set, which includes read-only operations like ClassLoader.getResource(), FileInputStream, and File.exists(). Since Zip Slip targets file extraction (write) vulnerabilities, read-only sinks are now excluded via a ReadOnlyPathSink class, reducing false positives. Add test cases for read-only archive entry name usage patterns: - ClassLoader.getSystemResources() (classpath resource lookup) - FileInputStream (read-only file access) - File.exists() (file inspection)
1 parent a8b52ac commit 9a69251

File tree

4 files changed

+91
-11
lines changed

4 files changed

+91
-11
lines changed

‎java/ql/lib/change-notes/2026-03-28-zipslip-narrow-sink.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* 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.

‎java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,61 @@ module ZipSlipFlow = TaintTracking::Global<ZipSlipConfig>;
5252

5353
/**
5454
* A sink that represents a file creation, such as a file write, copy or move operation.
55+
*
56+
* This excludes read-only operations (e.g., `ClassLoader.getResource`,
57+
* `FileInputStream`, `File.exists`) that are not relevant to Zip Slip,
58+
* which specifically targets file extraction (write) vulnerabilities.
5559
*/
5660
private class FileCreationSink extends DataFlow::Node {
57-
FileCreationSink() { sinkNode(this, "path-injection") }
61+
FileCreationSink() {
62+
sinkNode(this, "path-injection") and
63+
not this instanceof ReadOnlyPathSink
64+
}
65+
}
66+
67+
/**
68+
* A path-injection sink that only performs read or inspection operations
69+
* and is therefore not vulnerable to Zip Slip file extraction attacks.
70+
*/
71+
private class ReadOnlyPathSink extends DataFlow::Node {
72+
ReadOnlyPathSink() {
73+
sinkNode(this, "path-injection") and
74+
(
75+
// ClassLoader resource lookup methods
76+
exists(MethodCall mc | this.asExpr() = mc.getAnArgument() |
77+
mc.getMethod().getDeclaringType().hasQualifiedName("java.lang", ["ClassLoader", "Class"]) and
78+
mc.getMethod()
79+
.hasName(["getResource", "getResources", "getResourceAsStream",
80+
"getSystemResource", "getSystemResources", "getSystemResourceAsStream"])
81+
)
82+
or
83+
// File read-only inspection methods
84+
exists(MethodCall mc | this.asExpr() = mc.getQualifier() |
85+
mc.getMethod().getDeclaringType().hasQualifiedName("java.io", "File") and
86+
mc.getMethod()
87+
.hasName(["exists", "isFile", "isDirectory", "isHidden", "canRead", "canWrite",
88+
"canExecute", "getName", "getPath", "getAbsolutePath", "getCanonicalPath",
89+
"getCanonicalFile", "length", "lastModified"])
90+
)
91+
or
92+
// FileInputStream (read-only file access)
93+
exists(Call c | this.asExpr() = c.getAnArgument() |
94+
c.(ConstructorCall).getConstructedType().hasQualifiedName("java.io", "FileInputStream")
95+
or
96+
c.(ConstructorCall).getConstructedType().hasQualifiedName("java.io", "FileReader")
97+
)
98+
or
99+
// NIO read-only methods
100+
exists(MethodCall mc | this.asExpr() = mc.getAnArgument() |
101+
mc.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and
102+
mc.getMethod()
103+
.hasName(["exists", "notExists", "isDirectory", "isRegularFile", "isReadable",
104+
"isWritable", "isExecutable", "isHidden", "isSameFile", "isSymbolicLink",
105+
"probeContentType", "getFileStore", "getLastModifiedTime", "getOwner",
106+
"getAttribute", "readAttributes", "size",
107+
"readAllBytes", "readAllLines", "readString", "lines",
108+
"newBufferedReader", "newInputStream"])
109+
)
110+
)
111+
}
58112
}

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,18 @@
11
#select
2-
| 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 |
3-
| 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 |
4-
| 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 |
2+
| 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 |
3+
| 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 |
4+
| 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 |
55
edges
6-
| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:9:48:9:51 | file | provenance | AdditionalTaintStep Sink:MaD:1 |
7-
| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:10:49:10:52 | file | provenance | AdditionalTaintStep Sink:MaD:3 |
8-
| ZipTest.java:7:19:7:33 | getName(...) : String | ZipTest.java:11:36:11:39 | file | provenance | AdditionalTaintStep Sink:MaD:2 |
6+
| ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:10:48:10:51 | file | provenance | AdditionalTaintStep Sink:MaD:1 |
7+
| ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:11:49:11:52 | file | provenance | AdditionalTaintStep Sink:MaD:3 |
8+
| ZipTest.java:8:19:8:33 | getName(...) : String | ZipTest.java:12:36:12:39 | file | provenance | AdditionalTaintStep Sink:MaD:2 |
99
models
1010
| 1 | Sink: java.io; FileOutputStream; false; FileOutputStream; ; ; Argument[0]; path-injection; manual |
1111
| 2 | Sink: java.io; FileWriter; false; FileWriter; ; ; Argument[0]; path-injection; manual |
1212
| 3 | Sink: java.io; RandomAccessFile; false; RandomAccessFile; ; ; Argument[0]; path-injection; manual |
1313
nodes
14-
| ZipTest.java:7:19:7:33 | getName(...) : String | semmle.label | getName(...) : String |
15-
| ZipTest.java:9:48:9:51 | file | semmle.label | file |
16-
| ZipTest.java:10:49:10:52 | file | semmle.label | file |
17-
| ZipTest.java:11:36:11:39 | file | semmle.label | file |
14+
| ZipTest.java:8:19:8:33 | getName(...) : String | semmle.label | getName(...) : String |
15+
| ZipTest.java:10:48:10:51 | file | semmle.label | file |
16+
| ZipTest.java:11:49:11:52 | file | semmle.label | file |
17+
| ZipTest.java:12:36:12:39 | file | semmle.label | file |
1818
subpaths

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import java.io.*;
22
import java.nio.file.*;
33
import java.util.zip.*;
4+
import java.util.*;
45

56
public class ZipTest {
67
public void m1(ZipEntry entry, File dir) throws Exception {
@@ -60,4 +61,25 @@ public void m6(ZipEntry entry, Path dir) throws Exception {
6061
throw new Exception();
6162
OutputStream os = Files.newOutputStream(target); // OK
6263
}
64+
65+
// GOOD: Entry name used for read-only operations, not file extraction
66+
public void m7(ZipEntry entry) throws Exception {
67+
String name = entry.getName();
68+
// ClassLoader resource lookup is not a file write
69+
ClassLoader.getSystemResources(name); // OK - read-only resource lookup
70+
}
71+
72+
// GOOD: Entry name used for FileInputStream (read-only)
73+
public void m8(ZipEntry entry, File dir) throws Exception {
74+
String name = entry.getName();
75+
File file = new File(dir, name);
76+
FileInputStream fis = new FileInputStream(file); // OK - read-only
77+
}
78+
79+
// GOOD: Entry name used for File.exists() check (read-only)
80+
public void m9(ZipEntry entry, File dir) throws Exception {
81+
String name = entry.getName();
82+
File file = new File(dir, name);
83+
boolean exists = file.exists(); // OK - read-only inspection
84+
}
6385
}

0 commit comments

Comments
 (0)