From f261f34f57aa5f36812a03c50ac8487de8b4995c Mon Sep 17 00:00:00 2001 From: Slavomir Date: Sat, 22 May 2021 18:14:13 +0200 Subject: [PATCH 01/14] Add query to detect CORS misconfiguration --- .../CWE-942/CorsMisconfiguration.qhelp | 42 +++++++ .../CWE-942/CorsMisconfiguration.ql | 84 +++++++++++++ .../CWE-942/CorsMisconfigurationBad.go | 34 ++++++ .../CWE-942/CorsMisconfigurationGood.go | 32 +++++ .../CWE-942/CorsMisconfiguration.expected | 7 ++ .../CWE-942/CorsMisconfiguration.go | 110 ++++++++++++++++++ .../CWE-942/CorsMisconfiguration.qlref | 1 + 7 files changed, 310 insertions(+) create mode 100644 ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp create mode 100644 ql/src/experimental/CWE-942/CorsMisconfiguration.ql create mode 100644 ql/src/experimental/CWE-942/CorsMisconfigurationBad.go create mode 100644 ql/src/experimental/CWE-942/CorsMisconfigurationGood.go create mode 100644 ql/test/experimental/CWE-942/CorsMisconfiguration.expected create mode 100755 ql/test/experimental/CWE-942/CorsMisconfiguration.go create mode 100755 ql/test/experimental/CWE-942/CorsMisconfiguration.qlref diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp new file mode 100644 index 00000000000..50a27f669f6 --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp @@ -0,0 +1,42 @@ + + + +

+ When configuring the allowed origins for CORS requests that allow passing cookies + (i.e. the Access-Control-Allow-Credentials is set to true), + if the Access-Control-Allow-Origin header is set to a user-provided value (and not correctly validated), + or is set to special values such as * or null, then the users of that application might be + vulnerable to attacks where the attacker impersonates the user and sends request on their behalf. +

+ + +

+ When configuring CORS that allow credentials passing, + it's best not to use user-provided values for the allowed origins response header, + especially if the cookies grant session permissions on the user's account. +

+

+ It also can be very dangerous to set the allowed origins to * or null (which can be bypassed). +

+ + +

+ The first example shows a few possible CORS misconfiguration cases: +

+ +

+ The second example show better configurations: +

+ + + +
  • + Reference 1: PortSwigger Web Security Academy on CORS. +
    • +
  • + Reference 2: AppSec EU 2017 Exploiting CORS Misconfigurations For Bitcoins And Bounties by James Kettle. +
    • +     +     \ No newline at end of file diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql new file mode 100644 index 00000000000..5c34eaf4fa5 --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -0,0 +1,84 @@ +/** + * @name CORS misconfiguration + * @description If a CORS policy is configured to accept an origin value obtained from the request data, + * or is set to `*` or `null`, and it allows credential sharing, then the users of the + * application are vulnerable to the same range of attacks as in XSS (credential stealing, etc.). + * @kind problem + * @problem.severity warning + * @id go/cors-misconfiguration + * @tags security + * external/cwe/cwe-942 + * external/cwe/cwe-346 + */ + +import go + +/** + * Provides the name of the `Access-Control-Allow-Origin` header key. + */ +string headerAllowOrigin() { result = "Access-Control-Allow-Origin".toLowerCase() } + +/** + * Provides the name of the `Access-Control-Allow-Credentials` header key. + */ +string headerAllowCredentials() { result = "Access-Control-Allow-Credentials".toLowerCase() } + +/** + * A taint-tracking configuration for reasoning about when an UntrustedFlowSource + * flows to a HeaderWrite that writes an `Access-Control-Allow-Origin` header's value. + */ +class FlowsUntrustedToAllowOriginHeader extends TaintTracking::Configuration { + FlowsUntrustedToAllowOriginHeader() { this = "from-untrusted-to-allow-origin-header-value" } + + override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } + + predicate isSink(DataFlow::Node sink, HTTP::HeaderWrite hw) { + hw.getHeaderName() = headerAllowOrigin() and sink = hw.getValue() + } + + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } +} + +/** + * Holds if the provided `allowOriginHW` HeaderWrite's parent ResponseWriter + * also has another HeaderWrite that sets a `Access-Control-Allow-Credentials` + * header to `true`. + */ +predicate allowCredentialsIsSetToTrue(HTTP::HeaderWrite allowOriginHW) { + exists(HTTP::HeaderWrite allowCredentialsHW | + allowCredentialsHW.getHeaderName() = headerAllowCredentials() and + allowCredentialsHW.getHeaderValue() = "true" + | + allowOriginHW.getResponseWriter() = allowCredentialsHW.getResponseWriter() + ) +} + +/** + * Holds if the provided `allowOriginHW` HeaderWrite's value is set using an + * UntrustedFlowSource. + * The `message` parameter is populated with the warning message to be returned by the query. + */ +predicate flowsFromUntrustedToAllowOrigin(HTTP::HeaderWrite allowOriginHW, string message) { + exists(FlowsUntrustedToAllowOriginHeader cfg, DataFlow::PathNode source, DataFlow::PathNode sink | + cfg.hasFlowPath(source, sink) and + cfg.isSink(sink.getNode(), allowOriginHW) + | + message = + headerAllowOrigin() + " header is set to a user-defined value, and " + + headerAllowCredentials() + " is set to `true`" + ) +} + +from HTTP::HeaderWrite allowOriginHW, string message +where + ( + flowsFromUntrustedToAllowOrigin(allowOriginHW, message) + or + allowOriginHW.getHeaderName() = headerAllowOrigin() and + allowOriginHW.getHeaderValue() = ["*", "null"] and + message = + headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " + + headerAllowCredentials() + " is set to `true`" + ) and + allowCredentialsIsSetToTrue(allowOriginHW) +select allowOriginHW, message diff --git a/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go b/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go new file mode 100644 index 00000000000..2c54da4730d --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go @@ -0,0 +1,34 @@ +package main + +import "net/http" + +func main() {} + +// bad is an example of a bad implementation +func bad1() { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: all origins are allowed, + // and Access-Control-Allow-Credentials is set to 'true'. + w.Header().Set("Access-Control-Allow-Origin", "*") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) +} + +func bad2() { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: 'null' origin is allowed, + // and Access-Control-Allow-Credentials is set to 'true'. + w.Header().Set("Access-Control-Allow-Origin", "null") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) +} + +func bad3() { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // and `Access-Control-Allow-Credentials` is set to 'true': + origin := req.Header.Get("origin") + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) +} diff --git a/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go b/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go new file mode 100644 index 00000000000..24fcdd56d80 --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go @@ -0,0 +1,32 @@ +package main + +import "net/http" + +// good1 is an example of a good implementation +func good1() { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: all origins are allowed, + // but Access-Control-Allow-Credentials is not set. + w.Header().Set("Access-Control-Allow-Origin", "*") + }) +} + +func good2() { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: all origins are allowed, + // and some write methods are allowed, + // BUT `Access-Control-Allow-Credentials` is not set: + w.Header().Set("Access-Control-Allow-Origin", "*") + w.Header().Set("Access-Control-Allow-Methods", "GET,POST,PUT") + }) +} + +func good3() { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // BUT `Access-Control-Allow-Credentials` is set to 'false': + origin := req.Header.Get("origin") + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Credentials", "false") + }) +} diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected new file mode 100644 index 00000000000..c1bb884c613 --- /dev/null +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -0,0 +1,7 @@ +| CorsMisconfiguration.go:15:4:15:53 | call to Set | access-control-allow-origin header is set to `*`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:21:4:21:41 | call to Set | access-control-allow-origin header is set to `*`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:41:4:41:56 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:47:4:47:42 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:68:4:68:53 | call to Set | access-control-allow-origin header is set to `*`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:86:4:86:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:93:4:93:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.go b/ql/test/experimental/CWE-942/CorsMisconfiguration.go new file mode 100755 index 00000000000..96517d42a4b --- /dev/null +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.go @@ -0,0 +1,110 @@ +package main + +import "net/http" + +const ( + HeaderAllowOrigin = "Access-Control-Allow-Origin" + HeaderAllowCredentials = "Access-Control-Allow-Credentials" +) + +func main() { + { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: all origins are allowed, + // and Access-Control-Allow-Credentials is set to 'true'. + w.Header().Set("Access-Control-Allow-Origin", "*") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: all origins are allowed, + // and `Access-Control-Allow-Credentials` is set to 'true': + w.Header().Set(HeaderAllowOrigin, "*") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK: all origins are allowed, + // but Access-Control-Allow-Credentials is set to 'false'. + w.Header().Set("Access-Control-Allow-Origin", "*") + w.Header().Set("Access-Control-Allow-Credentials", "false") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK: all origins are allowed, + // but Access-Control-Allow-Credentials is not set. + w.Header().Set("Access-Control-Allow-Origin", "*") + }) + } + { + const Null = "null" + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: 'null' origin is allowed, + // and Access-Control-Allow-Credentials is set to 'true'. + w.Header().Set("Access-Control-Allow-Origin", "null") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: 'null' origin is allowed, + // and `Access-Control-Allow-Credentials` is set to 'true': + w.Header().Set(HeaderAllowOrigin, Null) + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK: 'null' origin is allowed, + // but Access-Control-Allow-Credentials is set to 'false'. + w.Header().Set("Access-Control-Allow-Origin", Null) + w.Header().Set("Access-Control-Allow-Credentials", "false") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK: 'null' origin is allowed, + // but Access-Control-Allow-Credentials is not set. + w.Header().Set("Access-Control-Allow-Origin", Null) + }) + } + + { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: all origins are allowed, + // and some write methods are allowed, + // and `Access-Control-Allow-Credentials` is set to 'true': + w.Header().Set("Access-Control-Allow-Origin", "*") + w.Header().Set("Access-Control-Allow-Methods", "GET,POST,PUT") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK: all origins are allowed, + // and some write methods are allowed, + // BUT `Access-Control-Allow-Credentials` is not set: + w.Header().Set("Access-Control-Allow-Origin", "*") + w.Header().Set("Access-Control-Allow-Methods", "GET,POST,PUT") + }) + } + + { + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // and `Access-Control-Allow-Credentials` is set to 'true': + origin := req.Header.Get("origin") + w.Header().Set(HeaderAllowOrigin, origin) + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // and `Access-Control-Allow-Credentials` is set to 'true': + origin := req.Header.Get("origin") + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set(HeaderAllowCredentials, "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // BUT `Access-Control-Allow-Credentials` is set to 'false': + origin := req.Header.Get("origin") + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Credentials", "false") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // BUT `Access-Control-Allow-Credentials` is not set: + origin := req.Header.Get("origin") + w.Header().Set("Access-Control-Allow-Origin", origin) + }) + } +} diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref b/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref new file mode 100755 index 00000000000..f23401c1091 --- /dev/null +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref @@ -0,0 +1 @@ +experimental/CWE-942/CorsMisconfiguration.ql From 924e445ce991ca4d4e5a7de66408a58a4026ea2e Mon Sep 17 00:00:00 2001 From: Slavomir Date: Sat, 22 May 2021 18:19:44 +0200 Subject: [PATCH 02/14] Add missing newline --- ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp index 50a27f669f6..c6493645c63 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp @@ -39,4 +39,4 @@ Reference 2: AppSec EU 2017 Exploiting CORS Misconfigurations For Bitcoins And Bounties by James Kettle. - \ No newline at end of file + From 9d1f13fe9be5d425bf88cdc60e5de1d7d2b0cb03 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Sat, 22 May 2021 18:32:48 +0200 Subject: [PATCH 03/14] Add `allowOriginIsWildcardOrNull` predicate --- .../CWE-942/CorsMisconfiguration.ql | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 5c34eaf4fa5..8aa06faccce 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -69,16 +69,24 @@ predicate flowsFromUntrustedToAllowOrigin(HTTP::HeaderWrite allowOriginHW, strin ) } +/** + * Holds if the provided `allowOriginHW` HeaderWrite is for a `Access-Control-Allow-Origin` + * header and the value is set to `*` or `null`. + */ +predicate allowOriginIsWildcardOrNull(HTTP::HeaderWrite allowOriginHW, string message) { + allowOriginHW.getHeaderName() = headerAllowOrigin() and + allowOriginHW.getHeaderValue() = ["*", "null"] and + message = + headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " + + headerAllowCredentials() + " is set to `true`" +} + from HTTP::HeaderWrite allowOriginHW, string message where ( flowsFromUntrustedToAllowOrigin(allowOriginHW, message) or - allowOriginHW.getHeaderName() = headerAllowOrigin() and - allowOriginHW.getHeaderValue() = ["*", "null"] and - message = - headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " + - headerAllowCredentials() + " is set to `true`" + allowOriginIsWildcardOrNull(allowOriginHW, message) ) and allowCredentialsIsSetToTrue(allowOriginHW) select allowOriginHW, message From 74f8f1dcdb2b6418efd1a6b24ad1da16ad5429f4 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 24 May 2021 15:19:35 +0200 Subject: [PATCH 04/14] Cleanup --- ql/src/experimental/CWE-942/CorsMisconfiguration.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 8aa06faccce..eb9cc52d8fe 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -83,10 +83,10 @@ predicate allowOriginIsWildcardOrNull(HTTP::HeaderWrite allowOriginHW, string me from HTTP::HeaderWrite allowOriginHW, string message where + allowCredentialsIsSetToTrue(allowOriginHW) and ( flowsFromUntrustedToAllowOrigin(allowOriginHW, message) or allowOriginIsWildcardOrNull(allowOriginHW, message) - ) and - allowCredentialsIsSetToTrue(allowOriginHW) + ) select allowOriginHW, message From 8525c58e1ae34f0a813ac17cdfae34d1fe9f0619 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 24 May 2021 15:19:50 +0200 Subject: [PATCH 05/14] Improve qhelp doc --- .../CWE-942/CorsMisconfiguration.qhelp | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp index c6493645c63..cbcf206c01a 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp @@ -4,11 +4,33 @@

    - When configuring the allowed origins for CORS requests that allow passing cookies - (i.e. the Access-Control-Allow-Credentials is set to true), - if the Access-Control-Allow-Origin header is set to a user-provided value (and not correctly validated), - or is set to special values such as * or null, then the users of that application might be - vulnerable to attacks where the attacker impersonates the user and sends request on their behalf. + Web browsers, by default, disallow cross-origin resource sharing via direct HTTP requests (i.e. using a JavaScript HTTP client). + Still, to satisfy some needs that arose with the growth of the web, an expedient was created to make exceptions possible. + CORS (Cross-origin resource sharing) is a mechanism that allows resources of a web endpoint (let's call it "Peer A") + to be accessed from another web page belonging to a different domain ("Peer B"). +

    +

    + For that to happen, Peer A needs to make available its CORS configuration via special headers on the desired endpoint + via the OPTIONS method. +

    +

    + This configuration can also allow the inclusion of cookies on the cross-origin request, + (i.e. when the Access-Control-Allow-Credentials header is set to true) + meaning that Peer B can send a request to Peer A that will include the cookies as if the request was executed by the user. +

    +

    + That can have dangerous effects if Peer B origin is not restricted correctly. + An example of a dangerous scenario is when Access-Control-Allow-Origin header is set to a value gotten from the Peer B's request + (and not correctly validated), or is set to special values such as * or null. + The above values can allow any Peer B to send requests to the misconfigured Peer A on behalf of the user. +

    +

    + Example scenario: + User is client of a bank that has its API misconfigured to accept CORS requests from any domain. + When the user loads an evil page, the evil page sends a request to the bank's API to transfer all funds + to evil party's account. + Given that the user was already logged in to the bank website, and had its session cookies set, + the evil party's request succeeds.

    From 56e99b6efb9b9f0161df52a461977e5d948c53cd Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 3 Jun 2021 10:50:50 +0200 Subject: [PATCH 06/14] Convert header values to lowercase before comparing --- ql/src/experimental/CWE-942/CorsMisconfiguration.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index eb9cc52d8fe..9099a6685a9 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -47,7 +47,7 @@ class FlowsUntrustedToAllowOriginHeader extends TaintTracking::Configuration { predicate allowCredentialsIsSetToTrue(HTTP::HeaderWrite allowOriginHW) { exists(HTTP::HeaderWrite allowCredentialsHW | allowCredentialsHW.getHeaderName() = headerAllowCredentials() and - allowCredentialsHW.getHeaderValue() = "true" + allowCredentialsHW.getHeaderValue().toLowerCase() = "true" | allowOriginHW.getResponseWriter() = allowCredentialsHW.getResponseWriter() ) @@ -75,7 +75,7 @@ predicate flowsFromUntrustedToAllowOrigin(HTTP::HeaderWrite allowOriginHW, strin */ predicate allowOriginIsWildcardOrNull(HTTP::HeaderWrite allowOriginHW, string message) { allowOriginHW.getHeaderName() = headerAllowOrigin() and - allowOriginHW.getHeaderValue() = ["*", "null"] and + allowOriginHW.getHeaderValue().toLowerCase() = ["*", "null"] and message = headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " + headerAllowCredentials() + " is set to `true`" From 4662358b8d58624f598bfee9a5a361b9e7364d2a Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 3 Jun 2021 12:53:52 +0200 Subject: [PATCH 07/14] Add flag checks --- .../CWE-942/CorsMisconfiguration.ql | 26 ++++++++ .../CWE-942/CorsMisconfiguration.expected | 1 + .../CWE-942/CorsMisconfiguration.go | 62 +++++++++++++++++++ 3 files changed, 89 insertions(+) mode change 100755 => 100644 ql/test/experimental/CWE-942/CorsMisconfiguration.go diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 9099a6685a9..a2c6bd3ef08 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -12,6 +12,19 @@ */ import go +import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag + +/** + * A flag indicating a check for satisfied permissions or test configuration. + */ +class AllowedFlag extends FlagKind { + AllowedFlag() { this = "allowed" } + + bindingset[result] + override string getAFlagName() { + result.regexpMatch("(?i).*(allow|match|check|debug|devel|insecure).*") + } +} /** * Provides the name of the `Access-Control-Allow-Origin` header key. @@ -36,6 +49,14 @@ class FlowsUntrustedToAllowOriginHeader extends TaintTracking::Configuration { hw.getHeaderName() = headerAllowOrigin() and sink = hw.getValue() } + override predicate isSanitizer(DataFlow::Node node) { + exists(ControlFlow::ConditionGuardNode cgn | + cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) + | + cgn.dominates(node.getBasicBlock()) + ) + } + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } } @@ -88,5 +109,10 @@ where flowsFromUntrustedToAllowOrigin(allowOriginHW, message) or allowOriginIsWildcardOrNull(allowOriginHW, message) + ) and + not exists(ControlFlow::ConditionGuardNode cgn | + cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) + | + cgn.dominates(allowOriginHW.getBasicBlock()) ) select allowOriginHW, message diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected index c1bb884c613..40ab35a0d87 100644 --- a/ql/test/experimental/CWE-942/CorsMisconfiguration.expected +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -5,3 +5,4 @@ | CorsMisconfiguration.go:68:4:68:53 | call to Set | access-control-allow-origin header is set to `*`, and access-control-allow-credentials is set to `true` | | CorsMisconfiguration.go:86:4:86:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | | CorsMisconfiguration.go:93:4:93:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:100:5:100:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.go b/ql/test/experimental/CWE-942/CorsMisconfiguration.go old mode 100755 new mode 100644 index 96517d42a4b..5570f3c243e --- a/ql/test/experimental/CWE-942/CorsMisconfiguration.go +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.go @@ -93,6 +93,14 @@ func main() { w.Header().Set("Access-Control-Allow-Origin", origin) w.Header().Set(HeaderAllowCredentials, "true") }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // and `Access-Control-Allow-Credentials` is set to 'true': + if origin := req.Header.Get("Origin"); origin != "" { + w.Header().Set("Access-Control-Allow-Origin", origin) + } + w.Header().Set(HeaderAllowCredentials, "true") + }) http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, // BUT `Access-Control-Allow-Credentials` is set to 'false': @@ -106,5 +114,59 @@ func main() { origin := req.Header.Get("origin") w.Header().Set("Access-Control-Allow-Origin", origin) }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // and `Access-Control-Allow-Credentials` is set to 'true', + // BUT there is a check on the origin header: + origin := req.Header.Get("origin") + if allowedOrigin(origin) { + w.Header().Set("Access-Control-Allow-Origin", origin) + } + w.Header().Set(HeaderAllowCredentials, "true") + }) + allowedOrigins := map[string]bool{ + "example.com": true, + } + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // and `Access-Control-Allow-Credentials` is set to 'true', + // BUT there is a check on the origin header: + origin := req.Header.Get("origin") + isAllowed := allowedOrigins[origin] + if isAllowed { + w.Header().Set("Access-Control-Allow-Origin", origin) + } + w.Header().Set(HeaderAllowCredentials, "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, + // and `Access-Control-Allow-Credentials` is set to 'true', + // BUT there is a check on the origin header: + origin := req.Header.Get("origin") + allowAllHosts := true + allowedHost := isAllowedHost([]string{"example.com"}, origin) + if allowAllHosts { + w.Header().Set("Access-Control-Allow-Origin", "*") + } else if allowedHost { + w.Header().Set("Access-Control-Allow-Origin", origin) + } + w.Header().Set(HeaderAllowCredentials, "true") + }) } } + +func isAllowedHost(allowed []string, try string) bool { + for _, v := range allowed { + if try == v { + return true + } + } + return false +} + +func allowedOrigin(origin string) bool { + if origin == "example.com" { + return true + } + return false +} From 824b5a4b52b92859b4963f702b3c117e729b815e Mon Sep 17 00:00:00 2001 From: Slavomir Date: Sat, 5 Jun 2021 10:40:28 +0200 Subject: [PATCH 08/14] Wildcard origin does not allow `Access-Control-Allow-Credentials: true` --- .../CWE-942/CorsMisconfiguration.qhelp | 2 +- .../CWE-942/CorsMisconfiguration.ql | 8 ++-- .../CWE-942/CorsMisconfigurationBad.go | 12 +----- .../CWE-942/CorsMisconfigurationGood.go | 19 ---------- .../CWE-942/CorsMisconfiguration.expected | 13 +++---- .../CWE-942/CorsMisconfiguration.go | 38 +------------------ 6 files changed, 12 insertions(+), 80 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp index cbcf206c01a..eafd34cd6c5 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp @@ -40,7 +40,7 @@ especially if the cookies grant session permissions on the user's account.

    - It also can be very dangerous to set the allowed origins to * or null (which can be bypassed). + It also can be very dangerous to set the allowed origins to null (which can be bypassed).

    diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index a2c6bd3ef08..44f4848b574 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -92,11 +92,11 @@ predicate flowsFromUntrustedToAllowOrigin(HTTP::HeaderWrite allowOriginHW, strin /** * Holds if the provided `allowOriginHW` HeaderWrite is for a `Access-Control-Allow-Origin` - * header and the value is set to `*` or `null`. + * header and the value is set to `null`. */ -predicate allowOriginIsWildcardOrNull(HTTP::HeaderWrite allowOriginHW, string message) { +predicate allowOriginIsNull(HTTP::HeaderWrite allowOriginHW, string message) { allowOriginHW.getHeaderName() = headerAllowOrigin() and - allowOriginHW.getHeaderValue().toLowerCase() = ["*", "null"] and + allowOriginHW.getHeaderValue().toLowerCase() = "null" and message = headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " + headerAllowCredentials() + " is set to `true`" @@ -108,7 +108,7 @@ where ( flowsFromUntrustedToAllowOrigin(allowOriginHW, message) or - allowOriginIsWildcardOrNull(allowOriginHW, message) + allowOriginIsNull(allowOriginHW, message) ) and not exists(ControlFlow::ConditionGuardNode cgn | cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) diff --git a/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go b/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go index 2c54da4730d..470735eb027 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go +++ b/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go @@ -4,17 +4,7 @@ import "net/http" func main() {} -// bad is an example of a bad implementation func bad1() { - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // BAD: all origins are allowed, - // and Access-Control-Allow-Credentials is set to 'true'. - w.Header().Set("Access-Control-Allow-Origin", "*") - w.Header().Set("Access-Control-Allow-Credentials", "true") - }) -} - -func bad2() { http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // BAD: 'null' origin is allowed, // and Access-Control-Allow-Credentials is set to 'true'. @@ -23,7 +13,7 @@ func bad2() { }) } -func bad3() { +func bad2() { http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, // and `Access-Control-Allow-Credentials` is set to 'true': diff --git a/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go b/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go index 24fcdd56d80..1924b004bdf 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go +++ b/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go @@ -2,26 +2,7 @@ package main import "net/http" -// good1 is an example of a good implementation func good1() { - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // OK-ish: all origins are allowed, - // but Access-Control-Allow-Credentials is not set. - w.Header().Set("Access-Control-Allow-Origin", "*") - }) -} - -func good2() { - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // OK-ish: all origins are allowed, - // and some write methods are allowed, - // BUT `Access-Control-Allow-Credentials` is not set: - w.Header().Set("Access-Control-Allow-Origin", "*") - w.Header().Set("Access-Control-Allow-Methods", "GET,POST,PUT") - }) -} - -func good3() { http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value, // BUT `Access-Control-Allow-Credentials` is set to 'false': diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected index 40ab35a0d87..6eedef35678 100644 --- a/ql/test/experimental/CWE-942/CorsMisconfiguration.expected +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -1,8 +1,5 @@ -| CorsMisconfiguration.go:15:4:15:53 | call to Set | access-control-allow-origin header is set to `*`, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:21:4:21:41 | call to Set | access-control-allow-origin header is set to `*`, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:41:4:41:56 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:47:4:47:42 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:68:4:68:53 | call to Set | access-control-allow-origin header is set to `*`, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:86:4:86:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:93:4:93:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:100:5:100:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:23:4:23:56 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:29:4:29:42 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:50:4:50:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:57:4:57:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:64:5:64:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.go b/ql/test/experimental/CWE-942/CorsMisconfiguration.go index 5570f3c243e..20d90e0a251 100644 --- a/ql/test/experimental/CWE-942/CorsMisconfiguration.go +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.go @@ -9,24 +9,6 @@ const ( func main() { { - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // BAD: all origins are allowed, - // and Access-Control-Allow-Credentials is set to 'true'. - w.Header().Set("Access-Control-Allow-Origin", "*") - w.Header().Set("Access-Control-Allow-Credentials", "true") - }) - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // BAD: all origins are allowed, - // and `Access-Control-Allow-Credentials` is set to 'true': - w.Header().Set(HeaderAllowOrigin, "*") - w.Header().Set("Access-Control-Allow-Credentials", "true") - }) - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // OK: all origins are allowed, - // but Access-Control-Allow-Credentials is set to 'false'. - w.Header().Set("Access-Control-Allow-Origin", "*") - w.Header().Set("Access-Control-Allow-Credentials", "false") - }) http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // OK: all origins are allowed, // but Access-Control-Allow-Credentials is not set. @@ -60,24 +42,6 @@ func main() { }) } - { - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // BAD: all origins are allowed, - // and some write methods are allowed, - // and `Access-Control-Allow-Credentials` is set to 'true': - w.Header().Set("Access-Control-Allow-Origin", "*") - w.Header().Set("Access-Control-Allow-Methods", "GET,POST,PUT") - w.Header().Set("Access-Control-Allow-Credentials", "true") - }) - http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { - // OK: all origins are allowed, - // and some write methods are allowed, - // BUT `Access-Control-Allow-Credentials` is not set: - w.Header().Set("Access-Control-Allow-Origin", "*") - w.Header().Set("Access-Control-Allow-Methods", "GET,POST,PUT") - }) - } - { http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, @@ -149,8 +113,8 @@ func main() { w.Header().Set("Access-Control-Allow-Origin", "*") } else if allowedHost { w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set(HeaderAllowCredentials, "true") } - w.Header().Set(HeaderAllowCredentials, "true") }) } } From c0f195ba164df16410583c27c37d0f1e496d6b73 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Sat, 19 Jun 2021 22:25:51 +0200 Subject: [PATCH 09/14] Reduce false positives --- .../CWE-942/CorsMisconfiguration.ql | 78 ++++++++++++ .../CWE-942/CorsMisconfiguration.expected | 10 +- .../CWE-942/CorsMisconfiguration.go | 113 +++++++++++++++++- 3 files changed, 195 insertions(+), 6 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 44f4848b574..b05e90a6e62 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -102,6 +102,83 @@ predicate allowOriginIsNull(HTTP::HeaderWrite allowOriginHW, string message) { headerAllowCredentials() + " is set to `true`" } +/** + * A read on a map type. + */ +class MapRead extends DataFlow::ElementReadNode { + MapRead() { this.getBase().getType() instanceof MapType } +} + +/** + * A taint-tracking configuration for reasoning about when an UntrustedFlowSource + * flows somewhere. + */ +class FlowsFromUntrusted extends TaintTracking::Configuration { + FlowsFromUntrusted() { this = "from-untrusted" } + + override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } + + override predicate isSink(DataFlow::Node sink) { any() } +} + +/** + * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`. + */ +predicate untrustedFlowsToExpr(Expr dst) { + exists(FlowsFromUntrusted cfg, DataFlow::PathNode source, DataFlow::PathNode sink | + cfg.hasFlowPath(source, sink) and + sink.getNode().asExpr() = dst + ) +} + +/** + * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`. + */ +predicate untrustedFlowsTo(DataFlow::Node dst) { untrustedFlowsToExpr(dst.asExpr()) } + +/** + * Holds if the provided `allowOriginHW` is guarded by a check on an `UntrustedFlowSource` + * which (supposedly) is an `Origin` header. + */ +predicate isGuardedByCheckOnUntrusted(HTTP::HeaderWrite allowOriginHW) { + exists(ControlFlow::ConditionGuardNode cgn, IfStmt ifs | + exists(Expr child, Expr operand | + child = ifs.getCond().getAChildExpr*() and + ( + operand = child or + operand = child.(LorExpr).getAnOperand() or + operand = child.(LandExpr).getAnOperand() + ) and + ( + exists(DataFlow::CallExpr call | call = operand | + call.getTarget().hasQualifiedName("strings", "HasSuffix") and + untrustedFlowsToExpr(call.getArgument(0)) + ) + or + exists(MapRead mapRead | + operand = mapRead.asExpr() and + untrustedFlowsTo(mapRead.getIndex().getAPredecessor*()) + // TODO: add _, ok : map[untrusted]; ok + ) + or + exists(EqlExpr comp | + operand = comp and + ( + untrustedFlowsToExpr(comp.getLeftOperand()) and + not comp.getRightOperand().(StringLit).getStringValue() = "" + or + untrustedFlowsToExpr(comp.getRightOperand()) and + not comp.getLeftOperand().(StringLit).getStringValue() = "" + ) + ) + ) + ) + | + cgn.getCondition() = ifs.getCond() and + cgn.dominates(allowOriginHW.getBasicBlock()) + ) +} + from HTTP::HeaderWrite allowOriginHW, string message where allowCredentialsIsSetToTrue(allowOriginHW) and @@ -110,6 +187,7 @@ where or allowOriginIsNull(allowOriginHW, message) ) and + not isGuardedByCheckOnUntrusted(allowOriginHW) and not exists(ControlFlow::ConditionGuardNode cgn | cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) | diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected index 6eedef35678..2ab9324fe8c 100644 --- a/ql/test/experimental/CWE-942/CorsMisconfiguration.expected +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -1,5 +1,5 @@ -| CorsMisconfiguration.go:23:4:23:56 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:29:4:29:42 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:50:4:50:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:57:4:57:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | -| CorsMisconfiguration.go:64:5:64:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:26:4:26:56 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:32:4:32:42 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:53:4:53:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:60:4:60:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | +| CorsMisconfiguration.go:67:5:67:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | diff --git a/ql/test/experimental/CWE-942/CorsMisconfiguration.go b/ql/test/experimental/CWE-942/CorsMisconfiguration.go index 20d90e0a251..cac752dbcb2 100644 --- a/ql/test/experimental/CWE-942/CorsMisconfiguration.go +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.go @@ -1,6 +1,9 @@ package main -import "net/http" +import ( + "net/http" + "strings" +) const ( HeaderAllowOrigin = "Access-Control-Allow-Origin" @@ -116,9 +119,117 @@ func main() { w.Header().Set(HeaderAllowCredentials, "true") } }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the input origin header is validated agains a whitelist. + responseHeader := w.Header() + { + origin := req.Header.Get("origin") + if origin != "" && origin != "null" { + if len(AccessControlAllowOrigins) == 0 || + AccessControlAllowOrigins[origin] { + responseHeader.Set("Access-Control-Allow-Origin", origin) + responseHeader.Set("Access-Control-Allow-Credentials", "true") + } + } else { + responseHeader.Set("Access-Control-Allow-Origin", "*") + } + } + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + originSuffix := ".example.com" + // OK-ish: the input origin header is validated agains a suffix. + origin := req.Header.Get("Origin") + if origin != "" && (originSuffix == "" || strings.HasSuffix(origin, originSuffix)) { + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Credentials", "true") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type, x-requested-by, *") + w.Header().Set("Access-Control-Allow-Methods", "GET, PUT, POST, DELETE, OPTIONS") + if req.Method == http.MethodOptions { + w.WriteHeader(http.StatusOK) + return + } + } + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + originSuffix := ".example.com" + // OK-ish: the input origin header is validated agains a whitelist. + origin := req.Header.Get("Origin") + if origin != "" && (originSuffix == "" || AccessControlAllowOrigins[origin]) { + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Credentials", "true") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type, x-requested-by, *") + w.Header().Set("Access-Control-Allow-Methods", "GET, PUT, POST, DELETE, OPTIONS") + if req.Method == http.MethodOptions { + w.WriteHeader(http.StatusOK) + return + } + } + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the input origin header is validated agains a whitelist. + origin := req.Header.Get("origin") + if origin != "" && origin != "null" { + if len(AccessControlAllowOrigins) == 0 || AccessControlAllowOrigins[origin] { + w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Credentials", "true") + } + } else { + w.Header().Set("Access-Control-Allow-Origin", "*") + } + }) + // http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // // OK-ish: the input origin header is validated agains a whitelist. + // origin := req.Header.Get("origin") + // if origin != "" && origin != "null" { + // if _, ok := AccessControlAllowOrigins[origin]; ok { + // w.Header().Set("Access-Control-Allow-Origin", origin) + // w.Header().Set("Access-Control-Allow-Credentials", "true") + // } + // } else { + // w.Header().Set("Access-Control-Allow-Origin", "*") + // } + // }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the input origin header is validated agains a whitelist. + if origin := req.Header.Get("Origin"); cors[origin] { + w.Header().Set("Access-Control-Allow-Origin", origin) + } else if len(origin) > 0 && cors["*"] { + w.Header().Set("Access-Control-Allow-Origin", origin) + } + + w.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Content-Length, Accept-Encoding, X-Token, X-Client") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) + http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { + // OK-ish: the input origin header is validated agains a whitelist. + origin := req.Header.Get("origin") + for _, v := range GetAllowOrigin() { + if v == origin { + w.Header().Add("Access-Control-Allow-Origin", origin) + break + } + } + + w.Header().Set("Access-Control-Allow-Methods", "POST, GET, OPTIONS") + w.Header().Set("Access-Control-Allow-Headers", "Content-Type, Content-Length, Accept-Encoding, X-Token, X-Client") + w.Header().Set("Access-Control-Allow-Credentials", "true") + }) } } +var ( + cors = map[string]bool{"*": true} +) + +func GetAllowOrigin() []string { + return []string{ + "example.com", + } +} + +var AccessControlAllowOrigins map[string]bool + func isAllowedHost(allowed []string, try string) bool { for _, v := range allowed { if try == v { From 66bd56f444e48371a2333f84f6edbcf5beee3ae2 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Mon, 5 Jul 2021 13:14:56 +0200 Subject: [PATCH 10/14] Don't use any() as sink --- .../CWE-942/CorsMisconfiguration.ql | 99 +++++++++---------- 1 file changed, 47 insertions(+), 52 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index b05e90a6e62..8140dc2d74a 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -118,63 +118,58 @@ class FlowsFromUntrusted extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } - override predicate isSink(DataFlow::Node sink) { any() } -} + override predicate isSink(DataFlow::Node sink) { isSink(sink, _) } -/** - * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`. - */ -predicate untrustedFlowsToExpr(Expr dst) { - exists(FlowsFromUntrusted cfg, DataFlow::PathNode source, DataFlow::PathNode sink | - cfg.hasFlowPath(source, sink) and - sink.getNode().asExpr() = dst - ) -} - -/** - * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`. - */ -predicate untrustedFlowsTo(DataFlow::Node dst) { untrustedFlowsToExpr(dst.asExpr()) } - -/** - * Holds if the provided `allowOriginHW` is guarded by a check on an `UntrustedFlowSource` - * which (supposedly) is an `Origin` header. - */ -predicate isGuardedByCheckOnUntrusted(HTTP::HeaderWrite allowOriginHW) { - exists(ControlFlow::ConditionGuardNode cgn, IfStmt ifs | - exists(Expr child, Expr operand | - child = ifs.getCond().getAChildExpr*() and - ( - operand = child or - operand = child.(LorExpr).getAnOperand() or - operand = child.(LandExpr).getAnOperand() - ) and - ( - exists(DataFlow::CallExpr call | call = operand | - call.getTarget().hasQualifiedName("strings", "HasSuffix") and - untrustedFlowsToExpr(call.getArgument(0)) - ) - or - exists(MapRead mapRead | - operand = mapRead.asExpr() and - untrustedFlowsTo(mapRead.getIndex().getAPredecessor*()) - // TODO: add _, ok : map[untrusted]; ok - ) - or - exists(EqlExpr comp | - operand = comp and - ( - untrustedFlowsToExpr(comp.getLeftOperand()) and - not comp.getRightOperand().(StringLit).getStringValue() = "" - or - untrustedFlowsToExpr(comp.getRightOperand()) and - not comp.getLeftOperand().(StringLit).getStringValue() = "" + predicate isSink(DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn) { + exists(IfStmt ifs | + exists(Expr child, Expr operand | + child = ifs.getCond().getAChildExpr*() and + ( + operand = child or + operand = child.(LorExpr).getAnOperand() or + operand = child.(LandExpr).getAnOperand() + ) and + ( + // + exists(DataFlow::CallExpr call | call = operand | + call.getTarget().hasQualifiedName("strings", "HasSuffix") and + sink.asExpr() = call.getArgument(0) + ) + or + exists(MapRead mapRead | + operand = mapRead.asExpr() and + sink = mapRead.getIndex().getAPredecessor*() + // TODO: add _, ok : map[untrusted]; ok + ) + or + exists(EqlExpr comp | + operand = comp and + ( + sink.asExpr() = comp.getLeftOperand() and + not comp.getRightOperand().(StringLit).getStringValue() = "" + or + sink.asExpr() = comp.getRightOperand() and + not comp.getLeftOperand().(StringLit).getStringValue() = "" + ) ) ) ) + | + cgn.getCondition() = ifs.getCond() ) + } +} + +/** + * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`. + */ +predicate flowsToGuardedByCheckOnUntrusted(HTTP::HeaderWrite allowOriginHW) { + exists( + FlowsFromUntrusted cfg, DataFlow::PathNode source, DataFlow::PathNode sink, + ControlFlow::ConditionGuardNode cgn + | + cfg.hasFlowPath(source, sink) and cfg.isSink(sink.getNode(), cgn) | - cgn.getCondition() = ifs.getCond() and cgn.dominates(allowOriginHW.getBasicBlock()) ) } @@ -187,7 +182,7 @@ where or allowOriginIsNull(allowOriginHW, message) ) and - not isGuardedByCheckOnUntrusted(allowOriginHW) and + not flowsToGuardedByCheckOnUntrusted(allowOriginHW) and not exists(ControlFlow::ConditionGuardNode cgn | cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) | From 92e0f02d2a2908baa0f28c3faecbddb37aaf5e6d Mon Sep 17 00:00:00 2001 From: Slavomir Date: Thu, 15 Jul 2021 15:06:28 +0200 Subject: [PATCH 11/14] Remove special cases inside if --- ql/src/experimental/CWE-942/CorsMisconfiguration.ql | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 8140dc2d74a..bd6297afb92 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -124,11 +124,7 @@ class FlowsFromUntrusted extends TaintTracking::Configuration { exists(IfStmt ifs | exists(Expr child, Expr operand | child = ifs.getCond().getAChildExpr*() and - ( - operand = child or - operand = child.(LorExpr).getAnOperand() or - operand = child.(LandExpr).getAnOperand() - ) and + operand = child and ( // exists(DataFlow::CallExpr call | call = operand | From e92738a93fa493a1245f7acd8a524bb254a04277 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Fri, 16 Jul 2021 00:42:36 +0300 Subject: [PATCH 12/14] Apply suggestions from code review Co-authored-by: Chris Smowton --- ql/src/experimental/CWE-942/CorsMisconfiguration.ql | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index bd6297afb92..17f9dd54753 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -122,11 +122,9 @@ class FlowsFromUntrusted extends TaintTracking::Configuration { predicate isSink(DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn) { exists(IfStmt ifs | - exists(Expr child, Expr operand | - child = ifs.getCond().getAChildExpr*() and - operand = child and + exists(Expr operand | + operand = ifs.getCond().getAChildExpr*() and ( - // exists(DataFlow::CallExpr call | call = operand | call.getTarget().hasQualifiedName("strings", "HasSuffix") and sink.asExpr() = call.getArgument(0) From 52b650a1be5f1e3ba755ce45dda42f65a4665bc5 Mon Sep 17 00:00:00 2001 From: Slavomir Date: Fri, 16 Jul 2021 00:01:55 +0200 Subject: [PATCH 13/14] Add AllowOriginHeaderWrite and AllowCredentialsHeaderWrite classes --- .../CWE-942/CorsMisconfiguration.ql | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 17f9dd54753..6efb24c852a 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -36,6 +36,20 @@ string headerAllowOrigin() { result = "Access-Control-Allow-Origin".toLowerCase( */ string headerAllowCredentials() { result = "Access-Control-Allow-Credentials".toLowerCase() } +/** + * An `Access-Control-Allow-Origin` header write. + */ +class AllowOriginHeaderWrite extends HTTP::HeaderWrite { + AllowOriginHeaderWrite() { this.getHeaderName() = headerAllowOrigin() } +} + +/** + * An `Access-Control-Allow-Credentials` header write. + */ +class AllowCredentialsHeaderWrite extends HTTP::HeaderWrite { + AllowCredentialsHeaderWrite() { this.getHeaderName() = headerAllowCredentials() } +} + /** * A taint-tracking configuration for reasoning about when an UntrustedFlowSource * flows to a HeaderWrite that writes an `Access-Control-Allow-Origin` header's value. @@ -45,9 +59,7 @@ class FlowsUntrustedToAllowOriginHeader extends TaintTracking::Configuration { override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource } - predicate isSink(DataFlow::Node sink, HTTP::HeaderWrite hw) { - hw.getHeaderName() = headerAllowOrigin() and sink = hw.getValue() - } + predicate isSink(DataFlow::Node sink, AllowOriginHeaderWrite hw) { sink = hw.getValue() } override predicate isSanitizer(DataFlow::Node node) { exists(ControlFlow::ConditionGuardNode cgn | @@ -65,9 +77,8 @@ class FlowsUntrustedToAllowOriginHeader extends TaintTracking::Configuration { * also has another HeaderWrite that sets a `Access-Control-Allow-Credentials` * header to `true`. */ -predicate allowCredentialsIsSetToTrue(HTTP::HeaderWrite allowOriginHW) { - exists(HTTP::HeaderWrite allowCredentialsHW | - allowCredentialsHW.getHeaderName() = headerAllowCredentials() and +predicate allowCredentialsIsSetToTrue(AllowOriginHeaderWrite allowOriginHW) { + exists(AllowCredentialsHeaderWrite allowCredentialsHW | allowCredentialsHW.getHeaderValue().toLowerCase() = "true" | allowOriginHW.getResponseWriter() = allowCredentialsHW.getResponseWriter() @@ -79,7 +90,7 @@ predicate allowCredentialsIsSetToTrue(HTTP::HeaderWrite allowOriginHW) { * UntrustedFlowSource. * The `message` parameter is populated with the warning message to be returned by the query. */ -predicate flowsFromUntrustedToAllowOrigin(HTTP::HeaderWrite allowOriginHW, string message) { +predicate flowsFromUntrustedToAllowOrigin(AllowOriginHeaderWrite allowOriginHW, string message) { exists(FlowsUntrustedToAllowOriginHeader cfg, DataFlow::PathNode source, DataFlow::PathNode sink | cfg.hasFlowPath(source, sink) and cfg.isSink(sink.getNode(), allowOriginHW) @@ -94,8 +105,7 @@ predicate flowsFromUntrustedToAllowOrigin(HTTP::HeaderWrite allowOriginHW, strin * Holds if the provided `allowOriginHW` HeaderWrite is for a `Access-Control-Allow-Origin` * header and the value is set to `null`. */ -predicate allowOriginIsNull(HTTP::HeaderWrite allowOriginHW, string message) { - allowOriginHW.getHeaderName() = headerAllowOrigin() and +predicate allowOriginIsNull(AllowOriginHeaderWrite allowOriginHW, string message) { allowOriginHW.getHeaderValue().toLowerCase() = "null" and message = headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " + @@ -157,7 +167,7 @@ class FlowsFromUntrusted extends TaintTracking::Configuration { /** * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`. */ -predicate flowsToGuardedByCheckOnUntrusted(HTTP::HeaderWrite allowOriginHW) { +predicate flowsToGuardedByCheckOnUntrusted(AllowOriginHeaderWrite allowOriginHW) { exists( FlowsFromUntrusted cfg, DataFlow::PathNode source, DataFlow::PathNode sink, ControlFlow::ConditionGuardNode cgn @@ -168,7 +178,7 @@ predicate flowsToGuardedByCheckOnUntrusted(HTTP::HeaderWrite allowOriginHW) { ) } -from HTTP::HeaderWrite allowOriginHW, string message +from AllowOriginHeaderWrite allowOriginHW, string message where allowCredentialsIsSetToTrue(allowOriginHW) and ( From 87afdae1c751b34ca5ec9c2245b11a3b9bc5df38 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 16 Jul 2021 10:47:06 +0100 Subject: [PATCH 14/14] use hasFlowTo where possible --- ql/src/experimental/CWE-942/CorsMisconfiguration.ql | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql index 6efb24c852a..fa07af5914f 100644 --- a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -91,9 +91,9 @@ predicate allowCredentialsIsSetToTrue(AllowOriginHeaderWrite allowOriginHW) { * The `message` parameter is populated with the warning message to be returned by the query. */ predicate flowsFromUntrustedToAllowOrigin(AllowOriginHeaderWrite allowOriginHW, string message) { - exists(FlowsUntrustedToAllowOriginHeader cfg, DataFlow::PathNode source, DataFlow::PathNode sink | - cfg.hasFlowPath(source, sink) and - cfg.isSink(sink.getNode(), allowOriginHW) + exists(FlowsUntrustedToAllowOriginHeader cfg, DataFlow::Node sink | + cfg.hasFlowTo(sink) and + cfg.isSink(sink, allowOriginHW) | message = headerAllowOrigin() + " header is set to a user-defined value, and " + @@ -168,11 +168,8 @@ class FlowsFromUntrusted extends TaintTracking::Configuration { * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`. */ predicate flowsToGuardedByCheckOnUntrusted(AllowOriginHeaderWrite allowOriginHW) { - exists( - FlowsFromUntrusted cfg, DataFlow::PathNode source, DataFlow::PathNode sink, - ControlFlow::ConditionGuardNode cgn - | - cfg.hasFlowPath(source, sink) and cfg.isSink(sink.getNode(), cgn) + exists(FlowsFromUntrusted cfg, DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn | + cfg.hasFlowTo(sink) and cfg.isSink(sink, cgn) | cgn.dominates(allowOriginHW.getBasicBlock()) )