Merge pull request #3019 from erik-krogh/ArrayStep

Approved by asgerf

javascript/ql/src/javascript.qll

@@ -5,6 +5,7 @@
 import Customizations
 import semmle.javascript.Aliases
 import semmle.javascript.AMD
+import semmle.javascript.Arrays
 import semmle.javascript.AST
 import semmle.javascript.BasicBlocks
 import semmle.javascript.Base64

javascript/ql/src/semmle/javascript/Arrays.qll

@@ -0,0 +1,264 @@
+import javascript
+private import semmle.javascript.dataflow.InferredTypes
+
+/**
+ * Classes and predicates for modelling TaintTracking steps for arrays.
+ */
+module ArrayTaintTracking {
+  /**
+   * A taint propagating data flow edge caused by the builtin array functions.
+   */
+  private class ArrayFunctionTaintStep extends TaintTracking::AdditionalTaintStep {
+    DataFlow::CallNode call;
+
+    ArrayFunctionTaintStep() { this = call }
+
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      arrayFunctionTaintStep(pred, succ, call)
+    }
+  }
+
+  /**
+   * A taint propagating data flow edge from `pred` to `succ` caused by a call `call` to a builtin array functions.
+   */
+  predicate arrayFunctionTaintStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::CallNode call) {
+    // `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
+      call.(DataFlow::MethodCallNode).getMethodName() = name and
+      pred = call.getReceiver() 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
+    // `array.push(...e)`, `array.unshift(...e)`: if `e` is tainted, then so is `array`.
+    exists(string name |
+      name = "push" or
+      name = "unshift"
+    |
+      pred = call.getASpreadArgument() and
+      // Make sure we handle reflective calls
+      succ = call.getReceiver().getALocalSource() and
+      call.getCalleeName() = name
+    )
+    or
+    // `array.splice(i, del, e)`: if `e` is tainted, then so is `array`.
+    exists(string name | name = "splice" |
+      pred = call.getArgument(2) 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
+    or
+    // `e = arr1.concat(arr2, arr3)`: if any of the `arr` is tainted, then so is `e`.
+    call.(DataFlow::MethodCallNode).calls(pred, "concat") and
+    succ = call
+    or
+    call.(DataFlow::MethodCallNode).getMethodName() = "concat" and
+    succ = call and
+    pred = call.getAnArgument()
+  }
+}
+
+/**
+ * Classes and predicates for modelling data-flow for arrays.
+ */
+private module ArrayDataFlow {
+  /**
+   * Gets a pseudo-field representing an element inside an array.
+   */
+  private string arrayElement() { result = "$arrayElement$" }
+
+  /**
+   * A step for storing an element on an array using `arr.push(e)` or `arr.unshift(e)`.
+   */
+  private class ArrayAppendStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+    ArrayAppendStep() {
+      this.getMethodName() = "push" or
+      this.getMethodName() = "unshift"
+    }
+
+    override predicate storeStep(DataFlow::Node element, DataFlow::Node obj, string prop) {
+      prop = arrayElement() and
+      (element = this.getAnArgument() or element = this.getASpreadArgument()) and
+      obj = this.getReceiver().getALocalSource()
+    }
+  }
+
+  /**
+   * A step for reading/writing an element from an array inside a for-loop.
+   * E.g. a read from `foo[i]` to `bar` in `for(var i = 0; i < arr.length; i++) {bar = foo[i]}`.
+   */
+  private class ArrayIndexingStep extends DataFlow::AdditionalFlowStep, DataFlow::Node {
+    DataFlow::PropRef read;
+
+    ArrayIndexingStep() {
+      read = this and
+      forex(InferredType type | type = read.getPropertyNameExpr().flow().analyze().getAType() |
+        type = TTNumber()
+      ) and
+      exists(VarAccess i, ExprOrVarDecl init |
+        i = read.getPropertyNameExpr() and init = any(ForStmt f).getInit()
+      |
+        i.getVariable().getADefinition() = init or
+        i.getVariable().getADefinition().(VariableDeclarator).getDeclStmt() = init
+      )
+    }
+
+    override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
+      prop = arrayElement() and
+      obj = this.(DataFlow::PropRead).getBase() and
+      element = this
+    }
+
+    override predicate storeStep(DataFlow::Node element, DataFlow::Node obj, string prop) {
+      prop = arrayElement() and
+      element = this.(DataFlow::PropWrite).getRhs() and
+      this = obj.(DataFlow::SourceNode).getAPropertyWrite()
+    }
+  }
+
+  /**
+   * A step for retrieving an element from an array using `.pop()` or `.shift()`.
+   * E.g. `array.pop()`.
+   */
+  private class ArrayPopStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+    ArrayPopStep() {
+      getMethodName() = "pop" or
+      getMethodName() = "shift"
+    }
+
+    override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
+      prop = arrayElement() and
+      obj = this.getReceiver() and
+      element = this
+    }
+  }
+
+  /**
+   * A step for iterating an array using `map` or `forEach`.
+   *
+   * Array elements can be loaded from the array `arr` to `e` in e.g: `arr.forEach(e => ...)`.
+   *
+   * And array elements can be stored into a resulting array using `map(...)`.
+   * E.g. in `arr.map(e => foo)`, the resulting array (`arr.map(e => foo)`) will contain the element `foo`.
+   *
+   * And the second parameter in the callback is the array ifself, so there is a `loadStoreStep` from the array to that second parameter.
+   */
+  private class ArrayIteration extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+    ArrayIteration() {
+      this.getMethodName() = "map" or
+      this.getMethodName() = "forEach"
+    }
+
+    override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
+      prop = arrayElement() and
+      obj = this.getReceiver() and
+      element = getCallback(0).getParameter(0)
+    }
+
+    override predicate storeStep(DataFlow::Node element, DataFlow::Node obj, string prop) {
+      this.getMethodName() = "map" and
+      prop = arrayElement() and
+      element = this.getCallback(0).getAReturn() and
+      obj = this
+    }
+
+    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = arrayElement() and
+      pred = this.getReceiver() and
+      succ = getCallback(0).getParameter(2)
+    }
+  }
+
+  /**
+   * A step for creating an array and storing the elements in the array.
+   */
+  private class ArrayCreationStep extends DataFlow::AdditionalFlowStep, DataFlow::Node {
+    ArrayCreationStep() { this instanceof DataFlow::ArrayCreationNode }
+
+    override predicate storeStep(DataFlow::Node element, DataFlow::Node obj, string prop) {
+      prop = arrayElement() and
+      element = this.(DataFlow::ArrayCreationNode).getAnElement() and
+      obj = this
+    }
+  }
+
+  /**
+   * A step modelling that `splice` can insert elements into an array.
+   * For example in `array.splice(i, del, e)`: if `e` is tainted, then so is `array
+   */
+  private class ArraySpliceStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+    ArraySpliceStep() { this.getMethodName() = "splice" }
+
+    override predicate storeStep(DataFlow::Node element, DataFlow::Node obj, string prop) {
+      prop = arrayElement() and
+      element = getArgument(2) and
+      obj = this.getReceiver().getALocalSource()
+    }
+  }
+
+  /**
+   * A step for modelling `concat`.
+   * For example in `e = arr1.concat(arr2, arr3)`: if any of the `arr` is tainted, then so is `e`.
+   */
+  private class ArrayConcatStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+    ArrayConcatStep() { this.getMethodName() = "concat" }
+
+    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = arrayElement() and
+      (pred = this.getReceiver() or pred = this.getAnArgument()) and
+      succ = this
+    }
+  }
+
+  /**
+   * A step for modelling that elements from an array `arr` also appear in the result from calling `slice`/`splice`/`filter`.
+   */
+  private class ArraySliceStep extends DataFlow::AdditionalFlowStep, DataFlow::MethodCallNode {
+    ArraySliceStep() {
+      this.getMethodName() = "slice" or
+      this.getMethodName() = "splice" or
+      this.getMethodName() = "filter"
+    }
+
+    override predicate loadStoreStep(DataFlow::Node pred, DataFlow::Node succ, string prop) {
+      prop = arrayElement() and
+      pred = this.getReceiver() and
+      succ = this
+    }
+  }
+}

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

@@ -587,8 +587,8 @@ class ArrayConstructorInvokeNode extends DataFlow::InvokeNode {
 }
 
 /**
- * A data flow node corresponding to the creation or a new array, either through an array literal
- * or an invocation of the `Array` constructor.
+ * A data flow node corresponding to the creation or a new array, either through an array literal,
+ * an invocation of the `Array` constructor, or the `Array.from` method.
  *
  *
  * Examples:

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

@@ -282,92 +282,7 @@ 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) {
-      arrayFunctionTaintStep(pred, succ, call)
-    }
-  }
-
-  /**
-   * A taint propagating data flow edge from `pred` to `succ` caused by a call `call` to a builtin array functions.
-   */
-  predicate arrayFunctionTaintStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::CallNode call) {
-    // `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
-      call.(DataFlow::MethodCallNode).getMethodName() = name and
-      pred = call.getReceiver() 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
-    // `array.push(...e)`, `array.unshift(...e)`: if `e` is tainted, then so is `array`.
-    exists(string name |
-      name = "push" or
-      name = "unshift"
-    |
-      pred = call.getASpreadArgument() and
-      // Make sure we handle reflective calls
-      succ = call.getReceiver().getALocalSource() and
-      call.getCalleeName() = name
-    )
-    or
-    // `array.splice(i, del, e)`: if `e` is tainted, then so is `array`.
-    exists(string name | name = "splice" |
-      pred = call.getArgument(2) 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
-    or
-    // `e = arr1.concat(arr2, arr3)`: if any of the `arr` is tainted, then so is `e`.
-    call.(DataFlow::MethodCallNode).calls(pred, "concat") and
-    succ = call
-    or
-    call.(DataFlow::MethodCallNode).getMethodName() = "concat" and
-    succ = call and
-    pred = call.getAnArgument()
-  }
+  predicate arrayFunctionTaintStep = ArrayTaintTracking::arrayFunctionTaintStep/3;
 
   /**
    * A taint propagating data flow edge for assignments of the form `o[k] = v`, where

javascript/ql/test/library-tests/Arrays/DataFlow.expected

@@ -0,0 +1,13 @@
+| arrays.js:2:16:2:23 | "source" | arrays.js:5:8:5:14 | obj.foo |
+| arrays.js:2:16:2:23 | "source" | arrays.js:11:10:11:15 | arr[i] |
+| arrays.js:2:16:2:23 | "source" | arrays.js:15:27:15:27 | e |
+| arrays.js:2:16:2:23 | "source" | arrays.js:16:23:16:23 | e |
+| arrays.js:2:16:2:23 | "source" | arrays.js:20:8:20:16 | arr.pop() |
+| arrays.js:18:22:18:29 | "source" | arrays.js:18:50:18:50 | e |
+| arrays.js:22:15:22:22 | "source" | arrays.js:23:8:23:17 | arr2.pop() |
+| arrays.js:25:15:25:22 | "source" | arrays.js:26:8:26:17 | arr3.pop() |
+| arrays.js:29:21:29:28 | "source" | arrays.js:30:8:30:17 | arr4.pop() |
+| arrays.js:29:21:29:28 | "source" | arrays.js:33:8:33:17 | arr5.pop() |
+| arrays.js:29:21:29:28 | "source" | arrays.js:35:8:35:26 | arr5.slice(2).pop() |
+| arrays.js:29:21:29:28 | "source" | arrays.js:41:8:41:17 | arr6.pop() |
+| arrays.js:44:4:44:11 | "source" | arrays.js:45:10:45:18 | ary.pop() |

javascript/ql/test/library-tests/Arrays/DataFlow.ql

@@ -0,0 +1,15 @@
+import javascript
+
+class ArrayFlowConfig extends DataFlow::Configuration {
+  ArrayFlowConfig() { this = "ArrayFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source.asExpr().getStringValue() = "source" }
+
+  override predicate isSink(DataFlow::Node sink) {
+    sink = any(DataFlow::CallNode call | call.getCalleeName() = "sink").getAnArgument()
+  }
+}
+
+from ArrayFlowConfig config, DataFlow::Node src, DataFlow::Node snk
+where config.hasFlow(src, snk)
+select src, snk

javascript/ql/test/library-tests/Arrays/arrays.js

@@ -0,0 +1,50 @@
+(function () {
+  let source = "source";
+
+  var obj = { foo: source };
+  sink(obj.foo); // NOT OK
+
+  var arr = [];
+  arr.push(source);
+
+  for (var i = 0; i < arr.length; i++) {
+    sink(arr[i]); // NOT OK
+  }
+
+
+  arr.forEach((e) => sink(e)); // NOT OK
+  arr.map((e) => sink(e)); // NOT OK
+
+  [1, 2, 3].map(i => "source").forEach(e => sink(e)); // NOT OK.
+
+  sink(arr.pop()); // NOT OK
+
+  var arr2 = ["source"];
+  sink(arr2.pop()); // NOT OK
+
+  var arr3 = ["source"];
+  sink(arr3.pop()); // NOT OK
+
+  var arr4 = [];
+  arr4.splice(0, 0, "source");
+  sink(arr4.pop()); // NOT OK
+
+  var arr5 = [].concat(arr4);
+  sink(arr5.pop()); // NOT OK
+
+  sink(arr5.slice(2).pop()); // NOT OK
+
+  var arr6 = [];
+  for (var i = 0; i < arr5.length; i++) {
+    arr6[i] = arr5[i];
+  }
+  sink(arr6.pop()); // NOT OK
+
+
+  ["source"].forEach((e, i, ary) => {
+    sink(ary.pop()); // NOT OK
+    sink(ary); // OK - its the array itself, not an element.
+  });
+
+  sink(arr[0]); // OK - tuple like usage. 
+});

javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected

@@ -2,6 +2,7 @@
 | a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
 | callback.js:16:14:16:21 | "source" | callback.js:13:14:13:14 | x |
+| callback.js:17:15:17:23 | "source2" | callback.js:13:14:13:14 | x |
 | callback.js:27:15:27:23 | "source3" | callback.js:13:14:13:14 | x |
 | destructuring.js:2:16:2:24 | "tainted" | destructuring.js:9:15:9:22 | tainted2 |
 | destructuring.js:19:15:19:23 | "tainted" | destructuring.js:14:15:14:15 | p |
@@ -44,6 +45,7 @@
 | tst2.js:6:24:6:37 | "also tainted" | tst2.js:11:15:11:24 | g(source2) |
 | tst6.mjs:12:14:12:21 | "source" | tst6.mjs:14:12:14:16 | a.m() |
 | tst6.mjs:16:15:16:23 | "source2" | tst6.mjs:18:13:18:24 | a.m.call(a2) |
+| tst.js:2:17:2:22 | "src1" | tst.js:28:20:28:22 | elt |
 | tst.js:2:17:2:22 | "src1" | tst.js:39:17:39:17 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:41:19:41:19 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:45:17:45:17 | x |

javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected

@@ -2,6 +2,7 @@
 | a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
 | callback.js:16:14:16:21 | "source" | callback.js:13:14:13:14 | x |
+| callback.js:17:15:17:23 | "source2" | callback.js:13:14:13:14 | x |
 | callback.js:27:15:27:23 | "source3" | callback.js:13:14:13:14 | x |
 | custom.js:1:14:1:26 | "verschmutzt" | custom.js:2:15:2:20 | quelle |
 | destructuring.js:2:16:2:24 | "tainted" | destructuring.js:9:15:9:22 | tainted2 |
@@ -45,6 +46,7 @@
 | tst2.js:6:24:6:37 | "also tainted" | tst2.js:11:15:11:24 | g(source2) |
 | tst6.mjs:12:14:12:21 | "source" | tst6.mjs:14:12:14:16 | a.m() |
 | tst6.mjs:16:15:16:23 | "source2" | tst6.mjs:18:13:18:24 | a.m.call(a2) |
+| tst.js:2:17:2:22 | "src1" | tst.js:28:20:28:22 | elt |
 | tst.js:2:17:2:22 | "src1" | tst.js:39:17:39:17 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:41:19:41:19 | x |
 | tst.js:2:17:2:22 | "src1" | tst.js:45:17:45:17 | x |

javascript/ql/test/library-tests/InterProceduralFlow/TrackedNodes.expected

@@ -1,3 +1,6 @@
+| missing | callback.js:17:15:17:23 | "source2" | callback.js:8:16:8:20 | xs[i] |
+| missing | callback.js:17:15:17:23 | "source2" | callback.js:12:16:12:16 | x |
+| missing | callback.js:17:15:17:23 | "source2" | callback.js:13:14:13:14 | x |
 | missing | promises.js:1:2:1:2 | source | promises.js:6:26:6:28 | val |
 | missing | promises.js:1:2:1:2 | source | promises.js:7:16:7:18 | val |
 | missing | promises.js:1:2:1:2 | source | promises.js:37:11:37:11 | v |
@@ -30,3 +33,5 @@
 | missing | promises.js:16:7:16:21 | exceptional return of rej(rej_source) | promises.js:24:20:24:20 | v |
 | missing | promises.js:32:24:32:37 | "also tainted" | promises.js:37:11:37:11 | v |
 | missing | promises.js:32:24:32:37 | "also tainted" | promises.js:38:32:38:32 | v |
+| missing | tst.js:2:17:2:22 | "src1" | tst.js:27:22:27:24 | elt |
+| missing | tst.js:2:17:2:22 | "src1" | tst.js:28:20:28:22 | elt |

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

@@ -521,6 +521,7 @@ edges
 | tst.js:73:3:73:19 | document.location | tst.js:73:3:73:26 | documen ... .search |
 | tst.js:73:3:73:19 | document.location | tst.js:73:3:73:26 | documen ... .search |
 | tst.js:73:3:73:26 | documen ... .search | tst.js:73:1:73:27 | [,docum ... search] |
+| tst.js:73:3:73:26 | documen ... .search | tst.js:73:46:73:46 | x |
 | tst.js:73:46:73:46 | x | tst.js:76:20:76:20 | x |
 | tst.js:73:46:73:46 | x | tst.js:76:20:76:20 | x |
 | tst.js:80:49:80:65 | document.location | tst.js:80:49:80:72 | documen ... .search |

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

@@ -525,6 +525,7 @@ edges
 | tst.js:73:3:73:19 | document.location | tst.js:73:3:73:26 | documen ... .search |
 | tst.js:73:3:73:19 | document.location | tst.js:73:3:73:26 | documen ... .search |
 | tst.js:73:3:73:26 | documen ... .search | tst.js:73:1:73:27 | [,docum ... search] |
+| tst.js:73:3:73:26 | documen ... .search | tst.js:73:46:73:46 | x |
 | tst.js:73:46:73:46 | x | tst.js:76:20:76:20 | x |
 | tst.js:73:46:73:46 | x | tst.js:76:20:76:20 | x |
 | tst.js:80:49:80:65 | document.location | tst.js:80:49:80:72 | documen ... .search |