JS: Add store/load steps for the new argument arrays

2026-04-25 08:45:14 +02:00 · 2024-08-09 11:17:35 +02:00
parent 623dbda77d
commit fa7ad03068
5 changed files with 158 additions and 72 deletions
--- a/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/DataFlow.qll
@@ -1780,8 +1780,7 @@ module DataFlow {
    )
  }

-  /** A load step from a reflective parameter node to each parameter. */
-  private class ReflectiveParamsStep extends PreCallGraphStep {
+  private class ReflectiveParamsStep extends LegacyPreCallGraphStep {
    override predicate loadStep(DataFlow::Node obj, DataFlow::Node element, string prop) {
      exists(DataFlow::ReflectiveParametersNode params, DataFlow::FunctionNode f, int i |
        f.getFunction() = params.getFunction() and
@@ -1793,7 +1792,7 @@ module DataFlow {
  }

  /** A taint step from the reflective parameters node to any parameter. */
-  private class ReflectiveParamsTaintStep extends TaintTracking::SharedTaintStep {
+  private class ReflectiveParamsTaintStep extends TaintTracking::LegacyTaintStep {
    override predicate step(DataFlow::Node obj, DataFlow::Node element) {
      exists(DataFlow::ReflectiveParametersNode params, DataFlow::FunctionNode f |
        f.getFunction() = params.getFunction() and
--- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowNode.qll
@@ -15,6 +15,12 @@ private import semmle.javascript.dataflow.internal.VariableCapture as VariableCa

 cached
 private module Cached {
+  private Content dynamicArgumentsContent() {
+    result.asArrayIndex() = [0 .. 10]
+    or
+    result.isUnknownArrayElement()
+  }
+
  /**
   * The raw data type underlying `DataFlow::Node`.
   */
@@ -39,6 +45,16 @@ private module Cached {
      f.getAParameter().isRestParameter() or f.usesArgumentsObject()
    } or
    TDynamicParameterArrayNode(Function f) or
+    /** Data about to be stored in the rest parameter object. Needed for shifting array indices. */
+    TRestParameterStoreNode(Function f, Content storeContent) {
+      f.getRestParameter().getIndex() > 0 and
+      storeContent = dynamicArgumentsContent()
+    } or
+    /** Data about to be stored in the dynamic argument array of an invocation. Needed for shifting array indices. */
+    TDynamicArgumentStoreNode(InvokeExpr invoke, Content storeContent) {
+      invoke.isSpreadArgument(_) and
+      storeContent = dynamicArgumentsContent()
+    } or
    TDestructuredModuleImportNode(ImportDeclaration decl) {
      exists(decl.getASpecifier().getImportedName())
    } or
@@ -49,7 +65,7 @@ private module Cached {
    TExceptionalInvocationReturnNode(InvokeExpr e) or
    TGlobalAccessPathRoot() or
    TTemplatePlaceholderTag(Templating::TemplatePlaceholderTag tag) or
-    TReflectiveParametersNode(Function f) or
+    TReflectiveParametersNode(Function f) { f.usesArgumentsObject() } or
    TExprPostUpdateNode(AST::ValueNode e) {
      e = any(InvokeExpr invoke).getAnArgument() or
      e = any(PropAccess access).getBase() or
--- a/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll
@@ -113,6 +113,42 @@ class DynamicArgumentArrayNode extends DataFlow::Node, TDynamicArgumentArrayNode
  override Location getLocation() { result = invoke.getLocation() }
 }

+/**
+ * Intermediate node with data that will be stored in `DyanmicArgumentArrayNode`.
+ */
+class DynamicArgumentStoreNode extends DataFlow::Node, TDynamicArgumentStoreNode {
+  private InvokeExpr invoke;
+  private Content content;
+
+  DynamicArgumentStoreNode() { this = TDynamicArgumentStoreNode(invoke, content) }
+
+  override StmtContainer getContainer() { result = invoke.getContainer() }
+
+  override string toString() { result = "[dynamic argument store node] content=" + content }
+
+  override Location getLocation() { result = invoke.getLocation() }
+}
+
+/**
+ * Intermediate node with data that will be stored in the function's rest parameter node.
+ */
+class RestParameterStoreNode extends DataFlow::Node, TRestParameterStoreNode {
+  private Function function;
+  private Content content;
+
+  RestParameterStoreNode() { this = TRestParameterStoreNode(function, content) }
+
+  override StmtContainer getContainer() { result = function }
+
+  override string toString() {
+    result =
+      "[rest parameter store node] '..." + function.getRestParameter().getName() + "' content=" +
+        content
+  }
+
+  override Location getLocation() { result = function.getRestParameter().getLocation() }
+}
+
 /**
 * A parameter containing an array of all positional arguments with an obvious index, i.e. not affected by spread or `.apply()`.
 *
@@ -297,8 +333,6 @@ private predicate isParameterNodeImpl(Node p, DataFlowCallable c, ParameterPosit
  or
  pos.isFunctionSelfReference() and p = TFunctionSelfReferenceNode(c.asSourceCallable())
  or
-  pos.isArgumentsArray() and p = TReflectiveParametersNode(c.asSourceCallable()) // TODO: remove
-  or
  pos.isStaticArgumentArray() and p = TStaticParameterArrayNode(c.asSourceCallable())
  or
  pos.isDynamicArgumentArray() and p = TDynamicParameterArrayNode(c.asSourceCallable())
@@ -317,6 +351,12 @@ predicate isParameterNode(ParameterNode p, DataFlowCallable c, ParameterPosition
 private predicate isArgumentNodeImpl(Node n, DataFlowCall call, ArgumentPosition pos) {
  n = call.asOrdinaryCall().getArgument(pos.asPositional())
  or
+  exists(InvokeExpr invoke |
+    call.asOrdinaryCall() = TReflectiveCallNode(invoke, "apply") and
+    pos.isDynamicArgumentArray() and
+    n = TValueNode(invoke.getArgument(1))
+  )
+  or
  pos.isThis() and n = call.asOrdinaryCall().(DataFlow::CallNode).getReceiver()
  or
  exists(DataFlow::PartialInvokeNode invoke, DataFlow::Node callback |
@@ -343,10 +383,6 @@ private predicate isArgumentNodeImpl(Node n, DataFlowCall call, ArgumentPosition
  or
  pos.isThis() and n = TConstructorThisArgumentNode(call.asOrdinaryCall().asExpr())
  or
-  // For now, treat all spread argument as flowing into the 'arguments' array, regardless of preceding arguments
-  n = call.asOrdinaryCall().getASpreadArgument() and
-  pos.isArgumentsArray()
-  or
  // receiver of accessor call
  pos.isThis() and n = call.asAccessorCall().getBase()
  or
@@ -953,6 +989,39 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
  or
  // NOTE: For consistency with readStep/storeStep, we do not translate these steps to jump steps automatically.
  DataFlow::AdditionalFlowStep::step(node1, node2)
+  or
+  exists(InvokeExpr invoke |
+    // When the first argument is a spread argument, flow into the argument array as a local flow step
+    // to ensure we preserve knowledge about array indices
+    node1 = TValueNode(invoke.getArgument(0).stripParens().(SpreadElement).getOperand()) and
+    node2 = TDynamicArgumentArrayNode(invoke)
+  )
+  or
+  exists(Function f |
+    // When the first parameter is a rest parameter, flow into the rest parameter as a local flow step
+    // to ensure we preserve knowledge about array indices
+    (node1 = TStaticParameterArrayNode(f) or node1 = TDynamicParameterArrayNode(f))
+  |
+    // rest parameter at initial position
+    exists(Parameter rest |
+      rest = f.getParameter(0) and
+      rest.isRestParameter() and
+      node2 = TValueNode(rest)
+    )
+    or
+    // 'arguments' array
+    node2 = TReflectiveParametersNode(f)
+  )
+  or
+  // Prepare to store non-spread arguments after a spread into the dynamic arguments array
+  exists(InvokeExpr invoke, int n, Expr argument, Content storeContent |
+    invoke.getArgument(n) = argument and
+    not argument instanceof SpreadElement and
+    n > firstSpreadArgumentIndex(invoke) and
+    node1 = TValueNode(argument) and
+    node2 = TDynamicArgumentStoreNode(invoke, storeContent) and
+    storeContent.isUnknownArrayElement()
+  )
 }

 predicate localMustFlowStep(Node node1, Node node2) { node1 = node2.getImmediatePredecessor() }
@@ -1018,6 +1087,42 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
  )
  or
  DataFlow::AdditionalFlowStep::readStep(node1, c, node2)
+  or
+  // Pass dynamic arguments into plain parameters
+  exists(Function function, Parameter param, int n |
+    param = function.getParameter(n) and
+    not param.isRestParameter() and
+    node1 = TDynamicParameterArrayNode(function) and
+    node2 = TValueNode(param) and
+    c = ContentSet::arrayElementFromInt(n)
+  )
+  or
+  // Prepare to store dynamic and static arguments into the rest parameter array when it isn't the first parameter
+  exists(Function function, Content content, int restIndex |
+    restIndex = function.getRestParameter().getIndex() and
+    restIndex > 0 and
+    (node1 = TStaticParameterArrayNode(function) or node1 = TDynamicParameterArrayNode(function)) and
+    node2 = TRestParameterStoreNode(function, content)
+  |
+    // shift known array indices
+    c.asArrayIndex() = content.asArrayIndex() + restIndex
+    or
+    content.isUnknownArrayElement() and // TODO: don't read unknown array elements from static array
+    c = ContentSet::arrayElementUnknown()
+  )
+  or
+  // Prepare to store spread arguments into the dynamic arguments array, when it isn't the initial spread argument
+  exists(InvokeExpr invoke, int n, Expr argument, Content storeContent |
+    invoke.getArgument(n).stripParens().(SpreadElement).getOperand() = argument and
+    n > 0 and // n=0 is handled as a value step
+    node1 = TValueNode(argument) and
+    node2 = TDynamicArgumentStoreNode(invoke, storeContent) and
+    if n > firstSpreadArgumentIndex(invoke)
+    then
+      c = ContentSet::arrayElement() and // unknown start index when not the first spread operator
+      storeContent.isUnknownArrayElement()
+    else storeContent.asArrayIndex() = n + c.asArrayIndex()
+  )
 }

 /** Gets the post-update node for which `node` is the corresponding pre-update node. */
@@ -1032,6 +1137,11 @@ private Node tryGetPostUpdate(Node node) {
  result = node
 }

+pragma[nomagic]
+private int firstSpreadArgumentIndex(InvokeExpr expr) {
+  result = min(int i | expr.isSpreadArgument(i))
+}
+
 /**
 * Holds if data can flow from `node1` to `node2` via a store into `c`.  Thus,
 * `node2` references an object with a content `c.getAStoreContent()` that
@@ -1063,6 +1173,25 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
  )
  or
  DataFlow::AdditionalFlowStep::storeStep(node1, c, node2)
+  or
+  exists(Function f, Content storeContent |
+    node1 = TRestParameterStoreNode(f, storeContent) and
+    node2 = TValueNode(f.getRestParameter()) and
+    c.asSingleton() = storeContent
+  )
+  or
+  exists(InvokeExpr invoke, Content storeContent |
+    node1 = TDynamicArgumentStoreNode(invoke, storeContent) and
+    node2 = TDynamicArgumentArrayNode(invoke) and
+    c.asSingleton() = storeContent
+  )
+  or
+  exists(InvokeExpr invoke, int n |
+    node1 = TValueNode(invoke.getArgument(n)) and
+    node2 = TStaticArgumentArrayNode(invoke) and
+    c.asArrayIndex() = n and
+    not n >= firstSpreadArgumentIndex(invoke)
+  )
 }

 /**
@@ -1112,6 +1241,9 @@ predicate expectsContent(Node n, ContentSet c) {
  c = MkPromiseFilter()
  or
  any(AdditionalFlowInternal flow).expectsContent(n, c)
+  or
+  c = ContentSet::arrayElement() and
+  n instanceof TDynamicParameterArrayNode
 }

 abstract class NodeRegion extends Unit {
--- a/javascript/ql/test/library-tests/TripleDot/test.expected
+++ b/javascript/ql/test/library-tests/TripleDot/test.expected
@@ -1,61 +0,0 @@
-| tst.js:5:14:5:20 | rest[0] | Unexpected result: hasTaintFlow=t1.1 |
-| tst.js:5:24:5:45 | // $ ha ... ow=t1.1 | Missing result:hasValueFlow=t1.1 |
-| tst.js:6:14:6:20 | rest[1] | Unexpected result: hasTaintFlow=t1.1 |
-| tst.js:6:24:6:45 | // $ ha ... ow=t1.2 | Missing result:hasValueFlow=t1.2 |
-| tst.js:7:31:7:70 | // $ ha ... ow=t1.2 | Missing result:hasTaintFlow=t1.2 |
-| tst.js:15:31:15:70 | // $ ha ... ow=t2.3 | Missing result:hasTaintFlow=t2.3 |
-| tst.js:22:14:22:14 | x | Unexpected result: hasTaintFlow=t3.1 |
-| tst.js:22:18:22:39 | // $ ha ... ow=t3.1 | Missing result:hasValueFlow=t3.1 |
-| tst.js:23:14:23:14 | y | Unexpected result: hasTaintFlow=t3.1 |
-| tst.js:23:18:23:39 | // $ ha ... ow=t3.2 | Missing result:hasValueFlow=t3.2 |
-| tst.js:24:14:24:14 | z | Unexpected result: hasTaintFlow=t3.1 |
-| tst.js:24:18:24:39 | // $ ha ... ow=t3.3 | Missing result:hasValueFlow=t3.3 |
-| tst.js:34:14:34:14 | w | Unexpected result: hasTaintFlow=t4.1 |
-| tst.js:35:14:35:14 | x | Unexpected result: hasTaintFlow=t4.1 |
-| tst.js:35:18:35:39 | // $ ha ... ow=t4.1 | Missing result:hasValueFlow=t4.1 |
-| tst.js:36:14:36:14 | y | Unexpected result: hasTaintFlow=t4.1 |
-| tst.js:36:18:36:39 | // $ ha ... ow=t4.2 | Missing result:hasValueFlow=t4.2 |
-| tst.js:37:14:37:14 | z | Unexpected result: hasTaintFlow=t4.1 |
-| tst.js:37:18:37:39 | // $ ha ... ow=t4.3 | Missing result:hasValueFlow=t4.3 |
-| tst.js:47:14:47:14 | w | Unexpected result: hasTaintFlow=t5.2 |
-| tst.js:47:14:47:14 | w | Unexpected result: hasTaintFlow=t5.3 |
-| tst.js:47:14:47:14 | w | Unexpected result: hasValueFlow=t5.1 |
-| tst.js:48:14:48:14 | x | Unexpected result: hasTaintFlow=t5.1 |
-| tst.js:48:14:48:14 | x | Unexpected result: hasTaintFlow=t5.3 |
-| tst.js:48:14:48:14 | x | Unexpected result: hasValueFlow=t5.2 |
-| tst.js:48:18:48:39 | // $ ha ... ow=t5.1 | Missing result:hasValueFlow=t5.1 |
-| tst.js:49:14:49:14 | y | Unexpected result: hasTaintFlow=t5.1 |
-| tst.js:49:14:49:14 | y | Unexpected result: hasTaintFlow=t5.2 |
-| tst.js:49:14:49:14 | y | Unexpected result: hasValueFlow=t5.3 |
-| tst.js:49:18:49:39 | // $ ha ... ow=t5.2 | Missing result:hasValueFlow=t5.2 |
-| tst.js:50:14:50:14 | z | Unexpected result: hasTaintFlow=t5.1 |
-| tst.js:50:14:50:14 | z | Unexpected result: hasTaintFlow=t5.2 |
-| tst.js:50:14:50:14 | z | Unexpected result: hasTaintFlow=t5.3 |
-| tst.js:50:18:50:39 | // $ ha ... ow=t5.3 | Missing result:hasValueFlow=t5.3 |
-| tst.js:61:28:61:49 | // $ ha ... ow=t6.1 | Missing result:hasValueFlow=t6.1 |
-| tst.js:62:28:62:49 | // $ ha ... ow=t6.2 | Missing result:hasValueFlow=t6.2 |
-| tst.js:63:28:63:49 | // $ ha ... ow=t6.3 | Missing result:hasValueFlow=t6.3 |
-| tst.js:70:18:70:39 | // $ ha ... ow=t7.1 | Missing result:hasValueFlow=t7.1 |
-| tst.js:71:18:71:39 | // $ ha ... ow=t7.2 | Missing result:hasValueFlow=t7.2 |
-| tst.js:72:18:72:39 | // $ ha ... ow=t7.3 | Missing result:hasValueFlow=t7.3 |
-| tst.js:82:14:82:14 | x | Unexpected result: hasTaintFlow=t8.2 |
-| tst.js:82:14:82:14 | x | Unexpected result: hasTaintFlow=t8.4 |
-| tst.js:83:14:83:14 | y | Unexpected result: hasTaintFlow=t8.1 |
-| tst.js:83:14:83:14 | y | Unexpected result: hasTaintFlow=t8.3 |
-| tst.js:83:18:83:85 | // $ ha ... ow=t8.4 | Fixed spurious result:hasValueFlow=t8.3 |
-| tst.js:84:14:84:14 | z | Unexpected result: hasTaintFlow=t8.1 |
-| tst.js:84:14:84:14 | z | Unexpected result: hasTaintFlow=t8.2 |
-| tst.js:84:14:84:14 | z | Unexpected result: hasTaintFlow=t8.3 |
-| tst.js:84:14:84:14 | z | Unexpected result: hasTaintFlow=t8.4 |
-| tst.js:84:18:84:85 | // $ ha ... ow=t8.4 | Fixed spurious result:hasValueFlow=t8.3 |
-| tst.js:84:18:84:85 | // $ ha ... ow=t8.4 | Fixed spurious result:hasValueFlow=t8.4 |
-| tst.js:84:18:84:85 | // $ ha ... ow=t8.4 | Missing result:hasValueFlow=t8.3 |
-| tst.js:94:18:94:39 | // $ ha ... ow=t9.1 | Missing result:hasValueFlow=t9.1 |
-| tst.js:95:18:95:39 | // $ ha ... ow=t9.2 | Missing result:hasValueFlow=t9.2 |
-| tst.js:96:18:96:39 | // $ ha ... ow=t9.3 | Missing result:hasValueFlow=t9.3 |
-| tst.js:106:14:106:14 | x | Unexpected result: hasTaintFlow=t10.1 |
-| tst.js:106:18:106:40 | // $ ha ... w=t10.1 | Missing result:hasValueFlow=t10.1 |
-| tst.js:107:14:107:14 | y | Unexpected result: hasTaintFlow=t10.1 |
-| tst.js:107:18:107:40 | // $ ha ... w=t10.2 | Missing result:hasValueFlow=t10.2 |
-| tst.js:108:14:108:14 | z | Unexpected result: hasTaintFlow=t10.1 |
-| tst.js:108:18:108:40 | // $ ha ... w=t10.3 | Missing result:hasValueFlow=t10.3 |
--- a/javascript/ql/test/library-tests/TripleDot/tst.js
+++ b/javascript/ql/test/library-tests/TripleDot/tst.js
@@ -79,7 +79,7 @@ function t7() {

 function t8() {
    function finalTarget(x, y, z) {
-        sink(x); // $ hasValueFlow=t8.1 SPURIOUS: hasValueFlow=t8.3
+        sink(x); // $ hasValueFlow=t8.1 SPURIOUS: hasValueFlow=t8.3 hasValueFlow=t8.4
        sink(y); // $ hasValueFlow=t8.2 SPURIOUS: hasValueFlow=t8.3 hasValueFlow=t8.4
        sink(z); // $ hasValueFlow=t8.3 SPURIOUS: hasValueFlow=t8.3 hasValueFlow=t8.4
    }