Merge pull request #17213 from asgerf/jss/spread-argument

JS: Improve handling of spread arguments and rest parameters [shared data flow branch]

Author: asgerf

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

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

@@ -260,6 +260,14 @@ module TaintTracking {
     )
   }
 
+  private class HeapLegacyTaintStep extends LegacyTaintStep {
+    override predicate heapStep(DataFlow::Node pred, DataFlow::Node succ) {
+      // arrays with tainted elements are tainted (in old data flow)
+      succ.(DataFlow::ArrayCreationNode).getAnElement() = pred and
+      not any(PromiseAllCreation call).getArrayNode() = succ
+    }
+  }
+
   /**
    * A taint propagating data flow edge through object or array elements and
    * promises.
@@ -274,10 +282,6 @@ module TaintTracking {
       // spreading a tainted value into an array literal gives a tainted array
       succ.(DataFlow::ArrayCreationNode).getASpreadArgument() = pred
       or
-      // arrays with tainted elements and objects with tainted property names are tainted
-      succ.(DataFlow::ArrayCreationNode).getAnElement() = pred and
-      not any(PromiseAllCreation call).getArrayNode() = succ
-      or
       // reading from a tainted object yields a tainted result
       succ.(DataFlow::PropRead).getBase() = pred and
       not (

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

@@ -255,7 +255,10 @@ module Public {
     Content asSingleton() { this = MkSingletonContent(result) }
 
     /** Gets the property name to be accessed. */
-    PropertyName asPropertyName() { result = this.asSingleton().asPropertyName() }
+    PropertyName asPropertyName() {
+      // TODO: array indices should be mapped to a ContentSet that also reads from UnknownArrayElement
+      result = this.asSingleton().asPropertyName()
+    }
 
     /** Gets the array index to be accessed. */
     int asArrayIndex() { result = this.asSingleton().asArrayIndex() }

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()
@@ -37,6 +39,16 @@ private module ConsistencyConfig implements InputSig<Location, JSDataFlow> {
     isAmbientNode(call.asOrdinaryCall()) or
     isAmbientNode(call.asAccessorCall())
   }
+
+  predicate argHasPostUpdateExclude(ArgumentNode node) {
+    // Side-effects directly on these can't propagate back to the caller, and for longer access paths it's too imprecise
+    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

@@ -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`.
    */
@@ -33,6 +39,25 @@ private module Cached {
     } or
     TThisNode(StmtContainer f) { f.(Function).getThisBinder() = f or f instanceof TopLevel } or
     TFunctionSelfReferenceNode(Function f) or
+    TStaticArgumentArrayNode(InvokeExpr node) or
+    TDynamicArgumentArrayNode(InvokeExpr node) { node.isSpreadArgument(_) } or
+    TStaticParameterArrayNode(Function f) {
+      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
+    TApplyCallTaintNode(MethodCallExpr node) {
+      node.getMethodName() = "apply" and exists(node.getArgument(1))
+    } or
     TDestructuredModuleImportNode(ImportDeclaration decl) {
       exists(decl.getASpecifier().getImportedName())
     } or
@@ -43,7 +68,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
@@ -58,6 +83,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.
@@ -96,7 +122,8 @@ private class TEarlyStageNode =
       TFunctionSelfReferenceNode or TDestructuredModuleImportNode or THtmlAttributeNode or
       TFunctionReturnNode or TExceptionalFunctionReturnNode or TExceptionalInvocationReturnNode or
       TGlobalAccessPathRoot or TTemplatePlaceholderTag or TReflectiveParametersNode or
-      TExprPostUpdateNode or TConstructorThisArgumentNode;
+      TExprPostUpdateNode or TConstructorThisArgumentNode or TStaticArgumentArrayNode or
+      TDynamicArgumentArrayNode or TStaticParameterArrayNode or TDynamicParameterArrayNode;
 
 /**
  * A data-flow node that is not a flow summary node.