From b23c75afb686d320eb3e833e40ab8e3f7dcfe7e1 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Wed, 25 Mar 2020 03:42:44 -0700 Subject: [PATCH 01/17] RequestForgery: move query from experimental --- .../RequestForgery => Security/CWE-918}/RequestForgery.ql | 2 +- .../RequestForgery => semmle/go/security}/RequestForgery.qll | 0 .../go/security}/RequestForgeryCustomizations.qll | 0 3 files changed, 1 insertion(+), 1 deletion(-) rename ql/src/{experimental/RequestForgery => Security/CWE-918}/RequestForgery.ql (92%) rename ql/src/{experimental/RequestForgery => semmle/go/security}/RequestForgery.qll (100%) rename ql/src/{experimental/RequestForgery => semmle/go/security}/RequestForgeryCustomizations.qll (100%) diff --git a/ql/src/experimental/RequestForgery/RequestForgery.ql b/ql/src/Security/CWE-918/RequestForgery.ql similarity index 92% rename from ql/src/experimental/RequestForgery/RequestForgery.ql rename to ql/src/Security/CWE-918/RequestForgery.ql index 1db31170eb0..c3dd2a2decd 100644 --- a/ql/src/experimental/RequestForgery/RequestForgery.ql +++ b/ql/src/Security/CWE-918/RequestForgery.ql @@ -9,7 +9,7 @@ */ import go -import RequestForgery::RequestForgery +import semmle.go.security.RequestForgery::RequestForgery import DataFlow::PathGraph from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink, DataFlow::Node request diff --git a/ql/src/experimental/RequestForgery/RequestForgery.qll b/ql/src/semmle/go/security/RequestForgery.qll similarity index 100% rename from ql/src/experimental/RequestForgery/RequestForgery.qll rename to ql/src/semmle/go/security/RequestForgery.qll diff --git a/ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll b/ql/src/semmle/go/security/RequestForgeryCustomizations.qll similarity index 100% rename from ql/src/experimental/RequestForgery/RequestForgeryCustomizations.qll rename to ql/src/semmle/go/security/RequestForgeryCustomizations.qll From 6876eabf546ed85a365fcb1733a8d79212098408 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 04:23:20 -0700 Subject: [PATCH 02/17] RequestForgery: Add query help --- ql/src/Security/CWE-918/RequestForgery.go | 18 ++++++ ql/src/Security/CWE-918/RequestForgery.qhelp | 56 +++++++++++++++++++ ql/src/Security/CWE-918/RequestForgeryGood.go | 25 +++++++++ 3 files changed, 99 insertions(+) create mode 100644 ql/src/Security/CWE-918/RequestForgery.go create mode 100644 ql/src/Security/CWE-918/RequestForgery.qhelp create mode 100644 ql/src/Security/CWE-918/RequestForgeryGood.go 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..9692adeb016 --- /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 tomaintain 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/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) +} From 12928d9f1763714c555c6b016c37c05701cdd031 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 05:32:13 -0700 Subject: [PATCH 03/17] HTTP: Add model for Client.Do --- ql/src/semmle/go/frameworks/HTTP.qll | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/ql/src/semmle/go/frameworks/HTTP.qll b/ql/src/semmle/go/frameworks/HTTP.qll index e657b20a1ad..6b4f3cc4a01 100644 --- a/ql/src/semmle/go/frameworks/HTTP.qll +++ b/ql/src/semmle/go/frameworks/HTTP.qll @@ -195,4 +195,32 @@ 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() { + exists(DataFlow::CallNode call, FunctionOutput outp | + call.getTarget().hasQualifiedName("net/http", "NewRequest") and + outp.isResult(0) + | + this.getArgument(0) = outp.getNode(call).getASuccessor*() and + result = call.getArgument(1) + ) + or + exists(DataFlow::CallNode call, FunctionOutput outp | + call.getTarget().hasQualifiedName("net/http", "NewRequestWithContext") and + outp.isResult(0) + | + this.getArgument(0) = outp.getNode(call).getASuccessor*() and + result = call.getArgument(2) + ) + or + exists(Write w, Field f | + f.hasQualifiedName("net/http", "Request", "URL") and + w.writesField(this.getArgument(0).getAPredecessor*(), f, result) + ) + } + } } From e9b0f88946378e1e0c220f1ddf752bd09c36f6c6 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 05:32:48 -0700 Subject: [PATCH 04/17] RequestForgery: Add taint step for URL Host assignment --- ql/src/semmle/go/security/RequestForgery.qll | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ql/src/semmle/go/security/RequestForgery.qll b/ql/src/semmle/go/security/RequestForgery.qll index 347d7d0085e..38827b0dfd4 100644 --- a/ql/src/semmle/go/security/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 From 314787956b8007db23efb589883edced50f4f943 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 05:33:24 -0700 Subject: [PATCH 05/17] Allow write base to be inside an implicit dereference --- ql/src/semmle/go/controlflow/ControlFlowGraph.qll | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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() ) From 830c3fce2a44be2b7159c54bc6f61c336b6214e4 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 04:23:43 -0700 Subject: [PATCH 06/17] RequestForgery: Add tests --- .../Security/CWE-918/RequestForgery.expected | 38 +++++++++++++++++ .../Security/CWE-918/RequestForgery.go | 18 ++++++++ .../Security/CWE-918/RequestForgery.qlref | 1 + .../Security/CWE-918/RequestForgeryGood.go | 25 +++++++++++ ql/test/query-tests/Security/CWE-918/tst.go | 42 +++++++++++++++++++ ql/test/query-tests/Security/CWE-918/util.go | 5 +++ 6 files changed, 129 insertions(+) create mode 100644 ql/test/query-tests/Security/CWE-918/RequestForgery.expected create mode 100644 ql/test/query-tests/Security/CWE-918/RequestForgery.go create mode 100644 ql/test/query-tests/Security/CWE-918/RequestForgery.qlref create mode 100644 ql/test/query-tests/Security/CWE-918/RequestForgeryGood.go create mode 100644 ql/test/query-tests/Security/CWE-918/tst.go create mode 100644 ql/test/query-tests/Security/CWE-918/util.go 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{}) { + +} From 89a03c8b67422cdbd64f6f81709d24d1539ed414 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 08:18:14 -0700 Subject: [PATCH 07/17] RequestForgery: Add high precision --- ql/src/Security/CWE-918/RequestForgery.ql | 1 + 1 file changed, 1 insertion(+) diff --git a/ql/src/Security/CWE-918/RequestForgery.ql b/ql/src/Security/CWE-918/RequestForgery.ql index c3dd2a2decd..930f767e430 100644 --- a/ql/src/Security/CWE-918/RequestForgery.ql +++ b/ql/src/Security/CWE-918/RequestForgery.ql @@ -3,6 +3,7 @@ * @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 From 3577d7560729da52282d61bae7e18effd7276779 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 27 Mar 2020 08:19:49 -0700 Subject: [PATCH 08/17] RequestForgery: Add change note --- change-notes/1.24/analysis-go.md | 39 ++++++++++++++++---------------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index fd94d3788ef..6d02ed26eb9 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** | +|-------------------------------------------------------------------------------|-----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| 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. | From 1c859a899115c0aabb2d546e507a88ec0299c50a Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Mon, 30 Mar 2020 02:04:03 -0700 Subject: [PATCH 09/17] Address review comments --- ql/src/Security/CWE-918/RequestForgery.qhelp | 4 ++-- ql/src/semmle/go/frameworks/HTTP.qll | 17 ++++++++--------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/ql/src/Security/CWE-918/RequestForgery.qhelp b/ql/src/Security/CWE-918/RequestForgery.qhelp index 9692adeb016..c9937e5669f 100644 --- a/ql/src/Security/CWE-918/RequestForgery.qhelp +++ b/ql/src/Security/CWE-918/RequestForgery.qhelp @@ -19,8 +19,8 @@ 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 tomaintain a -list of authorized request targets and choose from that list based on the user input provided. +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.

    diff --git a/ql/src/semmle/go/frameworks/HTTP.qll b/ql/src/semmle/go/frameworks/HTTP.qll index 6b4f3cc4a01..44e0cd23533 100644 --- a/ql/src/semmle/go/frameworks/HTTP.qll +++ b/ql/src/semmle/go/frameworks/HTTP.qll @@ -201,22 +201,21 @@ private module StdlibHttp { ClientDo() { this.getTarget().hasQualifiedName("net/http", "Client", "Do") } override DataFlow::Node getUrl() { - exists(DataFlow::CallNode call, FunctionOutput outp | - call.getTarget().hasQualifiedName("net/http", "NewRequest") and - outp.isResult(0) - | - this.getArgument(0) = outp.getNode(call).getASuccessor*() and + // 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 - exists(DataFlow::CallNode call, FunctionOutput outp | - call.getTarget().hasQualifiedName("net/http", "NewRequestWithContext") and - outp.isResult(0) + // 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) = outp.getNode(call).getASuccessor*() and + 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) From 4bcffe2d4773fc153b49c86d474194b99f0a8ef4 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 31 Mar 2020 00:37:17 -0700 Subject: [PATCH 10/17] 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, _) } } From 4b3982154a9b9ae5a47b020b3649c78adec75936 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Tue, 31 Mar 2020 18:37:47 -0700 Subject: [PATCH 11/17] Add a SafeUrlFlow configuration --- ql/src/Security/CWE-601/OpenUrlRedirect.ql | 4 +- ql/src/Security/CWE-918/RequestForgery.ql | 5 ++- ql/src/semmle/go/security/OpenUrlRedirect.qll | 38 ---------------- .../OpenUrlRedirectCustomizations.qll | 28 +++++++++--- ql/src/semmle/go/security/RequestForgery.qll | 39 ---------------- .../security/RequestForgeryCustomizations.qll | 25 ++++++++++- ql/src/semmle/go/security/SafeUrlFlow.qll | 45 +++++++++++++++++++ .../go/security/SafeUrlFlowCustomizations.qll | 21 +++++++++ 8 files changed, 116 insertions(+), 89 deletions(-) create mode 100644 ql/src/semmle/go/security/SafeUrlFlow.qll create mode 100644 ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll 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.ql b/ql/src/Security/CWE-918/RequestForgery.ql index c3e4e63e51b..4541a687804 100644 --- a/ql/src/Security/CWE-918/RequestForgery.ql +++ b/ql/src/Security/CWE-918/RequestForgery.ql @@ -11,11 +11,12 @@ import go import semmle.go.security.RequestForgery::RequestForgery +import semmle.go.security.SafeUrlFlow import DataFlow::PathGraph from - Configuration cfg, SafeUrlConfiguration scfg, DataFlow::PathNode source, DataFlow::PathNode sink, - DataFlow::Node request + 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() and 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..38a374d9965 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 request forgery, 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 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 = "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/semmle/go/security/RequestForgery.qll b/ql/src/semmle/go/security/RequestForgery.qll index 78eaf605c76..38827b0dfd4 100644 --- a/ql/src/semmle/go/security/RequestForgery.qll +++ b/ql/src/semmle/go/security/RequestForgery.qll @@ -4,7 +4,6 @@ */ import go -import OpenUrlRedirectCustomizations module RequestForgery { import RequestForgeryCustomizations::RequestForgery @@ -40,42 +39,4 @@ 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 cdc55abc4d6..aecd5e077fb 100644 --- a/ql/src/semmle/go/security/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; @@ -59,3 +60,23 @@ module RequestForgery { 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..5f1d79960b2 --- /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 + * `OpenUrlRedirect::Configuration` is needed, otherwise + * `OpenUrlRedirectCustomizations` should be imported instead. + */ + +import go + +module SafeUrlFlow { + import SafeUrlFlowCustomizations::SafeUrlFlow + + /** + * A data-flow 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..b7d7a110ecf --- /dev/null +++ b/ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll @@ -0,0 +1,21 @@ +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, _) } + } +} From 4e5b17e18d1c6604d02ad8e7e8b95a78aa00ae49 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 2 Apr 2020 00:58:09 -0700 Subject: [PATCH 12/17] Sanitize hostname if there is a slash and a previous component --- ql/src/semmle/go/security/UrlConcatenation.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ql/src/semmle/go/security/UrlConcatenation.qll b/ql/src/semmle/go/security/UrlConcatenation.qll index 9c75b3f8279..c0ff4ff960c 100644 --- a/ql/src/semmle/go/security/UrlConcatenation.qll +++ b/ql/src/semmle/go/security/UrlConcatenation.qll @@ -55,6 +55,9 @@ private predicate concatenationHasHostnameSanitizingSubstring(StringOps::Concate exists(StringOps::ConcatenationLeaf lf | lf = cat.getALeaf() | lf.getStringValue().regexpMatch(".*([?#]|[^?#:/\\\\][/\\\\]).*|[/\\\\][^/\\\\].*") or + lf.getStringValue() = "/" and + exists(lf.getPreviousLeaf()) + or hasHostnameSanitizingSubstring(lf.asNode()) ) } From c68e5095084c66611c653cddf0cc687f8bd16404 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 2 Apr 2020 01:47:17 -0700 Subject: [PATCH 13/17] OpenUrlRedirect: Fix some comments --- ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll index 38a374d9965..7c4b0854daf 100644 --- a/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll +++ b/ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll @@ -162,13 +162,13 @@ module OpenUrlRedirect { } } -/** A sink for request forgery, considered as a sink for safe URL flow. */ +/** 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 for request forgery, considered as a sanitizer for a safe + * A read of a field considered unsafe to redirect to, considered as a sanitizer for a safe * URL. */ private class UnsafeFieldReadSanitizer extends SafeUrlFlow::SanitizerEdge { From 3c02b3ab749953324c46d366760869d0339755da Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 2 Apr 2020 01:52:44 -0700 Subject: [PATCH 14/17] Add SafeUrlFlowCustomizations doc comment --- ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll b/ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll index b7d7a110ecf..5e2299c2ed3 100644 --- a/ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll +++ b/ql/src/semmle/go/security/SafeUrlFlowCustomizations.qll @@ -1,3 +1,8 @@ +/** + * 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 { From e27947e280fb054367b9812806cfc2996d017e80 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 2 Apr 2020 02:16:40 -0700 Subject: [PATCH 15/17] Add comment for new url concatenation sanitizer --- ql/src/semmle/go/security/UrlConcatenation.qll | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ql/src/semmle/go/security/UrlConcatenation.qll b/ql/src/semmle/go/security/UrlConcatenation.qll index c0ff4ff960c..9415d4a8acf 100644 --- a/ql/src/semmle/go/security/UrlConcatenation.qll +++ b/ql/src/semmle/go/security/UrlConcatenation.qll @@ -55,6 +55,8 @@ 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 From ea3a7e8038fbed8ee321eaa3752164976e10e70a Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Thu, 2 Apr 2020 01:51:31 -0700 Subject: [PATCH 16/17] Apply suggestions from code review Co-Authored-By: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> --- ql/src/semmle/go/security/SafeUrlFlow.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ql/src/semmle/go/security/SafeUrlFlow.qll b/ql/src/semmle/go/security/SafeUrlFlow.qll index 5f1d79960b2..ff14492aa87 100644 --- a/ql/src/semmle/go/security/SafeUrlFlow.qll +++ b/ql/src/semmle/go/security/SafeUrlFlow.qll @@ -3,8 +3,8 @@ * safe flow from URLs. * * Note, for performance reasons: only import this file if - * `OpenUrlRedirect::Configuration` is needed, otherwise - * `OpenUrlRedirectCustomizations` should be imported instead. + * `SafeUrlFlow::Configuration` is needed, otherwise + * `SafeUrlFlowCustomizations` should be imported instead. */ import go @@ -13,7 +13,7 @@ module SafeUrlFlow { import SafeUrlFlowCustomizations::SafeUrlFlow /** - * A data-flow configuration for reasoning about safe URLs. + * A taint-tracking configuration for reasoning about safe URLs. */ class Configuration extends TaintTracking::Configuration { Configuration() { this = "SafeUrlFlow" } From dcd6aaf69a67ec2e365737993687ea1231518643 Mon Sep 17 00:00:00 2001 From: Sauyon Lee Date: Fri, 3 Apr 2020 00:01:19 -0700 Subject: [PATCH 17/17] Alphabetize change notes --- change-notes/1.24/analysis-go.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/change-notes/1.24/analysis-go.md b/change-notes/1.24/analysis-go.md index 6d02ed26eb9..f71ef2a1613 100644 --- a/change-notes/1.24/analysis-go.md +++ b/change-notes/1.24/analysis-go.md @@ -26,11 +26,11 @@ The CodeQL library for Go now contains a folder of simple "cookbook" queries tha | **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. | -| 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. |