Merge pull request #1288 from xiemaisi/js/fix-end-node-labels

Approved by asger-semmle

Author: semmle-qlci

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

@@ -557,7 +557,8 @@ private predicate callInputStep(
 /**
  * Holds if `input`, which is either an argument to `f` at `invk` or a definition
  * that is captured by `f`, may flow to `nd` under configuration `cfg` (possibly through
- * callees) along a path summarized by `summary`.
+ * callees, but not containing any unmatched calls or returns) along a path summarized by
+ * `summary`.
  *
  * Note that the summary does not take the initial step from argument to parameter
  * into account.
@@ -577,7 +578,7 @@ private predicate reachableFromInput(
 }
 
 /**
- * Holds if there is a step from `pred` to `succ` under `cfg` that can be appended
+ * Holds if there is a level step from `pred` to `succ` under `cfg` that can be appended
  * to a path represented by `oldSummary` yielding a path represented by `newSummary`.
  */
 pragma[noinline]
@@ -587,6 +588,7 @@ private predicate appendStep(
 ) {
   exists(PathSummary stepSummary |
     flowStep(pred, cfg, succ, stepSummary) and
+    stepSummary.isLevel() and
     newSummary = oldSummary.append(stepSummary)
   )
 }
@@ -819,7 +821,7 @@ private predicate reachableFromSource(
     isSource(nd, cfg, lbl) and
     not cfg.isBarrier(nd) and
     not cfg.isLabeledBarrier(nd, lbl) and
-    summary = MkPathSummary(false, false, lbl, lbl)
+    summary = PathSummary::level(lbl)
   )
   or
   exists(DataFlow::Node pred, PathSummary oldSummary, PathSummary newSummary |
@@ -952,14 +954,19 @@ class PathNode extends TPathNode {
  * A path node corresponding to a flow source.
  */
 class SourcePathNode extends PathNode {
-  SourcePathNode() { isSource(nd, cfg, _) }
+  SourcePathNode() {
+    exists(FlowLabel lbl |
+      summary = PathSummary::level(lbl) and
+      isSource(nd, cfg, lbl)
+    )
+  }
 }
 
 /**
  * A path node corresponding to a flow sink.
  */
 class SinkPathNode extends PathNode {
-  SinkPathNode() { isSink(nd, cfg, _) }
+  SinkPathNode() { isSink(nd, cfg, summary.getEndLabel()) }
 }
 
 /**

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

@@ -131,8 +131,8 @@ private module NodeTracking {
 
   /**
    * Holds if `input`, which is either an argument to `f` at `invk` or a definition
-   * that is captured by `f`, may flow to `nd` (possibly through callees) along
-   * a path summarized by `summary`.
+   * that is captured by `f`, may flow to `nd` (possibly through callees, but not containing
+   * any unmatched calls or returns) along a path summarized by `summary`.
    */
   private predicate reachableFromInput(
     Function f, DataFlow::Node invk, DataFlow::Node input, DataFlow::Node nd, PathSummary summary
@@ -143,6 +143,7 @@ private module NodeTracking {
     exists(DataFlow::Node mid, PathSummary oldSummary, PathSummary newSummary |
       reachableFromInput(f, invk, input, mid, oldSummary) and
       flowStep(mid, nd, newSummary) and
+      newSummary.isLevel() and
       summary = oldSummary.append(newSummary)
     )
   }

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

@@ -340,12 +340,17 @@ class PathSummary extends TPathSummary {
 
   PathSummary() { this = MkPathSummary(hasReturn, hasCall, start, end) }
 
-  /** Indicates whether the path represented by this summary contains any return steps. */
+  /** Indicates whether the path represented by this summary contains any unmatched return steps. */
   boolean hasReturn() { result = hasReturn }
 
-  /** Indicates whether the path represented by this summary contains any call steps. */
+  /** Indicates whether the path represented by this summary contains any unmatched call steps. */
   boolean hasCall() { result = hasCall }
 
+  /** Holds if the path represented by this summary contains no unmatched call or return steps. */
+  predicate isLevel() {
+    hasReturn = false and hasCall = false
+  }
+
   /** Gets the flow label describing the value at the start of this flow path. */
   FlowLabel getStartLabel() { result = start }
 
@@ -406,7 +411,13 @@ module PathSummary {
   /**
    * Gets a summary describing a path without any calls or returns.
    */
-  PathSummary level() { exists(FlowLabel lbl | result = MkPathSummary(false, false, lbl, lbl)) }
+  PathSummary level() { result = level(_) }
+
+  /**
+   * Gets a summary describing a path without any calls or returns, transforming `lbl` into
+   * itself.
+   */
+  PathSummary level(FlowLabel lbl) { result = MkPathSummary(false, false, lbl, lbl) }
 
   /**
    * Gets a summary describing a path with one or more calls, but no returns.

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

@@ -0,0 +1,2 @@
+| tst5.mjs:13:12:13:20 | source(0) | tst5.mjs:18:6:18:9 | flow |
+| tst5.mjs:15:8:15:19 | source(flow) | tst5.mjs:16:13:16:16 | flow |

javascript/ql/test/library-tests/InterProceduralFlow/FlowLabels.ql

@@ -0,0 +1,33 @@
+import javascript
+
+class Parity extends DataFlow::FlowLabel {
+  Parity() { this = "even" or this = "odd" }
+
+  Parity flip() { result != this }
+}
+
+class Config extends DataFlow::Configuration {
+  Config() { this = "config" }
+
+  override predicate isSource(DataFlow::Node nd, DataFlow::FlowLabel lbl) {
+    nd.(DataFlow::CallNode).getCalleeName() = "source" and
+    lbl = "even"
+  }
+
+  override predicate isSink(DataFlow::Node nd, DataFlow::FlowLabel lbl) {
+    nd = any(DataFlow::CallNode c | c.getCalleeName() = "sink").getAnArgument() and
+    lbl = "even"
+  }
+
+  override predicate isAdditionalFlowStep(DataFlow::Node pred,  DataFlow::Node succ, DataFlow::FlowLabel predLabel, DataFlow::FlowLabel succLabel) {
+    exists(DataFlow::CallNode c | c = succ |
+      c.getCalleeName() = "inc" and
+      pred = c.getAnArgument() and
+      succLabel = predLabel.(Parity).flip()
+    )
+  }
+}
+
+from Config cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select source, sink

javascript/ql/test/library-tests/InterProceduralFlow/tst5.mjs

@@ -0,0 +1,18 @@
+function source(x) {
+  return x;
+}
+
+function sink(x) {
+  return x;
+}
+
+function inc(x) {
+  return x+1;
+}
+
+var flow = source(0);  // source
+flow = inc(flow);
+flow = source(flow);   // source
+flow = sink(flow);     // sink for line 15, but not for line 13 (wrong parity)
+flow = inc(flow);
+sink(flow);            // sink for line 13, but not for line 15 (wrong parity)

javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected

@@ -78,14 +78,12 @@ edges
 | tst.js:6:34:6:55 | documen ... on.href | tst.js:6:20:6:56 | indirec ... n.href) |
 #select
 | tst2.js:4:21:4:55 | href.su ... '?')+1) | tst2.js:2:14:2:28 | window.location | tst2.js:4:21:4:55 | href.su ... '?')+1) | Untrusted URL redirection due to $@. | tst2.js:2:14:2:28 | window.location | user-provided value |
-| tst2.js:4:21:4:55 | href.su ... '?')+1) | tst2.js:2:14:2:28 | window.location | tst2.js:4:21:4:55 | href.su ... '?')+1) | Untrusted URL redirection due to $@. | tst2.js:2:14:2:28 | window.location | user-provided value |
 | tst6.js:4:21:4:28 | redirect | tst6.js:2:18:2:45 | $locati ... irect') | tst6.js:4:21:4:28 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
 | tst6.js:6:17:6:24 | redirect | tst6.js:2:18:2:45 | $locati ... irect') | tst6.js:6:17:6:24 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
 | tst6.js:8:21:8:56 | $locati ... + "foo" | tst6.js:8:21:8:48 | $locati ... irect') | tst6.js:8:21:8:56 | $locati ... + "foo" | Untrusted URL redirection due to $@. | tst6.js:8:21:8:48 | $locati ... irect') | user-provided value |
 | tst7.js:2:12:2:35 | documen ... .search | tst7.js:2:12:2:28 | document.location | tst7.js:2:12:2:35 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:2:12:2:28 | document.location | user-provided value |
 | tst7.js:5:27:5:50 | documen ... .search | tst7.js:5:27:5:43 | document.location | tst7.js:5:27:5:50 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:5:27:5:43 | document.location | user-provided value |
 | tst9.js:2:21:2:55 | documen ... ring(1) | tst9.js:2:21:2:37 | document.location | tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:37 | document.location | user-provided value |
-| tst9.js:2:21:2:55 | documen ... ring(1) | tst9.js:2:21:2:37 | document.location | tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:37 | document.location | user-provided value |
 | tst10.js:5:17:5:46 | '/' + d ... .search | tst10.js:5:23:5:39 | document.location | tst10.js:5:17:5:46 | '/' + d ... .search | Untrusted URL redirection due to $@. | tst10.js:5:23:5:39 | document.location | user-provided value |
 | tst10.js:8:17:8:47 | '//' +  ... .search | tst10.js:8:24:8:40 | document.location | tst10.js:8:17:8:47 | '//' +  ... .search | Untrusted URL redirection due to $@. | tst10.js:8:24:8:40 | document.location | user-provided value |
 | tst10.js:11:17:11:50 | '//foo' ... .search | tst10.js:11:27:11:43 | document.location | tst10.js:11:17:11:50 | '//foo' ... .search | Untrusted URL redirection due to $@. | tst10.js:11:27:11:43 | document.location | user-provided value |