JS: make string formatting a taint step

change-notes/1.18/analysis-javascript.md

@@ -12,6 +12,8 @@
 
 * Modelling of taint flow through the array operations `map` and `join` has been improved. This may give additional results for the security queries.
 
+* The taint tracking library recognizes more ways in which taint propagates. In particular, some flow through string formatters is now recognized. This may give additional results for the security queries.
+
 * The taint tracking library now recognizes additional sanitization patterns. This may give fewer false-positive 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:
@@ -37,6 +39,7 @@
   - [extend2](https://github.com/eggjs/extend2)
   - [fast-json-parse](https://github.com/mcollina/fast-json-parse)
   - [forge](https://github.com/digitalbazaar/forge)
+  - [format-util](https://github.com/tmpfs/format-util)
   - [global](https://www.npmjs.com/package/global)
   - [he](https://github.com/mathiasbynens/he)
   - [html-entities](https://github.com/mdevils/node-html-entities)
@@ -58,13 +61,17 @@
   - [object.assign](https://github.com/ljharb/object.assign)
   - [object.defaults](https://github.com/jonschlinkert/object.defaults)
   - [parse-json](https://github.com/sindresorhus/parse-json)
-  - [React Native](https://facebook.github.io/react-native/)
+  - [printf](https://github.com/adaltas/node-printf)
+  - [printj](https://github.com/SheetJS/printj)
   - [q](http://documentup.com/kriskowal/q/)
   - [ramda](https://ramdajs.com)
+  - [React Native](https://facebook.github.io/react-native/)
   - [safe-json-parse](https://github.com/Raynos/safe-json-parse)
   - [sanitize](https://github.com/pocketly/node-sanitize)
   - [sanitizer](https://github.com/theSmaw/Caja-HTML-Sanitizer)
   - [smart-extend](https://github.com/danielkalen/smart-extend)
+  - [sprintf.js](https://github.com/alexei/sprintf.js)
+  - [string-template](https://github.com/Matt-Esch/string-template)
   - [underscore](https://underscorejs.org)
   - [util-extend](https://github.com/isaacs/util-extend)
   - [utils-merge](https://github.com/jaredhanson/utils-merge)

javascript/ql/src/javascript.qll

@@ -66,6 +66,7 @@ import semmle.javascript.frameworks.React
 import semmle.javascript.frameworks.ReactNative
 import semmle.javascript.frameworks.Request
 import semmle.javascript.frameworks.SQL
+import semmle.javascript.frameworks.StringFormatters
 import semmle.javascript.frameworks.UriLibraries
 import semmle.javascript.frameworks.XmlParsers
 import semmle.javascript.frameworks.xUnit

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

@@ -408,6 +408,27 @@ module TaintTracking {
     }
   }
 
+  /**
+   * A taint propagating data flow edge arising from string formatting.
+   */
+  private class StringFormattingTaintStep extends AdditionalTaintStep {
+
+    PrintfStyleCall call;
+
+    StringFormattingTaintStep() {
+      this = call and
+      call.returnsFormatted()
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      succ = this and (
+        pred = call.getFormatString()
+        or
+        pred = call.getFormatArgument(_)
+      )
+    }
+  }
+
   /**
    * A taint propagating data flow edge arising from JSON unparsing.
    */

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

@@ -0,0 +1,108 @@
+/**
+ * Provides classes for modeling string formatting libraries.
+ */
+
+import javascript
+
+/**
+ * A printf-style call that substitutes the embedded format specifiers of a format string for the format arguments.
+ */
+abstract class PrintfStyleCall extends DataFlow::CallNode {
+  /**
+   * Gets the format string.
+   */
+  abstract DataFlow::Node getFormatString();
+
+  /**
+   * Gets the ith argument to the format string.
+   */
+  abstract DataFlow::Node getFormatArgument(int i);
+
+  /**
+   * Holds if this call returns the formatted string.
+   */
+  abstract predicate returnsFormatted();
+}
+
+private class LibraryFormatter extends PrintfStyleCall {
+
+  int formatIndex;
+
+  boolean returns;
+
+  LibraryFormatter() {
+    // built-in Node.js functions
+    exists (string mod, string meth |
+      returns = false and
+      mod = "console" and (
+        (
+          meth = "debug" or
+          meth = "error" or
+          meth = "info" or
+          meth = "log" or
+          meth = "trace" or
+          meth = "warn"
+        ) and
+        formatIndex = 0
+        or
+        meth = "assert" and formatIndex = 1
+      )
+      or
+      returns = true and
+      mod = "util" and (
+        (meth = "format" or meth = "log") and formatIndex = 0
+        or
+        meth = "formatWithOptions" and formatIndex = 1
+      )
+      |
+      // `console` and `util` are available both as modules...
+      this = DataFlow::moduleMember(mod, meth).getACall()
+      or
+      // ...and as globals
+      this = DataFlow::globalVarRef(mod).getAMemberCall(meth)
+    )
+    or
+    returns = true and (
+      // https://www.npmjs.com/package/printf
+      this = DataFlow::moduleImport("printf").getACall() and
+      formatIndex in [0..1]
+      or
+      // https://www.npmjs.com/package/printj
+      exists (string fn | fn = "sprintf" or fn = "vsprintf" |
+        this = DataFlow::moduleMember("printj", fn).getACall() and
+        formatIndex = 0
+      )
+      or
+      // https://www.npmjs.com/package/format-util
+      this = DataFlow::moduleImport("format-util").getACall() and
+      formatIndex = 0
+      or
+      // https://www.npmjs.com/package/string-template
+      this = DataFlow::moduleImport("string-template").getACall() and
+      formatIndex = 0
+      or
+      this = DataFlow::moduleImport("string-template/compile").getACall() and
+      formatIndex = 0
+      or
+      // https://www.npmjs.com/package/sprintf-js
+      exists (string meth | meth = "sprintf" or meth = "vsprintf" |
+        this = DataFlow::moduleMember("sprintf-js", meth).getACall() and
+        formatIndex = 0
+      )
+    )
+  }
+
+  override DataFlow::Node getFormatString() {
+    result = getArgument(formatIndex)
+  }
+
+  override DataFlow::Node getFormatArgument(int i) {
+    i >= 0 and
+    result = getArgument(formatIndex + 1 + i)
+  }
+
+  override predicate returnsFormatted() {
+    returns = true
+  }
+
+}

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

@@ -58,57 +58,10 @@ module TaintedFormatString {
    */
   class FormatSink extends Sink {
     FormatSink() {
-      exists (DataFlow::CallNode call, int argIdx |
-        // built-in Node.js functions
-        exists (string mod, string meth |
-          mod = "console" and
-          (meth = "debug" or meth = "error" or meth = "info" or
-           meth = "log" or meth = "trace" or meth = "warn") and
-          argIdx = 0
-          or
-          mod = "console" and meth = "assert" and argIdx = 1
-          or
-          mod = "util" and (meth = "format" or meth = "log") and argIdx = 0
-          or
-          mod = "util" and meth = "formatWithOptions" and argIdx = 1
-          |
-          // `console` and `util` are available both as modules...
-          call = DataFlow::moduleMember(mod, meth).getACall()
-          or
-          // ...and as globals
-          call = DataFlow::globalVarRef(mod).getAMemberCall(meth)
-        )
-        or
-        // https://www.npmjs.com/package/printf
-        call = DataFlow::moduleImport("printf").getACall() and
-        argIdx in [0..1]
-        or
-        // https://www.npmjs.com/package/printj
-        exists (string fn | fn = "sprintf" or fn = "vsprintf" |
-          call = DataFlow::moduleMember("printj", fn).getACall() and
-          argIdx = 0
-        )
-        or
-        // https://www.npmjs.com/package/format-util
-        call = DataFlow::moduleImport("format-util").getACall() and
-        argIdx = 0
-        or
-        // https://www.npmjs.com/package/string-template
-        call = DataFlow::moduleImport("string-template").getACall() and
-        argIdx = 0
-        or
-        call = DataFlow::moduleImport("string-template/compile").getACall() and
-        argIdx = 0
-        or
-        // https://www.npmjs.com/package/sprintf-js
-        exists (string meth | meth = "sprintf" or meth = "vsprintf" |
-          call = DataFlow::moduleMember("sprintf-js", meth).getACall() and
-          argIdx = 0
-        )
-        |
-        this = call.getArgument(argIdx) and
+      exists(PrintfStyleCall printf |
+        this = printf.getFormatString() and
         // exclude trivial case where there are no arguments to interpolate
-        exists(call.getArgument(argIdx+1))
+        exists(printf.getFormatArgument(_))
       )
     }
   }

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

@@ -1,5 +1,7 @@
 | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
 | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
+| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
+| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | 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 |

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

@@ -6,6 +6,11 @@ edges
 | etherpad.js:9:16:9:47 | req.que ... esponse | etherpad.js:9:16:9:53 | req.que ... e + ")" |
 | etherpad.js:9:16:9:53 | req.que ... e + ")" | etherpad.js:9:5:9:53 | response |
 | etherpad.js:11:3:11:3 | response | etherpad.js:11:12:11:19 | response |
+| formatting.js:4:9:4:29 | evil | formatting.js:6:43:6:46 | evil |
+| formatting.js:4:9:4:29 | evil | formatting.js:7:49:7:52 | evil |
+| formatting.js:4:16:4:29 | req.query.evil | formatting.js:4:9:4:29 | evil |
+| formatting.js:6:43:6:46 | evil | formatting.js:6:14:6:47 | util.fo ... , evil) |
+| formatting.js:7:49:7:52 | evil | formatting.js:7:14:7:53 | require ... , evil) |
 | promises.js:5:3:5:59 | new Pro ... .data)) | promises.js:6:11:6:11 | x |
 | promises.js:5:44:5:57 | req.query.data | promises.js:5:3:5:59 | new Pro ... .data)) |
 | promises.js:5:44:5:57 | req.query.data | promises.js:6:11:6:11 | x |
@@ -18,6 +23,8 @@ edges
 #select
 | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | ReflectedXss.js:8:33:8:45 | req.params.id | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
 | etherpad.js:11:12:11:19 | response | etherpad.js:9:16:9:30 | req.query.jsonp | etherpad.js:11:12:11:19 | response | Cross-site scripting vulnerability due to $@. | etherpad.js:9:16:9:30 | req.query.jsonp | user-provided value |
+| formatting.js:6:14:6:47 | util.fo ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
+| formatting.js:7:14:7:53 | require ... , evil) | formatting.js:4:16:4:29 | req.query.evil | formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | promises.js:6:25:6:25 | x | promises.js:5:44:5:57 | req.query.data | 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 |
 | promises.js:6:25:6:25 | x | promises.js:5:44:5:57 | req.query.data | 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 | tst2.js:6:9:6:9 | p | tst2.js:7:12:7:12 | p | Cross-site scripting vulnerability due to $@. | tst2.js:6:9:6:9 | p | user-provided value |

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

@@ -1,4 +1,6 @@
 | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
+| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
+| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | 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 |

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

@@ -3,6 +3,8 @@ WARNING: Type XssDataFlowConfiguration has been deprecated and may be removed in
 WARNING: Type XssDataFlowConfiguration has been deprecated and may be removed in future (ReflectedXssWithCustomSanitizer_old.ql:20,6-30)
 WARNING: Predicate flowsFrom has been deprecated and may be removed in future (ReflectedXssWithCustomSanitizer_old.ql:21,11-20)
 | ReflectedXss.js:8:14:8:45 | "Unknow ... rams.id | Cross-site scripting vulnerability due to $@. | ReflectedXss.js:8:33:8:45 | req.params.id | user-provided value |
+| formatting.js:6:14:6:47 | util.fo ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
+| formatting.js:7:14:7:53 | require ... , evil) | Cross-site scripting vulnerability due to $@. | formatting.js:4:16:4:29 | req.query.evil | user-provided value |
 | 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 |

javascript/ql/test/query-tests/Security/CWE-079/formatting.js

@@ -0,0 +1,8 @@
+var express = require('express');
+
+express().get('/user/', function(req, res) {
+    var evil = req.query.evil;
+    res.send(console.log("<div>%s</div>", evil)); // OK (returns undefined)
+    res.send(util.format("<div>%s</div>", evil)); // NOT OK
+    res.send(require("printf")("<div>%s</div>", evil)); // NOT OK
+});