Add query to detect CORS misconfiguration

ql/src/experimental/CWE-942/CorsMisconfiguration.qhelp

@@ -0,0 +1,64 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+	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").
+</p>
+<p>
+	For that to happen, Peer A needs to make available its CORS configuration via special headers on the desired endpoint
+	via the OPTIONS method.
+</p>
+<p>
+	This configuration can also allow the inclusion of cookies on the cross-origin request,
+	(i.e. when the <code>Access-Control-Allow-Credentials</code> 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.
+</p>
+<p>
+	That can have dangerous effects if Peer B origin is not restricted correctly. 
+	An example of a dangerous scenario is when <code>Access-Control-Allow-Origin</code> 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 <code>*</code> or <code>null</code>. 
+	The above values can allow any Peer B to send requests to the misconfigured Peer A on behalf of the user.
+</p>
+<p>
+	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.
+</p>
+</overview>
+<recommendation>
+<p>
+	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.
+</p>
+<p>
+	It also can be very dangerous to set the allowed origins to <code>null</code> (which can be bypassed).
+</p>
+</recommendation>
+<example>
+<p>
+	The first example shows a few possible CORS misconfiguration cases:
+</p>
+<sample src="CorsMisconfigurationBad.go"/>
+<p>
+	The second example show better configurations:
+</p>
+<sample src="CorsMisconfigurationGood.go"/>
+</example>
+<references>
+<li>
+	Reference 1: <a href="https://portswigger.net/web-security/cors">PortSwigger Web Security Academy on CORS</a>.
+</li>
+<li>
+	Reference 2: <a href="https://www.youtube.com/watch?v=wgkj4ZgxI4c">AppSec EU 2017 Exploiting CORS Misconfigurations For Bitcoins And Bounties by James Kettle</a>.
+</li>
+</references>
+</qhelp>

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

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")
+	})
+}

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")
+	})
+}

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` |