Java: Add ZipSlip query.

change-notes/1.19/analysis-java.md

@@ -6,6 +6,7 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Arbitrary file write during archive extraction ("Zip Slip") (`java/zipslip`) | security, external/cwe/cwe-022 | Identifies extraction routines that allow arbitrary file overwrite vulnerabilities. |
 | Missing catch of NumberFormatException (`java/uncaught-number-format-exception`) | reliability, external/cwe/cwe-248 | Finds calls to `Integer.parseInt` and similar string-to-number conversions that might raise a `NumberFormatException` without a corresponding `catch`-clause. |
 
 ## Changes to existing queries

java/ql/src/Security/CWE/CWE-022/ZipSlip.qhelp

@@ -0,0 +1,70 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Extracting files from a malicious zip archive (or another archive format)
+without validating that the destination file path
+is within the destination directory can cause files outside the destination directory to be
+overwritten, due to the possible presence of directory traversal elements (<code>..</code>) in
+archive paths.</p>
+
+<p>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 (<code>..</code>). If these
+file paths are used to determine an output file to write the contents of the archive item to, then
+the file may be written to 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.</p>
+
+<p>For example, if a zip file contains a file entry <code>..\sneaky-file</code>, and the zip file
+is extracted to the directory <code>c:\output</code>, then naively combining the paths would result
+in an output file path of <code>c:\output\..\sneaky-file</code>, which would cause the file to be
+written to <code>c:\sneaky-file</code>.</p>
+
+</overview>
+<recommendation>
+
+<p>Ensure that output paths constructed from zip archive entries are validated to prevent writing
+files to unexpected locations.</p>
+
+<p>The recommended way of writing an output file from a zip archive entry is to
+verify that the normalized full path of the output file starts with a prefix that matches the
+destination directory. Path normalization can be done with either
+<code>java.io.File.getCanonicalFile()</code> or <code>java.nio.file.Path.normalize()</code>.
+Prefix checking can be done with <code>String.startsWith(..)</code>, but it is better to use
+<code>java.nio.file.Path.startsWith(..)</code>, as the latter works on complete path segments.
+</p>
+
+<p>Another alternative is to validate archive entries against a whitelist of expected files.</p>
+
+</recommendation>
+<example>
+
+<p>In this example, a file path taken from a zip archive item entry is combined with a
+destination directory. The result is used as the destination file path without verifying that
+the result is within the destination directory. If provided with a zip file containing an archive
+path like <code>..\sneaky-file</code>, then this file would be written outside the destination
+directory.</p>
+
+<sample src="ZipSlipBad.java" />
+
+<p>To fix this vulnerability, we need to verify that the normalized <code>file</code> still has
+<code>destinationDir</code> as its prefix, and throw an exception if this is not the case.</p>
+
+<sample src="ZipSlipGood.java" />
+
+</example>
+<references>
+
+<li>
+Snyk:
+<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
+</li>
+<li>
+OWASP:
+<a href="https://www.owasp.org/index.php/Path_traversal">Path Traversal</a>.
+</li>
+
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-022/ZipSlip.ql

@@ -0,0 +1,176 @@
+/**
+ * @name Arbitrary file write during archive extraction ("Zip Slip")
+ * @description Extracting files from a malicious archive without validating that the
+ *              destination file path is within the destination directory can cause files outside
+ *              the destination directory to be overwritten.
+ * @kind problem
+ * @id java/zipslip
+ * @problem.severity error
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-022
+ */
+
+import java
+import semmle.code.java.controlflow.Guards
+import semmle.code.java.dataflow.SSA
+import semmle.code.java.dataflow.TaintTracking
+import DataFlow
+
+/**
+ * A method that returns the name of an archive entry.
+ */
+class ArchiveEntryNameMethod extends Method {
+  ArchiveEntryNameMethod() {
+    exists(RefType archiveEntry |
+      archiveEntry.hasQualifiedName("java.util.zip", "ZipEntry") or
+      archiveEntry.hasQualifiedName("org.apache.commons.compress.archivers", "ArchiveEntry")
+    |
+      this.getDeclaringType().getASupertype*() = archiveEntry and
+      this.hasName("getName")
+    )
+  }
+}
+
+/**
+ * An expression that will be treated as the destination of a write.
+ */
+class WrittenFileName extends Expr {
+  WrittenFileName() {
+    // Constructors that write to their first argument.
+    exists(ConstructorCall ctr | this = ctr.getArgument(0) |
+      exists(Class c | ctr.getConstructor() = c.getAConstructor() |
+        c.hasQualifiedName("java.io", "FileOutputStream") or
+        c.hasQualifiedName("java.io", "RandomAccessFile") or
+        c.hasQualifiedName("java.io", "FileWriter")
+      )
+    )
+    or
+    // Methods that write to their n'th argument
+    exists(MethodAccess call, int n | this = call.getArgument(n) |
+      call.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Files") and
+      (
+        call.getMethod().getName().regexpMatch("new.*Reader|newOutputStream|create.*") and n = 0
+        or
+        call.getMethod().hasName("copy") and n = 1
+        or
+        call.getMethod().hasName("move") and n = 1
+      )
+    )
+  }
+}
+
+/**
+ * Holds if `n1` to `n2` is a dataflow step that converts between `String`,
+ * `File`, and `Path`.
+ */
+predicate filePathStep(ExprNode n1, ExprNode n2) {
+  exists(ConstructorCall cc | cc.getConstructedType() instanceof TypeFile |
+    n1.asExpr() = cc.getAnArgument() and
+    n2.asExpr() = cc
+  )
+  or
+  exists(MethodAccess ma, Method m |
+    ma.getMethod() = m and
+    n1.asExpr() = ma.getQualifier() and
+    n2.asExpr() = ma
+  |
+    m.getDeclaringType() instanceof TypeFile and m.hasName("toPath")
+    or
+    m.getDeclaringType() instanceof TypePath and m.hasName("toAbsolutePath")
+  )
+}
+
+predicate fileTaintStep(ExprNode n1, ExprNode n2) {
+  exists(MethodAccess ma, Method m |
+    n1.asExpr() = ma.getQualifier() or
+    n1.asExpr() = ma.getAnArgument()
+  |
+    n2.asExpr() = ma and
+    ma.getMethod() = m and
+    m.getDeclaringType() instanceof TypePath and
+    m.hasName("resolve")
+  )
+}
+
+predicate localFileValueStep(Node n1, Node n2) {
+  localFlowStep(n1, n2) or
+  filePathStep(n1, n2)
+}
+
+predicate localFileValueStepPlus(Node n1, Node n2) = fastTC(localFileValueStep/2)(n1, n2)
+
+/**
+ * Holds if `check` is a guard that checks whether `var` is a file path with a
+ * specific prefix when put in canonical form, thus guarding against ZipSlip.
+ */
+predicate validateFilePath(SsaVariable var, Guard check) {
+  // `var.getCanonicalFile().toPath().startsWith(...)`,
+  // `var.getCanonicalPath().startsWith(...)`, or
+  // `var.toPath().normalize().startsWith(...)`
+  exists(MethodAccess normalize, MethodAccess startsWith, Node n1, Node n2, Node n3, Node n4 |
+    n1.asExpr() = var.getAUse() and
+    n2.asExpr() = normalize.getQualifier() and
+    (n1 = n2 or localFileValueStepPlus(n1, n2)) and
+    n3.asExpr() = normalize and
+    n4.asExpr() = startsWith.getQualifier() and
+    (n3 = n4 or localFileValueStepPlus(n3, n4)) and
+    check = startsWith and
+    startsWith.getMethod().hasName("startsWith") and
+    (
+      normalize.getMethod().hasName("getCanonicalFile") or
+      normalize.getMethod().hasName("getCanonicalPath") or
+      normalize.getMethod().hasName("normalize")
+    )
+  )
+}
+
+/**
+ * Holds if `m` validates its `arg`th parameter.
+ */
+predicate validationMethod(Method m, int arg) {
+  exists(Guard check, SsaImplicitInit var, ControlFlowNode exit, ControlFlowNode normexit |
+    validateFilePath(var, check) and
+    var.isParameterDefinition(m.getParameter(arg)) and
+    exit = m and
+    normexit.getANormalSuccessor() = exit and
+    1 = strictcount(ControlFlowNode n | n.getANormalSuccessor() = exit)
+  |
+    check.(ConditionNode).getATrueSuccessor() = exit or
+    check.controls(normexit.getBasicBlock(), true)
+  )
+}
+
+class ZipSlipConfiguration extends TaintTracking::Configuration {
+  ZipSlipConfiguration() { this = "ZipSlip" }
+
+  override predicate isSource(Node source) {
+    source.asExpr().(MethodAccess).getMethod() instanceof ArchiveEntryNameMethod
+  }
+
+  override predicate isSink(Node sink) { sink.asExpr() instanceof WrittenFileName }
+
+  override predicate isAdditionalTaintStep(Node n1, Node n2) {
+    filePathStep(n1, n2) or fileTaintStep(n1, n2)
+  }
+
+  override predicate isSanitizer(Node node) {
+    exists(Guard g, SsaVariable var, RValue varuse | validateFilePath(var, g) |
+      varuse = node.asExpr() and
+      varuse = var.getAUse() and
+      g.controls(varuse.getBasicBlock(), true)
+    )
+    or
+    exists(MethodAccess ma, int pos, RValue rv |
+      validationMethod(ma.getMethod(), pos) and
+      ma.getArgument(pos) = rv and
+      adjacentUseUseSameVar(rv, node.asExpr()) and
+      ma.getBasicBlock().bbDominates(node.asExpr().getBasicBlock())
+    )
+  }
+}
+
+from Node source, Node sink
+where any(ZipSlipConfiguration c).hasFlow(source, sink)
+select source, "Unsanitized archive entry, which may contain '..', is used in a $@.", sink,
+  "file system operation"

java/ql/src/Security/CWE/CWE-022/ZipSlipBad.java

@@ -0,0 +1,5 @@
+void writeZipEntry(ZipEntry entry, File destinationDir) {
+    File file = new File(destinationDir, entry.getName());
+    FileOutputStream fos = new FileOutputStream(file); // BAD
+    // ... write entry to fos ...
+}

java/ql/src/Security/CWE/CWE-022/ZipSlipGood.java

@@ -0,0 +1,7 @@
+void writeZipEntry(ZipEntry entry, File destinationDir) {
+    File file = new File(destinationDir, entry.getName());
+    if (!file.toPath().normalize().startsWith(destinationDir.toPath()))
+        throw new Exception("Bad zip entry");
+    FileOutputStream fos = new FileOutputStream(file); // OK
+    // ... write entry to fos ...
+}

java/ql/src/semmle/code/java/JDK.qll

@@ -133,8 +133,8 @@ class TypeObjectOutputStream extends RefType {
 /** The class `java.nio.file.Paths`. */
 class TypePaths extends Class { TypePaths() { this.hasQualifiedName("java.nio.file", "Paths") } }
 
-/** The class `java.nio.file.Path`. */
-class TypePath extends Class { TypePath() { this.hasQualifiedName("java.nio.file", "Path") } }
+/** The type `java.nio.file.Path`. */
+class TypePath extends RefType { TypePath() { this.hasQualifiedName("java.nio.file", "Path") } }
 
 /** The class `java.nio.file.FileSystem`. */
 class TypeFileSystem extends Class {

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

@@ -0,0 +1,3 @@
+| ZipTest.java:7:19:7:33 | getName(...) | 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(...) | 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(...) | Unsanitized archive entry, which may contain '..', is used in a $@. | ZipTest.java:11:36:11:39 | file | file system operation |

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

@@ -0,0 +1 @@
+Security/CWE/CWE-022/ZipSlip.ql

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

@@ -0,0 +1,54 @@
+import java.io.*;
+import java.nio.file.*;
+import java.util.zip.*;
+
+public class ZipTest {
+  public void m1(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    FileOutputStream os = new FileOutputStream(file); // ZipSlip
+    RandomAccessFile raf = new RandomAccessFile(file, "rw"); // ZipSlip
+    FileWriter fw = new FileWriter(file); // ZipSlip
+  }
+
+  public void m2(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    File canFile = file.getCanonicalFile();
+    String canDir = dir.getCanonicalPath();
+    if (!canFile.toPath().startsWith(canDir))
+      throw new Exception();
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+
+  public void m3(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    if (!file.toPath().normalize().startsWith(dir.toPath()))
+      throw new Exception();
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+
+  private void validate(File tgtdir, File file) {
+    File canFile = file.getCanonicalFile();
+    if (!canFile.toPath().startsWith(tgtdir.toPath()))
+      throw new Exception();
+  }
+
+  public void m4(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    validate(dir, file);
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+
+  public void m5(ZipEntry entry, File dir) {
+    String name = entry.getName();
+    File file = new File(dir, name);
+    Path absfile = file.toPath().toAbsolutePath().normalize();
+    Path absdir = dir.toPath().toAbsolutePath().normalize();
+    if (!absfile.startsWith(absdir))
+      throw new Exception();
+    FileOutputStream os = new FileOutputStream(file); // OK
+  }
+}