Java: Diff-informed XSS.ql

This change makes the XSS query fully diff-informed, including the
discovery of sinks. This involved making a helper data-flow analysis
diff-informed, which required punching through some abstraction layers.

An alternative would have been to use `DataFlow::SimpleGlobal` or other
means to make the analysis faster, but I didn't want to risk removing
good taint steps such as wrapping one `OutputStream` in another.

Author: jbj

java/ql/lib/semmle/code/java/frameworks/JaxWS.qll

@@ -417,7 +417,7 @@ class JaxRSConsumesAnnotation extends JaxRSAnnotation {
 }
 
 /** A default sink representing methods susceptible to XSS attacks. */
-private class JaxRSXssSink extends XssSink {
+private class JaxRSXssSink extends AbstractXssSink {
   JaxRSXssSink() {
     exists(JaxRsResourceMethod resourceMethod, ReturnStmt rs |
       resourceMethod = any(JaxRsResourceClass resourceClass).getAResourceMethod() and

java/ql/lib/semmle/code/java/frameworks/spring/SpringHttp.qll

@@ -46,7 +46,7 @@ private predicate specifiesContentType(SpringRequestMappingMethod method) {
   exists(method.getAProducesExpr())
 }
 
-private class SpringXssSink extends XSS::XssSink {
+private class SpringXssSink extends XSS::AbstractXssSink {
   SpringXssSink() {
     exists(SpringRequestMappingMethod requestMappingMethod, ReturnStmt rs |
       requestMappingMethod = rs.getEnclosingCallable() and

java/ql/lib/semmle/code/java/security/XSS.qll

@@ -13,17 +13,36 @@ private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.FlowSinks
 
-/** A sink that represent a method that outputs data without applying contextual output encoding. */
-abstract class XssSink extends ApiSinkNode { }
-
 /** A sanitizer that neutralizes dangerous characters that can be used to perform a XSS attack. */
 abstract class XssSanitizer extends DataFlow::Node { }
 
+/**
+ * A sink that represent a method that outputs data without applying contextual
+ * output encoding. Extend this class to add more sinks that should be
+ * considered XSS sinks by every query. To find the full set of XSS sinks, use
+ * `XssSink` instead.
+ */
+abstract class AbstractXssSink extends DataFlow::Node { }
+
+/** A default sink representing methods susceptible to XSS attacks. */
+private class DefaultXssSink extends AbstractXssSink {
+  DefaultXssSink() { sinkNode(this, ["html-injection", "js-injection"]) }
+}
+
+/**
+ * A sink that represent a method that outputs data without applying contextual
+ * output encoding. To add more sinks, extend `AbstractXssSink` rather than
+ * this class. To find XSS sinks efficiently for a diff-informed query, use the
+ * `XssDiffInformed` module instead.
+ */
+final class XssSink extends ApiSinkNode instanceof XssDiffInformed<xssNotDiffInformed/0>::XssSink {
+}
+
 /**
  * A sink that represent a method that outputs data without applying contextual output encoding,
  * and which should truncate flow paths such that downstream sinks are not flagged as well.
  */
-abstract class XssSinkBarrier extends XssSink { }
+abstract class XssSinkBarrier extends AbstractXssSink { }
 
 /**
  * A unit class for adding additional taint steps.
@@ -39,19 +58,6 @@ class XssAdditionalTaintStep extends Unit {
   abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
 }
 
-/** A default sink representing methods susceptible to XSS attacks. */
-private class DefaultXssSink extends XssSink {
-  DefaultXssSink() {
-    sinkNode(this, ["html-injection", "js-injection"])
-    or
-    exists(MethodCall ma |
-      ma.getMethod() instanceof WritingMethod and
-      XssVulnerableWriterSourceToWritingMethodFlow::flowToExpr(ma.getQualifier()) and
-      this.asExpr() = ma.getArgument(_)
-    )
-  }
-}
-
 /** A default sanitizer that considers numeric and boolean typed data safe for writing to output. */
 private class DefaultXssSanitizer extends XssSanitizer {
   DefaultXssSanitizer() {
@@ -62,20 +68,6 @@ private class DefaultXssSanitizer extends XssSanitizer {
   }
 }
 
-/** A configuration that tracks data from a servlet writer to an output method. */
-private module XssVulnerableWriterSourceToWritingMethodFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node src) { src instanceof XssVulnerableWriterSourceNode }
-
-  predicate isSink(DataFlow::Node sink) {
-    exists(MethodCall ma |
-      sink.asExpr() = ma.getQualifier() and ma.getMethod() instanceof WritingMethod
-    )
-  }
-}
-
-private module XssVulnerableWriterSourceToWritingMethodFlow =
-  TaintTracking::Global<XssVulnerableWriterSourceToWritingMethodFlowConfig>;
-
 /** A method that can be used to output data to an output stream or writer. */
 private class WritingMethod extends Method {
   WritingMethod() {
@@ -113,6 +105,77 @@ class XssVulnerableWriterSourceNode extends ApiSourceNode {
   XssVulnerableWriterSourceNode() { this.asExpr() instanceof XssVulnerableWriterSource }
 }
 
+/** A nullary predicate. */
+signature predicate xssNullaryPredicate();
+
+/** Holds always. Use this predicate as parameter to `XssDiffInformed` to disable diff-informed mode. */
+predicate xssNotDiffInformed() { any() }
+
+/**
+ * A module for finding XSS sinks faster in a diff-informed query. The
+ * `hasSourceInDiffRange` parameter should hold if the overall data-flow
+ * configuration of the query has any sources in the diff range.
+ */
+module XssDiffInformed<xssNullaryPredicate/0 hasSourceInDiffRange> {
+  final private class Node = DataFlow::Node;
+
+  /**
+   * A diff-informed replacement for the top-level `XssSink`, omitting for
+   * efficiency some sinks that would never be reported by a diff-informed
+   * query.
+   */
+  final class XssSink extends Node {
+    XssSink() {
+      this instanceof AbstractXssSink
+      or
+      isResponseWriterSink(this)
+    }
+  }
+
+  predicate isResponseWriterSink(DataFlow::Node sink) {
+    exists(MethodCall ma |
+      // This code mirrors `getASelectedSinkLocation`.
+      ma.getMethod() instanceof WritingMethod and
+      XssVulnerableWriterSourceToWritingMethodFlow::flowToExpr(ma.getQualifier()) and
+      sink.asExpr() = ma.getArgument(_)
+    )
+  }
+
+  /** A configuration that tracks data from a servlet writer to an output method. */
+  private module XssVulnerableWriterSourceToWritingMethodFlowConfig implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node src) { src instanceof XssVulnerableWriterSourceNode }
+
+    predicate isSink(DataFlow::Node sink) {
+      exists(MethodCall ma |
+        sink.asExpr() = ma.getQualifier() and ma.getMethod() instanceof WritingMethod
+      )
+    }
+
+    predicate observeDiffInformedIncrementalMode() {
+      // Since this configuration is for finding sinks to be used in a main
+      // data-flow configuration, this configuration should only restrict the
+      // sinks to be found if there are no main-configuration sources in the
+      // diff range. That's because if there is such a source, we need to
+      // report query results for it even with sinks outside the diff range.
+      not hasSourceInDiffRange()
+    }
+
+    // The sources are not exposed outside this file module, so we know the
+    // query will not select them.
+    Location getASelectedSourceLocation(DataFlow::Node source) { none() }
+
+    Location getASelectedSinkLocation(DataFlow::Node sink) {
+      // This code mirrors `isResponseWriterSink`.
+      exists(MethodCall ma | result = ma.getAnArgument().getLocation() |
+        sink.asExpr() = ma.getQualifier() and ma.getMethod() instanceof WritingMethod
+      )
+    }
+  }
+
+  private module XssVulnerableWriterSourceToWritingMethodFlow =
+    TaintTracking::Global<XssVulnerableWriterSourceToWritingMethodFlowConfig>;
+}
+
 /**
  * Holds if `s` is an HTTP Content-Type vulnerable to XSS.
  */

java/ql/lib/semmle/code/java/security/XssLocalQuery.qll

@@ -11,7 +11,9 @@ private import semmle.code.java.security.XSS
 deprecated module XssLocalConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
 
-  predicate isSink(DataFlow::Node sink) { sink instanceof XssSink }
+  predicate isSink(DataFlow::Node sink) {
+    sink instanceof XssDiffInformed<XssLocalFlow::hasSourceInDiffRange/0>::XssSink
+  }
 
   predicate isBarrier(DataFlow::Node node) { node instanceof XssSanitizer }
 

java/ql/lib/semmle/code/java/security/XssQuery.qll

@@ -11,7 +11,9 @@ import semmle.code.java.security.XSS
 module XssConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource }
 
-  predicate isSink(DataFlow::Node sink) { sink instanceof XssSink }
+  predicate isSink(DataFlow::Node sink) {
+    sink instanceof XssDiffInformed<XssFlow::hasSourceInDiffRange/0>::XssSink
+  }
 
   predicate isBarrier(DataFlow::Node node) { node instanceof XssSanitizer }