JS: mark PrintfStyleCall as a taint step

Author: Esben Sparre Andreasen

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

@@ -17,15 +17,23 @@ abstract class PrintfStyleCall extends DataFlow::CallNode {
    * 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
@@ -40,6 +48,7 @@ private class LibraryFormatter extends PrintfStyleCall {
         meth = "assert" and formatIndex = 1
       )
       or
+      returns = true and
       mod = "util" and (
         (meth = "format" or meth = "log") and formatIndex = 0
         or
@@ -53,7 +62,7 @@ private class LibraryFormatter extends PrintfStyleCall {
       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]
@@ -91,4 +100,9 @@ private class LibraryFormatter extends PrintfStyleCall {
     i >= 0 and
     result = getArgument(formatIndex + 1 + i)
   }
+
+  override predicate returnsFormatted() {
+    returns = true
+  }
+
 }

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