Merge pull request #10126 from erik-krogh/myApply

JS: precise flow through calls to `.apply()`

Author: erik-krogh

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

@@ -261,14 +261,12 @@ private module ArrayDataFlow {
   /**
    * A step for creating an array and storing the elements in the array.
    */
-  private class ArrayCreationStep extends DataFlow::SharedFlowStep {
+  private class ArrayCreationStep extends PreCallGraphStep {
     override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
       exists(DataFlow::ArrayCreationNode array, int i |
         element = array.getElement(i) and
         obj = array and
-        if array = any(PromiseAllCreation c).getArrayNode()
-        then prop = arrayElement(i)
-        else prop = arrayElement()
+        prop = arrayElement(i)
       )
     }
   }
@@ -346,6 +344,14 @@ private module ArrayLibraries {
     result = DataFlow::globalVarRef("Array").getAMemberCall("from")
     or
     result = DataFlow::moduleImport("array-from").getACall()
+    or
+    // Array.prototype.slice.call acts the same as Array.from, and is sometimes used with e.g. the arguments object.
+    result =
+      DataFlow::globalVarRef("Array")
+          .getAPropertyRead("prototype")
+          .getAPropertyRead("slice")
+          .getAMethodCall("call") and
+    result.getNumArgument() = 1
   }
 
   /**

javascript/ql/lib/semmle/javascript/PackageExports.qll

@@ -11,36 +11,14 @@ private import semmle.javascript.internal.CachedStages
  * Gets a parameter that is a library input to a top-level package.
  */
 cached
-DataFlow::SourceNode getALibraryInputParameter() {
+DataFlow::Node getALibraryInputParameter() {
   Stages::Taint::ref() and
   exists(int bound, DataFlow::FunctionNode func |
     func = getAValueExportedByPackage().getABoundFunctionValue(bound)
   |
     result = func.getParameter(any(int arg | arg >= bound))
     or
-    result = getAnArgumentsRead(func.getFunction())
-  )
-}
-
-private DataFlow::SourceNode getAnArgumentsRead(Function func) {
-  exists(DataFlow::PropRead read |
-    not read.getPropertyName() = "length" and
-    result = read
-  |
-    read.getBase() = func.getArgumentsVariable().getAnAccess().flow()
-    or
-    exists(DataFlow::MethodCallNode call |
-      call =
-        DataFlow::globalVarRef("Array")
-            .getAPropertyRead("prototype")
-            .getAPropertyRead("slice")
-            .getAMethodCall("call")
-      or
-      call = DataFlow::globalVarRef("Array").getAMethodCall("from")
-    |
-      call.getArgument(0) = func.getArgumentsVariable().getAnAccess().flow() and
-      call.flowsTo(read.getBase())
-    )
+    result = func.getFunction().getArgumentsVariable().getAnAccess().flow()
   )
 }
 

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

@@ -1267,56 +1267,47 @@ private predicate loadStep(
  * If `onlyRelevantInCall` is true, the `base` object will not be propagated out of return edges, because
  * the flow that originally reached `base.startProp` used a call edge.
  */
-pragma[nomagic]
+pragma[noopt]
 private predicate reachableFromStoreBase(
   string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
-  DataFlow::Configuration cfg, PathSummary summary, boolean onlyRelevantInCall
+  DataFlow::Configuration cfg, TPathSummary summary, boolean onlyRelevantInCall
 ) {
-  exists(PathSummary s1, PathSummary s2, DataFlow::Node rhs |
-    reachableFromSource(rhs, cfg, s1) and
-    onlyRelevantInCall = s1.hasCall()
-    or
-    reachableFromStoreBase(_, _, _, rhs, cfg, s1, onlyRelevantInCall)
-  |
+  exists(TPathSummary s1, TPathSummary s2, DataFlow::Node rhs |
     storeStep(rhs, nd, startProp, cfg, s2) and
     endProp = startProp and
     base = nd and
-    summary =
-      MkPathSummary(false, s2.hasCall(), DataFlow::FlowLabel::data(), DataFlow::FlowLabel::data())
+    exists(boolean hasCall, DataFlow::FlowLabel data |
+      hasCall = hasCall(s2) and
+      data = DataFlow::FlowLabel::data() and
+      summary = MkPathSummary(false, hasCall, data, data)
+    )
+  |
+    reachableFromSource(rhs, cfg, s1) and
+    onlyRelevantInCall = hasCall(s1)
+    or
+    reachableFromStoreBase(_, _, _, rhs, cfg, s1, onlyRelevantInCall)
   )
   or
-  exists(PathSummary newSummary, PathSummary oldSummary |
-    reachableFromStoreBaseStep(startProp, endProp, base, nd, cfg, oldSummary, newSummary,
-      onlyRelevantInCall) and
-    summary = oldSummary.appendValuePreserving(newSummary)
-  )
-}
-
-/**
- * Holds if `base` is the base of a write to property `endProp`, and `nd` is reachable
- * from `base` under configuration `cfg` (possibly through callees) along a path whose
- * last step is summarized by `newSummary`, and the previous steps are summarized
- * by `oldSummary`.
- */
-pragma[noinline]
-private predicate reachableFromStoreBaseStep(
-  string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
-  DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary newSummary,
-  boolean onlyRelevantInCall
-) {
-  exists(DataFlow::Node mid |
+  exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
     reachableFromStoreBase(startProp, endProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
     flowStep(mid, cfg, nd, newSummary) and
-    onlyRelevantInCall.booleanAnd(newSummary.hasReturn()) = false
+    exists(boolean hasReturn |
+      hasReturn = newSummary.hasReturn() and
+      onlyRelevantInCall.booleanAnd(hasReturn) = false
+    )
     or
     exists(string midProp |
       reachableFromStoreBase(startProp, midProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
       isAdditionalLoadStoreStep(mid, nd, midProp, endProp, cfg) and
       newSummary = PathSummary::level()
     )
+  |
+    summary = oldSummary.appendValuePreserving(newSummary)
   )
 }
 
+private boolean hasCall(PathSummary summary) { result = summary.hasCall() }
+
 /**
  * Holds if the value of `pred` is written to a property of some base object, and that base
  * object may flow into the base of property read `succ` under configuration `cfg` along

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

@@ -1029,6 +1029,32 @@ module DataFlow {
     override File getFile() { result = function.getFile() }
   }
 
+  /**
+   * A data flow node representing the arguments object given to a function.
+   */
+  class ReflectiveParametersNode extends DataFlow::Node, TReflectiveParametersNode {
+    Function function;
+
+    ReflectiveParametersNode() { this = TReflectiveParametersNode(function) }
+
+    override string toString() { result = "'arguments' object of " + function.describe() }
+
+    override predicate hasLocationInfo(
+      string filepath, int startline, int startcolumn, int endline, int endcolumn
+    ) {
+      function.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    }
+
+    override BasicBlock getBasicBlock() { result = function.getEntry().getBasicBlock() }
+
+    /**
+     * Gets the function whose `arguments` object is represented by this node.
+     */
+    Function getFunction() { result = function }
+
+    override File getFile() { result = function.getFile() }
+  }
+
   /**
    * A data flow node representing the exceptions thrown by the callee of an invocation.
    */
@@ -1627,6 +1653,35 @@ module DataFlow {
     exists(Function f | not f.isAsyncOrGenerator() |
       DataFlow::functionReturnNode(succ, f) and pred = valueNode(f.getAReturnedExpr())
     )
+    or
+    // from a reflective params node to a reference to the arguments object.
+    exists(DataFlow::ReflectiveParametersNode params, Function f | f = params.getFunction() |
+      succ = f.getArgumentsVariable().getAnAccess().flow() and
+      pred = params
+    )
+  }
+
+  /** A load step from a reflective parameter node to each parameter. */
+  private class ReflectiveParamsStep extends PreCallGraphStep {
+    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
+        obj = params and
+        prop = i + "" and
+        element = f.getParameter(i)
+      )
+    }
+  }
+
+  /** A taint step from the reflective parameters node to any parameter. */
+  private class ReflectiveParamsTaintStep extends TaintTracking::SharedTaintStep {
+    override predicate step(DataFlow::Node obj, DataFlow::Node element) {
+      exists(DataFlow::ReflectiveParametersNode params, DataFlow::FunctionNode f |
+        f.getFunction() = params.getFunction() and
+        obj = params and
+        element = f.getAParameter()
+      )
+    }
   }
 
   /**

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

@@ -332,6 +332,8 @@ module SourceNode {
       or
       // Include return nodes because they model the implicit Promise creation in async functions.
       DataFlow::functionReturnNode(this, _)
+      or
+      this instanceof DataFlow::ReflectiveParametersNode
     }
   }
 }

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

@@ -31,4 +31,5 @@ newtype TNode =
   TExceptionalFunctionReturnNode(Function f) or
   TExceptionalInvocationReturnNode(InvokeExpr e) or
   TGlobalAccessPathRoot() or
-  TTemplatePlaceholderTag(Templating::TemplatePlaceholderTag tag)
+  TTemplatePlaceholderTag(Templating::TemplatePlaceholderTag tag) or
+  TReflectiveParametersNode(Function f)

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

@@ -224,6 +224,14 @@ private module CachedSteps {
       or
       arg = invk.(DataFlow::PropWrite).getRhs() and
       parm = DataFlow::parameterNode(f.getParameter(0))
+      or
+      calls(invk, f) and
+      exists(MethodCallExpr apply |
+        invk = DataFlow::reflectiveCallNode(apply) and
+        apply.getMethodName() = "apply" and
+        arg = apply.getArgument(1).flow()
+      ) and
+      parm.(DataFlow::ReflectiveParametersNode).getFunction() = f
     )
     or
     exists(DataFlow::Node callback, int i, Parameter p, Function target |

javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentCustomizations.qll

@@ -68,7 +68,7 @@ module PrototypePollutingAssignment {
   /**
    * A parameter of an exported function, seen as a source prototype-polluting assignment.
    */
-  class ExternalInputSource extends Source, DataFlow::SourceNode {
+  class ExternalInputSource extends Source {
     ExternalInputSource() { this = Exports::getALibraryInputParameter() }
 
     override string describe() { result = "library input" }

javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeCodeConstructionCustomizations.qll

@@ -21,11 +21,11 @@ module UnsafeCodeConstruction {
   /**
    * A parameter of an exported function, seen as a source.
    */
-  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+  class ExternalInputSource extends Source {
     ExternalInputSource() {
       this = Exports::getALibraryInputParameter() and
       // permit parameters that clearly are intended to contain executable code.
-      not this.getName() = "code"
+      not this.(DataFlow::ParameterNode).getName() = "code"
     }
   }
 

javascript/ql/lib/semmle/javascript/security/dataflow/UnsafeHtmlConstructionCustomizations.qll

@@ -22,7 +22,7 @@ module UnsafeHtmlConstruction {
   /**
    * A parameter of an exported function, seen as a source for usnafe HTML constructed from input.
    */
-  class ExternalInputSource extends Source, DataFlow::ParameterNode {
+  class ExternalInputSource extends Source {
     ExternalInputSource() {
       this = Exports::getALibraryInputParameter() and
       // An AMD-style module sometimes loads the jQuery library in a way which looks like library input.