Merge pull request #889 from jcreedcmu/jcreed/tarslip

JavaScript: Add new query for ZipSlip (CWE-022).

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

@@ -0,0 +1,67 @@
+<!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 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 check that
+<code>".."</code> does not occur in the path.
+</p>
+
+</recommendation>
+
+<example>
+<p>
+In this example an archive is extracted without validating file paths.
+If <code>archive.zip</code> contained relative paths (for
+instance, if it were created by something like <code>zip archive.zip
+../file.txt</code>) then executing this code could write to locations
+outside the destination directory.
+</p>
+
+<sample src="ZipSlipBad.js" />
+
+<p>To fix this vulnerability, we need to check that the path does not
+contain any <code>".."</code> elements in it.
+</p>
+
+<sample src="ZipSlipGood.js" />
+
+</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>

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

@@ -0,0 +1,22 @@
+/**
+ * @name Arbitrary file write during zip extraction ("Zip Slip")
+ * @description 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.
+ * @kind path-problem
+ * @id js/zipslip
+ * @problem.severity error
+ * @precision medium
+ * @tags security
+ *       external/cwe/cwe-022
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.ZipSlip::ZipSlip
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "Unsanitized zip archive $@, which may contain '..', is used in a file system operation.",
+  source.getNode(), "item path"

javascript/ql/src/Security/CWE-022/ZipSlipBad.js

@@ -0,0 +1,10 @@
+const fs = require('fs');
+const unzip = require('unzip');
+
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = entry.path;
+    // BAD: This could write any file on the filesystem.
+    entry.pipe(fs.createWriteStream(fileName));
+  });

javascript/ql/src/Security/CWE-022/ZipSlipGood.js

@@ -0,0 +1,15 @@
+const fs = require('fs');
+const unzip = require('unzip');
+
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = entry.path;
+    // GOOD: ensures the path is safe to write to.
+    if (fileName.indexOf('..') == -1) {
+      entry.pipe(fs.createWriteStream(fileName));
+    }
+    else {
+      console.log('skipping bad path', fileName);
+    }
+  });

javascript/ql/src/semmle/javascript/security/dataflow/ZipSlip.qll

@@ -0,0 +1,111 @@
+/**
+ * Provides a taint tracking configuration for reasoning about unsafe zip extraction.
+ */
+
+import javascript
+
+module ZipSlip {
+  /**
+   * A data flow source for unsafe zip extraction.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for unsafe zip extraction.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for unsafe zip extraction.
+   */
+  abstract class SanitizerGuard extends TaintTracking::SanitizerGuardNode, DataFlow::ValueNode { }
+
+  /** A taint tracking configuration for unsafe zip extraction. */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "ZipSlip" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode nd) {
+      nd instanceof SanitizerGuard
+    }
+  }
+
+  /**
+   * Gets a node that can be a parsed zip archive.
+   */
+  private DataFlow::SourceNode parsedArchive() {
+    result = DataFlow::moduleImport("unzip").getAMemberCall("Parse")
+    or
+    // `streamProducer.pipe(unzip.Parse())` is a typical (but not
+    // universal) pattern when using nodejs streams, whose return
+    // value is the parsed stream.
+    exists(DataFlow::MethodCallNode pipe |
+      pipe = result and
+      pipe.getMethodName() = "pipe" and
+      parsedArchive().flowsTo(pipe.getArgument(0))
+    )
+  }
+
+  /** A zip archive entry path access, as a source for unsafe zip extraction. */
+  class UnzipEntrySource extends Source {
+    // For example, in
+    // ```javascript
+    // const unzip = require('unzip');
+    //
+    // fs.createReadStream('archive.zip')
+    //   .pipe(unzip.Parse())
+    //   .on('entry', entry => {
+    //      const path = entry.path;
+    //   });
+    // ```
+    // there is an `UnzipEntrySource` node corresponding to
+    // the expression `entry.path`.
+    UnzipEntrySource() {
+      exists(DataFlow::CallNode cn |
+        cn = parsedArchive().getAMemberCall("on") and
+        cn.getArgument(0).mayHaveStringValue("entry") and
+        this = cn.getCallback(1)
+        .getParameter(0)
+        .getAPropertyRead("path"))
+    }
+  }
+
+  /** A call to `fs.createWriteStream`, as a sink for unsafe zip extraction. */
+  class CreateWriteStreamSink extends Sink {
+    CreateWriteStreamSink() {
+      // This is not covered by `FileSystemWriteSink`, because it is
+      // required that a write actually takes place to the stream.
+      // However, we want to consider even the bare `createWriteStream`
+      // to be a zipslip vulnerability since it may truncate an
+      // existing file.
+      this = DataFlow::moduleImport("fs").getAMemberCall("createWriteStream").getArgument(0)
+    }
+  }
+
+  /** A file path of a file write, as a sink for unsafe zip extraction. */
+  class FileSystemWriteSink extends Sink {
+    FileSystemWriteSink() { exists(FileSystemWriteAccess fsw | fsw.getAPathArgument() = this) }
+  }
+
+  /**
+   * Gets a string which is sufficient to exclude to make
+   * a filepath definitely not refer to parent directories.
+   */
+  private string getAParentDirName() { result = ".." or result = "../" }
+
+  /** A check that a path string does not include '..' */
+  class NoParentDirSanitizerGuard extends SanitizerGuard {
+    StringOps::Includes incl;
+
+    NoParentDirSanitizerGuard() { this = incl }
+
+    override predicate sanitizes(boolean outcome, Expr e) {
+      incl.getPolarity().booleanNot() = outcome and
+      incl.getBaseString().asExpr() = e and
+      incl.getSubstring().mayHaveStringValue(getAParentDirName())
+    }
+  }
+}

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

@@ -0,0 +1,17 @@
+nodes
+| ZipSlipBad2.js:5:9:5:46 | fileName |
+| ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
+| ZipSlipBad2.js:5:37:5:46 | entry.path |
+| ZipSlipBad2.js:6:22:6:29 | fileName |
+| ZipSlipBad.js:7:11:7:31 | fileName |
+| ZipSlipBad.js:7:22:7:31 | entry.path |
+| ZipSlipBad.js:8:37:8:44 | fileName |
+edges
+| ZipSlipBad2.js:5:9:5:46 | fileName | ZipSlipBad2.js:6:22:6:29 | fileName |
+| ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path | ZipSlipBad2.js:5:9:5:46 | fileName |
+| ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:5:20:5:46 | 'output ... ry.path |
+| ZipSlipBad.js:7:11:7:31 | fileName | ZipSlipBad.js:8:37:8:44 | fileName |
+| ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:7:11:7:31 | fileName |
+#select
+| ZipSlipBad2.js:6:22:6:29 | fileName | ZipSlipBad2.js:5:37:5:46 | entry.path | ZipSlipBad2.js:6:22:6:29 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad2.js:5:37:5:46 | entry.path | item path |
+| ZipSlipBad.js:8:37:8:44 | fileName | ZipSlipBad.js:7:22:7:31 | entry.path | ZipSlipBad.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:7:22:7:31 | entry.path | item path |

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

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

javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js

@@ -0,0 +1,9 @@
+const fs = require('fs');
+const unzip = require('unzip');
+
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = entry.path;
+    entry.pipe(fs.createWriteStream(fileName));
+  });

javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad2.js

@@ -0,0 +1,8 @@
+var fs = require('fs');
+var unzip = require('unzip');
+fs.readFile('path/to/archive.zip', function (err, zipContents) {
+  unzip.Parse(zipContents).on('entry', function (entry) {
+    var fileName = 'output/path/' + entry.path;
+    fs.writeFileSync(fileName, entry.contents);
+  });
+});

javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipGood.js

@@ -0,0 +1,14 @@
+const fs = require('fs');
+const unzip = require('unzip');
+
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = entry.path;
+    if (fileName.indexOf('..') == -1) {
+      entry.pipe(fs.createWriteStream(fileName));
+    }
+    else {
+      console.log('skipping bad path', fileName);
+    }
+  });

javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/externs.js

@@ -0,0 +1,11 @@
+/**
+ * @externs
+ */
+var fs = {};
+
+/**
+ * @param {string} filename
+ * @param {*} data
+ * @return {void}
+ */
+fs.writeFileSync = function(filename, data) {};