From b057ce8d46f3408199e036aee266b3f67ccb33b4 Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Fri, 20 Mar 2020 13:54:13 -0700 Subject: [PATCH 1/5] Concepts: Add `HTTP::ClientRequest` class and module. Extensible model of client requests to a URL. Ported from the CodeQL JavaScript library. --- ql/src/semmle/go/Concepts.qll | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/ql/src/semmle/go/Concepts.qll b/ql/src/semmle/go/Concepts.qll index 57cf1a5516e..b43d75cf8cb 100644 --- a/ql/src/semmle/go/Concepts.qll +++ b/ql/src/semmle/go/Concepts.qll @@ -527,6 +527,43 @@ module HTTP { ResponseWriter getResponseWriter() { result = self.getResponseWriter() } } + /** Provides a class for modeling new HTTP client request APIs. */ + module ClientRequest { + /** + * A call that performs a request to a URL. + * + * Example: An HTTP POST request is a client request that sends some + * `data` to a `url`, where both the headers and the body of the request + * contribute to the `data`. + * + * Extend this class to model new APIs. If you want to refine existing API models, + * extend `HTTP::ClientRequest` instead. + */ + abstract class Range extends DataFlow::Node { + /** + * Gets the URL of the request. + */ + abstract DataFlow::Node getUrl(); + } + } + + /** + * A call that performs a request to a URL. + * + * Extend this class to refine existing API models. If you want to model new APIs, + * extend `HTTP::ClientRequest::Range` instead. + */ + class ClientRequest extends DataFlow::Node { + ClientRequest::Range self; + + ClientRequest() { this = self } + + /** + * Gets the URL of the request. + */ + DataFlow::Node getUrl() { result = self.getUrl() } + } + /** Provides a class for modeling new HTTP redirect APIs. */ module Redirect { /** From d41e6a9d85c8bb14322e066033ca13934012e6cc Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Fri, 20 Mar 2020 13:55:44 -0700 Subject: [PATCH 2/5] Model HTTP request functions in `net/http` package. --- ql/src/semmle/go/frameworks/HTTP.qll | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/ql/src/semmle/go/frameworks/HTTP.qll b/ql/src/semmle/go/frameworks/HTTP.qll index d16e9df2a41..20da009e8d1 100644 --- a/ql/src/semmle/go/frameworks/HTTP.qll +++ b/ql/src/semmle/go/frameworks/HTTP.qll @@ -147,4 +147,21 @@ private module StdlibHttp { override HTTP::ResponseWriter getResponseWriter() { result.getANode() = this.getArgument(0) } } + + /** A call to a function in the `net/http` package that performs an HTTP request to a URL. */ + private class RequestCall extends HTTP::ClientRequest::Range, DataFlow::CallNode { + RequestCall() { + exists(string functionName | + ( + this.getTarget().hasQualifiedName("net/http", functionName) + or + this.getTarget().(Method).hasQualifiedName("net/http", "Client", functionName) + ) and + (functionName = "Get" or functionName = "Post" or functionName = "PostForm") + ) + } + + /** Gets the URL of the request. */ + override DataFlow::Node getUrl() { result = this.getArgument(0) } + } } From f9845322366cc08884e47e74c0f953eb8adace9c Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Fri, 20 Mar 2020 13:57:50 -0700 Subject: [PATCH 3/5] Experimental: Add query for request forgery. Tracks the flow of tainted data from untrusted input to the URL of an HTTP request. Ported from the corresponding query for JavaScript, though currently limited in scope. Includes companion libraries for customisation. --- .../RequestForgery/RequestForgery.ql | 21 ++++++++ .../RequestForgery/RequestForgery.qll | 30 ++++++++++++ .../RequestForgeryCustomizations.qll | 49 +++++++++++++++++++ 3 files changed, 100 insertions(+) create mode 100644 ql/src/experimental/RequestForgery/RequestForgery.ql create mode 100644 ql/src/experimental/RequestForgery/RequestForgery.qll create mode 100644 ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll diff --git a/ql/src/experimental/RequestForgery/RequestForgery.ql b/ql/src/experimental/RequestForgery/RequestForgery.ql new file mode 100644 index 00000000000..11cbe507bd4 --- /dev/null +++ b/ql/src/experimental/RequestForgery/RequestForgery.ql @@ -0,0 +1,21 @@ +/** + * @name Uncontrolled data used in network request + * @description Sending network requests with user-controlled data allows for request forgery attacks. + * @kind path-problem + * @problem.severity error + * @precision medium + * @id go/request-forgery + * @tags security + * external/cwe/cwe-918 + */ + +import go +import RequestForgery::RequestForgery +import DataFlow::PathGraph + +from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request +where + cfg.hasFlowPath(source, sink) and + request = sink.getNode().(Sink).getARequest() +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/experimental/RequestForgery/RequestForgery.qll b/ql/src/experimental/RequestForgery/RequestForgery.qll new file mode 100644 index 00000000000..068ccd29cae --- /dev/null +++ b/ql/src/experimental/RequestForgery/RequestForgery.qll @@ -0,0 +1,30 @@ +/** + * Provides a taint-tracking configuration for reasoning about request forgery + * (SSRF) vulnerabilities. + */ + +import go + +module RequestForgery { + import RequestForgeryCustomizations::RequestForgery + + /** + * A taint-tracking configuration for reasoning about request forgery. + */ + class Configuration extends TaintTracking::Configuration { + Configuration() { this = "RequestForgery" } + + override predicate isSource(DataFlow::Node source) { source instanceof Source } + + override predicate isSink(DataFlow::Node sink) { sink instanceof Sink } + + override predicate isSanitizer(DataFlow::Node node) { + super.isSanitizer(node) or + node instanceof Sanitizer + } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + super.isSanitizerGuard(guard) or guard instanceof SanitizerGuard + } + } +} diff --git a/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll b/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll new file mode 100644 index 00000000000..17ff6028b96 --- /dev/null +++ b/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll @@ -0,0 +1,49 @@ +/** + * Provides classes and predicates used by the request forgery query. + */ + +import go + +/** Provides classes and predicates for the request forgery query. */ +module RequestForgery { + /** A data flow source for request forgery vulnerabilities. */ + abstract class Source extends DataFlow::Node { } + + /** A data flow sink for request forgery vulnerabilities. */ + abstract class Sink extends DataFlow::Node { + /** Gets a request that uses this sink. */ + abstract DataFlow::Node getARequest(); + + /** + * Gets the name of a part of the request that may be tainted by this sink, + * such as the URL or the host. + */ + abstract string getKind(); + } + + /** A sanitizer for request forgery vulnerabilities. */ + abstract class Sanitizer extends DataFlow::Node { } + + /** + * A sanitizer guard for request forgery vulnerabilities. + */ + abstract class SanitizerGuard extends DataFlow::BarrierGuard { } + + /** + * A third-party controllable input, considered as a flow source for request forgery. + */ + class UntrustedFlowAsSource extends Source, UntrustedFlowSource { } + + /** + * The URL of a URL request, viewed as a sink for request forgery. + */ + private class ClientRequestUrlAsSink extends Sink { + HTTP::ClientRequest request; + + ClientRequestUrlAsSink() { this = request.getUrl() } + + override DataFlow::Node getARequest() { result = request } + + override string getKind() { result = "URL" } + } +} From 4f32d6651c377a2c8170436d5647c382713f3cb1 Mon Sep 17 00:00:00 2001 From: Aditya Sharad Date: Fri, 20 Mar 2020 15:02:28 -0700 Subject: [PATCH 4/5] Experimental: Add sanitiser edge for request forgery. Consider a URL string sanitised if the hostname cannot be controlled. This approach is used by URL redirection queries. --- ql/src/experimental/RequestForgery/RequestForgery.qll | 5 +++++ .../RequestForgery/RequestForgeryCustomizations.qll | 8 ++++++++ 2 files changed, 13 insertions(+) diff --git a/ql/src/experimental/RequestForgery/RequestForgery.qll b/ql/src/experimental/RequestForgery/RequestForgery.qll index 068ccd29cae..347d7d0085e 100644 --- a/ql/src/experimental/RequestForgery/RequestForgery.qll +++ b/ql/src/experimental/RequestForgery/RequestForgery.qll @@ -23,6 +23,11 @@ module RequestForgery { node instanceof Sanitizer } + override predicate isSanitizerOut(DataFlow::Node node) { + super.isSanitizerOut(node) or + node instanceof SanitizerEdge + } + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { super.isSanitizerGuard(guard) or guard instanceof SanitizerGuard } diff --git a/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll b/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll index 17ff6028b96..563b99dd474 100644 --- a/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll +++ b/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll @@ -3,6 +3,7 @@ */ import go +import semmle.go.security.UrlConcatenation /** Provides classes and predicates for the request forgery query. */ module RequestForgery { @@ -24,6 +25,9 @@ module RequestForgery { /** A sanitizer for request forgery vulnerabilities. */ abstract class Sanitizer extends DataFlow::Node { } + /** An outgoing sanitizer edge for request forgery vulnerabilities. */ + abstract class SanitizerEdge extends DataFlow::Node { } + /** * A sanitizer guard for request forgery vulnerabilities. */ @@ -46,4 +50,8 @@ module RequestForgery { override string getKind() { result = "URL" } } + + private class HostnameSanitizer extends SanitizerEdge { + HostnameSanitizer() { hostnameSanitizingPrefixEdge(this, _) } + } } From c44e5379df0d00978c8a1afb0aff115ba4b8f276 Mon Sep 17 00:00:00 2001 From: Aditya Sharad <6874315+adityasharad@users.noreply.github.com> Date: Mon, 23 Mar 2020 09:18:47 -0700 Subject: [PATCH 5/5] Experimental: Remove query precision for now. Address review comment. --- ql/src/experimental/RequestForgery/RequestForgery.ql | 1 - 1 file changed, 1 deletion(-) diff --git a/ql/src/experimental/RequestForgery/RequestForgery.ql b/ql/src/experimental/RequestForgery/RequestForgery.ql index 11cbe507bd4..1db31170eb0 100644 --- a/ql/src/experimental/RequestForgery/RequestForgery.ql +++ b/ql/src/experimental/RequestForgery/RequestForgery.ql @@ -3,7 +3,6 @@ * @description Sending network requests with user-controlled data allows for request forgery attacks. * @kind path-problem * @problem.severity error - * @precision medium * @id go/request-forgery * @tags security * external/cwe/cwe-918