JS: Fix issue for .apply() calls

Author: asgerf

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll

@@ -55,6 +55,9 @@ private module Cached {
       invoke.isSpreadArgument(_) and
       storeContent = dynamicArgumentsContent()
     } or
+    TApplyCallTaintNode(MethodCallExpr node) {
+      node.getMethodName() = "apply" and exists(node.getArgument(1))
+    } or
     TDestructuredModuleImportNode(ImportDeclaration decl) {
       exists(decl.getASpecifier().getImportedName())
     } or

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

@@ -197,6 +197,28 @@ class DynamicParameterArrayNode extends DataFlow::Node, TDynamicParameterArrayNo
   override Location getLocation() { result = function.getLocation() }
 }
 
+/**
+ * Node with taint input from the second argument of `.apply()` and with a store edge back into that same argument.
+ *
+ * This ensures that if `.apply()` is called with a tainted value (not inside a content) the taint is
+ * boxed in an `ArrayElement` content. This is necessary for the target function to propagate the taint.
+ */
+class ApplyCallTaintNode extends DataFlow::Node, TApplyCallTaintNode {
+  private MethodCallExpr apply;
+
+  ApplyCallTaintNode() { this = TApplyCallTaintNode(apply) }
+
+  override StmtContainer getContainer() { result = apply.getContainer() }
+
+  override string toString() { result = "[apply call taint node]" }
+
+  override Location getLocation() { result = apply.getArgument(1).getLocation() }
+
+  MethodCallExpr getMethodCallExpr() { result = apply }
+
+  DataFlow::Node getArrayNode() { result = apply.getArgument(1).flow() }
+}
+
 cached
 newtype TReturnKind =
   MkNormalReturnKind() or
@@ -1233,6 +1255,12 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
     c.asArrayIndex() = n and
     not n >= firstSpreadArgumentIndex(invoke)
   )
+  or
+  exists(ApplyCallTaintNode taintNode |
+    node1 = taintNode and
+    node2 = taintNode.getArrayNode() and
+    c = ContentSet::arrayElementUnknown()
+  )
 }
 
 /**

javascript/ql/lib/semmle/javascript/dataflow/internal/TaintTrackingPrivate.qll

@@ -25,7 +25,12 @@ predicate defaultAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2)
     node1 = TValueNode(invoke.getAnArgument().stripParens().(SpreadElement).getOperand()) and
     node2 = TDynamicArgumentStoreNode(invoke, c) and
     c.isUnknownArrayElement()
-    // TODO: we need a similar case for .apply() calls
+  )
+  or
+  // If the array in an .apply() call is tainted (not inside a content), box it in an array element (similar to the case above).
+  exists(ApplyCallTaintNode taintNode |
+    node1 = taintNode.getArrayNode() and
+    node2 = taintNode
   )
 }
 

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

@@ -1,6 +1,4 @@
 legacyDataFlowDifference
-| array-mutation.js:31:33:31:40 | source() | array-mutation.js:32:8:32:8 | h | only flow with OLD data flow library |
-| array-mutation.js:35:36:35:43 | source() | array-mutation.js:36:8:36:8 | i | only flow with OLD data flow library |
 | arrays-init.js:2:16:2:23 | source() | arrays-init.js:27:8:27:13 | arr[0] | only flow with OLD data flow library |
 | arrays-init.js:2:16:2:23 | source() | arrays-init.js:33:8:33:13 | arr[0] | only flow with OLD data flow library |
 | arrays-init.js:2:16:2:23 | source() | arrays-init.js:35:8:35:13 | arr[2] | only flow with OLD data flow library |
@@ -47,6 +45,8 @@ flow
 | 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 |
 | array-mutation.js:39:17:39:24 | source() | array-mutation.js:40:8:40:8 | j |
 | arrays-init.js:2:16:2:23 | source() | arrays-init.js:17:8:17:13 | arr[1] |
 | arrays-init.js:2:16:2:23 | source() | arrays-init.js:22:8:22:13 | arr[6] |

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

@@ -29,11 +29,11 @@ function test(x, y) {
 
   let h = [];
   Array.prototype.push.apply(h, source());
-  sink(h); // NOT OK [INCONSISTENCY]
+  sink(h); // NOT OK
 
   let i = [];
   Array.prototype.unshift.apply(i, source());
-  sink(i); // NOT OK [INCONSISTENCY]
+  sink(i); // NOT OK
 
   let j = [];
   j[j.length] = source();