Python: Port py/reflective-xss to use proper source/sink customization

python/ql/src/Security/CWE-079/ReflectedXss.ql

@@ -17,7 +17,7 @@ import python
 import semmle.python.security.dataflow.ReflectedXSS
 import DataFlow::PathGraph
 
-from ReflectedXssConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+from ReflectedXSS::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where config.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to $@.",
   source.getNode(), "a user-provided value"

python/ql/src/semmle/python/security/dataflow/ReflectedXSS.qll

@@ -1,41 +1,42 @@
 /**
- * Provides a taint-tracking configuration for detecting reflected server-side
- * cross-site scripting vulnerabilities.
+ * Provides a taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `ReflectedXSS::Configuration` is needed, otherwise
+ * `ReflectedXSSCustomizations` should be imported instead.
  */
 
-import python
+private import python
 import semmle.python.dataflow.new.DataFlow
 import semmle.python.dataflow.new.TaintTracking
-import semmle.python.Concepts
-import semmle.python.dataflow.new.RemoteFlowSources
-import semmle.python.dataflow.new.BarrierGuards
 
 /**
- * A taint-tracking configuration for detecting reflected server-side cross-site
- * scripting vulnerabilities.
+ * Provides a taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
  */
-class ReflectedXssConfiguration extends TaintTracking::Configuration {
-  ReflectedXssConfiguration() { this = "ReflectedXssConfiguration" }
+module ReflectedXSS {
+  import ReflectedXSSCustomizations::ReflectedXSS
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+  /**
+   * A taint-tracking configuration for detecting "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "ReflectedXSS" }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(HTTP::Server::HttpResponse response |
-      response.getMimetype().toLowerCase() = "text/html" and
-      sink = response.getBody()
-    )
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+
+    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+      guard instanceof SanitizerGuard
+    }
   }
-
-  override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
-    guard instanceof StringConstCompare
-  }
-
-  // TODO: For now, since there is not an `isSanitizingStep` member-predicate part of a
-  // `TaintTracking::Configuration`, we use treat the output is a taint-sanitizer. This
-  // is slightly imprecise, which you can see in the `m_unsafe + SAFE` test-case in
-  // python/ql/test/library-tests/frameworks/markupsafe/taint_test.py
-  //
-  // However, it is better than `getAnInput()`. Due to use-use flow, that would remove
-  // the taint-flow to `SINK()` in `some_escape(tainted); SINK(tainted)`.
-  override predicate isSanitizer(DataFlow::Node node) { node = any(HtmlEscaping esc).getOutput() }
 }
+
+/**
+ * DEPRECATED: Don't extend this class for customization, since this will lead to bad
+ * performance, instead use the new `ReflectedXSSCustomizations.qll` file, and extend
+ * its' classes.
+ */
+deprecated class ReflectedXssConfiguration = ReflectedXSS::Configuration;

python/ql/src/semmle/python/security/dataflow/ReflectedXSSCustomizations.qll

@@ -0,0 +1,76 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "reflected server-side cross-site scripting"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+
+private import python
+private import semmle.python.dataflow.new.DataFlow
+private import semmle.python.Concepts
+private import semmle.python.dataflow.new.RemoteFlowSources
+private import semmle.python.dataflow.new.BarrierGuards
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "reflected server-side cross-site scripting"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+module ReflectedXSS {
+  /**
+   * A data flow source for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for "reflected server-side cross-site scripting" vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * The body of a HTTP response that will be returned from a server, considered as a flow sink.
+   */
+  class ServerHttpResponseBodyAsSink extends Sink {
+    ServerHttpResponseBodyAsSink() {
+      exists(HTTP::Server::HttpResponse response |
+        response.getMimetype().toLowerCase() = "text/html" and
+        this = response.getBody()
+      )
+    }
+  }
+
+  /**
+   * An HTML escaping, considered as a sanitizer.
+   */
+  class HtmlEscapingAsSanitizer extends Sanitizer {
+    HtmlEscapingAsSanitizer() {
+      // TODO: For now, since there is not an `isSanitizingStep` member-predicate part of a
+      // `TaintTracking::Configuration`, we use treat the output is a taint-sanitizer. This
+      // is slightly imprecise, which you can see in the `m_unsafe + SAFE` test-case in
+      // python/ql/test/library-tests/frameworks/markupsafe/taint_test.py
+      //
+      // However, it is better than `getAnInput()`. Due to use-use flow, that would remove
+      // the taint-flow to `SINK()` in `some_escape(tainted); SINK(tainted)`.
+      this = any(HtmlEscaping esc).getOutput()
+    }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+}