OpenUrlRedirect: Expand safe URL flow configuration

Also add some more tests

ql/src/semmle/go/frameworks/Stdlib.qll

@@ -114,9 +114,7 @@ module IoUtil {
    * from its first argument to its first result.
    */
   private class ReadAll extends TaintTracking::FunctionModel {
-    ReadAll() {
-      hasQualifiedName("io/ioutil", "ReadAll")
-    }
+    ReadAll() { hasQualifiedName("io/ioutil", "ReadAll") }
 
     override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
       inp.isParameter(0) and outp.isResult(0)
@@ -201,7 +199,7 @@ module OS {
 /** Provides models of commonly used functions in the `path` package. */
 module Path {
   /** A path-manipulating function in the `path` package. */
-  private class PathManipulatingFunction extends TaintTracking::FunctionModel {
+  class PathManipulatingFunction extends TaintTracking::FunctionModel {
     PathManipulatingFunction() {
       exists(string fn | hasQualifiedName("path", fn) |
         fn = "Base" or

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

@@ -58,6 +58,8 @@ module OpenUrlRedirect {
 
     override predicate isSource(DataFlow::Node source) {
       source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", "URL")
+      or
+      source.(DataFlow::FieldReadNode).getField().hasQualifiedName("net/http", "Request", "Host")
     }
 
     override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
@@ -65,9 +67,32 @@ module OpenUrlRedirect {
     override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
       TaintTracking::functionModelStep(any(SafeUrlMethod m), pred, succ)
       or
+      TaintTracking::functionModelStep(any(Path::PathManipulatingFunction pmf), pred, succ)
+      or
+      TaintTracking::functionModelStep(any(StringRightTrimmer rt), pred, succ)
+      or
+      TaintTracking::referenceStep(pred, succ)
+      or
+      TaintTracking::stringConcatStep(pred, succ)
+      or
+      TaintTracking::tupleStep(pred, succ)
+      or
       exists(DataFlow::FieldReadNode frn | succ = frn |
-        frn.getBase() = pred and frn.getFieldName() = "Host"
+        frn.getBase() = pred and
+        (frn.getFieldName() = "Host" or frn.getFieldName() = "Path")
+      )
+      or
+      exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Path") |
+        w.writesField(v.getAUse(), f, pred) and succ = v.getAUse()
       )
     }
+
+    override predicate isBarrierOut(DataFlow::Node node) {
+      exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Path") |
+        w.writesField(node.getASuccessor(), f, _)
+      )
+    }
+
+    override int explorationLimit() { result = 30 }
   }
 }

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

@@ -39,7 +39,9 @@ module OpenUrlRedirect {
     SafeUrlMethod() {
       this instanceof StringMethod
       or
-      exists(string m | this.hasQualifiedName("net/url", "URL", m) | m = "Hostname" or m = "Port")
+      exists(string m | this.hasQualifiedName("net/url", "URL", m) |
+        m = "Hostname" or m = "Port" or m = "RequestURI"
+      )
     }
 
     override predicate hasTaintFlow(DataFlow::FunctionInput inp, DataFlow::FunctionOutput outp) {
@@ -47,6 +49,18 @@ module OpenUrlRedirect {
     }
   }
 
+  /**
+   * A function that trims the right hand side of a string, considered to preserve the safeness
+   * of taint flow from the full request URL.
+   */
+  class StringRightTrimmer extends Strings::Trimmer {
+    StringRightTrimmer() {
+      this.hasQualifiedName("strings", "TrimSuffix") or
+      this.hasQualifiedName("strings", "TrimRight") or
+      this.hasQualifiedName("strings", "TrimRightFunc")
+    }
+  }
+
   /**
    * A source of third-party user input, considered as a flow source for URL redirects.
    */

ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go

@@ -139,5 +139,29 @@ func serveStdlib() {
 		http.Redirect(w, r, target, 302)
 	})
 
+	http.HandleFunc("/ex8", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+
+		// GOOD: Only safe parts of the URL are used
+		url := *r.URL
+		if url.Scheme == "http" {
+			url.Scheme = "https"
+			http.Redirect(w, r, url.String(), 302)
+		} else {
+			// ...
+		}
+	})
+
+	http.HandleFunc("/ex8", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+
+		// GOOD: Only safe parts of the URL are used
+		if r.URL.Scheme == "http" {
+			http.Redirect(w, r, "https://"+r.URL.RequestURI(), 302)
+		} else {
+			// ...
+		}
+	})
+
 	http.ListenAndServe(":80", nil)
 }