From 4bcffe2d4773fc153b49c86d474194b99f0a8ef4 Mon Sep 17 00:00:00 2001
From: Sauyon Lee <sauyon@github.com>
Date: Tue, 31 Mar 2020 00:37:17 -0700
Subject: [PATCH] RequestForgery: Add a safe URL sanitizer

---
 ql/src/Security/CWE-918/RequestForgery.ql     |  8 +++-
 ql/src/semmle/go/security/RequestForgery.qll  | 39 +++++++++++++++++++
 .../security/RequestForgeryCustomizations.qll |  4 ++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/ql/src/Security/CWE-918/RequestForgery.ql b/ql/src/Security/CWE-918/RequestForgery.ql
index 930f767e430..c3e4e63e51b 100644
--- a/ql/src/Security/CWE-918/RequestForgery.ql
+++ b/ql/src/Security/CWE-918/RequestForgery.ql
@@ -13,9 +13,13 @@ import go
 import semmle.go.security.RequestForgery::RequestForgery
 import DataFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request
+from
+  Configuration cfg, SafeUrlConfiguration scfg, DataFlow::PathNode source, DataFlow::PathNode sink,
+  DataFlow::Node request
 where
   cfg.hasFlowPath(source, sink) and
-  request = sink.getNode().(Sink).getARequest()
+  request = sink.getNode().(Sink).getARequest() and
+  // this excludes flow from safe parts of request URLs, for example the full URL
+  not scfg.hasFlow(_, sink.getNode())
 select request, source, sink, "The $@ of this request depends on $@.", sink.getNode(),
   sink.getNode().(Sink).getKind(), source, "a user-provided value"
diff --git a/ql/src/semmle/go/security/RequestForgery.qll b/ql/src/semmle/go/security/RequestForgery.qll
index 38827b0dfd4..78eaf605c76 100644
--- a/ql/src/semmle/go/security/RequestForgery.qll
+++ b/ql/src/semmle/go/security/RequestForgery.qll
@@ -4,6 +4,7 @@
  */
 
 import go
+import OpenUrlRedirectCustomizations
 
 module RequestForgery {
   import RequestForgeryCustomizations::RequestForgery
@@ -39,4 +40,42 @@ module RequestForgery {
       super.isSanitizerGuard(guard) or guard instanceof SanitizerGuard
     }
   }
+
+  /**
+   * A data-flow configuration for reasoning about safe URLs for request forgery.
+   */
+  class SafeUrlConfiguration extends TaintTracking::Configuration {
+    SafeUrlConfiguration() { this = "SafeUrlFlow" }
+
+    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 }
+
+    override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
+      // propagate to a URL when its host is assigned to
+      exists(Write w, Field f, SsaWithFields v | f.hasQualifiedName("net/url", "URL", "Host") |
+        w.writesField(v.getAUse(), f, pred) and succ = v.getAUse()
+      )
+    }
+
+    override predicate isSanitizerOut(DataFlow::Node node) {
+      // block propagation of this safe value when its host is overwritten
+      exists(Write w, Field f | f.hasQualifiedName("net/url", "URL", "Host") |
+        w.writesField(node.getASuccessor(), f, _)
+      )
+      or
+      exists(DataFlow::FieldReadNode frn, string name |
+        (name = "RawQuery" or name = "Fragment" or name = "User") and
+        frn.getField().hasQualifiedName("net/url", "URL")
+      |
+        node = frn.getBase()
+      )
+      or
+      TaintTracking::functionModelStep(any(OpenUrlRedirect::UnsafeUrlMethod um), node, _)
+    }
+  }
 }
diff --git a/ql/src/semmle/go/security/RequestForgeryCustomizations.qll b/ql/src/semmle/go/security/RequestForgeryCustomizations.qll
index 563b99dd474..cdc55abc4d6 100644
--- a/ql/src/semmle/go/security/RequestForgeryCustomizations.qll
+++ b/ql/src/semmle/go/security/RequestForgeryCustomizations.qll
@@ -51,6 +51,10 @@ module RequestForgery {
     override string getKind() { result = "URL" }
   }
 
+  /**
+   * A value that is the result of prepending a string that prevents any value from controlling the
+   * host of a URL.
+   */
   private class HostnameSanitizer extends SanitizerEdge {
     HostnameSanitizer() { hostnameSanitizingPrefixEdge(this, _) }
   }