Merge pull request #49 from asger-semmle/array-map-taint

JavaScript: add taint steps through Array 'join' and 'map' methods

Author: Max Schaefer

change-notes/1.18/analysis-javascript.md

@@ -6,6 +6,8 @@
 
 * Modelling of data flow through destructuring assignments has been improved. This may give additional results for the security queries and other queries that rely on data flow.
 
+* Modelling of taint flow through the array operations `map` and `join` has been improved. This may give additional results for the security queries.
+
 * Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following libraries:
   - [bluebird](http://bluebirdjs.com)
   - [browserid-crypto](https://github.com/mozilla/browserid-crypto)

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -207,6 +207,13 @@ module TaintTracking {
           this = DataFlow::parameterNode(p) and
           pred.asExpr() = m.getReceiver()
         )
+        or
+        // `array.map` with tainted return value in callback
+        exists (MethodCallExpr m, Function f |
+          this.asExpr() = m and
+          m.getMethodName() = "map" and
+          m.getArgument(0) = f and // Require the argument to be a closure to avoid spurious call/return flow
+          pred = f.getAReturnedExpr().flow())
       )
       or
       // reading from a tainted object yields a tainted result
@@ -365,7 +372,9 @@ module TaintTracking {
             name = "trimRight" or
             // sorted, interesting, properties of Object.prototype
             name = "toString" or
-            name = "valueOf"
+            name = "valueOf" or
+            // sorted, interesting, properties of Array.prototype
+            name = "join"
           ) or
           exists (int i | pred.asExpr() = astNode.(MethodCallExpr).getArgument(i) |
             name = "concat" or

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

@@ -21,6 +21,8 @@
 | TaintedPath.js:85:31:85:74 | require ... ).query | This path depends on $@. | TaintedPath.js:85:61:85:67 | req.url | a user-provided value |
 | TaintedPath.js:86:31:86:73 | require ... ).query | This path depends on $@. | TaintedPath.js:86:60:86:66 | req.url | a user-provided value |
 | TaintedPath.js:94:48:94:60 | req.params[0] | This path depends on $@. | TaintedPath.js:94:48:94:60 | req.params[0] | a user-provided value |
+| tainted-array-steps.js:11:29:11:54 | ['publi ... in('/') | This path depends on $@. | tainted-array-steps.js:9:24:9:30 | req.url | a user-provided value |
+| tainted-array-steps.js:15:29:15:43 | parts.join('/') | This path depends on $@. | tainted-array-steps.js:9:24:9:30 | req.url | a user-provided value |
 | tainted-require.js:7:19:7:37 | req.param("module") | This path depends on $@. | tainted-require.js:7:19:7:37 | req.param("module") | a user-provided value |
 | tainted-sendFile.js:7:16:7:33 | req.param("gimme") | This path depends on $@. | tainted-sendFile.js:7:16:7:33 | req.param("gimme") | a user-provided value |
 | views.js:1:43:1:55 | req.params[0] | This path depends on $@. | views.js:1:43:1:55 | req.params[0] | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-022/tainted-array-steps.js

@@ -0,0 +1,18 @@
+var fs = require('fs'),
+    http = require('http'),
+    url = require('url'),
+    sanitize = require('sanitize-filename'),
+    pathModule = require('path')
+    ;
+
+var server = http.createServer(function(req, res) {
+  let path = url.parse(req.url, true).query.path;
+  // BAD: taint is preserved
+  res.write(fs.readFileSync(['public', path].join('/')));
+  // BAD: taint is preserved
+  let parts = ['public', path];
+  parts = parts.map(x => x.toLowerCase());
+  res.write(fs.readFileSync(parts.join('/')));
+});
+
+server.listen();