Merge pull request #75 from sauyon/ssrf-refinement

SSRF query refinements

change-notes/1.24/analysis-go.md

@@ -11,25 +11,26 @@
 
 The CodeQL library for Go now contains a folder of simple "cookbook" queries that show how to access basic Go elements using the predicates defined by the standard library. They're intended to give you a starting point for your own experiments and to help you work out the best way to frame your questions using CodeQL. You can find them in the `examples/snippets` folder in the [CodeQL for Go repository](https://github.com/github/codeql-go/tree/master/ql/examples/snippets).
 
-| **Query**                                                                 | **Tags**                                                                   | **Purpose**                                                                                                                                            |
-|---------------------------------------------------------------------------|----------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------|
-| Bad check of redirect URL (`go/bad-redirect-check`) | correctness, security, external/cwe/cwe-601 | Highlights checks that ensure redirect URLs start with `/` but don't check for `//` or `/\`. Results are shown on LGTM by default. |
-| Constant length comparison (`go/constant-length-comparison`)     | correctness | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. |
-| Impossible interface nil check (`go/impossible-interface-nil-check`) | correctness | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default. |
-| Incomplete URL scheme check (`go/incomplete-url-scheme-check`) | correctness, security, external/cwe/cwe-020 | Highlights checks for `javascript` URLs that do not take `data` or `vbscript` URLs into account. Results are shown on LGTM by default. |
-| Potentially unsafe quoting (`go/unsafe-quoting`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-089, external/cwe/cwe-094 | Highlights code that constructs a quoted string literal containing data that may itself contain quotes. Results are shown on LGTM by default. |
-| Size computation for allocation may overflow (`go/allocation-size-overflow`) | correctness, security, external/cwe/cwe-190 | Highlights code that computes the size of an allocation based on the size of a potentially large object. Results are shown on LGTM by default. |
-| XPath injection (`go/xml/xpath-injection`) | security, external/cwe/cwe-643 | Highlights code that uses remote input in an XPath expression. Results are shown on LGTM by default. |
+| **Query**                                                                    | **Tags**                                                                                | **Purpose**                                                                                                                                                                         |
+|------------------------------------------------------------------------------|-----------------------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Bad check of redirect URL (`go/bad-redirect-check`)                          | correctness, security, external/cwe/cwe-601                                             | Highlights checks that ensure redirect URLs start with `/` but don't check for `//` or `/\`. Results are shown on LGTM by default.                                                  |
+| Constant length comparison (`go/constant-length-comparison`)                 | correctness                                                                             | Highlights code that checks the length of an array or slice against a constant before indexing it using a variable, suggesting a logic error. Results are shown on LGTM by default. |
+| Impossible interface nil check (`go/impossible-interface-nil-check`)         | correctness                                                                             | Highlights code that compares an interface value that cannot be `nil` to `nil`, suggesting a logic error. Results are shown on LGTM by default.                                     |
+| Incomplete URL scheme check (`go/incomplete-url-scheme-check`)               | correctness, security, external/cwe/cwe-020                                             | Highlights checks for `javascript` URLs that do not take `data` or `vbscript` URLs into account. Results are shown on LGTM by default.                                              |
+| Potentially unsafe quoting (`go/unsafe-quoting`)                             | correctness, security, external/cwe/cwe-078, external/cwe/cwe-089, external/cwe/cwe-094 | Highlights code that constructs a quoted string literal containing data that may itself contain quotes. Results are shown on LGTM by default.                                       |
+| Size computation for allocation may overflow (`go/allocation-size-overflow`) | correctness, security, external/cwe/cwe-190                                             | Highlights code that computes the size of an allocation based on the size of a potentially large object. Results are shown on LGTM by default.                                      |
+| Uncontrolled data used in network request (`go/request-forgery`)             | correctness, security, external/cwe/cwe-918                                             | Highlights code that uses uncontrolled user input to make a request. Results are shown on LGTM by default.                                                                          |
+| XPath injection (`go/xml/xpath-injection`)                                   | security, external/cwe/cwe-643                                                          | Highlights code that uses remote input in an XPath expression. Results are shown on LGTM by default.                                                                                |
 
 ## Changes to existing queries
 
-| **Query**                                                                     | **Expected impact**   | **Change**                                                                                                                                                                  |
-|-------------------------------------------------------------------------------|-----------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| Database query built from user-controlled sources (`go/sql-injection`)        | More results          | The library models used by the query have been improved, allowing it to flag more potentially problematic cases.                                                            |
-| Identical operands (`go/redundant-operation`)                                 | Fewer false positives | The query no longer flags cases where the operands have the same value but are syntactically distinct, since this is usually intentional.                                   |
-| Incomplete regular expression for hostnames (`go/incomplete-hostname-regexp`) | More results          | The query now flags unescaped dots before the TLD in a hostname regex.                                                                                                      |
-| Reflected cross-site scripting (`go/reflected-xss`)                           | Fewer results         | Untrusted input flowing into an HTTP header definition or into an `fmt.Fprintf` call with a constant prefix is no longer flagged, since it is in both cases often harmless. |
-| Useless assignment to field (`go/useless-assignment-to-field`)                | Fewer false positives | The query now conservatively handles fields promoted through embedded pointer types.                                                                                        |
-| Bitwise exclusive-or used like exponentiation (`go/mistyped-exponentiation`)  | Fewer false positives | The query now identifies when the value of an xor is assigned to a mask object, and excludes such results.                                                                  |
-| Open URL redirect (`go/unvalidated-url-redirection`)                          | Fewer false positives | The query now identifies some sources that are not attacker-controlled, and excludes results with such sources.                                                             |
-| Command built from user-controlled sources (`go/command-injection`) | More results          | The library models used by the query have been improved, allowing it to flag more potentially problematic cases, including sources that flow into shells, sudo, or programming-language interpreters as arguments. |
+| **Query**                                                                     | **Expected impact**   | **Change**                                                                                                                                                                                                         |
+|-------------------------------------------------------------------------------|-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Bitwise exclusive-or used like exponentiation (`go/mistyped-exponentiation`)  | Fewer false positives | The query now identifies when the value of an xor is assigned to a mask object, and excludes such results.                                                                                                         |
+| Command built from user-controlled sources (`go/command-injection`)           | More results          | The library models used by the query have been improved, allowing it to flag more potentially problematic cases, including sources that flow into shells, sudo, or programming-language interpreters as arguments. |
+| Database query built from user-controlled sources (`go/sql-injection`)        | More results          | The library models used by the query have been improved, allowing it to flag more potentially problematic cases.                                                                                                   |
+| Identical operands (`go/redundant-operation`)                                 | Fewer false positives | The query no longer flags cases where the operands have the same value but are syntactically distinct, since this is usually intentional.                                                                          |
+| Incomplete regular expression for hostnames (`go/incomplete-hostname-regexp`) | More results          | The query now flags unescaped dots before the TLD in a hostname regex.                                                                                                                                             |
+| Open URL redirect (`go/unvalidated-url-redirection`)                          | Fewer false positives | The query now identifies some sources that are not attacker-controlled, and excludes results with such sources.                                                                                                    |
+| Reflected cross-site scripting (`go/reflected-xss`)                           | Fewer results         | Untrusted input flowing into an HTTP header definition or into an `fmt.Fprintf` call with a constant prefix is no longer flagged, since it is in both cases often harmless.                                        |
+| Useless assignment to field (`go/useless-assignment-to-field`)                | Fewer false positives | The query now conservatively handles fields promoted through embedded pointer types.                                                                                                                               |

ql/src/Security/CWE-601/OpenUrlRedirect.ql

@@ -12,10 +12,12 @@
 
 import go
 import semmle.go.security.OpenUrlRedirect::OpenUrlRedirect
+import semmle.go.security.SafeUrlFlow
 import DataFlow::PathGraph
 
 from
-  Configuration cfg, SafeUrlConfiguration scfg, DataFlow::PathNode source, DataFlow::PathNode sink
+  Configuration cfg, SafeUrlFlow::Configuration scfg, DataFlow::PathNode source,
+  DataFlow::PathNode sink
 where
   cfg.hasFlowPath(source, sink) and
   // this excludes flow from safe parts of request URLs, for example the full URL when the

ql/src/Security/CWE-918/RequestForgery.go

@@ -0,0 +1,18 @@
+package main
+
+import (
+	"net/http"
+)
+
+func handler(w http.ResponseWriter, req *http.Request) {
+	target := req.FormValue("target")
+
+	// BAD: `target` is controlled by the attacker
+	resp, err := http.Get("https://" + target + ".example.com/data/")
+	if err != nil {
+		// error handling
+	}
+
+	// process request response
+	use(resp)
+}

ql/src/Security/CWE-918/RequestForgery.qhelp

@@ -0,0 +1,56 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+Directly incorporating user input into an HTTP request without validating the input can facilitate
+different kinds of request forgery attacks, where the attacker essentially controls the request.
+
+If the vulnerable request is in server-side code, then security mechanisms, such as external
+firewalls, can be bypassed.
+
+If the vulnerable request is in client-side code, then unsuspecting users can send malicious
+requests to other servers, potentially resulting in a DDOS attack.
+</p>
+</overview>
+
+<recommendation>
+<p>
+To guard against request forgery, it is advisable to avoid putting user input directly into a
+network request. If a flexible network request mechanism is required, it is recommended to maintain
+a list of authorized request targets and choose from that list based on the user input provided.
+</p>
+</recommendation>
+
+<example>
+<p>
+The following example shows an HTTP request parameter being used directly in a URL request without
+validating the input, which facilitates an SSRF attack. The request <code>http.Get(...)</code> is
+vulnerable since attackers can choose the value of <code>target</code> to be anything they want. For
+instance, the attacker can choose <code>"internal.example.com/#"</code> as the target, causing the
+URL used in the request to be <code>"https://internal.example.com/#.example.com/data"</code>.
+</p>
+
+<p>
+A request to <code>https://internal.example.com</code> may be problematic if that server is not
+meant to be directly accessible from the attacker's machine.
+</p>
+
+<sample src="RequestForgery.go"/>
+
+<p>
+One way to remedy the problem is to use the user input to select a known fixed string before
+performing the request:
+</p>
+
+<sample src="RequestForgeryGood.go"/>
+</example>
+
+<references>
+
+<li>OWASP: <a href="https://www.owasp.org/index.php/Server_Side_Request_Forgery">SSRF</a></li>
+
+</references>
+</qhelp>

ql/src/Security/CWE-918/RequestForgery.ql

@@ -3,18 +3,24 @@
  * @description Sending network requests with user-controlled data allows for request forgery attacks.
  * @kind path-problem
  * @problem.severity error
+ * @precision high
  * @id go/request-forgery
  * @tags security
  *       external/cwe/cwe-918
  */
 
 import go
-import RequestForgery::RequestForgery
+import semmle.go.security.RequestForgery::RequestForgery
+import semmle.go.security.SafeUrlFlow
 import DataFlow::PathGraph
 
-from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request
+from
+  Configuration cfg, SafeUrlFlow::Configuration 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"

ql/src/Security/CWE-918/RequestForgeryGood.go

@@ -0,0 +1,25 @@
+package main
+
+import (
+	"net/http"
+)
+
+func handler1(w http.ResponseWriter, req *http.Request) {
+	target := req.FormValue("target")
+
+	var subdomain string
+	if target == "EU" {
+		subdomain = "europe"
+	} else {
+		subdomain = "world"
+	}
+
+	// GOOD: `subdomain` is controlled by the server
+	resp, err := http.Get("https://" + subdomain + ".example.com/data/")
+	if err != nil {
+		// error handling
+	}
+
+	// process request response
+	use(resp)
+}

ql/src/semmle/go/controlflow/ControlFlowGraph.qll

@@ -129,7 +129,10 @@ module ControlFlow {
     /** Holds if this node sets the value of field `f` on `base` to `rhs`. */
     predicate writesField(DataFlow::Node base, Field f, DataFlow::Node rhs) {
       exists(IR::FieldTarget trg | trg = self.getLhs() |
-        trg.getBase() = base.asInstruction() and
+        (
+          trg.getBase() = base.asInstruction() or
+          trg.getBase() = MkImplicitDeref(base.asExpr())
+        ) and
         trg.getField() = f and
         self.getRhs() = rhs.asInstruction()
       )

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

@@ -195,4 +195,31 @@ private module StdlibHttp {
     /** Gets the URL of the request. */
     override DataFlow::Node getUrl() { result = this.getArgument(0) }
   }
+
+  /** A call to the Client.Do function in the `net/http` package. */
+  private class ClientDo extends HTTP::ClientRequest::Range, DataFlow::MethodCallNode {
+    ClientDo() { this.getTarget().hasQualifiedName("net/http", "Client", "Do") }
+
+    override DataFlow::Node getUrl() {
+      // A URL passed to `NewRequest`, whose result is passed to this `Do` call
+      exists(DataFlow::CallNode call | call.getTarget().hasQualifiedName("net/http", "NewRequest") |
+        this.getArgument(0) = call.getResult(0).getASuccessor*() and
+        result = call.getArgument(1)
+      )
+      or
+      // A URL passed to `NewRequestWithContext`, whose result is passed to this `Do` call
+      exists(DataFlow::CallNode call |
+        call.getTarget().hasQualifiedName("net/http", "NewRequestWithContext")
+      |
+        this.getArgument(0) = call.getResult(0).getASuccessor*() and
+        result = call.getArgument(2)
+      )
+      or
+      // A URL assigned to a request that is passed to this `Do` call
+      exists(Write w, Field f |
+        f.hasQualifiedName("net/http", "Request", "URL") and
+        w.writesField(this.getArgument(0).getAPredecessor*(), f, result)
+      )
+    }
+  }
 }

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

@@ -52,42 +52,4 @@ module OpenUrlRedirect {
       guard instanceof BarrierGuard
     }
   }
-
-  /**
-   * A data-flow configuration for reasoning about safe URLs for unvalidated URL redirections.
-   */
-  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(UnsafeUrlMethod um), node, _)
-    }
-  }
 }

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

@@ -6,6 +6,7 @@
 
 import go
 import UrlConcatenation
+import SafeUrlFlowCustomizations
 
 /**
  * Provides extension points for customizing the taint-tracking configuration for reasoning about
@@ -32,13 +33,6 @@ module OpenUrlRedirect {
    */
   abstract class BarrierGuard extends DataFlow::BarrierGuard { }
 
-  /**
-   * A method on a `net/url.URL` that is considered unsafe to redirect to.
-   */
-  class UnsafeUrlMethod extends URL::UrlGetter {
-    UnsafeUrlMethod() { this.getName() = "Query" }
-  }
-
   /**
    * A source of third-party user input, considered as a flow source for URL redirects.
    */
@@ -167,3 +161,23 @@ module OpenUrlRedirect {
     }
   }
 }
+
+/** A sink for an open redirect, considered as a sink for safe URL flow. */
+private class SafeUrlSink extends SafeUrlFlow::Sink {
+  SafeUrlSink() { this instanceof OpenUrlRedirect::Sink }
+}
+
+/**
+ * A read of a field considered unsafe to redirect to, considered as a sanitizer for a safe
+ * URL.
+ */
+private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge {
+  UnsafeFieldReadSanitizer() {
+    exists(DataFlow::FieldReadNode frn, string name |
+      (name = "User" or name = "RawQuery" or name = "Fragment" or name = "User") and
+      frn.getField().hasQualifiedName("net/url", "URL")
+    |
+      this = frn.getBase()
+    )
+  }
+}

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

@@ -18,6 +18,13 @@ module RequestForgery {
 
     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 isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node) or
       node instanceof Sanitizer

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

@@ -3,7 +3,8 @@
  */
 
 import go
-import semmle.go.security.UrlConcatenation
+import UrlConcatenation
+import SafeUrlFlowCustomizations
 
 /** Provides classes and predicates for the request forgery query. */
 module RequestForgery {
@@ -39,7 +40,7 @@ module RequestForgery {
   class UntrustedFlowAsSource extends Source, UntrustedFlowSource { }
 
   /**
-   * The URL of a URL request, viewed as a sink for request forgery.
+   * The URL of an HTTP request, viewed as a sink for request forgery.
    */
   private class ClientRequestUrlAsSink extends Sink {
     HTTP::ClientRequest request;
@@ -51,7 +52,31 @@ 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, _) }
   }
 }
+
+/** A sink for request forgery, considered as a sink for safe URL flow. */
+private class SafeUrlSink extends SafeUrlFlow::Sink {
+  SafeUrlSink() { this instanceof RequestForgery::Sink }
+}
+
+/**
+ * A read of a field considered unsafe for request forgery, considered as a sanitizer for a safe
+ * URL.
+ */
+private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge {
+  UnsafeFieldReadSanitizer() {
+    exists(DataFlow::FieldReadNode frn, string name |
+      (name = "RawQuery" or name = "Fragment" or name = "User") and
+      frn.getField().hasQualifiedName("net/url", "URL")
+    |
+      this = frn.getBase()
+    )
+  }
+}

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

@@ -0,0 +1,45 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about
+ * safe flow from URLs.
+ *
+ * Note, for performance reasons: only import this file if
+ * `SafeUrlFlow::Configuration` is needed, otherwise
+ * `SafeUrlFlowCustomizations` should be imported instead.
+ */
+
+import go
+
+module SafeUrlFlow {
+  import SafeUrlFlowCustomizations::SafeUrlFlow
+
+  /**
+   * A taint-tracking configuration for reasoning about safe URLs.
+   */
+  class Configuration extends TaintTracking::Configuration {
+    Configuration() { 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
+      node instanceof SanitizerEdge
+    }
+  }
+}

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

@@ -0,0 +1,26 @@
+/**
+ * Provides default sources, sinks and sanitisers for reasoning about
+ * safe URL flow, as well as extension points for adding your own.
+ */
+
+import go
+
+module SafeUrlFlow {
+  /** A sink for safe URL flow. */
+  abstract class Sink extends DataFlow::Node { }
+
+  /** An outgoing sanitizer edge for safe URL flow. */
+  abstract class SanitizerEdge extends DataFlow::Node { }
+
+  /**
+   * A method on a `net/url.URL` that is considered unsafe to use.
+   */
+  private class UnsafeUrlMethod extends URL::UrlGetter {
+    UnsafeUrlMethod() { this.getName() = "Query" }
+  }
+
+  /** A function model step using `UnsafeUrlMethod`, considered as a sanitizer for safe URL flow. */
+  private class UnsafeUrlMethodEdge extends SanitizerEdge {
+    UnsafeUrlMethodEdge() { TaintTracking::functionModelStep(any(UnsafeUrlMethod um), this, _) }
+  }
+}

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

@@ -55,6 +55,11 @@ private predicate concatenationHasHostnameSanitizingSubstring(StringOps::Concate
   exists(StringOps::ConcatenationLeaf lf | lf = cat.getALeaf() |
     lf.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*")
     or
+    // this deals with cases such as `Sprintf("%s/%s", hostname, taint)`, which should be safe as
+    // long as `hostname` is not user-controlled
+    lf.getStringValue() = "/" and
+    exists(lf.getPreviousLeaf())
+    or
     hasHostnameSanitizingSubstring(lf.asNode())
   )
 }

ql/test/query-tests/Security/CWE-918/RequestForgery.expected

@@ -0,0 +1,38 @@
+edges
+| RequestForgery.go:8:12:8:34 | call to FormValue : string | RequestForgery.go:11:24:11:65 | ...+... |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:14:11:14:17 | tainted |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:18:12:18:18 | tainted |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:21:34:21:40 | tainted |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:24:66:24:72 | tainted |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:27:11:27:29 | ...+... |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:29:11:29:40 | ...+... |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:36:2:36:2 | implicit dereference : URL |
+| tst.go:10:13:10:35 | call to FormValue : string | tst.go:37:11:37:20 | call to String |
+| tst.go:35:2:35:2 | definition of u [pointer] : URL | tst.go:36:2:36:2 | u [pointer] : URL |
+| tst.go:36:2:36:2 | implicit dereference : URL | tst.go:35:2:35:2 | definition of u [pointer] : URL |
+| tst.go:36:2:36:2 | implicit dereference : URL | tst.go:36:2:36:2 | implicit dereference : URL |
+| tst.go:36:2:36:2 | implicit dereference : URL | tst.go:37:11:37:20 | call to String |
+| tst.go:36:2:36:2 | u [pointer] : URL | tst.go:36:2:36:2 | implicit dereference : URL |
+nodes
+| RequestForgery.go:8:12:8:34 | call to FormValue : string | semmle.label | call to FormValue : string |
+| RequestForgery.go:11:24:11:65 | ...+... | semmle.label | ...+... |
+| tst.go:10:13:10:35 | call to FormValue : string | semmle.label | call to FormValue : string |
+| tst.go:14:11:14:17 | tainted | semmle.label | tainted |
+| tst.go:18:12:18:18 | tainted | semmle.label | tainted |
+| tst.go:21:34:21:40 | tainted | semmle.label | tainted |
+| tst.go:24:66:24:72 | tainted | semmle.label | tainted |
+| tst.go:27:11:27:29 | ...+... | semmle.label | ...+... |
+| tst.go:29:11:29:40 | ...+... | semmle.label | ...+... |
+| tst.go:35:2:35:2 | definition of u [pointer] : URL | semmle.label | definition of u [pointer] : URL |
+| tst.go:36:2:36:2 | implicit dereference : URL | semmle.label | implicit dereference : URL |
+| tst.go:36:2:36:2 | u [pointer] : URL | semmle.label | u [pointer] : URL |
+| tst.go:37:11:37:20 | call to String | semmle.label | call to String |
+#select
+| RequestForgery.go:11:15:11:66 | call to Get | RequestForgery.go:8:12:8:34 | call to FormValue : string | RequestForgery.go:11:24:11:65 | ...+... | The $@ of this request depends on $@. | RequestForgery.go:11:24:11:65 | ...+... | URL | RequestForgery.go:8:12:8:34 | call to FormValue : string | a user-provided value |
+| tst.go:14:2:14:18 | call to Get | tst.go:10:13:10:35 | call to FormValue : string | tst.go:14:11:14:17 | tainted | The $@ of this request depends on $@. | tst.go:14:11:14:17 | tainted | URL | tst.go:10:13:10:35 | call to FormValue : string | a user-provided value |
+| tst.go:18:2:18:38 | call to Post | tst.go:10:13:10:35 | call to FormValue : string | tst.go:18:12:18:18 | tainted | The $@ of this request depends on $@. | tst.go:18:12:18:18 | tainted | URL | tst.go:10:13:10:35 | call to FormValue : string | a user-provided value |
+| tst.go:22:2:22:14 | call to Do | tst.go:10:13:10:35 | call to FormValue : string | tst.go:21:34:21:40 | tainted | The $@ of this request depends on $@. | tst.go:21:34:21:40 | tainted | URL | tst.go:10:13:10:35 | call to FormValue : string | a user-provided value |
+| tst.go:25:2:25:14 | call to Do | tst.go:10:13:10:35 | call to FormValue : string | tst.go:24:66:24:72 | tainted | The $@ of this request depends on $@. | tst.go:24:66:24:72 | tainted | URL | tst.go:10:13:10:35 | call to FormValue : string | a user-provided value |
+| tst.go:27:2:27:30 | call to Get | tst.go:10:13:10:35 | call to FormValue : string | tst.go:27:11:27:29 | ...+... | The $@ of this request depends on $@. | tst.go:27:11:27:29 | ...+... | URL | tst.go:10:13:10:35 | call to FormValue : string | a user-provided value |
+| tst.go:29:2:29:41 | call to Get | tst.go:10:13:10:35 | call to FormValue : string | tst.go:29:11:29:40 | ...+... | The $@ of this request depends on $@. | tst.go:29:11:29:40 | ...+... | URL | tst.go:10:13:10:35 | call to FormValue : string | a user-provided value |
+| tst.go:37:2:37:21 | call to Get | tst.go:10:13:10:35 | call to FormValue : string | tst.go:37:11:37:20 | call to String | The $@ of this request depends on $@. | tst.go:37:11:37:20 | call to String | URL | tst.go:10:13:10:35 | call to FormValue : string | a user-provided value |

ql/test/query-tests/Security/CWE-918/RequestForgery.go

@@ -0,0 +1,18 @@
+package main
+
+import (
+	"net/http"
+)
+
+func handler(w http.ResponseWriter, req *http.Request) {
+	target := req.FormValue("target")
+
+	// BAD: `target` is controlled by the attacker
+	resp, err := http.Get("https://" + target + ".example.com/data/")
+	if err != nil {
+		// error handling
+	}
+
+	// process request response
+	use(resp)
+}

ql/test/query-tests/Security/CWE-918/RequestForgery.qlref

@@ -0,0 +1 @@
+Security/CWE-918/RequestForgery.ql

ql/test/query-tests/Security/CWE-918/RequestForgeryGood.go

@@ -0,0 +1,25 @@
+package main
+
+import (
+	"net/http"
+)
+
+func handler1(w http.ResponseWriter, req *http.Request) {
+	target := req.FormValue("target")
+
+	var subdomain string
+	if target == "EU" {
+		subdomain = "europe"
+	} else {
+		subdomain = "world"
+	}
+
+	// GOOD: `subdomain` is controlled by the server
+	resp, err := http.Get("https://" + subdomain + ".example.com/data/")
+	if err != nil {
+		// error handling
+	}
+
+	// process request response
+	use(resp)
+}

ql/test/query-tests/Security/CWE-918/tst.go

@@ -0,0 +1,42 @@
+package main
+
+import (
+	"context"
+	"net/http"
+	"net/url"
+)
+
+func handler2(w http.ResponseWriter, req *http.Request) {
+	tainted := req.FormValue("target")
+
+	http.Get("example.com") // OK
+
+	http.Get(tainted) // Not OK
+
+	http.Head(tainted) // OK
+
+	http.Post(tainted, "text/basic", nil) // Not OK
+
+	client := &http.Client{}
+	rq, _ := http.NewRequest("GET", tainted, nil)
+	client.Do(rq) // Not OK
+
+	rq, _ = http.NewRequestWithContext(context.Background(), "GET", tainted, nil)
+	client.Do(rq) // Not OK
+
+	http.Get("http://" + tainted) // Not OK
+
+	http.Get("http://example.com" + tainted) // Not OK
+
+	http.Get("http://example.com/" + tainted) // OK
+
+	http.Get("http://example.com/?" + tainted) // OK
+
+	u, _ := url.Parse("http://example.com/relative-path")
+	u.Host = tainted
+	http.Get(u.String()) // Not OK
+}
+
+func main() {
+
+}

ql/test/query-tests/Security/CWE-918/util.go

@@ -0,0 +1,5 @@
+package main
+
+func use(vars ...interface{}) {
+
+}