Merge pull request #3480 from erik-krogh/moreSlip

Approved by esbena
2025-12-16 16:53:25 +01:00 · 2020-05-16 21:17:27 +01:00
parent a42d80aa14 6d79bab7e4
commit 8d41ce1630
7 changed files with 64 additions and 27 deletions
--- a/change-notes/1.25/analysis-javascript.md
+++ b/change-notes/1.25/analysis-javascript.md
@@ -27,7 +27,7 @@
 | Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
 | Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
 | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
-| Zip Slip (`js/zipslip`) | More results | This query now recognizes zip-slip vulnerabilities involving links. |
+| Zip Slip (`js/zipslip`) | More results | This query now recognizes additional vulnerabilities. |

 ## Changes to libraries

--- a/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
+++ b/javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll
@@ -306,7 +306,7 @@ module NodeJSLib {

    FsFlowTarget() {
      exists(DataFlow::CallNode call, string methodName |
-        call = DataFlow::moduleMember("fs", methodName).getACall()
+        call = FS::moduleMember(methodName).getACall()
      |
        methodName = "realpathSync" and
        tainted = call.getArgument(0) and
@@ -430,27 +430,32 @@ module NodeJSLib {
  }

  /**
-   * A member `member` from module `fs` or its drop-in replacements `graceful-fs`, `fs-extra`, `original-fs`.
+   * Provides predicates for working with the "fs" module and its variants as a single module.
   */
-  private DataFlow::SourceNode fsModuleMember(string member) {
-    result = fsModule(DataFlow::TypeTracker::end()).getAPropertyRead(member)
-  }
+  module FS {
+    /**
+     * A member `member` from module `fs` or its drop-in replacements `graceful-fs`, `fs-extra`, `original-fs`.
+     */
+    DataFlow::SourceNode moduleMember(string member) {
+      result = fsModule(DataFlow::TypeTracker::end()).getAPropertyRead(member)
+    }

-  private DataFlow::SourceNode fsModule(DataFlow::TypeTracker t) {
-    exists(string moduleName |
-      moduleName = "fs" or
-      moduleName = "graceful-fs" or
-      moduleName = "fs-extra" or
-      moduleName = "original-fs"
-    |
-      result = DataFlow::moduleImport(moduleName)
+    private DataFlow::SourceNode fsModule(DataFlow::TypeTracker t) {
+      exists(string moduleName |
+        moduleName = "fs" or
+        moduleName = "graceful-fs" or
+        moduleName = "fs-extra" or
+        moduleName = "original-fs"
+      |
+        result = DataFlow::moduleImport(moduleName)
+        or
+        // extra support for flexible names
+        result.asExpr().(Require).getArgument(0).mayHaveStringValue(moduleName)
+      ) and
+      t.start()
      or
-      // extra support for flexible names
-      result.asExpr().(Require).getArgument(0).mayHaveStringValue(moduleName)
-    ) and
-    t.start()
-    or
-    exists(DataFlow::TypeTracker t2 | result = fsModule(t2).track(t2, t))
+      exists(DataFlow::TypeTracker t2 | result = fsModule(t2).track(t2, t))
+    }
  }

  /**
@@ -459,7 +464,7 @@ module NodeJSLib {
  private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::CallNode {
    string methodName;

-    NodeJSFileSystemAccess() { this = maybePromisified(fsModuleMember(methodName)).getACall() }
+    NodeJSFileSystemAccess() { this = maybePromisified(FS::moduleMember(methodName)).getACall() }

    /**
     * Gets the name of the called method.
@@ -582,8 +587,8 @@ module NodeJSLib {
        name = "readdir" or
        name = "realpath"
      |
-        this = fsModuleMember(name).getACall().getCallback([1 .. 2]).getParameter(1) or
-        this = fsModuleMember(name + "Sync").getACall()
+        this = FS::moduleMember(name).getACall().getCallback([1 .. 2]).getParameter(1) or
+        this = FS::moduleMember(name + "Sync").getACall()
      )
    }
  }
--- a/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/TaintedPathCustomizations.qll
@@ -155,11 +155,11 @@ module TaintedPath {
      input = getAnArgument() and
      output = this
      or
-      this = DataFlow::moduleMember("fs", "realpathSync").getACall() and
+      this = NodeJSLib::FS::moduleMember("realpathSync").getACall() and
      input = getArgument(0) and
      output = this
      or
-      this = DataFlow::moduleMember("fs", "realpath").getACall() and
+      this = NodeJSLib::FS::moduleMember("realpath").getACall() and
      input = getArgument(0) and
      output = getCallback(1).getParameter(1)
    }
--- a/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlipCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/ZipSlipCustomizations.qll
@@ -107,7 +107,14 @@ module ZipSlip {
      // 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)
+      this = NodeJSLib::FS::moduleMember("createWriteStream").getACall().getArgument(0)
+      or
+      // Not covered by `FileSystemWriteSink` because a later call
+      // to `fs.write` is required for a write to take place.
+      exists(DataFlow::CallNode call | this = call.getArgument(0) |
+        call = NodeJSLib::FS::moduleMember(["open", "openSync"]).getACall() and
+        call.getArgument(1).getStringValue().regexpMatch("(?i)w.{0,2}")
+      )
    }
  }

--- a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlip.expected
@@ -24,6 +24,11 @@ nodes
 | ZipSlipBad.js:15:22:15:31 | entry.path |
 | ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:16:30:16:37 | fileName |
+| ZipSlipBad.js:22:11:22:31 | fileName |
+| ZipSlipBad.js:22:22:22:31 | entry.path |
+| ZipSlipBad.js:22:22:22:31 | entry.path |
+| ZipSlipBad.js:23:28:23:35 | fileName |
+| ZipSlipBad.js:23:28:23:35 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path |
@@ -46,6 +51,10 @@ edges
 | ZipSlipBad.js:15:11:15:31 | fileName | ZipSlipBad.js:16:30:16:37 | fileName |
 | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
 | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:15:11:15:31 | fileName |
+| ZipSlipBad.js:22:11:22:31 | fileName | ZipSlipBad.js:23:28:23:35 | fileName |
+| ZipSlipBad.js:22:11:22:31 | fileName | ZipSlipBad.js:23:28:23:35 | fileName |
+| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
+| ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:22:11:22:31 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 | ZipSlipBadUnzipper.js:7:9:7:29 | fileName | ZipSlipBadUnzipper.js:8:37:8:44 | fileName |
 | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:7:9:7:29 | fileName |
@@ -57,4 +66,5 @@ edges
 | 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 |
 | ZipSlipBad.js:16:30:16:37 | fileName | ZipSlipBad.js:15:22:15:31 | entry.path | ZipSlipBad.js:16:30:16:37 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:15:22:15:31 | entry.path | item path |
+| ZipSlipBad.js:23:28:23:35 | fileName | ZipSlipBad.js:22:22:22:31 | entry.path | ZipSlipBad.js:23:28:23:35 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBad.js:22:22:22:31 | entry.path | item path |
 | ZipSlipBadUnzipper.js:8:37:8:44 | fileName | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | ZipSlipBadUnzipper.js:8:37:8:44 | fileName | Unsanitized zip archive $@, which may contain '..', is used in a file system operation. | ZipSlipBadUnzipper.js:7:20:7:29 | entry.path | item path |
--- a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/ZipSlipBad.js
@@ -15,3 +15,10 @@ fs.createReadStream('archive.zip')
    const fileName = entry.path;
    entry.pipe(Writer({path: fileName}));
  });
+
+fs.createReadStream('archive.zip')
+  .pipe(unzip.Parse())
+  .on('entry', entry => {
+    const fileName = entry.path;
+    var file = fs.openSync(fileName, "w");
+  });
--- a/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/externs.js
+++ b/javascript/ql/test/query-tests/Security/CWE-022/ZipSlip/externs.js
@@ -15,4 +15,12 @@ fs.writeFileSync = function(filename, data) {};
 * @param {(string|Buffer)} dstpath
 * @return {void}
 */
-fs.linkSync = function(srcpath, dstpath) {};
+fs.linkSync = function(srcpath, dstpath) {};
+
+/**
+ * @param {(string|Buffer)} path
+ * @param {(string|number)} flags
+ * @param {number=} mode
+ * @return {number}
+ */
+fs.openSync = function(path, flags, mode) {};