JS: Support for dynamic args to flow summaries

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

@@ -24,6 +24,8 @@ private module ConsistencyConfig implements InputSig<Location, JSDataFlow> {
     or
     n instanceof FlowSummaryIntermediateAwaitStoreNode
     or
+    n instanceof FlowSummaryDynamicParameterArrayNode
+    or
     n instanceof GenericSynthesizedNode
     or
     n = DataFlow::globalAccessPathRootPseudoNode()
@@ -43,6 +45,10 @@ private module ConsistencyConfig implements InputSig<Location, JSDataFlow> {
     node instanceof TStaticArgumentArrayNode or
     node instanceof TDynamicArgumentArrayNode
   }
+
+  predicate reverseReadExclude(DataFlow::Node node) {
+    node instanceof FlowSummaryDynamicParameterArrayNode
+  }
 }
 
 module Consistency = MakeConsistency<Location, JSDataFlow, JSTaintFlow, ConsistencyConfig>;

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

@@ -80,6 +80,7 @@ private module Cached {
     TConstructorThisArgumentNode(InvokeExpr e) { e instanceof NewExpr or e instanceof SuperCall } or
     TConstructorThisPostUpdate(Constructor ctor) or
     TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or
+    TFlowSummaryDynamicParameterArrayNode(FlowSummaryImpl::Public::SummarizedCallable callable) or
     TFlowSummaryIntermediateAwaitStoreNode(FlowSummaryImpl::Private::SummaryNode sn) {
       // NOTE: This dependency goes through the 'Steps' module whose instantiation depends on the call graph,
       //       but the specific predicate we're referering to does not use that information.

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

@@ -30,6 +30,19 @@ class FlowSummaryNode extends DataFlow::Node, TFlowSummaryNode {
   override string toString() { result = this.getSummaryNode().toString() }
 }
 
+class FlowSummaryDynamicParameterArrayNode extends DataFlow::Node,
+  TFlowSummaryDynamicParameterArrayNode
+{
+  private FlowSummaryImpl::Public::SummarizedCallable callable;
+
+  FlowSummaryDynamicParameterArrayNode() { this = TFlowSummaryDynamicParameterArrayNode(callable) }
+
+  FlowSummaryImpl::Public::SummarizedCallable getSummarizedCallable() { result = callable }
+
+  cached
+  override string toString() { result = "[dynamic parameter array] " + callable }
+}
+
 class FlowSummaryIntermediateAwaitStoreNode extends DataFlow::Node,
   TFlowSummaryIntermediateAwaitStoreNode
 {
@@ -342,6 +355,12 @@ private predicate isParameterNodeImpl(Node p, DataFlowCallable c, ParameterPosit
     FlowSummaryImpl::Private::summaryParameterNode(summaryNode.getSummaryNode(), pos) and
     c.asLibraryCallable() = summaryNode.getSummarizedCallable()
   )
+  or
+  exists(FlowSummaryImpl::Public::SummarizedCallable callable |
+    c.asLibraryCallable() = callable and
+    pos.isDynamicArgumentArray() and
+    p = TFlowSummaryDynamicParameterArrayNode(callable)
+  )
 }
 
 predicate isParameterNode(ParameterNode p, DataFlowCallable c, ParameterPosition pos) {
@@ -410,6 +429,8 @@ DataFlowCallable nodeGetEnclosingCallable(Node node) {
   or
   result.asLibraryCallable() = node.(FlowSummaryNode).getSummarizedCallable()
   or
+  result.asLibraryCallable() = node.(FlowSummaryDynamicParameterArrayNode).getSummarizedCallable()
+  or
   result.asLibraryCallable() = node.(FlowSummaryIntermediateAwaitStoreNode).getSummarizedCallable()
   or
   node = TGenericSynthesizedNode(_, _, result)
@@ -1118,6 +1139,17 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
       storeContent.isUnknownArrayElement()
     else storeContent.asArrayIndex() = n + c.asArrayIndex()
   )
+  or
+  exists(FlowSummaryNode parameter, ParameterPosition pos |
+    FlowSummaryImpl::Private::summaryParameterNode(parameter.getSummaryNode(), pos) and
+    node1 = TFlowSummaryDynamicParameterArrayNode(parameter.getSummarizedCallable()) and
+    node2 = parameter and
+    (
+      c.asArrayIndex() = pos.asPositional()
+      or
+      c = ContentSet::arrayElementLowerBound(pos.asPositionalLowerBound())
+    )
+  )
 }
 
 /** Gets the post-update node for which `node` is the corresponding pre-update node. */

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

@@ -35,8 +35,6 @@ private predicate positionName(ParameterPosition pos, string operand) {
   or
   pos.isFunctionSelfReference() and operand = "function"
   or
-  pos.isDynamicArgumentArray() and operand = "arguments-array" // TODO: remove and handle automatically
-  or
   operand = pos.asPositionalLowerBound() + ".."
 }
 

javascript/ql/lib/semmle/javascript/internal/flow_summaries/Arrays.qll

@@ -101,17 +101,11 @@ class ArrayConstructorSummary extends SummarizedCallable {
 
   override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
     preservesValue = true and
-    (
     input = "Argument[0..]" and
     output = "ReturnValue.ArrayElement"
     or
-      input = "Argument[arguments-array].WithArrayElement" and
-      output = "ReturnValue"
-    )
-    or
-    // TODO: workaround for WithArrayElement not being converted to a taint step
     preservesValue = false and
-    input = "Argument[arguments-array]" and
+    input = "Argument[0..]" and
     output = "ReturnValue"
   }
 }
@@ -437,8 +431,7 @@ class PushLike extends SummarizedCallable {
 
   override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
     preservesValue = true and
-    // TODO: make it so `arguments-array` is handled without needing to reference it explicitly in every flow-summary
+    input = "Argument[0..]" and
-    input = ["Argument[0..]", "Argument[arguments-array].ArrayElement"] and
     output = "Argument[this].ArrayElement"
   }
 }

javascript/ql/test/library-tests/FlowSummary/test.expected

@@ -1,7 +0,0 @@
-| library-tests/FlowSummary/tst.js:278 | expected an alert, but found none | NOT OK | ConsistencyConfig |
-| library-tests/FlowSummary/tst.js:282 | expected an alert, but found none | NOT OK | ConsistencyConfig |
-| library-tests/FlowSummary/tst.js:283 | expected an alert, but found none | NOT OK | ConsistencyConfig |
-| library-tests/FlowSummary/tst.js:286 | expected an alert, but found none | NOT OK | ConsistencyConfig |
-| library-tests/FlowSummary/tst.js:287 | expected an alert, but found none | NOT OK | ConsistencyConfig |
-| library-tests/FlowSummary/tst.js:290 | expected an alert, but found none | NOT OK | ConsistencyConfig |
-| library-tests/FlowSummary/tst.js:291 | expected an alert, but found none | NOT OK | ConsistencyConfig |

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

@@ -275,19 +275,27 @@ function m18() {
   const dynamicParam0 = mkSummary("Argument[0..]", "ReturnValue");
   const dynamicParam1 = mkSummary("Argument[1..]", "ReturnValue");
 
-  sink(staticParam0(...source())); // NOT OK
+  sink(staticParam0(...[source()])); // NOT OK
-  sink(staticParam0("safe", ...source())); // OK
+  sink(staticParam0(...["safe", source()])); // OK
+  sink(staticParam0(...[source(), "safe", ])); // NOT OK
+  sink(staticParam0("safe", ...[source()])); // OK
   sink(staticParam0(source(), ...["safe"])); // NOT OK
 
-  sink(staticParam1(...source())); // NOT OK
+  sink(staticParam1(...[source()])); // OK
-  sink(staticParam1("safe", ...source())); // NOT OK
+  sink(staticParam1(...["safe", source()])); // NOT OK
+  sink(staticParam1(...[source(), "safe", ])); // OK
+  sink(staticParam1("safe", ...[source()])); // NOT OK
   sink(staticParam1(source(), ...["safe"])); // OK
 
-  sink(dynamicParam0(...source())); // NOT OK
+  sink(dynamicParam0(...[source()])); // NOT OK
-  sink(dynamicParam0("safe", ...source())); // NOT OK
+  sink(dynamicParam0(...["safe", source()])); // NOT OK
+  sink(dynamicParam0(...[source(), "safe", ])); // NOT OK
+  sink(dynamicParam0("safe", ...[source()])); // NOT OK
   sink(dynamicParam0(source(), ...["safe"])); // NOT OK
 
-  sink(dynamicParam1(...source())); // NOT OK
+  sink(dynamicParam1(...[source()])); // OK
-  sink(dynamicParam1("safe", ...source())); // NOT OK
+  sink(dynamicParam1(...["safe", source()])); // NOT OK
+  sink(dynamicParam1(...[source(), "safe", ])); // OK
+  sink(dynamicParam1("safe", ...[source()])); // NOT OK
   sink(dynamicParam1(source(), ...["safe"])); // OK
 }