JS: js/reflected-xss through file names

change-notes/1.19/analysis-javascript.md

@@ -0,0 +1,22 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+* Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
+  - file system access, for example through [fs-extra](https://github.com/jprichardson/node-fs-extra)
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+| *@name of query (Query ID)* | *Tags*    |*Aim of the new query and whether it is enabled by default or not*  |
+
+## Changes to existing queries
+
+| **Query**                      | **Expected impact**        | **Change**                                   |
+|--------------------------------|----------------------------|----------------------------------------------|
+| Reflected cross-site scripting | More true-positive results | This rule now treats file names as a source. |
+
+
+## Changes to QL libraries
+

javascript/ql/src/semmle/javascript/Concepts.qll

@@ -29,6 +29,13 @@ abstract class FileSystemAccess extends DataFlow::Node {
   abstract DataFlow::Node getAPathArgument();
 }
 
+/**
+ * A data flow node that contains a file name or an array of file names from the local file system.
+ */
+abstract class FileNameSource extends DataFlow::Node {
+
+}
+
 /**
  * A data flow node that performs a database access.
  */

javascript/ql/src/semmle/javascript/frameworks/NodeJSLib.qll

@@ -336,17 +336,26 @@ module NodeJSLib {
     )
   }
 
+  /**
+   * A member `member` from module `fs` or its drop-in replacements `graceful-fs` or `fs-extra`.
+   */
+  private DataFlow::SourceNode fsModuleMember(string member) {
+    exists (string moduleName |
+      moduleName = "fs" or
+      moduleName = "graceful-fs" or
+      moduleName = "fs-extra" |
+      result = DataFlow::moduleMember(moduleName, member)
+    )
+  }
 
   /**
-   * A call to a method from module `fs` or `graceful-fs`.
+   * A call to a method from module `fs`, `graceful-fs` or `fs-extra`.
    */
   private class NodeJSFileSystemAccess extends FileSystemAccess, DataFlow::CallNode {
     string methodName;
 
     NodeJSFileSystemAccess() {
-      exists (string moduleName | this = DataFlow::moduleMember(moduleName, methodName).getACall() |
-        moduleName = "fs" or moduleName = "graceful-fs"
-      )
+      this = fsModuleMember(methodName).getACall()
     }
 
     override DataFlow::Node getAPathArgument() {
@@ -356,6 +365,22 @@ module NodeJSLib {
     }
   }
 
+  /**
+   * A data flow node that contains one or more file names from the local file system.
+   */
+  private class NodeJSFileNameSource extends FileNameSource {
+
+    NodeJSFileNameSource() {
+      exists (string name |
+        name = "readdir" or
+        name = "realpath" |
+        this = fsModuleMember(name).getACall().getCallback([1..2]).getParameter(1) or
+        this = fsModuleMember(name + "Sync").getACall()
+      )
+    }
+
+  }
+
   /**
    * A call to a method from module `child_process`.
    */

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

@@ -53,6 +53,13 @@ module ReflectedXss {
     }
   }
 
+  /** A file name, considered as a flow source for reflected XSS. */
+  class FileNameSourceAsSource extends Source {
+    FileNameSourceAsSource() {
+      this instanceof FileNameSource
+    }
+  }
+
   /**
    * An expression that is sent as part of an HTTP response, considered as an XSS sink.
    *

javascript/ql/test/query-tests/Security/CWE-079/ReflectedXss.expected

@@ -5,3 +5,7 @@
 | promises.js:6:25:6:25 | x | Cross-site scripting vulnerability due to $@. | promises.js:5:44:5:57 | req.query.data | user-provided value |
 | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |
 | tst2.js:8:12:8:12 | r | Cross-site scripting vulnerability due to $@. | tst2.js:6:12:6:15 | q: r | user-provided value |
+| xss-through-filenames.js:8:18:8:23 | files1 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:7:43:7:48 | files1 | user-provided value |
+| xss-through-filenames.js:26:19:26:24 | files1 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:25:43:25:48 | files1 | user-provided value |
+| xss-through-filenames.js:33:19:33:24 | files2 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:25:43:25:48 | files1 | user-provided value |
+| xss-through-filenames.js:37:19:37:24 | files3 | Cross-site scripting vulnerability due to $@. | xss-through-filenames.js:25:43:25:48 | files1 | user-provided value |

javascript/ql/test/query-tests/Security/CWE-079/xss-through-filenames.js

@@ -0,0 +1,40 @@
+var http = require('http');
+var fs = require('fs');
+
+var express = require('express');
+
+express().get('/', function(req, res) {
+    fs.readdir("/myDir", function (error, files1) {
+        res.send(files1); // NOT OK
+    });
+});
+
+/**
+ * The essence of a real world vulnerability.
+ */
+http.createServer(function (req, res) {
+
+    function format(files2) {
+        var files3 = [];
+        files2.sort(sort).forEach(function (file) {
+            files3.push('<li>' + file + '</li>');
+        });
+        return files3.join('');
+    }
+
+    fs.readdir("/myDir", function (error, files1) {
+        res.write(files1); // NOT OK
+
+        var dirs = [];
+        var files2 = [];
+        files1.forEach(function (file) {
+            files2.push(file);
+        });
+        res.write(files2); // NOT OK
+
+        var files3 = format(files2);
+
+        res.write(files3);  // NOT OK
+
+    });
+});