Merge pull request #2855 from asger-semmle/js/returned-partial-call

Approved by esbena

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -13,7 +13,9 @@
 
 * The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
 
-* Calls can now be resolved to class members in more cases, leading to more results from the security queries.
+* The call graph construction has been improved, leading to more results from the security queries:
+  - Calls can now be resolved to indirectly-defined class members in more cases.
+  - Calls through partial invocations such as `.bind` can now be resolved in more cases.
 
 * Support for the following frameworks and libraries has been improved:
   - [Electron](https://electronjs.org/)

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

@@ -71,6 +71,7 @@
 private import javascript
 private import internal.FlowSteps
 private import internal.AccessPaths
+private import internal.CallGraphs
 
 /**
  * A data flow tracking configuration for finding inter-procedural paths from
@@ -620,10 +621,11 @@ private predicate exploratoryFlowStep(
   isAdditionalStoreStep(pred, succ, _, cfg) or
   isAdditionalLoadStep(pred, succ, _, cfg) or
   isAdditionalLoadStoreStep(pred, succ, _, cfg) or
-  // the following two disjuncts taken together over-approximate flow through
+  // the following three disjuncts taken together over-approximate flow through
   // higher-order calls
   callback(pred, succ) or
-  succ = pred.(DataFlow::FunctionNode).getAParameter()
+  succ = pred.(DataFlow::FunctionNode).getAParameter() or
+  exploratoryBoundInvokeStep(pred, succ)
 }
 
 /**
@@ -751,7 +753,7 @@ private predicate flowThroughCall(
 ) {
   exists(Function f, DataFlow::ValueNode ret |
     ret.asExpr() = f.getAReturnedExpr() and
-    calls(output, f) and // Do not consider partial calls
+    (calls(output, f) or callsBound(output, f, _)) and // Do not consider partial calls
     reachableFromInput(f, output, input, ret, cfg, summary) and
     not isBarrierEdge(cfg, ret, output) and
     not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
@@ -761,7 +763,7 @@ private predicate flowThroughCall(
   exists(Function f, DataFlow::Node invk, DataFlow::Node ret |
     DataFlow::exceptionalFunctionReturnNode(ret, f) and
     DataFlow::exceptionalInvocationReturnNode(output, invk.asExpr()) and
-    calls(invk, f) and
+    (calls(invk, f) or callsBound(invk, f, _)) and
     reachableFromInput(f, invk, input, ret, cfg, summary) and
     not isBarrierEdge(cfg, ret, output) and
     not isLabeledBarrierEdge(cfg, ret, output, summary.getEndLabel()) and
@@ -1032,6 +1034,13 @@ private predicate flowIntoHigherOrderCall(
     succ = cb.getParameter(i) and
     summary = oldSummary.append(PathSummary::call())
   )
+  or
+  exists(DataFlow::SourceNode cb, DataFlow::FunctionNode f, int i, int boundArgs, PathSummary oldSummary |
+    higherOrderCall(pred, cb, i, cfg, oldSummary) and
+    cb = CallGraph::getABoundFunctionReference(f, boundArgs, false) and
+    succ = f.getParameter(boundArgs + i) and
+    summary = oldSummary.append(PathSummary::call())
+  )
 }
 
 /**

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

@@ -147,7 +147,7 @@ module DataFlow {
     final FunctionNode getABoundFunctionValue(int boundArgs) {
       result = getAFunctionValue() and boundArgs = 0
       or
-      CallGraph::getABoundFunctionReference(result, boundArgs).flowsTo(this)
+      CallGraph::getABoundFunctionReference(result, boundArgs, _).flowsTo(this)
     }
 
     /**

javascript/ql/src/semmle/javascript/dataflow/internal/CallGraphs.qll

@@ -74,7 +74,7 @@ module CallGraph {
    */
   pragma[nomagic]
   private
-  DataFlow::SourceNode getABoundFunctionReference(DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t) {
+  DataFlow::SourceNode getABoundFunctionReferenceAux(DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t) {
     exists(DataFlow::PartialInvokeNode partial, DataFlow::Node callback |
       result = partial.getBoundFunction(callback, boundArgs) and
       getAFunctionReference(function, 0, t.continue()).flowsTo(callback)
@@ -90,7 +90,7 @@ module CallGraph {
   private
   DataFlow::SourceNode getABoundFunctionReferenceAux(DataFlow::FunctionNode function, int boundArgs, DataFlow::TypeTracker t, DataFlow::StepSummary summary) {
     exists(DataFlow::SourceNode prev |
-      prev = getABoundFunctionReference(function, boundArgs, t) and
+      prev = getABoundFunctionReferenceAux(function, boundArgs, t) and
       DataFlow::StepSummary::step(prev, result, summary)
     )
   }
@@ -100,8 +100,12 @@ module CallGraph {
    * with `function` as the underlying function.
    */
   cached
-  DataFlow::SourceNode getABoundFunctionReference(DataFlow::FunctionNode function, int boundArgs) {
-    result = getABoundFunctionReference(function, boundArgs, DataFlow::TypeTracker::end())
+  DataFlow::SourceNode getABoundFunctionReference(DataFlow::FunctionNode function, int boundArgs, boolean contextDependent) {
+    exists(DataFlow::TypeTracker t |
+      result = getABoundFunctionReferenceAux(function, boundArgs, t) and
+      t.end() and
+      contextDependent = t.hasCall()
+    )
   }
 
   /**

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

@@ -6,6 +6,7 @@
 
 import javascript
 import semmle.javascript.dataflow.Configuration
+import semmle.javascript.dataflow.internal.CallGraphs
 
 /**
  * Holds if flow should be tracked through properties of `obj`.
@@ -91,6 +92,32 @@ private module CachedSteps {
   cached
   predicate calls(DataFlow::InvokeNode invk, Function f) { f = invk.getACallee(0) }
 
+  /**
+   * Holds if `invk` may invoke a bound version of `f` with `boundArgs` already bound.
+   *
+   * The receiver is assumed to be bound as well, and should not propagate into `f`.
+   *
+   * Does not hold for context-dependent call sites, such as callback invocations.
+   */
+  cached
+  predicate callsBound(DataFlow::InvokeNode invk, Function f, int boundArgs) {
+    CallGraph::getABoundFunctionReference(f.flow(), boundArgs, false).flowsTo(invk.getCalleeNode())
+  }
+
+  /**
+   * Holds if `pred` may flow to `succ` through an invocation of a bound function.
+   *
+   * Should only be used for graph pruning, as the edge may lead to spurious flow.
+   */
+  cached
+  predicate exploratoryBoundInvokeStep(DataFlow::Node pred, DataFlow::Node succ) {
+    exists(DataFlow::InvokeNode invk, DataFlow::FunctionNode f, int i, int boundArgs |
+      CallGraph::getABoundFunctionReference(f, boundArgs, _).flowsTo(invk.getCalleeNode()) and
+      pred = invk.getArgument(i) and
+      succ = f.getParameter(i + boundArgs)
+    )
+  }
+
   /**
    * Holds if `invk` may invoke `f` indirectly through the given `callback` argument.
    *
@@ -141,6 +168,14 @@ private module CachedSteps {
       partiallyCalls(invk, callback, f) and
       parm = DataFlow::thisNode(f)
     )
+    or
+    exists(int boundArgs, int i, Parameter p |
+      callsBound(invk, f, boundArgs) and
+      f.getParameter(boundArgs + i) = p and
+      not p.isRestParameter() and
+      arg = invk.getArgument(i) and
+      parm = DataFlow::parameterNode(p)
+    )
   }
 
   /**
@@ -158,7 +193,7 @@ private module CachedSteps {
    */
   cached
   predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
-    exists(Function f | calls(succ, f) |
+    exists(Function f | calls(succ, f) or callsBound(succ, f, _) |
       returnExpr(f, pred, _)
       or
       succ instanceof DataFlow::NewNode and
@@ -167,8 +202,11 @@ private module CachedSteps {
     or
     exists(InvokeExpr invoke, Function fun |
       DataFlow::exceptionalFunctionReturnNode(pred, fun) and
-      DataFlow::exceptionalInvocationReturnNode(succ, invoke) and
+      DataFlow::exceptionalInvocationReturnNode(succ, invoke)
+    |
       calls(invoke.flow(), fun)
+      or
+      callsBound(invoke.flow(), fun, _)
     )
   }
 
@@ -464,4 +502,3 @@ module PathSummary {
    */
   PathSummary return() { exists(FlowLabel lbl | result = MkPathSummary(true, false, lbl, lbl)) }
 }
-

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.expected

@@ -1,5 +1,5 @@
 spuriousCallee
 missingCallee
-| constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} |
-| constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} |
+| constructor-field.ts:40:5:40:14 | f3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 |
+| constructor-field.ts:71:1:71:11 | bf3.build() | constructor-field.ts:13:3:13:12 | build() {} | -1 |
 badAnnotation

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/Test.ql

@@ -33,16 +33,37 @@ class AnnotatedCall extends InvokeExpr {
   string getCallTargetName() { result = calls }
 
   AnnotatedFunction getAnExpectedCallee() { result.getCalleeName() = getCallTargetName() }
+
+  int getBoundArgs() {
+    result = getAnnotation(this, "boundArgs").toInt()
+  }
+
+  int getBoundArgsOrMinusOne() {
+    result = getBoundArgs()
+    or
+    not exists(getBoundArgs()) and
+    result = -1
+  }
+}
+
+predicate callEdge(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
+  FlowSteps::calls(call.flow(), target) and boundArgs = -1
+  or
+  FlowSteps::callsBound(call.flow(), target, boundArgs)
 }
 
-query predicate spuriousCallee(AnnotatedCall call, AnnotatedFunction target) {
-  FlowSteps::calls(call.flow(), target) and
-  not target = call.getAnExpectedCallee()
+query predicate spuriousCallee(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
+  callEdge(call, target, boundArgs) and
+  not (
+    target = call.getAnExpectedCallee() and
+    boundArgs = call.getBoundArgsOrMinusOne()
+  )
 }
 
-query predicate missingCallee(AnnotatedCall call, AnnotatedFunction target) {
-  not FlowSteps::calls(call.flow(), target) and
-  target = call.getAnExpectedCallee()
+query predicate missingCallee(AnnotatedCall call, AnnotatedFunction target, int boundArgs) {
+  not callEdge(call, target, boundArgs) and
+  target = call.getAnExpectedCallee() and
+  boundArgs = call.getBoundArgsOrMinusOne()
 }
 
 query predicate badAnnotation(string name) {

javascript/ql/test/library-tests/CallGraphs/AnnotatedTest/bound-function.js

@@ -0,0 +1,31 @@
+var url = require('url')
+var http = require('http')
+
+function Mount () {}
+
+/** name:mount.serve */
+Mount.prototype.serve = function (x) {
+}
+
+function makeMount() {
+  var m = new Mount()
+  return m.serve.bind(m);
+}
+
+function makeMount2(x) {
+  var m = new Mount()
+  return m.serve.bind(m, x);
+}
+
+var mount = makeMount()
+var mount2 = makeMount2(null);
+
+http.createServer(function (req, res) {
+  /** calls:mount.serve */
+  /** boundArgs:0 */
+  mount(req, res)
+
+  /** calls:mount.serve */
+  /** boundArgs:1 */
+  mount2(res)
+});

javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected

@@ -22,6 +22,10 @@
 | partial.js:5:15:5:24 | "tainted1" | partial.js:15:15:15:15 | x |
 | partial.js:5:15:5:24 | "tainted1" | partial.js:21:15:21:15 | x |
 | partial.js:5:15:5:24 | "tainted1" | partial.js:27:15:27:15 | x |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:10:15:10:15 | y |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:16:15:16:15 | y |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:22:15:22:15 | y |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:28:15:28:15 | y |
 | promises.js:2:16:2:24 | "tainted" | promises.js:7:16:7:18 | val |
 | promises.js:2:16:2:24 | "tainted" | promises.js:38:32:38:32 | v |
 | promises.js:11:22:11:31 | "resolved" | promises.js:19:20:19:20 | v |

javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected

@@ -23,6 +23,10 @@
 | partial.js:5:15:5:24 | "tainted1" | partial.js:15:15:15:15 | x |
 | partial.js:5:15:5:24 | "tainted1" | partial.js:21:15:21:15 | x |
 | partial.js:5:15:5:24 | "tainted1" | partial.js:27:15:27:15 | x |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:10:15:10:15 | y |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:16:15:16:15 | y |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:22:15:22:15 | y |
+| partial.js:6:15:6:24 | "tainted2" | partial.js:28:15:28:15 | y |
 | promises.js:2:16:2:24 | "tainted" | promises.js:7:16:7:18 | val |
 | promises.js:2:16:2:24 | "tainted" | promises.js:38:32:38:32 | v |
 | promises.js:11:22:11:31 | "resolved" | promises.js:19:20:19:20 | v |