Merge pull request #1578 from asger-semmle/splice

Approved by xiemaisi

Author: semmle-qlci

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

@@ -841,6 +841,12 @@ module DataFlow {
       /** Gets the data flow node corresponding to an argument of this invocation. */
       abstract DataFlow::Node getAnArgument();
 
+      /**
+       * Gets a data flow node corresponding to an array of values being passed as
+       * individual arguments to this invocation.
+       */
+      abstract DataFlow::Node getASpreadArgument();
+
       /** Gets the number of arguments of this invocation, if it can be determined. */
       abstract int getNumArgument();
     }
@@ -889,6 +895,12 @@ module DataFlow {
         )
       }
 
+      override DataFlow::Node getASpreadArgument() {
+        exists(SpreadElement arg | arg = astNode.getAnArgument() |
+          result = DataFlow::valueNode(arg.getOperand())
+        )
+      }
+
       override int getNumArgument() {
         not astNode.isSpreadArgument(_) and result = astNode.getNumArgument()
       }
@@ -929,7 +941,9 @@ module DataFlow {
 
       ReflectiveCallNodeDef() { this = TReflectiveCallNode(originalCall.asExpr(), kind) }
 
-      override string getCalleeName() { none() }
+      override string getCalleeName() {
+        result = originalCall.getReceiver().asExpr().(PropAccess).getPropertyName()
+      }
 
       override DataFlow::Node getCalleeNode() { result = originalCall.getReceiver() }
 
@@ -943,6 +957,14 @@ module DataFlow {
         kind = "call" and result = originalCall.getAnArgument() and result != getReceiver()
       }
 
+      override DataFlow::Node getASpreadArgument() {
+        kind = "apply" and
+        result = originalCall.getArgument(1)
+        or
+        kind = "call" and
+        result = originalCall.getASpreadArgument()
+      }
+
       override int getNumArgument() {
         result >= 0 and kind = "call" and result = originalCall.getNumArgument() - 1
       }

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

@@ -67,6 +67,19 @@ class InvokeNode extends DataFlow::SourceNode {
   /** Gets the data flow node corresponding to the last argument of this invocation. */
   DataFlow::Node getLastArgument() { result = getArgument(getNumArgument() - 1) }
 
+  /**
+   * Gets a data flow node corresponding to an array of values being passed as
+   * individual arguments to this invocation.
+   *
+   * Examples:
+   * ```
+   * x.push(...args);                     // 'args' is a spread argument
+   * x.push(x, ...args, y, ...more);      // 'args' and 'more' are a spread arguments
+   * Array.prototype.push.apply(x, args); // 'args' is a spread argument
+   * ```
+  .*/
+  DataFlow::Node getASpreadArgument() { result = impl.getASpreadArgument() }
+
   /** Gets the number of arguments of this invocation, if it can be determined. */
   int getNumArgument() { result = impl.getNumArgument() }
 

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

@@ -290,6 +290,25 @@ module TaintTracking {
         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

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

@@ -9,6 +9,11 @@ typeInferenceMismatch
 | addexpr.js:11:15:11:22 | source() | addexpr.js:21:8:21:12 | value |
 | advanced-callgraph.js:2:13:2:20 | source() | advanced-callgraph.js:6:22:6:22 | v |
 | array-callback.js:2:23:2:30 | source() | array-callback.js:4:10:4:10 | x |
+| array-mutation.js:19:18:19:25 | source() | array-mutation.js:20:8:20:8 | e |
+| array-mutation.js:23:13:23:20 | source() | array-mutation.js:24:8:24:8 | f |
+| array-mutation.js:27:16:27:23 | source() | array-mutation.js:28:8:28:8 | g |
+| array-mutation.js:31:33:31:40 | source() | array-mutation.js:32:8:32:8 | h |
+| array-mutation.js:35:36:35:43 | source() | array-mutation.js:36:8:36:8 | i |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:4:8:4:8 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:13:10:13:10 | x |
 | booleanOps.js:2:11:2:18 | source() | booleanOps.js:19:10:19:10 | x |

javascript/ql/test/library-tests/TaintTracking/array-mutation.js

@@ -0,0 +1,37 @@
+function test(x, y) {
+  let a = [];
+  a.splice(source(), x);
+  sink(a); // OK
+
+  let b = [];
+  b.splice(x, source());
+  sink(b); // OK
+
+  let c = [];
+  c.splice(source(), x, y);
+  sink(c); // OK
+
+  let d = [];
+  d.splice(x, source(), y);
+  sink(d); // OK
+
+  let e = [];
+  e.splice(x, y, source());
+  sink(e); // NOT OK
+
+  let f = [];
+  f.push(...source());
+  sink(f); // NOT OK
+
+  let g = [];
+  g.unshift(...source());
+  sink(g); // NOT OK
+
+  let h = [];
+  Array.prototype.push.apply(h, source());
+  sink(h); // NOT OK
+
+  let i = [];
+  Array.prototype.unshift.apply(i, source());
+  sink(i); // NOT OK
+}