diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index fd94d3788ef..f71ef2a1613 100644 --- a/change-notes/1.24/analysis-go.md +++ b/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. | diff --git a/ql/src/Security/CWE-601/OpenUrlRedirect.ql b/ql/src/Security/CWE-601/OpenUrlRedirect.ql index daa79e9258a..befba5a8c75 100644 --- a/ql/src/Security/CWE-601/OpenUrlRedirect.ql +++ b/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 diff --git a/ql/src/Security/CWE-918/RequestForgery.go b/ql/src/Security/CWE-918/RequestForgery.go new file mode 100644 index 00000000000..c88e9dc5f74 --- /dev/null +++ b/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) +} diff --git a/ql/src/Security/CWE-918/RequestForgery.qhelp b/ql/src/Security/CWE-918/RequestForgery.qhelp new file mode 100644 index 00000000000..c9937e5669f --- /dev/null +++ b/ql/src/Security/CWE-918/RequestForgery.qhelp @@ -0,0 +1,56 @@ + + + + +

+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. +

+
+ + +

+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. +

+
+ + +

+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 http.Get(...) is +vulnerable since attackers can choose the value of target to be anything they want. For +instance, the attacker can choose "internal.example.com/#" as the target, causing the +URL used in the request to be "https://internal.example.com/#.example.com/data". +

+ +

+A request to https://internal.example.com may be problematic if that server is not +meant to be directly accessible from the attacker's machine. +

+ + + +

+One way to remedy the problem is to use the user input to select a known fixed string before +performing the request: +

+ + +
+ + + +
  • OWASP: SSRF
  • + +
    +
    diff --git a/ql/src/experimental/RequestForgery/RequestForgery.ql b/ql/src/Security/CWE-918/RequestForgery.ql similarity index 55% rename from ql/src/experimental/RequestForgery/RequestForgery.ql rename to ql/src/Security/CWE-918/RequestForgery.ql index 1db31170eb0..4541a687804 100644 --- a/ql/src/experimental/RequestForgery/RequestForgery.ql +++ b/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" diff --git a/ql/src/Security/CWE-918/RequestForgeryGood.go b/ql/src/Security/CWE-918/RequestForgeryGood.go new file mode 100644 index 00000000000..92f7b5fc1d5 --- /dev/null +++ b/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) +} diff --git a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll b/ql/src/semmle/go/controlflow/ControlFlowGraph.qll index e56ca535e5f..2a7a82f4e18 100644 --- a/ql/src/semmle/go/controlflow/ControlFlowGraph.qll +++ b/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() ) diff --git a/ql/src/semmle/go/frameworks/HTTP.qll b/ql/src/semmle/go/frameworks/HTTP.qll index e657b20a1ad..44e0cd23533 100644 --- a/ql/src/semmle/go/frameworks/HTTP.qll +++ b/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) + ) + } + } } diff --git a/ql/src/semmle/go/security/OpenUrlRedirect.qll b/ql/src/semmle/go/security/OpenUrlRedirect.qll index fb3efd2006f..c639aeb00d1 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirect.qll +++ b/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, _) - } - } } diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 5dee255d104..7c4b0854daf 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/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() + ) + } +} diff --git a/ql/src/experimental/RequestForgery/RequestForgery.qll b/ql/src/semmle/go/security/RequestForgery.qll similarity index 75% rename from ql/src/experimental/RequestForgery/RequestForgery.qll rename to ql/src/semmle/go/security/RequestForgery.qll index 347d7d0085e..38827b0dfd4 100644 --- a/ql/src/experimental/RequestForgery/RequestForgery.qll +++ b/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 diff --git a/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll b/ql/src/semmle/go/security/RequestForgeryCustomizations.qll similarity index 64% rename from ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll rename to ql/src/semmle/go/security/RequestForgeryCustomizations.qll index 563b99dd474..aecd5e077fb 100644 --- a/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll +++ b/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() + ) + } +} diff --git a/ql/src/semmle/go/security/SafeUrlFlow.qll b/ql/src/semmle/go/security/SafeUrlFlow.qll new file mode 100644 index 00000000000..ff14492aa87 --- /dev/null +++ b/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 + } + } +} diff --git a/ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll b/ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll new file mode 100644 index 00000000000..5e2299c2ed3 --- /dev/null +++ b/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, _) } + } +} diff --git a/ql/src/semmle/go/security/UrlConcatenation.qll b/ql/src/semmle/go/security/UrlConcatenation.qll index 9c75b3f8279..9415d4a8acf 100644 --- a/ql/src/semmle/go/security/UrlConcatenation.qll +++ b/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()) ) } diff --git a/ql/test/query-tests/Security/CWE-918/RequestForgery.expected b/ql/test/query-tests/Security/CWE-918/RequestForgery.expected new file mode 100644 index 00000000000..5b8fb08dce1 --- /dev/null +++ b/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 | diff --git a/ql/test/query-tests/Security/CWE-918/RequestForgery.go b/ql/test/query-tests/Security/CWE-918/RequestForgery.go new file mode 100644 index 00000000000..c88e9dc5f74 --- /dev/null +++ b/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) +} diff --git a/ql/test/query-tests/Security/CWE-918/RequestForgery.qlref b/ql/test/query-tests/Security/CWE-918/RequestForgery.qlref new file mode 100644 index 00000000000..fcb4e41daf8 --- /dev/null +++ b/ql/test/query-tests/Security/CWE-918/RequestForgery.qlref @@ -0,0 +1 @@ +Security/CWE-918/RequestForgery.ql diff --git a/ql/test/query-tests/Security/CWE-918/RequestForgeryGood.go b/ql/test/query-tests/Security/CWE-918/RequestForgeryGood.go new file mode 100644 index 00000000000..92f7b5fc1d5 --- /dev/null +++ b/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) +} diff --git a/ql/test/query-tests/Security/CWE-918/tst.go b/ql/test/query-tests/Security/CWE-918/tst.go new file mode 100644 index 00000000000..0e04429580c --- /dev/null +++ b/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() { + +} diff --git a/ql/test/query-tests/Security/CWE-918/util.go b/ql/test/query-tests/Security/CWE-918/util.go new file mode 100644 index 00000000000..5655c4f0cff --- /dev/null +++ b/ql/test/query-tests/Security/CWE-918/util.go @@ -0,0 +1,5 @@ +package main + +func use(vars ...interface{}) { + +}