Merge pull request github#3480 from erik-krogh/moreSlip

Approved by esbena

Author: semmle-qlci

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
 

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()
       )
     }
   }

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)
     }

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}")
+      )
     }
   }
 

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 |

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");
+  });

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) {};