From 82635a46ade4112d23f01f673ada7af9db569b82 Mon Sep 17 00:00:00 2001
From: Sauyon Lee <sauyon@github.com>
Date: Wed, 8 Jan 2020 23:48:52 -0800
Subject: [PATCH] OpenUrlRedirect: only make some parts of the URL untrusted

---
 .../OpenUrlRedirectCustomizations.qll         | 33 ++++++++++++++++++-
 .../CWE-601/OpenUrlRedirect/stdlib.go         | 12 +++++++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll
index 4f3d6aa87ec..4c3d1124e29 100644
--- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll
+++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll
@@ -30,8 +30,39 @@ module OpenUrlRedirect {
 
   /**
    * A source of third-party user input, considered as a flow source for URL redirects.
+   *
+   * Excludes the URL field, as it is handled more precisely in `UntrustedUrlField`.
    */
-  class UntrustedFlowAsSource extends Source, UntrustedFlowSource { }
+  class UntrustedFlowAsSource extends Source, UntrustedFlowSource {
+    UntrustedFlowAsSource() {
+      forex(SelectorExpr sel | this.asExpr() = sel | sel.getSelector().getName() != "URL")
+    }
+  }
+
+  /**
+   * An access to a user-controlled URL field, considered as a flow source for URL redirects.
+   */
+  class UntrustedUrlField extends Source, DataFlow::ExprNode {
+    override SelectorExpr expr;
+
+    UntrustedUrlField() {
+      exists(Type req, Type baseType, string fieldName |
+        req.hasQualifiedName("net/http", "Request") and
+        baseType = expr.getBase().(SelectorExpr).getBase().getType() and
+        expr.getBase().(SelectorExpr).getSelector().getName() = "URL" and
+        (baseType = req or baseType = req.getPointerType()) and
+        fieldName = expr.getSelector().getName() and
+        (
+          fieldName = "User" or
+          fieldName = "Path" or
+          fieldName = "RawPath" or
+          fieldName = "ForceQuery" or
+          fieldName = "RawQuery" or
+          fieldName = "Fragment"
+        )
+      )
+    }
+  }
 
   /**
    * An HTTP redirect, considered as a sink for `Configuration`.
diff --git a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go
index e9a0f18c16c..f6d663e3550 100644
--- a/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go
+++ b/ql/test/query-tests/Security/CWE-601/OpenUrlRedirect/stdlib.go
@@ -103,5 +103,17 @@ func serveStdlib() {
 		}
 	})
 
+	http.HandleFunc("/ex8", func(w http.ResponseWriter, r *http.Request) {
+		r.ParseForm()
+
+		// GOOD: this only rewrites	the scheme, which is not dangerous as the host cannot change.
+		if r.URL.Scheme == "http" {
+			r.URL.Scheme = "https"
+			http.Redirect(w, r, r.URL.String(), 302)
+		} else {
+			// ...
+		}
+	})
+
 	http.ListenAndServe(":80", nil)
 }