Skip to content

Commit b03513b

Browse files
smowtonsmowton
authored
Merge pull request github#542 from gagliardetto/cors-misconfig
Add query to detect CORS misconfiguration
2 parents 73227f1 + 87afdae commit b03513b

7 files changed

+546
-0
lines changed

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

+64
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Web browsers, by default, disallow cross-origin resource sharing via direct HTTP requests (i.e. using a JavaScript HTTP client).
8+
Still, to satisfy some needs that arose with the growth of the web, an expedient was created to make exceptions possible.
9+
CORS (Cross-origin resource sharing) is a mechanism that allows resources of a web endpoint (let's call it "Peer A")
10+
to be accessed from another web page belonging to a different domain ("Peer B").
11+
</p>
12+
<p>
13+
For that to happen, Peer A needs to make available its CORS configuration via special headers on the desired endpoint
14+
via the OPTIONS method.
15+
</p>
16+
<p>
17+
This configuration can also allow the inclusion of cookies on the cross-origin request,
18+
(i.e. when the <code>Access-Control-Allow-Credentials</code> header is set to true)
19+
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.
20+
</p>
21+
<p>
22+
That can have dangerous effects if Peer B origin is not restricted correctly.
23+
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
24+
(and not correctly validated), or is set to special values such as <code>*</code> or <code>null</code>.
25+
The above values can allow any Peer B to send requests to the misconfigured Peer A on behalf of the user.
26+
</p>
27+
<p>
28+
Example scenario:
29+
User is client of a bank that has its API misconfigured to accept CORS requests from any domain.
30+
When the user loads an evil page, the evil page sends a request to the bank's API to transfer all funds
31+
to evil party's account.
32+
Given that the user was already logged in to the bank website, and had its session cookies set,
33+
the evil party's request succeeds.
34+
</p>
35+
</overview>
36+
<recommendation>
37+
<p>
38+
When configuring CORS that allow credentials passing,
39+
it's best not to use user-provided values for the allowed origins response header,
40+
especially if the cookies grant session permissions on the user's account.
41+
</p>
42+
<p>
43+
It also can be very dangerous to set the allowed origins to <code>null</code> (which can be bypassed).
44+
</p>
45+
</recommendation>
46+
<example>
47+
<p>
48+
The first example shows a few possible CORS misconfiguration cases:
49+
</p>
50+
<sample src="CorsMisconfigurationBad.go"/>
51+
<p>
52+
The second example show better configurations:
53+
</p>
54+
<sample src="CorsMisconfigurationGood.go"/>
55+
</example>
56+
<references>
57+
<li>
58+
Reference 1: <a href="https://portswigger.net/web-security/cors">PortSwigger Web Security Academy on CORS</a>.
59+
</li>
60+
<li>
61+
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>.
62+
</li>
63+
</references>
64+
</qhelp>

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

+192
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
/**
2+
* @name CORS misconfiguration
3+
* @description If a CORS policy is configured to accept an origin value obtained from the request data,
4+
* or is set to `*` or `null`, and it allows credential sharing, then the users of the
5+
* application are vulnerable to the same range of attacks as in XSS (credential stealing, etc.).
6+
* @kind problem
7+
* @problem.severity warning
8+
* @id go/cors-misconfiguration
9+
* @tags security
10+
* external/cwe/cwe-942
11+
* external/cwe/cwe-346
12+
*/
13+
14+
import go
15+
import semmle.go.security.InsecureFeatureFlag::InsecureFeatureFlag
16+
17+
/**
18+
* A flag indicating a check for satisfied permissions or test configuration.
19+
*/
20+
class AllowedFlag extends FlagKind {
21+
AllowedFlag() { this = "allowed" }
22+
23+
bindingset[result]
24+
override string getAFlagName() {
25+
result.regexpMatch("(?i).*(allow|match|check|debug|devel|insecure).*")
26+
}
27+
}
28+
29+
/**
30+
* Provides the name of the `Access-Control-Allow-Origin` header key.
31+
*/
32+
string headerAllowOrigin() { result = "Access-Control-Allow-Origin".toLowerCase() }
33+
34+
/**
35+
* Provides the name of the `Access-Control-Allow-Credentials` header key.
36+
*/
37+
string headerAllowCredentials() { result = "Access-Control-Allow-Credentials".toLowerCase() }
38+
39+
/**
40+
* An `Access-Control-Allow-Origin` header write.
41+
*/
42+
class AllowOriginHeaderWrite extends HTTP::HeaderWrite {
43+
AllowOriginHeaderWrite() { this.getHeaderName() = headerAllowOrigin() }
44+
}
45+
46+
/**
47+
* An `Access-Control-Allow-Credentials` header write.
48+
*/
49+
class AllowCredentialsHeaderWrite extends HTTP::HeaderWrite {
50+
AllowCredentialsHeaderWrite() { this.getHeaderName() = headerAllowCredentials() }
51+
}
52+
53+
/**
54+
* A taint-tracking configuration for reasoning about when an UntrustedFlowSource
55+
* flows to a HeaderWrite that writes an `Access-Control-Allow-Origin` header's value.
56+
*/
57+
class FlowsUntrustedToAllowOriginHeader extends TaintTracking::Configuration {
58+
FlowsUntrustedToAllowOriginHeader() { this = "from-untrusted-to-allow-origin-header-value" }
59+
60+
override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }
61+
62+
predicate isSink(DataFlow::Node sink, AllowOriginHeaderWrite hw) { sink = hw.getValue() }
63+
64+
override predicate isSanitizer(DataFlow::Node node) {
65+
exists(ControlFlow::ConditionGuardNode cgn |
66+
cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _)
67+
|
68+
cgn.dominates(node.getBasicBlock())
69+
)
70+
}
71+
72+
override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
73+
}
74+
75+
/**
76+
* Holds if the provided `allowOriginHW` HeaderWrite's parent ResponseWriter
77+
* also has another HeaderWrite that sets a `Access-Control-Allow-Credentials`
78+
* header to `true`.
79+
*/
80+
predicate allowCredentialsIsSetToTrue(AllowOriginHeaderWrite allowOriginHW) {
81+
exists(AllowCredentialsHeaderWrite allowCredentialsHW |
82+
allowCredentialsHW.getHeaderValue().toLowerCase() = "true"
83+
|
84+
allowOriginHW.getResponseWriter() = allowCredentialsHW.getResponseWriter()
85+
)
86+
}
87+
88+
/**
89+
* Holds if the provided `allowOriginHW` HeaderWrite's value is set using an
90+
* UntrustedFlowSource.
91+
* The `message` parameter is populated with the warning message to be returned by the query.
92+
*/
93+
predicate flowsFromUntrustedToAllowOrigin(AllowOriginHeaderWrite allowOriginHW, string message) {
94+
exists(FlowsUntrustedToAllowOriginHeader cfg, DataFlow::Node sink |
95+
cfg.hasFlowTo(sink) and
96+
cfg.isSink(sink, allowOriginHW)
97+
|
98+
message =
99+
headerAllowOrigin() + " header is set to a user-defined value, and " +
100+
headerAllowCredentials() + " is set to `true`"
101+
)
102+
}
103+
104+
/**
105+
* Holds if the provided `allowOriginHW` HeaderWrite is for a `Access-Control-Allow-Origin`
106+
* header and the value is set to `null`.
107+
*/
108+
predicate allowOriginIsNull(AllowOriginHeaderWrite allowOriginHW, string message) {
109+
allowOriginHW.getHeaderValue().toLowerCase() = "null" and
110+
message =
111+
headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " +
112+
headerAllowCredentials() + " is set to `true`"
113+
}
114+
115+
/**
116+
* A read on a map type.
117+
*/
118+
class MapRead extends DataFlow::ElementReadNode {
119+
MapRead() { this.getBase().getType() instanceof MapType }
120+
}
121+
122+
/**
123+
* A taint-tracking configuration for reasoning about when an UntrustedFlowSource
124+
* flows somewhere.
125+
*/
126+
class FlowsFromUntrusted extends TaintTracking::Configuration {
127+
FlowsFromUntrusted() { this = "from-untrusted" }
128+
129+
override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }
130+
131+
override predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
132+
133+
predicate isSink(DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn) {
134+
exists(IfStmt ifs |
135+
exists(Expr operand |
136+
operand = ifs.getCond().getAChildExpr*() and
137+
(
138+
exists(DataFlow::CallExpr call | call = operand |
139+
call.getTarget().hasQualifiedName("strings", "HasSuffix") and
140+
sink.asExpr() = call.getArgument(0)
141+
)
142+
or
143+
exists(MapRead mapRead |
144+
operand = mapRead.asExpr() and
145+
sink = mapRead.getIndex().getAPredecessor*()
146+
// TODO: add _, ok : map[untrusted]; ok
147+
)
148+
or
149+
exists(EqlExpr comp |
150+
operand = comp and
151+
(
152+
sink.asExpr() = comp.getLeftOperand() and
153+
not comp.getRightOperand().(StringLit).getStringValue() = ""
154+
or
155+
sink.asExpr() = comp.getRightOperand() and
156+
not comp.getLeftOperand().(StringLit).getStringValue() = ""
157+
)
158+
)
159+
)
160+
)
161+
|
162+
cgn.getCondition() = ifs.getCond()
163+
)
164+
}
165+
}
166+
167+
/**
168+
* Holds if the provided `dst` is also destination of a `UntrustedFlowSource`.
169+
*/
170+
predicate flowsToGuardedByCheckOnUntrusted(AllowOriginHeaderWrite allowOriginHW) {
171+
exists(FlowsFromUntrusted cfg, DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn |
172+
cfg.hasFlowTo(sink) and cfg.isSink(sink, cgn)
173+
|
174+
cgn.dominates(allowOriginHW.getBasicBlock())
175+
)
176+
}
177+
178+
from AllowOriginHeaderWrite allowOriginHW, string message
179+
where
180+
allowCredentialsIsSetToTrue(allowOriginHW) and
181+
(
182+
flowsFromUntrustedToAllowOrigin(allowOriginHW, message)
183+
or
184+
allowOriginIsNull(allowOriginHW, message)
185+
) and
186+
not flowsToGuardedByCheckOnUntrusted(allowOriginHW) and
187+
not exists(ControlFlow::ConditionGuardNode cgn |
188+
cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _)
189+
|
190+
cgn.dominates(allowOriginHW.getBasicBlock())
191+
)
192+
select allowOriginHW, message

‎ql/src/experimental/CWE-942/CorsMisconfigurationBad.go

+24
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package main
2+
3+
import "net/http"
4+
5+
func main() {}
6+
7+
func bad1() {
8+
http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
9+
// BAD: 'null' origin is allowed,
10+
// and Access-Control-Allow-Credentials is set to 'true'.
11+
w.Header().Set("Access-Control-Allow-Origin", "null")
12+
w.Header().Set("Access-Control-Allow-Credentials", "true")
13+
})
14+
}
15+
16+
func bad2() {
17+
http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
18+
// BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value,
19+
// and `Access-Control-Allow-Credentials` is set to 'true':
20+
origin := req.Header.Get("origin")
21+
w.Header().Set("Access-Control-Allow-Origin", origin)
22+
w.Header().Set("Access-Control-Allow-Credentials", "true")
23+
})
24+
}

‎ql/src/experimental/CWE-942/CorsMisconfigurationGood.go

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
package main
2+
3+
import "net/http"
4+
5+
func good1() {
6+
http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
7+
// OK-ish: the `Access-Control-Allow-Origin` header is set using a user-defined value,
8+
// BUT `Access-Control-Allow-Credentials` is set to 'false':
9+
origin := req.Header.Get("origin")
10+
w.Header().Set("Access-Control-Allow-Origin", origin)
11+
w.Header().Set("Access-Control-Allow-Credentials", "false")
12+
})
13+
}

‎ql/test/experimental/CWE-942/CorsMisconfiguration.expected

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
| 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` |
2+
| 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` |
3+
| 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` |
4+
| 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` |
5+
| 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` |

0 commit comments

Comments
 (0)