Merge pull request #192 from esben-semmle/js/additional-array-taint-steps

Approved by asger-semmle

Author: semmle-qlci

change-notes/1.19/analysis-javascript.md

@@ -2,6 +2,8 @@
 
 ## General improvements
 
+* Modelling of taint flow through array operations has been improved. This may give additional results for the security queries.
+
 ## New queries
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |

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

@@ -185,38 +185,15 @@ module TaintTracking {
 
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
       succ = this and
-      (
-        exists (Expr e, Expr f | e = this.asExpr() and f = pred.asExpr() |
-          // arrays with tainted elements and objects with tainted property names are tainted
-          e.(ArrayExpr).getAnElement() = f or
-          exists (Property prop | e.(ObjectExpr).getAProperty() = prop |
-            prop.isComputed() and f = prop.getNameExpr()
-          )
-          or
-          // awaiting a tainted expression gives a tainted result
-          e.(AwaitExpr).getOperand() = f
+      exists (Expr e, Expr f | e = this.asExpr() and f = pred.asExpr() |
+        // arrays with tainted elements and objects with tainted property names are tainted
+        e.(ArrayExpr).getAnElement() = f or
+        exists (Property prop | e.(ObjectExpr).getAProperty() = prop |
+          prop.isComputed() and f = prop.getNameExpr()
         )
         or
-        // `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are
-        // `elt` and `ary`; similar for `forEach`
-        exists (MethodCallExpr m, Function f, int i, SimpleParameter p |
-          (m.getMethodName() = "map" or m.getMethodName() = "forEach") and
-          (i = 0 or i = 2) and
-          m.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and
-          p = f.getParameter(i) and
-          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
-        // `array.push(e)`: if `e` is tainted, then so is `array`
-        succ.(DataFlow::SourceNode).getAMethodCall("push").getAnArgument() = pred
+        // awaiting a tainted expression gives a tainted result
+        e.(AwaitExpr).getOperand() = f
       )
       or
       // reading from a tainted object yields a tainted result
@@ -233,6 +210,61 @@ module TaintTracking {
     }
   }
 
+  /**
+   * A taint propagating data flow edge caused by the builtin array functions.
+   */
+  private class ArrayFunctionTaintStep extends AdditionalTaintStep {
+    DataFlow::CallNode call;
+
+    ArrayFunctionTaintStep() {
+      this = call
+    }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      // `array.map(function (elt, i, ary) { ... })`: if `array` is tainted, then so are
+      // `elt` and `ary`; similar for `forEach`
+      exists (string name, Function f, int i |
+        (name = "map" or name = "forEach") and
+        (i = 0 or i = 2) and
+        call.getArgument(0).analyze().getAValue().(AbstractFunction).getFunction() = f and
+        pred.(DataFlow::SourceNode).getAMethodCall(name) = call and
+        succ = DataFlow::parameterNode(f.getParameter(i))
+      )
+      or
+      // `array.map` with tainted return value in callback
+      exists (DataFlow::FunctionNode f |
+        call.(DataFlow::MethodCallNode).getMethodName() = "map" and
+        call.getArgument(0) = f and // Require the argument to be a closure to avoid spurious call/return flow
+        pred = f.getAReturn() and
+        succ = call
+      )
+      or
+      // `array.push(e)`, `array.unshift(e)`: if `e` is tainted, then so is `array`.
+      exists (string name |
+        name = "push" or
+        name = "unshift" |
+        pred = call.getAnArgument() and
+        succ.(DataFlow::SourceNode).getAMethodCall(name) = call
+      )
+      or
+      // `e = array.pop()`, `e = array.shift()`, or similar: if `array` is tainted, then so is `e`.
+      exists (string name |
+        name = "pop" or
+        name = "shift" or
+        name = "slice" or
+        name = "splice" |
+        call.(DataFlow::MethodCallNode).calls(pred, name) and
+        succ = call
+      )
+      or
+      // `e = Array.from(x)`: if `x` is tainted, then so is `e`.
+      call = DataFlow::globalVarRef("Array").getAPropertyRead("from").getACall() and
+      pred = call.getAnArgument() and
+      succ = call
+    }
+
+  }
+
   /**
    * A taint propagating data flow edge for assignments of the form `o[k] = v`, where
    * `k` is not a constant and `o` refers to some object literal; in this case, we consider

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

@@ -3,3 +3,15 @@
 | 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 |
+| tst.js:2:13:2:20 | source() | tst.js:23:10:23:10 | b |
+| tst.js:2:13:2:20 | source() | tst.js:25:10:25:16 | x.pop() |
+| tst.js:2:13:2:20 | source() | tst.js:26:10:26:18 | x.shift() |
+| tst.js:2:13:2:20 | source() | tst.js:27:10:27:18 | x.slice() |
+| tst.js:2:13:2:20 | source() | tst.js:28:10:28:19 | x.splice() |
+| tst.js:2:13:2:20 | source() | tst.js:30:10:30:22 | Array.from(x) |
+| tst.js:2:13:2:20 | source() | tst.js:33:14:33:16 | elt |
+| tst.js:2:13:2:20 | source() | tst.js:35:14:35:16 | ary |
+| tst.js:2:13:2:20 | source() | tst.js:39:14:39:16 | elt |
+| tst.js:2:13:2:20 | source() | tst.js:41:14:41:16 | ary |
+| tst.js:2:13:2:20 | source() | tst.js:44:10:44:30 | innocen ... ) => x) |
+| tst.js:2:13:2:20 | source() | tst.js:45:10:45:24 | x.map(x2 => x2) |

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

@@ -18,4 +18,30 @@ function test() {
     a.push(x);
     sink(a); // NOT OK
 
+    var b = [];
+    b.unshift(x);
+    sink(b); // NOT OK
+
+    sink(x.pop()); // NOT OK
+    sink(x.shift()); // NOT OK
+    sink(x.slice()); // NOT OK
+    sink(x.splice()); // NOT OK
+
+    sink(Array.from(x)); // NOT OK
+
+    x.map((elt, i, ary) => {
+        sink(elt); // NOT OK
+        sink(i); // OK
+        sink(ary); // NOT OK
+    });
+
+    x.forEach((elt, i, ary) => {
+        sink(elt); // NOT OK
+        sink(i); // OK
+        sink(ary); // NOT OK
+    });
+
+    sink(innocent.map(() => x)); // NOT OK
+    sink(x.map(x2 => x2)); // NOT OK
+
 }