OpenUrlRedirect: Use a data-flow configuration to track whole URLs

ql/src/semmle/go/security/OpenUrlRedirect.qll

@@ -21,7 +21,10 @@ module OpenUrlRedirect {
 
     override predicate isSource(DataFlow::Node source) { source instanceof Source }
 
-    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+    override predicate isSink(DataFlow::Node sink) {
+      sink instanceof Sink and
+      not sink instanceof NotSink
+    }
 
     override predicate isBarrier(DataFlow::Node node) { node instanceof Barrier }
 

ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll

@@ -18,6 +18,14 @@ module OpenUrlRedirect {
    */
   abstract class Sink extends DataFlow::Node { }
 
+  /**
+   * A data flow node that should not be considered a sink for unvalidated URL redirect
+   * vulnerabilities.
+   *
+   * This takes precedence over `Sink`, and so can be used to remove sinks that are safe.
+   */
+  abstract class NotSink extends DataFlow::Node { }
+
   /**
    * A barrier for unvalidated URL redirect vulnerabilities.
    */
@@ -33,57 +41,47 @@ module OpenUrlRedirect {
    *
    * Excludes the URL field, as it is handled more precisely in `UntrustedUrlField`.
    */
-  class UntrustedFlowAsSource extends Source, UntrustedFlowSource {
-    UntrustedFlowAsSource() {
-      forex(SelectorExpr sel | this.asExpr() = sel | sel.getSelector().getName() != "URL")
+  class UntrustedFlowAsSource extends Source, UntrustedFlowSource { }
+
+  class SafeUrlMethod extends TaintTracking::FunctionModel, Method {
+    SafeUrlMethod() {
+      this instanceof StringMethod
+      or
+      this instanceof URL::UrlGetter and
+      exists(string m | this.hasQualifiedName("net/url", "URL", m) | m = "Hostname" or m = "Port")
+    }
+
+    override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
+      inp.isReceiver() and outp.isResult()
     }
   }
 
-  /**
-   * An access to a user-controlled URL field, considered as a flow source for URL redirects.
-   */
-  class UntrustedUrlField extends Source, DataFlow::FieldReadNode {
-    UntrustedUrlField() {
-      exists(Type req, Type baseType, string fieldName, DataFlow::FieldReadNode url |
-        req.hasQualifiedName("net/http", "Request") and
-        baseType = url.getBase().getType() and
-        (baseType = req or baseType = req.getPointerType()) and
-        url.getFieldName() = "URL"
-      |
-        this.getBase() = url.getASuccessor*() and
-        this.getFieldName() = fieldName and
-        (
-          fieldName = "User" or
-          fieldName = "Path" or
-          fieldName = "RawPath" or
-          fieldName = "ForceQuery" or
-          fieldName = "RawQuery" or
-          fieldName = "Fragment"
-        )
+  private class SafeUrlConfiguration extends DataFlow2::Configuration {
+    SafeUrlConfiguration() { this = "FullUrlFlow" }
+
+    override predicate isSource(DataFlow::Node source) {
+      exists(Type req | req.hasQualifiedName("net/http", "Request") |
+        source.(DataFlow::FieldReadNode).getField() = req.getField("URL")
+      )
+    }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
+      TaintTracking::functionModelStep(any(SafeUrlMethod m), pred, succ)
+      or
+      exists(DataFlow::FieldReadNode frn | succ = frn |
+        frn.getBase() = pred and frn.getFieldName() = "Host"
       )
     }
   }
 
   /**
-   * An call to a user-controlled URL method, considered as a flow source for URL redirects.
+   * A safe part of a request URL to redirect to. This includes values such as the hostname and port
+   * which should not be considered as unsafe when redirected to.
    */
-  class UntrustedUrlMethod extends Source, DataFlow::MethodCallNode {
-    UntrustedUrlMethod() {
-      exists(Type req, Type baseType, string methodName, DataFlow::FieldReadNode url |
-        req.hasQualifiedName("net/http", "Request") and
-        baseType = url.getBase().getType() and
-        (baseType = req or baseType = req.getPointerType()) and
-        url.getFieldName() = "URL"
-      |
-        this.getReceiver() = url.getASuccessor*() and
-        this.getCalleeName() = methodName and
-        (
-          methodName = "EscapedPath" or
-          methodName = "Query" or
-          methodName = "RequestURI"
-        )
-      )
-    }
+  class SafeUrl extends NotSink, DataFlow::Node {
+    SafeUrl() { exists(SafeUrlConfiguration conf | conf.hasFlow(_, this)) }
   }
 
   /**