Merge pull request #8004 from ahmed-farid-dev/ZipSlip

Add query to detect ZipSlip

python/ql/src/experimental/Security/CWE-022/ZipSlip.qhelp

@@ -0,0 +1,56 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>Extracting files from a malicious zip archive 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 archive contains a file entry <code>..\sneaky-file</code>, and the Zip archive
+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 call <code>extract()</code> or <code>extractall()</code>.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In this example an archive is extracted without validating file paths.
+</p>
+
+<sample src="zipslip_bad.py" />
+
+<p>To fix this vulnerability, we need to call the function <code>extractall()</code>.
+</p>
+
+<sample src="zipslip_good.py" />
+
+</example>
+<references>
+<li>
+Snyk:
+<a href="https://snyk.io/research/zip-slip-vulnerability">Zip Slip Vulnerability</a>.
+</li>
+
+</references>
+</qhelp>

python/ql/src/experimental/Security/CWE-022/ZipSlip.ql

@@ -0,0 +1,22 @@
+/**
+ * @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 path-problem
+ * @id py/zipslip
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision high
+ * @tags security
+ *       external/cwe/cwe-022
+ */
+
+import python
+import experimental.semmle.python.security.ZipSlip
+import DataFlow::PathGraph
+
+from ZipSlipConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Extraction of zipfile from $@", source.getNode(),
+  "a potentially untrusted source"

python/ql/src/experimental/Security/CWE-022/zipslip_bad.py

@@ -0,0 +1,16 @@
+import zipfile
+import shutil
+
+def unzip(filename):
+    with tarfile.open(filename) as zipf:
+    #BAD : This could write any file on the filesystem.
+        for entry in zipf:
+            shutil.copyfile(entry, "/tmp/unpack/")
+          
+def unzip4(filename):
+    zf = zipfile.ZipFile(filename)
+    filelist = zf.namelist()
+    for x in filelist:
+        with zf.open(x) as srcf:
+            shutil.copyfileobj(srcf, dstfile)
+

python/ql/src/experimental/Security/CWE-022/zipslip_good.py

@@ -0,0 +1,10 @@
+import zipfile 
+
+def unzip(filename, dir):
+    zf = zipfile.ZipFile(filename)
+    zf.extractall(dir)
+    
+
+def unzip1(filename, dir):
+    zf = zipfile.ZipFile(filename)
+    zf.extract(dir)

python/ql/src/experimental/semmle/python/Concepts.qll

@@ -14,6 +14,73 @@ private import semmle.python.dataflow.new.RemoteFlowSources
 private import semmle.python.dataflow.new.TaintTracking
 private import experimental.semmle.python.Frameworks
 
+/** Provides classes for modeling copying file related APIs. */
+module CopyFile {
+  /**
+   * A data flow node for copying file.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `CopyFile` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Gets the argument containing the path.
+     */
+    abstract DataFlow::Node getAPathArgument();
+
+    /**
+     * Gets fsrc argument.
+     */
+    abstract DataFlow::Node getfsrcArgument();
+  }
+}
+
+/**
+ * A data flow node for copying file.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `CopyFile::Range` instead.
+ */
+class CopyFile extends DataFlow::Node {
+  CopyFile::Range range;
+
+  CopyFile() { this = range }
+
+  DataFlow::Node getAPathArgument() { result = range.getAPathArgument() }
+
+  DataFlow::Node getfsrcArgument() { result = range.getfsrcArgument() }
+}
+
+/** Provides classes for modeling log related APIs. */
+module LogOutput {
+  /**
+   * A data flow node for log output.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `LogOutput` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Get the parameter value of the log output function.
+     */
+    abstract DataFlow::Node getAnInput();
+  }
+}
+
+/**
+ * A data flow node for log output.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `LogOutput::Range` instead.
+ */
+class LogOutput extends DataFlow::Node {
+  LogOutput::Range range;
+
+  LogOutput() { this = range }
+
+  DataFlow::Node getAnInput() { result = range.getAnInput() }
+}
+
 /**
  * Since there is both XML module in normal and experimental Concepts,
  * we have to rename the experimental module as this.

python/ql/src/experimental/semmle/python/Frameworks.qll

@@ -14,3 +14,4 @@ private import experimental.semmle.python.libraries.PyJWT
 private import experimental.semmle.python.libraries.Python_JWT
 private import experimental.semmle.python.libraries.Authlib
 private import experimental.semmle.python.libraries.PythonJose
+private import experimental.semmle.python.frameworks.CopyFile

python/ql/src/experimental/semmle/python/frameworks/CopyFile.qll

@@ -0,0 +1,42 @@
+private import python
+private import experimental.semmle.python.Concepts
+private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.ApiGraphs
+
+private module CopyFileImpl {
+  /**
+   * The `shutil` module provides methods to copy or move files.
+   * See:
+   * - https://docs.python.org/3/library/shutil.html#shutil.copyfile
+   * - https://docs.python.org/3/library/shutil.html#shutil.copy
+   * - https://docs.python.org/3/library/shutil.html#shutil.copy2
+   * - https://docs.python.org/3/library/shutil.html#shutil.copytree
+   * - https://docs.python.org/3/library/shutil.html#shutil.move
+   */
+  private class CopyFiles extends DataFlow::CallCfgNode, CopyFile::Range {
+    CopyFiles() {
+      this =
+        API::moduleImport("shutil")
+            .getMember(["copyfile", "copy", "copy2", "copytree", "move"])
+            .getACall()
+    }
+
+    override DataFlow::Node getAPathArgument() {
+      result in [this.getArg(0), this.getArgByName("src")]
+    }
+
+    override DataFlow::Node getfsrcArgument() { none() }
+  }
+
+  // TODO: once we have flow summaries, model `shutil.copyfileobj` which copies the content between its' file-like arguments.
+  // See https://docs.python.org/3/library/shutil.html#shutil.copyfileobj
+  private class CopyFileobj extends DataFlow::CallCfgNode, CopyFile::Range {
+    CopyFileobj() { this = API::moduleImport("shutil").getMember("copyfileobj").getACall() }
+
+    override DataFlow::Node getfsrcArgument() {
+      result in [this.getArg(0), this.getArgByName("fsrc")]
+    }
+
+    override DataFlow::Node getAPathArgument() { none() }
+  }
+}

python/ql/src/experimental/semmle/python/security/ZipSlip.qll

@@ -0,0 +1,39 @@
+import python
+import experimental.semmle.python.Concepts
+import semmle.python.dataflow.new.DataFlow
+import semmle.python.ApiGraphs
+import semmle.python.dataflow.new.TaintTracking
+
+class ZipSlipConfig extends TaintTracking::Configuration {
+  ZipSlipConfig() { this = "ZipSlipConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    (
+      source =
+        API::moduleImport("zipfile").getMember("ZipFile").getReturn().getMember("open").getACall() or
+      source =
+        API::moduleImport("zipfile")
+            .getMember("ZipFile")
+            .getReturn()
+            .getMember("namelist")
+            .getACall() or
+      source = API::moduleImport("tarfile").getMember("open").getACall() or
+      source = API::moduleImport("tarfile").getMember("TarFile").getACall() or
+      source = API::moduleImport("bz2").getMember("open").getACall() or
+      source = API::moduleImport("bz2").getMember("BZ2File").getACall() or
+      source = API::moduleImport("gzip").getMember("GzipFile").getACall() or
+      source = API::moduleImport("gzip").getMember("open").getACall() or
+      source = API::moduleImport("lzma").getMember("open").getACall() or
+      source = API::moduleImport("lzma").getMember("LZMAFile").getACall()
+    ) and
+    not source.getScope().getLocation().getFile().inStdlib()
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    (
+      sink = any(CopyFile copyfile).getAPathArgument() or
+      sink = any(CopyFile copyfile).getfsrcArgument()
+    ) and
+    not sink.getScope().getLocation().getFile().inStdlib()
+  }
+}

python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.expected

@@ -0,0 +1,34 @@
+edges
+| zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | zipslip_bad.py:10:13:10:17 | SSA variable entry |
+| zipslip_bad.py:10:13:10:17 | SSA variable entry | zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry |
+| zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | zipslip_bad.py:16:13:16:17 | SSA variable entry |
+| zipslip_bad.py:16:13:16:17 | SSA variable entry | zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry |
+| zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | zipslip_bad.py:22:13:22:17 | SSA variable entry |
+| zipslip_bad.py:22:13:22:17 | SSA variable entry | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry |
+| zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | zipslip_bad.py:29:13:29:13 | SSA variable x |
+| zipslip_bad.py:29:13:29:13 | SSA variable x | zipslip_bad.py:30:25:30:25 | ControlFlowNode for x |
+| zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | zipslip_bad.py:35:9:35:9 | SSA variable x |
+| zipslip_bad.py:35:9:35:9 | SSA variable x | zipslip_bad.py:37:32:37:32 | ControlFlowNode for x |
+nodes
+| zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| zipslip_bad.py:10:13:10:17 | SSA variable entry | semmle.label | SSA variable entry |
+| zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry |
+| zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| zipslip_bad.py:16:13:16:17 | SSA variable entry | semmle.label | SSA variable entry |
+| zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry |
+| zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| zipslip_bad.py:22:13:22:17 | SSA variable entry | semmle.label | SSA variable entry |
+| zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry |
+| zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| zipslip_bad.py:29:13:29:13 | SSA variable x | semmle.label | SSA variable x |
+| zipslip_bad.py:30:25:30:25 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+| zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() |
+| zipslip_bad.py:35:9:35:9 | SSA variable x | semmle.label | SSA variable x |
+| zipslip_bad.py:37:32:37:32 | ControlFlowNode for x | semmle.label | ControlFlowNode for x |
+subpaths
+#select
+| zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | zipslip_bad.py:11:25:11:29 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:8:10:8:31 | ControlFlowNode for Attribute() | a potentially untrusted source |
+| zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | zipslip_bad.py:17:26:17:30 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:14:10:14:28 | ControlFlowNode for Attribute() | a potentially untrusted source |
+| zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | zipslip_bad.py:23:29:23:33 | ControlFlowNode for entry | Extraction of zipfile from $@ | zipslip_bad.py:20:10:20:27 | ControlFlowNode for Attribute() | a potentially untrusted source |
+| zipslip_bad.py:30:25:30:25 | ControlFlowNode for x | zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | zipslip_bad.py:30:25:30:25 | ControlFlowNode for x | Extraction of zipfile from $@ | zipslip_bad.py:27:10:27:22 | ControlFlowNode for Attribute() | a potentially untrusted source |
+| zipslip_bad.py:37:32:37:32 | ControlFlowNode for x | zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | zipslip_bad.py:37:32:37:32 | ControlFlowNode for x | Extraction of zipfile from $@ | zipslip_bad.py:34:16:34:28 | ControlFlowNode for Attribute() | a potentially untrusted source |

python/ql/test/experimental/query-tests/Security/CWE-022/ZipSlip.qlref

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

python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_bad.py

@@ -0,0 +1,39 @@
+import tarfile
+import shutil
+import bz2
+import gzip
+import zipfile
+
+def unzip(filename):
+    with tarfile.open(filename) as zipf:
+    #BAD : This could write any file on the filesystem.
+        for entry in zipf:
+            shutil.move(entry, "/tmp/unpack/")
+
+def unzip1(filename):
+    with gzip.open(filename) as zipf:
+    #BAD : This could write any file on the filesystem.
+        for entry in zipf:
+            shutil.copy2(entry, "/tmp/unpack/")
+
+def unzip2(filename):
+    with bz2.open(filename) as zipf:
+    #BAD : This could write any file on the filesystem.
+        for entry in zipf:
+            shutil.copyfile(entry, "/tmp/unpack/")
+
+def unzip3(filename):
+    zf = zipfile.ZipFile(filename)
+    with zf.namelist() as filelist:
+    #BAD : This could write any file on the filesystem.
+        for x in filelist:
+            shutil.copy(x, "/tmp/unpack/")
+
+def unzip4(filename):
+    zf = zipfile.ZipFile(filename)
+    filelist = zf.namelist()
+    for x in filelist:
+        with zf.open(x) as srcf:
+            shutil.copyfileobj(x, "/tmp/unpack/")
+
+import tty # to set the import root so we can identify the standard library

python/ql/test/experimental/query-tests/Security/CWE-022/zipslip_good.py

@@ -0,0 +1,14 @@
+import zipfile 
+import tarfile
+import shutil
+
+def unzip(filename, dir):
+    zf = zipfile.ZipFile(filename)
+    zf.extractall(dir)
+    
+
+def unzip1(filename, dir):
+    zf = zipfile.ZipFile(filename)
+    zf.extract(dir)
+  
+