Python: Port py/url-redirection to use proper source/sink customization

Author: RasmusWL

python/ql/src/Security/CWE-601/UrlRedirect.ql

@@ -16,7 +16,7 @@ import python
 import semmle.python.security.dataflow.UrlRedirect
 import DataFlow::PathGraph
 
-from UrlRedirectConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+from UrlRedirect::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where config.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "Untrusted URL redirection due to $@.", source.getNode(),
   "A user-provided value"

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

@@ -1,37 +1,42 @@
 /**
- * Provides a taint-tracking configuration for detecting URL redirection
- * vulnerabilities.
+ * Provides a taint-tracking configuration for detecting "URL redirection" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `UrlRedirect::Configuration` is needed, otherwise
+ * `UrlRedirectCustomizations` 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 URL redirection vulnerabilities.
+ * Provides a taint-tracking configuration for detecting "URL redirection" vulnerabilities.
  */
-class UrlRedirectConfiguration extends TaintTracking::Configuration {
-  UrlRedirectConfiguration() { this = "UrlRedirectConfiguration" }
+module UrlRedirect {
+  import UrlRedirectCustomizations::UrlRedirect
 
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+  /**
+   * A taint-tracking configuration for detecting "URL redirection" vulnerabilities.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { this = "UrlRedirect" }
 
-  override predicate isSink(DataFlow::Node sink) {
-    sink = any(HTTP::Server::HttpRedirectResponse e).getRedirectLocation()
-  }
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
 
-  override predicate isSanitizer(DataFlow::Node node) {
-    // Url redirection is a problem only if the user controls the prefix of the URL.
-    // TODO: This is a copy of the taint-sanitizer from the old points-to query, which doesn't
-    // cover formatting.
-    exists(BinaryExprNode string_concat | string_concat.getOp() instanceof Add |
-      string_concat.getRight() = node.asCfgNode()
-    )
-  }
+    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 StringConstCompare
+    override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
+      guard instanceof SanitizerGuard
+    }
   }
 }
+
+/**
+ * DEPRECATED: Don't extend this class for customization, since this will lead to bad
+ * performance, instead use the new `UrlRedirectCustomizations.qll` file, and extend
+ * its' classes.
+ */
+deprecated class UrlRedirectConfiguration = UrlRedirect::Configuration;

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

@@ -0,0 +1,71 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting
+ * "URL redirection"
+ * 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
+ * "URL redirection"
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+module UrlRedirect {
+  /**
+   * A data flow source for "URL redirection" vulnerabilities.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for "URL redirection" vulnerabilities.
+   */
+  abstract class Sink extends DataFlow::Node { }
+
+  /**
+   * A sanitizer for "URL redirection" vulnerabilities.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A sanitizer guard for "URL redirection" vulnerabilities.
+   */
+  abstract class SanitizerGuard extends DataFlow::BarrierGuard { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * A HTTP redirect response, considered as a flow sink.
+   */
+  class RedirectLocationAsSink extends Sink {
+    RedirectLocationAsSink() {
+      this = any(HTTP::Server::HttpRedirectResponse e).getRedirectLocation()
+    }
+  }
+
+  /**
+   * The right side of a string-concat, considered as a sanitizer.
+   */
+  class StringConcatAsSanitizer extends Sanitizer {
+    StringConcatAsSanitizer() {
+      // Url redirection is a problem only if the user controls the prefix of the URL.
+      // TODO: This is a copy of the taint-sanitizer from the old points-to query, which doesn't
+      // cover formatting.
+      exists(BinaryExprNode string_concat | string_concat.getOp() instanceof Add |
+        string_concat.getRight() = this.asCfgNode()
+      )
+    }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  class StringConstCompareAsSanitizerGuard extends SanitizerGuard, StringConstCompare { }
+}