JavaScript: Track flow through forwarding higher-order calls.

Max Schaefer · Max Schaefer · commit 7d2d33840aee · 2019-01-11T09:15:58.000Z
diff --git a/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll b/javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
@@ -442,7 +442,8 @@ private predicate exploratoryFlowStep(
   basicFlowStep(pred, succ, _, cfg) or
   basicStoreStep(pred, succ, _) or
   loadStep(pred, succ, _) or
-  approximateCallbackStep(pred, succ)
+  approximateCallbackStep(pred, succ) or
+  succ = pred.(DataFlow::FunctionNode).getAParameter()
 }
 
 /**
@@ -628,9 +629,20 @@ private predicate flowThroughProperty(
 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 |
-    reachableFromInput(f, outer, arg, inner.getArgument(i), cfg, summary) and
-    argumentPassing(outer, cb, f, inner.getCalleeNode().getALocalSource())
+  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) |
+    cbParm.flowsTo(inner.getCalleeNode()) and
+    i = j and
+    summary = oldSummary
+    or
+    exists (DataFlow::Node cbArg, PathSummary newSummary |
+      cbParm.flowsTo(cbArg) and
+      higherOrderCall(innerArg, cbArg, i, cfg, newSummary) and
+      summary = oldSummary.append(PathSummary::call()).append(newSummary)
+    )
   )
 }
 
diff --git a/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll b/javascript/ql/src/semmle/javascript/dataflow/TrackedNodes.qll
@@ -102,6 +102,8 @@ private module NodeTracking {
       loadStep(mid, nd, _)
       or
       approximateCallbackStep(mid, nd)
+      or
+      nd = mid.(DataFlow::FunctionNode).getAParameter()
     )
   }
 
@@ -213,9 +215,20 @@ private module NodeTracking {
   private predicate higherOrderCall(
     DataFlow::Node arg, DataFlow::Node cb, int i, PathSummary summary
   ) {
-    exists (Function f, DataFlow::InvokeNode outer, DataFlow::InvokeNode inner |
-      reachableFromInput(f, outer, arg, inner.getArgument(i), summary) and
-      argumentPassing(outer, cb, f, inner.getCalleeNode().getALocalSource())
+    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) |
+      cbParm.flowsTo(inner.getCalleeNode()) and
+      i = j and
+      summary = oldSummary
+      or
+      exists (DataFlow::Node cbArg, PathSummary newSummary |
+        cbParm.flowsTo(cbArg) and
+        higherOrderCall(innerArg, cbArg, i, newSummary) and
+        summary = oldSummary.append(PathSummary::call()).append(newSummary)
+      )
     )
   }
 
diff --git a/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll b/javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll
@@ -236,17 +236,17 @@ predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {
 }
 
 /**
- * Holds if there is a call with arguments `cb` and `pred`, and `succ` is
- * a parameter of a function that may flow into `cb`.
+ * Holds if there is a call with argument `pred`, and `succ` flows into the callee
+ * position of that call.
  *
  * This is an over-approximation of a possible data flow step through a callback
  * invocation.
  */
-predicate approximateCallbackStep(DataFlow::Node pred, DataFlow::Node succ) {
-  exists (DataFlow::InvokeNode invk, DataFlow::FunctionNode cb |
+predicate approximateCallbackStep(DataFlow::Node pred, DataFlow::SourceNode succ) {
+  exists (DataFlow::InvokeNode invk, DataFlow::ParameterNode cb |
     pred = invk.getAnArgument() and
-    cb.flowsTo(invk.getAnArgument()) and
-    succ = cb.getAParameter()
+    cb.flowsTo(invk.getCalleeNode()) and
+    callStep(any(DataFlow::Node nd | succ.flowsTo(nd)), cb)
   )
 }
 
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/DataFlow.expected
@@ -2,6 +2,7 @@
 | 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 |
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected b/javascript/ql/test/library-tests/InterProceduralFlow/GermanFlow.expected
@@ -2,6 +2,7 @@
 | 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 |
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected b/javascript/ql/test/library-tests/InterProceduralFlow/TaintTracking.expected
@@ -3,6 +3,7 @@
 | 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 |
diff --git a/javascript/ql/test/library-tests/InterProceduralFlow/callback.js b/javascript/ql/test/library-tests/InterProceduralFlow/callback.js
@@ -20,4 +20,12 @@ call(store, confounder); // call with different argument to make sure the call g
                          // 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

Original file line number	Diff line number	Diff line change
`@@ -236,17 +236,17 @@ predicate loadStep(DataFlow::Node pred, DataFlow::PropRead succ, string prop) {`
`236`	`236`	`}`
`237`	`237`
`238`	`238`	`/**`
`239`		- * Holds if there is a call with arguments `cb` and `pred`, and `succ` is
`240`		- * a parameter of a function that may flow into `cb`.
	`239`	+ * Holds if there is a call with argument `pred`, and `succ` flows into the callee
	`240`	`+ * position of that call.`
`241`	`241`	`*`
`242`	`242`	`* This is an over-approximation of a possible data flow step through a callback`
`243`	`243`	`* invocation.`
`244`	`244`	`*/`
`245`		`-predicate approximateCallbackStep(DataFlow::Node pred, DataFlow::Node succ) {`
`246`		`- exists (DataFlow::InvokeNode invk, DataFlow::FunctionNode cb \|`
	`245`	`+predicate approximateCallbackStep(DataFlow::Node pred, DataFlow::SourceNode succ) {`
	`246`	`+ exists (DataFlow::InvokeNode invk, DataFlow::ParameterNode cb \|`
`247`	`247`	`pred = invk.getAnArgument() and`
`248`		`- cb.flowsTo(invk.getAnArgument()) and`
`249`		`- succ = cb.getAParameter()`
	`248`	`+ cb.flowsTo(invk.getCalleeNode()) and`
	`249`	`+ callStep(any(DataFlow::Node nd \| succ.flowsTo(nd)), cb)`
`250`	`250`	`)`
`251`	`251`	`}`
`252`	`252`