Merge pull request #12845 from michaelnebel/csharp/xssrefactor

C#: Re-factor Xss to use the new data flow API.

Author: michaelnebel

csharp/ql/lib/semmle/code/csharp/security/dataflow/XSSQuery.qll

@@ -18,12 +18,10 @@ private import semmle.code.csharp.dataflow.TaintTracking2
  */
 predicate xssFlow(XssNode source, XssNode sink, string message) {
   // standard taint-tracking
-  exists(
-    TaintTrackingConfiguration c, DataFlow2::PathNode sourceNode, DataFlow2::PathNode sinkNode
-  |
+  exists(XssTracking::PathNode sourceNode, XssTracking::PathNode sinkNode |
     sourceNode = source.asDataFlowNode() and
     sinkNode = sink.asDataFlowNode() and
-    c.hasFlowPath(sourceNode, sinkNode) and
+    XssTracking::flowPath(sourceNode, sinkNode) and
     message =
       "is written to HTML or JavaScript" +
         any(string explanation |
@@ -45,7 +43,7 @@ predicate xssFlow(XssNode source, XssNode sink, string message) {
 module PathGraph {
   /** Holds if `(pred,succ)` is an edge in the graph of data flow path explanations. */
   query predicate edges(XssNode pred, XssNode succ) {
-    exists(DataFlow2::PathNode a, DataFlow2::PathNode b | DataFlow2::PathGraph::edges(a, b) |
+    exists(XssTracking::PathNode a, XssTracking::PathNode b | XssTracking::PathGraph::edges(a, b) |
       pred.asDataFlowNode() = a and
       succ.asDataFlowNode() = b
     )
@@ -56,7 +54,7 @@ module PathGraph {
 
   /** Holds if `n` is a node in the graph of data flow path explanations. */
   query predicate nodes(XssNode n, string key, string val) {
-    DataFlow2::PathGraph::nodes(n.asDataFlowNode(), key, val)
+    XssTracking::PathGraph::nodes(n.asDataFlowNode(), key, val)
     or
     xssFlow(n, n, _) and
     key = "semmle.label" and
@@ -69,13 +67,13 @@ module PathGraph {
    * `ret -> out` is summarized as the edge `arg -> out`.
    */
   query predicate subpaths(XssNode arg, XssNode par, XssNode ret, XssNode out) {
-    DataFlow2::PathGraph::subpaths(arg.asDataFlowNode(), par.asDataFlowNode(), ret.asDataFlowNode(),
-      out.asDataFlowNode())
+    XssTracking::PathGraph::subpaths(arg.asDataFlowNode(), par.asDataFlowNode(),
+      ret.asDataFlowNode(), out.asDataFlowNode())
   }
 }
 
 private newtype TXssNode =
-  TXssDataFlowNode(DataFlow2::PathNode node) or
+  TXssDataFlowNode(XssTracking::PathNode node) or
   TXssAspNode(AspInlineMember m)
 
 /**
@@ -90,21 +88,25 @@ class XssNode extends TXssNode {
   /** Gets the location of this node. */
   Location getLocation() { none() }
 
-  /** Gets the data flow node corresponding to this node, if any. */
-  DataFlow2::PathNode asDataFlowNode() { result = this.(XssDataFlowNode).getDataFlowNode() }
+  /**
+   * Gets the data flow node corresponding to this node, if any.
+   */
+  XssTracking::PathNode asDataFlowNode() { result = this.(XssDataFlowNode).getDataFlowNode() }
 
   /** Gets the ASP inline code element corresponding to this node, if any. */
   AspInlineMember asAspInlineMember() { result = this.(XssAspNode).getAspInlineMember() }
 }
 
-/** A data flow node, viewed as an XSS flow node. */
+/**
+ * A data flow node, viewed as an XSS flow node.
+ */
 class XssDataFlowNode extends TXssDataFlowNode, XssNode {
-  DataFlow2::PathNode node;
+  XssTracking::PathNode node;
 
   XssDataFlowNode() { this = TXssDataFlowNode(node) }
 
   /** Gets the data flow node corresponding to this node. */
-  DataFlow2::PathNode getDataFlowNode() { result = node }
+  XssTracking::PathNode getDataFlowNode() { result = node }
 
   override string toString() { result = node.toString() }
 
@@ -136,9 +138,11 @@ abstract class Source extends DataFlow::Node { }
 abstract class Sanitizer extends DataFlow::ExprNode { }
 
 /**
+ * DEPRECATED: Use `XssTracking` instead.
+ *
  * A taint-tracking configuration for cross-site scripting (XSS) vulnerabilities.
  */
-class TaintTrackingConfiguration extends TaintTracking2::Configuration {
+deprecated class TaintTrackingConfiguration extends TaintTracking2::Configuration {
   TaintTrackingConfiguration() { this = "XSSDataFlowConfiguration" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -148,6 +152,29 @@ class TaintTrackingConfiguration extends TaintTracking2::Configuration {
   override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
 }
 
+/**
+ * A taint-tracking configuration for cross-site scripting (XSS) vulnerabilities.
+ */
+module XssTrackingConfig implements DataFlow::ConfigSig {
+  /**
+   * Holds if `source` is a relevant data flow source.
+   */
+  predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  /**
+   * Holds if `sink` is a relevant data flow sink.
+   */
+  predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+  /**
+   * Holds if data flow through `node` is prohibited. This completely removes
+   * `node` from the data flow graph.
+   */
+  predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+}
+
+module XssTracking = TaintTracking::Global<XssTrackingConfig>;
+
 /** A source of remote user input. */
 private class RemoteSource extends Source instanceof RemoteFlowSource { }
 

csharp/ql/src/Security Features/CWE-079/StoredXSS.ql

@@ -16,18 +16,21 @@ import csharp
 import semmle.code.csharp.security.dataflow.flowsources.Stored
 import semmle.code.csharp.security.dataflow.XSSQuery
 import semmle.code.csharp.security.dataflow.XSSSinks
-import semmle.code.csharp.dataflow.DataFlow2
-import DataFlow2::PathGraph
+import StoredXss::PathGraph
 
-class StoredTaintTrackingConfiguration extends TaintTrackingConfiguration {
-  override predicate isSource(DataFlow2::Node source) { source instanceof StoredFlowSource }
+module StoredXssTrackingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof StoredFlowSource }
+
+  predicate isSink = XssTrackingConfig::isSink/1;
+
+  predicate isBarrier = XssTrackingConfig::isBarrier/1;
 }
 
-from
-  StoredTaintTrackingConfiguration c, DataFlow2::PathNode source, DataFlow2::PathNode sink,
-  string explanation
+module StoredXss = TaintTracking::Global<StoredXssTrackingConfig>;
+
+from StoredXss::PathNode source, StoredXss::PathNode sink, string explanation
 where
-  c.hasFlowPath(source, sink) and
+  StoredXss::flowPath(source, sink) and
   if exists(sink.getNode().(Sink).explanation())
   then explanation = " (" + sink.getNode().(Sink).explanation() + ")"
   else explanation = ""