Merge pull request #117 from esben-semmle/js/push-sort-taint-steps

JS: support `push` and `sort` taint steps for arrays

change-notes/1.18/analysis-javascript.md

@@ -10,7 +10,7 @@
 
 * Modelling of re-export declarations has been improved. This may result in fewer false-positive results for a variety of queries.
 
-* Modelling of taint flow through the array operations `map` and `join` has been improved. This may give additional results for the security queries.
+* Modelling of taint flow through array operations 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.
 

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

@@ -214,6 +214,9 @@ module TaintTracking {
           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
+        // `array.push(e)`: if `e` is tainted, then so is `array`
+        succ.(DataFlow::SourceNode).getAMethodCall("push").getAnArgument() = pred
       )
       or
       // reading from a tainted object yields a tainted result
@@ -508,6 +511,19 @@ module TaintTracking {
     }
   }
 
+  /**
+   * A taint propagating data flow edge arising from sorting.
+   */
+  private class SortTaintStep extends AdditionalTaintStep, DataFlow::MethodCallNode {
+    SortTaintStep() {
+      getMethodName() = "sort"
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      pred = getReceiver() and succ = this
+    }
+  }
+
   /**
    * A conditional checking a tainted string against a regular expression, which is
    * considered to be a sanitizer for all configurations.

javascript/ql/test/library-tests/TaintTracking/BasicTaintTracking.expected

@@ -1,2 +1,5 @@
 | tst.js:2:13:2:20 | source() | tst.js:4:10:4:10 | x |
 | tst.js:2:13:2:20 | source() | tst.js:5:10:5:22 | "/" + x + "!" |
+| tst.js:2:13:2:20 | source() | tst.js:14:10:14:17 | x.sort() |
+| tst.js:2:13:2:20 | source() | tst.js:17:10:17:10 | a |
+| tst.js:2:13:2:20 | source() | tst.js:19:10:19:10 | a |

javascript/ql/test/library-tests/TaintTracking/tst.js

@@ -10,4 +10,12 @@ function test() {
     sink(x === 1); // OK
     sink(undefined == x); // OK
     sink(x === x); // OK
+
+    sink(x.sort()); // NOT OK
+
+    var a = [];
+    sink(a); // NOT OK (flow-insensitive treatment of `a`)
+    a.push(x);
+    sink(a); // NOT OK
+
 }