From 9a69251ac2614cfa918b19ec4b6f9ee647645408 Mon Sep 17 00:00:00 2001 From: MarkLee131 Date: Sat, 28 Mar 2026 17:37:39 +0800 Subject: [PATCH] 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) --- .../2026-03-28-zipslip-narrow-sink.md | 4 ++ .../code/java/security/ZipSlipQuery.qll | 56 ++++++++++++++++++- .../CWE-022/semmle/tests/ZipSlip.expected | 20 +++---- .../CWE-022/semmle/tests/ZipTest.java | 22 ++++++++ 4 files changed, 91 insertions(+), 11 deletions(-) create mode 100644 java/ql/lib/change-notes/2026-03-28-zipslip-narrow-sink.md diff --git a/java/ql/lib/change-notes/2026-03-28-zipslip-narrow-sink.md b/java/ql/lib/change-notes/2026-03-28-zipslip-narrow-sink.md new file mode 100644 index 000000000000..35ca560a68ce --- /dev/null +++ b/java/ql/lib/change-notes/2026-03-28-zipslip-narrow-sink.md @@ -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. diff --git a/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll b/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll index 9e2e5e4a6c7e..fc2905192497 100644 --- a/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll +++ b/java/ql/lib/semmle/code/java/security/ZipSlipQuery.qll @@ -52,7 +52,61 @@ module ZipSlipFlow = TaintTracking::Global; /** * 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"]) + ) + ) + } } diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected index 0d081a456e8e..dbfac72fa825 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipSlip.expected @@ -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 diff --git a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java index b5bee61b9654..2e170269a38a 100644 --- a/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java +++ b/java/ql/test/query-tests/security/CWE-022/semmle/tests/ZipTest.java @@ -1,6 +1,7 @@ import java.io.*; import java.nio.file.*; import java.util.zip.*; +import java.util.*; public class ZipTest { public void m1(ZipEntry entry, File dir) throws Exception { @@ -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 + } }