Merge pull request #768 from xiemaisi/js/call-summaries

Approved by asger-semmle
2026-05-01 19:55:15 +02:00 · 2019-01-16 08:35:31 +00:00
parent 06d7953167 0877ec845a
commit 8655e5ae17
12 changed files with 243 additions and 20 deletions
--- a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -471,7 +471,11 @@ private predicate exploratoryFlowStep(
 ) {
  basicFlowStep(pred, succ, _, cfg) or
  basicStoreStep(pred, succ, _) or
-  loadStep(pred, succ, _)
+  loadStep(pred, succ, _) or
+  // the following two disjuncts taken together over-approximate flow through
+  // higher-order calls
+  callback(pred, succ) or
+  succ = pred.(DataFlow::FunctionNode).getAParameter()
 }

 /**
@@ -536,10 +540,7 @@ private predicate callInputStep(
 ) {
  (
    isRelevant(pred, cfg) and
-    exists(Parameter parm |
-      argumentPassing(invk, pred, f, parm) and
-      succ = DataFlow::parameterNode(parm)
-    )
+    argumentPassing(invk, pred, f, succ)
    or
    isRelevant(pred, cfg) and
    exists(SsaDefinition prevDef, SsaDefinition def |
@@ -655,6 +656,57 @@ private predicate flowThroughProperty(
  )
 }

+/**
+ * Holds if `arg` and `cb` are passed as arguments to a function which in turn
+ * invokes `cb`, passing `arg` as its `i`th argument.
+ *
+ * All of this is done under configuration `cfg`, and `arg` flows along a path
+ * summarized by `summary`, while `cb` is only tracked locally.
+ */
+private predicate higherOrderCall(
+  DataFlow::Node arg, DataFlow::Node cb, int i, DataFlow::Configuration cfg, PathSummary summary
+) {
+  exists (Function f, DataFlow::InvokeNode outer, DataFlow::InvokeNode inner, int j,
+    DataFlow::Node innerArg, DataFlow::ParameterNode cbParm, PathSummary oldSummary |
+    reachableFromInput(f, outer, arg, innerArg, cfg, oldSummary) and
+    argumentPassing(outer, cb, f, cbParm) and
+    innerArg = inner.getArgument(j) |
+    // direct higher-order call
+    cbParm.flowsTo(inner.getCalleeNode()) and
+    i = j and
+    summary = oldSummary
+    or
+    // indirect higher-order call
+    exists (DataFlow::Node cbArg, PathSummary newSummary |
+      cbParm.flowsTo(cbArg) and
+      higherOrderCall(innerArg, cbArg, i, cfg, newSummary) and
+      summary = oldSummary.append(PathSummary::call()).append(newSummary)
+    )
+  )
+}
+
+/**
+ * Holds if `pred` is passed as an argument to a function `f` which also takes a
+ * callback parameter `cb` and then invokes `cb`, passing `pred` into parameter `succ`
+ * of `cb`.
+ *
+ * All of this is done under configuration `cfg`, and `arg` flows along a path
+ * summarized by `summary`, while `cb` is only tracked locally.
+ */
+private predicate flowIntoHigherOrderCall(
+  DataFlow::Node pred, DataFlow::Node succ, DataFlow::Configuration cfg, PathSummary summary
+) {
+  exists(
+    DataFlow::Node fArg, DataFlow::FunctionNode cb,
+    int i, PathSummary oldSummary
+  |
+    higherOrderCall(pred, fArg, i, cfg, oldSummary) and
+    cb = fArg.getALocalSource() and
+    succ = cb.getParameter(i) and
+    summary = oldSummary.append(PathSummary::call())
+  )
+}
+
 /**
 * Holds if there is a flow step from `pred` to `succ` described by `summary`
 * under configuration `cfg`.
@@ -671,6 +723,9 @@ private predicate flowStep(
    or
    // Flow through a property write/read pair
    flowThroughProperty(pred, succ, cfg, summary)
+    or
+    // Flow into higher-order call
+    flowIntoHigherOrderCall(pred, succ, cfg, summary)
  ) and
  not cfg.isBarrier(succ) and
  not cfg.isBarrier(pred, succ) and
--- a/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/Nodes.qll
@@ -17,6 +17,9 @@ class ParameterNode extends DataFlow::SourceNode {

  /** Gets the name of this parameter. */
  string getName() { result = p.getName() }
+
+  /** Holds if this parameter is a rest parameter. */
+  predicate isRestParameter() { p.isRestParameter() }
 }

 /** A data flow node corresponding to a function invocation (with or without `new`). */
--- a/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll
@@ -782,7 +782,7 @@ module TaintTracking {
   * A function that returns the result of a sanitizer check.
   */
  private class SanitizingFunction extends Function {
-    Parameter sanitizedParameter;
+    DataFlow::ParameterNode sanitizedParameter;

    SanitizerGuardNode sanitizer;

@@ -806,11 +806,11 @@ module TaintTracking {
          or
          returnExpr = getAReturnedExpr()
        ) and
-        DataFlow::parameterNode(sanitizedParameter).flowsToExpr(e) and
+        sanitizedParameter.flowsToExpr(e) and
        sanitizer.sanitizes(sanitizerOutcome, e)
      ) and
      getNumParameter() = 1 and
-      sanitizedParameter = getParameter(0)
+      sanitizedParameter.getParameter() = getParameter(0)
    }

    /**
--- a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll
@@ -100,6 +100,10 @@ private module NodeTracking {
      basicStoreStep(mid, nd, _)
      or
      loadStep(mid, nd, _)
+      or
+      callback(mid, nd)
+      or
+      nd = mid.(DataFlow::FunctionNode).getAParameter()
    )
  }

@@ -113,10 +117,7 @@ private module NodeTracking {
  ) {
    isRelevant(pred) and
    (
-      exists(Parameter parm |
-        argumentPassing(invk, pred, f, parm) and
-        succ = DataFlow::parameterNode(parm)
-      )
+      argumentPassing(invk, pred, f, succ)
      or
      exists(SsaDefinition prevDef, SsaDefinition def |
        pred = DataFlow::ssaDefinitionNode(prevDef) and
@@ -206,6 +207,53 @@ private module NodeTracking {
    )
  }

+  /**
+   * Holds if `arg` and `cb` are passed as arguments to a function which in turn
+   * invokes `cb`, passing `arg` as its `i`th argument. `arg` flows along a path summarized
+   * by `summary`, while `cb` is only tracked locally.
+   */
+  private predicate higherOrderCall(
+    DataFlow::Node arg, DataFlow::Node cb, int i, PathSummary summary
+  ) {
+    exists (Function f, DataFlow::InvokeNode outer, DataFlow::InvokeNode inner, int j,
+      DataFlow::Node innerArg, DataFlow::ParameterNode cbParm, PathSummary oldSummary |
+      reachableFromInput(f, outer, arg, innerArg, oldSummary) and
+      argumentPassing(outer, cb, f, cbParm) and
+      innerArg = inner.getArgument(j) |
+      // direct higher-order call
+      cbParm.flowsTo(inner.getCalleeNode()) and
+      i = j and
+      summary = oldSummary
+      or
+      // indirect higher-order call
+      exists (DataFlow::Node cbArg, PathSummary newSummary |
+        cbParm.flowsTo(cbArg) and
+        higherOrderCall(innerArg, cbArg, i, newSummary) and
+        summary = oldSummary.append(PathSummary::call()).append(newSummary)
+      )
+    )
+  }
+
+  /**
+   * Holds if `pred` is passed as an argument to a function `f` which also takes a
+   * callback parameter `cb` and then invokes `cb`, passing `pred` into parameter `succ`
+   * of `cb`. `arg` flows along a path summarized by `summary`, while `cb` is only tracked
+   * locally.
+   */
+  private predicate flowIntoHigherOrderCall(
+    DataFlow::Node pred, DataFlow::Node succ, PathSummary summary
+  ) {
+    exists(
+      DataFlow::Node fArg, DataFlow::FunctionNode cb,
+      int i, PathSummary oldSummary
+    |
+      higherOrderCall(pred, fArg, i, oldSummary) and
+      cb = fArg.getALocalSource() and
+      succ = cb.getParameter(i) and
+      summary = oldSummary.append(PathSummary::call())
+    )
+  }
+
  /**
   * Holds if there is a flow step from `pred` to `succ` described by `summary`.
   */
@@ -219,6 +267,9 @@ private module NodeTracking {
    or
    // Flow through a property write/read pair
    flowThroughProperty(pred, succ, summary)
+    or
+    // Flow into higher-order call
+    flowIntoHigherOrderCall(pred, succ, summary)
  }

  /**
--- a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
+++ b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
@@ -88,11 +88,11 @@ predicate localFlowStep(
 * through invocation `invk` of function `f`.
 */
 predicate argumentPassing(
-  DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, Parameter parm
+  DataFlow::InvokeNode invk, DataFlow::ValueNode arg, Function f, DataFlow::ParameterNode parm
 ) {
  calls(invk, f) and
  exists(int i |
-    f.getParameter(i) = parm and
+    f.getParameter(i) = parm.getParameter() and
    not parm.isRestParameter() and
    arg = invk.getArgument(i)
  )
@@ -100,7 +100,7 @@ predicate argumentPassing(
  exists(DataFlow::Node callback, int i |
    invk.(DataFlow::AdditionalPartialInvokeNode).isPartialArgument(callback, arg, i) and
    partiallyCalls(invk, callback, f) and
-    parm = f.getParameter(i) and
+    parm.getParameter() = f.getParameter(i) and
    not parm.isRestParameter()
  )
 }
@@ -110,10 +110,7 @@ predicate argumentPassing(
 * to a function call.
 */
 predicate callStep(DataFlow::Node pred, DataFlow::Node succ) {
-  exists(Parameter parm |
-    argumentPassing(_, pred, _, parm) and
-    succ = DataFlow::parameterNode(parm)
-  )
+  argumentPassing(_, pred, _, succ)
 }

 /**
@@ -239,6 +236,34 @@ predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
  succ.accesses(pred, prop)
 }

+/**
+ * Holds if there is a higher-order call with argument `arg`, and `cb` is the local
+ * source of an argument that flows into the callee position of that call:
+ *
+ * ```
+ * function f(x, g) {
+ *   g(
+ *     x                 // arg
+ *   );
+ * }
+ *
+ * function cb() {      // cb
+ * }
+ *
+ * f(arg, cb);
+ *
+ * This is an over-approximation of a possible data flow step through a callback
+ * invocation.
+ */
+predicate callback(DataFlow::Node arg, DataFlow::SourceNode cb) {
+  exists (DataFlow::InvokeNode invk, DataFlow::ParameterNode cbParm, DataFlow::Node cbArg |
+    arg = invk.getAnArgument() and
+    cbParm.flowsTo(invk.getCalleeNode()) and
+    callStep(cbArg, cbParm) and
+    cb.flowsTo(cbArg)
+  )
+}
+
 /**
 * A utility class that is equivalent to `boolean` but does not require type joining.
 */
--- a/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected
@@ -1,6 +1,8 @@
 | a.js:1:15:1:23 | "tainted" | b.js:4:13:4:40 | whoKnow ... Tainted |
 | a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
+| callback.js:16:14:16:21 | "source" | callback.js:13:14:13:14 | x |
+| callback.js:27:15:27:23 | "source3" | callback.js:13:14:13:14 | x |
 | destructuring.js:2:16:2:24 | "tainted" | destructuring.js:9:15:9:22 | tainted2 |
 | destructuring.js:19:15:19:23 | "tainted" | destructuring.js:14:15:14:15 | p |
 | destructuring.js:20:15:20:28 | "also tainted" | destructuring.js:15:15:15:15 | r |
--- a/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected
@@ -1,6 +1,8 @@
 | a.js:1:15:1:23 | "tainted" | b.js:4:13:4:40 | whoKnow ... Tainted |
 | a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
+| callback.js:16:14:16:21 | "source" | callback.js:13:14:13:14 | x |
+| callback.js:27:15:27:23 | "source3" | callback.js:13:14:13:14 | x |
 | custom.js:1:14:1:26 | "verschmutzt" | custom.js:2:15:2:20 | quelle |
 | destructuring.js:2:16:2:24 | "tainted" | destructuring.js:9:15:9:22 | tainted2 |
 | destructuring.js:19:15:19:23 | "tainted" | destructuring.js:14:15:14:15 | p |
--- a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected
@@ -1,6 +1,9 @@
 | a.js:1:15:1:23 | "tainted" | b.js:4:13:4:40 | whoKnow ... Tainted |
 | a.js:1:15:1:23 | "tainted" | b.js:6:13:6:13 | x |
 | a.js:2:15:2:28 | "also tainted" | b.js:5:13:5:29 | notTaintedTrustMe |
+| callback.js:16:14:16:21 | "source" | callback.js:13:14:13:14 | x |
+| callback.js:17:15:17:23 | "source2" | callback.js:13:14:13:14 | x |
+| callback.js:27:15:27:23 | "source3" | callback.js:13:14:13:14 | x |
 | destructuring.js:2:16:2:24 | "tainted" | destructuring.js:5:14:5:20 | tainted |
 | destructuring.js:2:16:2:24 | "tainted" | destructuring.js:9:15:9:22 | tainted2 |
 | destructuring.js:19:15:19:23 | "tainted" | destructuring.js:14:15:14:15 | p |
--- a/javascript/ql/test/library-tests/InterProceduralFlow/callback.js
+++ b/javascript/ql/test/library-tests/InterProceduralFlow/callback.js
@@ -0,0 +1,31 @@
+function call(f, x) {
+  return f(x);
+}
+
+function map(f, xs) {
+  const res = [];
+  for (let i=0;i<xs.length;++i)
+    res[i] = f(xs[i]);
+  return res;
+}
+
+function store(x) {
+  let sink = x;
+}
+
+let source = "source";
+let source2 = "source2";
+call(store, source);
+call(store, confounder); // call with different argument to make sure the call graph builder
+                         // doesn't resolve the call on line 2 for us
+map(store, [source2]);
+
+function call2(x, f) {
+  call(f, x);
+}
+
+let source3 = "source3";
+call2(source3, store);
+call2(source3, confounder);
+
+// semmle-extractor-options: --source-type module
--- a/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-078/CommandInjection.expected
@@ -38,6 +38,21 @@ nodes
 | child_process-test.js:55:19:55:22 | args |
 | child_process-test.js:56:12:56:14 | cmd |
 | child_process-test.js:56:17:56:20 | args |
+| execSeries.js:3:20:3:22 | arr |
+| execSeries.js:5:4:5:3 | arr |
+| execSeries.js:6:14:6:16 | arr |
+| execSeries.js:6:14:6:21 | arr[i++] |
+| execSeries.js:13:19:13:26 | commands |
+| execSeries.js:14:13:14:20 | commands |
+| execSeries.js:14:24:14:30 | command |
+| execSeries.js:14:41:14:47 | command |
+| execSeries.js:18:7:18:58 | cmd |
+| execSeries.js:18:13:18:47 | require ... , true) |
+| execSeries.js:18:13:18:53 | require ... ).query |
+| execSeries.js:18:13:18:58 | require ... ry.path |
+| execSeries.js:18:34:18:40 | req.url |
+| execSeries.js:19:12:19:16 | [cmd] |
+| execSeries.js:19:13:19:15 | cmd |
 edges
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:17:13:17:15 | cmd |
 | child_process-test.js:6:9:6:49 | cmd | child_process-test.js:18:17:18:19 | cmd |
@@ -70,6 +85,21 @@ edges
 | child_process-test.js:48:16:48:17 | [] | child_process-test.js:48:9:48:17 | args |
 | child_process-test.js:55:14:55:16 | cmd | child_process-test.js:56:12:56:14 | cmd |
 | child_process-test.js:55:19:55:22 | args | child_process-test.js:56:17:56:20 | args |
+| execSeries.js:3:20:3:22 | arr | execSeries.js:5:4:5:3 | arr |
+| execSeries.js:5:4:5:3 | arr | execSeries.js:6:14:6:16 | arr |
+| execSeries.js:6:14:6:16 | arr | execSeries.js:6:14:6:21 | arr[i++] |
+| execSeries.js:6:14:6:21 | arr[i++] | execSeries.js:14:24:14:30 | command |
+| execSeries.js:13:19:13:26 | commands | execSeries.js:14:13:14:20 | commands |
+| execSeries.js:14:13:14:20 | commands | execSeries.js:3:20:3:22 | arr |
+| execSeries.js:14:13:14:20 | commands | execSeries.js:14:24:14:30 | command |
+| execSeries.js:14:24:14:30 | command | execSeries.js:14:41:14:47 | command |
+| execSeries.js:18:7:18:58 | cmd | execSeries.js:19:13:19:15 | cmd |
+| execSeries.js:18:13:18:47 | require ... , true) | execSeries.js:18:13:18:53 | require ... ).query |
+| execSeries.js:18:13:18:53 | require ... ).query | execSeries.js:18:13:18:58 | require ... ry.path |
+| execSeries.js:18:13:18:58 | require ... ry.path | execSeries.js:18:7:18:58 | cmd |
+| execSeries.js:18:34:18:40 | req.url | execSeries.js:18:13:18:47 | require ... , true) |
+| execSeries.js:19:12:19:16 | [cmd] | execSeries.js:13:19:13:26 | commands |
+| execSeries.js:19:13:19:15 | cmd | execSeries.js:19:12:19:16 | [cmd] |
 #select
 | child_process-test.js:17:13:17:15 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:17:13:17:15 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:18:17:18:19 | cmd | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:18:17:18:19 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
@@ -83,3 +113,4 @@ edges
 | child_process-test.js:44:5:44:34 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:51:5:51:39 | cp.exec ... , args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:50:15:50:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
 | child_process-test.js:56:3:56:21 | cp.spawn(cmd, args) | child_process-test.js:6:25:6:31 | req.url | child_process-test.js:43:15:43:17 | cmd | This command depends on $@. | child_process-test.js:6:25:6:31 | req.url | a user-provided value |
+| execSeries.js:14:41:14:47 | command | execSeries.js:18:34:18:40 | req.url | execSeries.js:14:41:14:47 | command | This command depends on $@. | execSeries.js:18:34:18:40 | req.url | a user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-078/execSeries.js
+++ b/javascript/ql/test/query-tests/Security/CWE-078/execSeries.js
@@ -0,0 +1,20 @@
+var exec = require('child_process').exec;
+
+function asyncEach(arr, iterator) {
+  var i = 0;
+  (function iterate() {
+    iterator(arr[i++], function () {
+      if (i < arr.length)
+        process.nextTick(iterate);
+    });
+  })();
+}
+
+function execEach(commands) {
+  asyncEach(commands, (command) => exec(command));
+};
+
+require('http').createServer(function(req, res) {
+  let cmd = require('url').parse(req.url, true).query.path;
+  execEach([cmd]);  // NOT OK
+});