Merge pull request #2201 from max-schaefer/js/avoid-duplicate-source-and-sink-nodes

Approved by asger-semmle

Author: semmle-qlci

javascript/ql/src/Security/Summaries/ExtractFlowStepSummaries.ql

@@ -14,17 +14,19 @@ import PortalExitSource
 import PortalEntrySink
 
 from
-  TaintTracking::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Portal p1,
-  Portal p2, DataFlow::FlowLabel lbl1, DataFlow::FlowLabel lbl2
+  TaintTracking::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink, Portal p1,
+  Portal p2, DataFlow::FlowLabel lbl1, DataFlow::FlowLabel lbl2, DataFlow::MidPathNode last
 where
-  cfg.hasFlowPath(source, sink) and
+  cfg = source.getConfiguration() and
+  last = source.getASuccessor*() and
+  sink = last.getASuccessor() and
   p1 = source.getNode().(PortalExitSource).getPortal() and
   p2 = sink.getNode().(PortalEntrySink).getPortal() and
-  lbl1 = sink.getPathSummary().getStartLabel() and
-  lbl2 = sink.getPathSummary().getEndLabel() and
+  lbl1 = last.getPathSummary().getStartLabel() and
+  lbl2 = last.getPathSummary().getEndLabel() and
   // avoid constructing infeasible paths
-  sink.getPathSummary().hasCall() = false and
-  sink.getPathSummary().hasReturn() = false and
+  last.getPathSummary().hasCall() = false and
+  last.getPathSummary().hasReturn() = false and
   // restrict to steps flow function parameters to returns
   p1.(ParameterPortal).getBasePortal() = p2.(ReturnPortal).getBasePortal() and
   // restrict to data/taint flow

javascript/ql/src/Security/Summaries/ExtractSinkSummaries.ql

@@ -11,10 +11,13 @@ import Configurations
 import PortalExitSource
 import SinkFromAnnotation
 
-from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Portal p
+from DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+  Portal p, DataFlow::MidPathNode last
 where
-  cfg.hasFlowPath(source, sink) and
+  cfg = source.getConfiguration() and
+  last = source.getASuccessor*() and
+  sink = last.getASuccessor() and
   p = source.getNode().(PortalExitSource).getPortal() and
   // avoid constructing infeasible paths
-  sink.getPathSummary().hasReturn() = false
-select p.toString(), source.getPathSummary().getStartLabel().toString(), cfg.toString()
+  last.getPathSummary().hasReturn() = false
+select p.toString(), last.getPathSummary().getStartLabel().toString(), cfg.toString()

javascript/ql/src/Security/Summaries/ExtractSourceSummaries.ql

@@ -11,10 +11,13 @@ import Configurations
 import PortalEntrySink
 import SourceFromAnnotation
 
-from DataFlow::Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, Portal p
+from DataFlow::Configuration cfg, DataFlow::SourcePathNode source, DataFlow::SinkPathNode sink,
+  Portal p, DataFlow::MidPathNode last
 where
-  cfg.hasFlowPath(source, sink) and
+  cfg = source.getConfiguration() and
+  last = source.getASuccessor*() and
+  sink = last.getASuccessor() and
   p = sink.getNode().(PortalEntrySink).getPortal() and
   // avoid constructing infeasible paths
-  sink.getPathSummary().hasCall() = false
-select p.toString(), sink.getPathSummary().getEndLabel().toString(), cfg.toString()
+  last.getPathSummary().hasCall() = false
+select p.toString(), last.getPathSummary().getEndLabel().toString(), cfg.toString()

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

@@ -454,6 +454,7 @@ private class FlowStepThroughImport extends AdditionalFlowStep, DataFlow::ValueN
 private predicate basicFlowStep(
   DataFlow::Node pred, DataFlow::Node succ, PathSummary summary, DataFlow::Configuration cfg
 ) {
+  isLive() and
   isRelevantForward(pred, cfg) and
   (
     // Local flow
@@ -888,9 +889,9 @@ private predicate flowsTo(
   PathNode flowsource, DataFlow::Node source, SinkPathNode flowsink, DataFlow::Node sink,
   DataFlow::Configuration cfg
 ) {
-  flowsource = MkPathNode(source, cfg, _) and
+  flowsource.wraps(source, cfg) and
   flowsink = flowsource.getASuccessor*() and
-  flowsink = MkPathNode(sink, id(cfg), _)
+  flowsink.wraps(sink, id(cfg))
 }
 
 /**
@@ -933,19 +934,45 @@ private predicate onPath(DataFlow::Node nd, DataFlow::Configuration cfg, PathSum
 }
 
 /**
- * Holds if `cfg` has at least one source and at least one sink.
+ * Holds if there is a configuration that has at least one source and at least one sink.
  */
 pragma[noinline]
-private predicate isLive(DataFlow::Configuration cfg) { isSource(_, cfg, _) and isSink(_, cfg, _) }
+private predicate isLive() { exists(DataFlow::Configuration cfg | isSource(_, cfg, _) and isSink(_, cfg, _)) }
 
 /**
  * A data flow node on an inter-procedural path from a source.
  */
 private newtype TPathNode =
-  MkPathNode(DataFlow::Node nd, DataFlow::Configuration cfg, PathSummary summary) {
-    isLive(cfg) and
+  MkSourceNode(DataFlow::Node nd, DataFlow::Configuration cfg) { isSourceNode(nd, cfg, _) }
+  or
+  MkMidNode(DataFlow::Node nd, DataFlow::Configuration cfg, PathSummary summary) {
+    isLive() and
     onPath(nd, cfg, summary)
   }
+  or
+  MkSinkNode(DataFlow::Node nd, DataFlow::Configuration cfg) { isSinkNode(nd, cfg, _) }
+
+/**
+ * Holds if `nd` is a source node for configuration `cfg`, and there is a path from `nd` to a sink
+ * with the given `summary`.
+ */
+private predicate isSourceNode(DataFlow::Node nd, DataFlow::Configuration cfg, PathSummary summary) {
+  exists(FlowLabel lbl | summary = PathSummary::level(lbl) |
+    isSource(nd, cfg, lbl) and
+    isLive() and
+    onPath(nd, cfg, summary)
+  )
+}
+
+/**
+ * Holds if `nd` is a sink node for configuration `cfg`, and there is a path from a source to `nd`
+ * with the given `summary`.
+ */
+private predicate isSinkNode(DataFlow::Node nd, DataFlow::Configuration cfg, PathSummary summary) {
+  isSink(nd, cfg, summary.getEndLabel()) and
+  isLive() and
+  onPath(nd, cfg, summary)
+}
 
 /**
  * Maps `cfg` to itself.
@@ -957,48 +984,46 @@ bindingset[cfg, result]
 private DataFlow::Configuration id(DataFlow::Configuration cfg) { result >= cfg and cfg >= result }
 
 /**
- * A data flow node on an inter-procedural path from a source to a sink.
+ * A data-flow node on an inter-procedural path from a source to a sink.
+ *
+ * A path node wraps a data-flow node `nd` and a data-flow configuration `cfg` such that `nd` is
+ * on a path from a source to a sink under `cfg`.
  *
- * A path node is a triple `(nd, cfg, summary)` where `nd` is a data flow node and `cfg`
- * is a data flow tracking configuration such that `nd` is on a path from a source to a
- * sink under `cfg` summarized by `summary`.
+ * There are three kinds of path nodes:
+ *
+ *  - source nodes: wrapping a source node and a configuration such that there is a path from that
+ *    source to some sink under the configuration;
+ *  - sink nodes: wrapping a sink node and a configuration such that there is a path from some source
+ *    to that sink under the configuration;
+ *  - mid nodes: wrapping a node, a configuration and a path summary such that there is a path from
+ *    some source to the node with the given summary that can be extended to a path to some sink node,
+ *    all under the configuration.
  */
 class PathNode extends TPathNode {
   DataFlow::Node nd;
-  DataFlow::Configuration cfg;
-  PathSummary summary;
+  Configuration cfg;
 
-  PathNode() { this = MkPathNode(nd, cfg, summary) }
+  PathNode() {
+    this = MkSourceNode(nd, cfg) or
+    this = MkMidNode(nd, cfg, _) or
+    this = MkSinkNode(nd, cfg)
+  }
 
-  /** Gets the underlying data flow node of this path node. */
-  DataFlow::Node getNode() { result = nd }
+  /** Holds if this path node wraps data-flow node `nd` and configuration `c`. */
+  predicate wraps(DataFlow::Node n, DataFlow::Configuration c) {
+    nd = n and cfg = c
+  }
 
-  /** Gets the underlying data flow tracking configuration of this path node. */
+  /** Gets the underlying configuration of this path node. */
   DataFlow::Configuration getConfiguration() { result = cfg }
 
-  /** Gets the summary of the path underlying this path node. */
-  PathSummary getPathSummary() { result = summary }
-
-  /**
-   * Gets a successor node of this path node, including hidden nodes.
-   */
-  private PathNode getASuccessorInternal() {
-    exists(DataFlow::Node succ, PathSummary newSummary |
-      flowStep(nd, id(cfg), succ, newSummary) and
-      result = MkPathNode(succ, id(cfg), summary.append(newSummary))
-    )
-  }
-
-  /**
-   * Gets a successor of this path node, if it is a hidden node.
-   */
-  private PathNode getAHiddenSuccessor() {
-    isHidden() and
-    result = getASuccessorInternal()
-  }
+  /** Gets the underlying data-flow node of this path node. */
+  DataFlow::Node getNode() { result = nd }
 
   /** Gets a successor node of this path node. */
-  PathNode getASuccessor() { result = getASuccessorInternal().getAHiddenSuccessor*() }
+  final PathNode getASuccessor() {
+    result = getASuccessor(this)
+  }
 
   /** Gets a textual representation of this path node. */
   string toString() { result = nd.toString() }
@@ -1015,6 +1040,68 @@ class PathNode extends TPathNode {
   ) {
     nd.hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
+}
+
+/** Gets the mid node corresponding to `src`. */
+private MidPathNode initialMidNode(SourcePathNode src) {
+  exists(DataFlow::Node nd, Configuration cfg, PathSummary summary |
+    result.wraps(nd, cfg, summary) and
+    src = MkSourceNode(nd, cfg) and
+    isSourceNode(nd, cfg, summary)
+  )
+}
+
+/** Gets the mid node corresponding to `snk`. */
+private MidPathNode finalMidNode(SinkPathNode snk) {
+  exists(DataFlow::Node nd, Configuration cfg, PathSummary summary |
+    result.wraps(nd, cfg, summary) and
+    snk = MkSinkNode(nd, cfg) and
+    isSinkNode(nd, cfg, summary)
+  )
+}
+
+/**
+ * Gets a node to which data from `nd` may flow in one step.
+ */
+private PathNode getASuccessor(PathNode nd) {
+  // source node to mid node
+  result = initialMidNode(nd)
+  or
+  // mid node to mid node
+  exists(Configuration cfg, DataFlow::Node predNd, PathSummary summary, DataFlow::Node succNd, PathSummary newSummary |
+    nd = MkMidNode(predNd, cfg, summary) and
+    flowStep(predNd, id(cfg), succNd, newSummary) and
+    result = MkMidNode(succNd, id(cfg), summary.append(newSummary))
+  )
+  or
+  // mid node to sink node
+  nd = finalMidNode(result)
+}
+
+private PathNode getASuccessorIfHidden(PathNode nd) {
+  nd.(MidPathNode).isHidden() and
+  result = getASuccessor(nd)
+}
+
+/**
+ * A path node corresponding to an intermediate node on a path from a source to a sink.
+ *
+ * A mid node is a triple `(nd, cfg, summary)` where `nd` is a data-flow node and `cfg`
+ * is a configuration such that `nd` is on a path from a source to a sink under `cfg`
+ * summarized by `summary`.
+ */
+class MidPathNode extends PathNode, MkMidNode {
+  PathSummary summary;
+
+  MidPathNode() { this = MkMidNode(nd, cfg, summary) }
+
+  /** Gets the summary of the path underlying this path node. */
+  PathSummary getPathSummary() { result = summary }
+
+  /** Holds if this path node wraps data-flow node `nd`, configuration `c` and summary `s`. */
+  predicate wraps(DataFlow::Node n, DataFlow::Configuration c, PathSummary s) {
+    nd = n and cfg = c and summary = s
+  }
 
   /**
    * Holds if this node is hidden from paths in path explanation queries, except
@@ -1034,20 +1121,15 @@ class PathNode extends TPathNode {
 /**
  * A path node corresponding to a flow source.
  */
-class SourcePathNode extends PathNode {
-  SourcePathNode() {
-    exists(FlowLabel lbl |
-      summary = PathSummary::level(lbl) and
-      isSource(nd, cfg, lbl)
-    )
-  }
+class SourcePathNode extends PathNode, MkSourceNode {
+  SourcePathNode() { this = MkSourceNode(nd, cfg) }
 }
 
 /**
  * A path node corresponding to a flow sink.
  */
-class SinkPathNode extends PathNode {
-  SinkPathNode() { isSink(nd, cfg, summary.getEndLabel()) }
+class SinkPathNode extends PathNode, MkSinkNode {
+  SinkPathNode() { this = MkSinkNode(nd, cfg) }
 }
 
 /**
@@ -1056,11 +1138,51 @@ class SinkPathNode extends PathNode {
 module PathGraph {
   /** Holds if `nd` is a node in the graph of data flow path explanations. */
   query predicate nodes(PathNode nd) {
-    not nd.isHidden() or
-    nd instanceof SourcePathNode or
-    nd instanceof SinkPathNode
+    not nd.(MidPathNode).isHidden()
+  }
+
+  /**
+   * Gets a node to which data from `nd` may flow in one step, skipping over hidden nodes.
+   */
+  private PathNode succ0(PathNode nd) {
+    result = getASuccessorIfHidden*(nd.getASuccessor()) and
+    // skip hidden nodes
+    nodes(nd) and nodes(result)
+  }
+
+  /**
+   * Gets a node to which data from `nd` may flow in one step, where outgoing edges from intermediate
+   * nodes are merged with any incoming edge from a corresponding source node.
+   *
+   * For example, assume that `src` is a source node for `nd1`, which has `nd2` as its direct
+   * successor. Then `succ0` will yield two edges `src` → `nd1` and `nd1` → `nd2`,
+   * to which `succ1` will add the edge `src` → `nd2`.
+   */
+  private PathNode succ1(PathNode nd) {
+    result = succ0(nd)
+    or
+    result = succ0(initialMidNode(nd))
+  }
+
+  /**
+   * Gets a node to which data from `nd` may flow in one step, where incoming edges into intermediate
+   * nodes are merged with any outgoing edge to a corresponding sink node.
+   *
+   * For example, assume that `snk` is a source node for `nd2`, which has `nd1` as its direct
+   * predecessor. Then `succ1` will yield two edges `nd1` → `nd2` and `nd2` → `snk`,
+   * while `succ2` will yield just one edge `nd1` → `snk`.
+   */
+  private PathNode succ2(PathNode nd) {
+    result = succ1(nd)
+    or
+    succ1(nd) = finalMidNode(result)
   }
 
   /** Holds if `pred` → `succ` is an edge in the graph of data flow path explanations. */
-  query predicate edges(PathNode pred, PathNode succ) { pred.getASuccessor() = succ }
+  query predicate edges(PathNode pred, PathNode succ) {
+    succ = succ2(pred) and
+    // skip over uninteresting edges
+    not succ = initialMidNode(pred) and
+    not pred = finalMidNode(succ)
+  }
 }