diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp new file mode 100644 index 00000000000..eafd34cd6c5 --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp @@ -0,0 +1,64 @@ + + + +

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

+
+ +

+ 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 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. +
  • +
    +
    diff --git a/ql/src/experimental/CWE-942/CorsMisconfiguration.ql b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql new file mode 100644 index 00000000000..fa07af5914f --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfiguration.ql @@ -0,0 +1,192 @@ +/** + * @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 +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. + */ +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() } + +/** + * 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. + */ +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, AllowOriginHeaderWrite hw) { 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, _) } +} + +/** + * 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(AllowOriginHeaderWrite allowOriginHW) { + exists(AllowCredentialsHeaderWrite allowCredentialsHW | + allowCredentialsHW.getHeaderValue().toLowerCase() = "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(AllowOriginHeaderWrite allowOriginHW, string message) { + 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 " + + headerAllowCredentials() + " is set to `true`" + ) +} + +/** + * Holds if the provided `allowOriginHW` HeaderWrite is for a `Access-Control-Allow-Origin` + * header and the value is set to `null`. + */ +predicate allowOriginIsNull(AllowOriginHeaderWrite allowOriginHW, string message) { + allowOriginHW.getHeaderValue().toLowerCase() = "null" and + message = + headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " + + 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) { isSink(sink, _) } + + predicate isSink(DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn) { + exists(IfStmt ifs | + 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) + ) + 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(AllowOriginHeaderWrite allowOriginHW) { + exists(FlowsFromUntrusted cfg, DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn | + cfg.hasFlowTo(sink) and cfg.isSink(sink, cgn) + | + cgn.dominates(allowOriginHW.getBasicBlock()) + ) +} + +from AllowOriginHeaderWrite allowOriginHW, string message +where + allowCredentialsIsSetToTrue(allowOriginHW) and + ( + flowsFromUntrustedToAllowOrigin(allowOriginHW, message) + or + allowOriginIsNull(allowOriginHW, message) + ) and + not flowsToGuardedByCheckOnUntrusted(allowOriginHW) and + not exists(ControlFlow::ConditionGuardNode cgn | + cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _) + | + cgn.dominates(allowOriginHW.getBasicBlock()) + ) +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..470735eb027 --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfigurationBad.go @@ -0,0 +1,24 @@ +package main + +import "net/http" + +func main() {} + +func bad1() { + 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 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': + 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..1924b004bdf --- /dev/null +++ b/ql/src/experimental/CWE-942/CorsMisconfigurationGood.go @@ -0,0 +1,13 @@ +package main + +import "net/http" + +func good1() { + 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..2ab9324fe8c --- /dev/null +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -0,0 +1,5 @@ +| 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 new file mode 100644 index 00000000000..cac752dbcb2 --- /dev/null +++ b/ql/test/experimental/CWE-942/CorsMisconfiguration.go @@ -0,0 +1,247 @@ +package main + +import ( + "net/http" + "strings" +) + +const ( + HeaderAllowOrigin = "Access-Control-Allow-Origin" + HeaderAllowCredentials = "Access-Control-Allow-Credentials" +) + +func main() { + { + 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: 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) { + // 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': + 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) + }) + 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") + } + }) + 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 { + return true + } + } + return false +} + +func allowedOrigin(origin string) bool { + if origin == "example.com" { + return true + } + return false +} 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