Skip to content

Commit dafcd5e

Browse files
KwstubbsKwstubbs
committed
Added support for Gin CORS
1 parent 70aa490 commit dafcd5e

File tree

4 files changed

+153
-15
lines changed

4 files changed

+153
-15
lines changed

‎go/ql/lib/go.qll‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import semmle.go.frameworks.Email
4141
import semmle.go.frameworks.Encoding
4242
import semmle.go.frameworks.Fiber
4343
import semmle.go.frameworks.Gin
44+
import semmle.go.frameworks.GinCors
4445
import semmle.go.frameworks.Glog
4546
import semmle.go.frameworks.GoKit
4647
import semmle.go.frameworks.GoMicro

‎go/ql/lib/semmle/go/frameworks/GinCors.qll‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/**
2+
* Provides classes for working with untrusted flow sources from the `github.com/gin-gonic/gin` package.
3+
*/
4+
5+
import go
6+
7+
module GinCors {
8+
/** Gets the package name `github.com/gin-gonic/gin`. */
9+
string packagePath() { result = package("github.com/gin-contrib/cors", "") }
10+
11+
/**
12+
* Data from a `Context` struct, considered as a source of untrusted flow.
13+
*/
14+
class New extends Function{
15+
New() {
16+
exists(Function f | f.hasQualifiedName(packagePath(), "New") | this = f)
17+
}
18+
19+
}
20+
class Config extends Variable {
21+
Config() { this.hasQualifiedName(packagePath(), "Config") }
22+
23+
// Field getAllowCredentials() {
24+
// result = this.getField("AllowCredentials")
25+
// }
26+
}
27+
class AllowCredentialsWrite extends DataFlow::ExprNode {
28+
DataFlow::Node base;
29+
GinConfig gc;
30+
AllowCredentialsWrite() {
31+
exists(Field f, Write w |
32+
f.hasQualifiedName(packagePath(), "Config", "AllowCredentials") and
33+
w.writesField(base, f, this) and
34+
this.getType() instanceof BoolType and
35+
(gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() = base.asInstruction() or
36+
gc.getV().getAUse() = base)
37+
)
38+
}
39+
GinConfig getConfig() { result = gc}
40+
}
41+
class AllowOriginsWrite extends DataFlow::ExprNode {
42+
DataFlow::Node base;
43+
GinConfig gc;
44+
AllowOriginsWrite() {
45+
exists(Field f, Write w |
46+
f.hasQualifiedName(packagePath(), "Config", "AllowOrigins") and
47+
w.writesField(base, f, this) and
48+
this.asExpr() instanceof SliceLit and
49+
(gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() = base.asInstruction() or
50+
gc.getV().getAUse() = base)
51+
)
52+
}
53+
GinConfig getConfig() { result = gc}
54+
}
55+
class AllowAllOriginsWrite extends DataFlow::ExprNode {
56+
DataFlow::Node base;
57+
58+
AllowAllOriginsWrite() {
59+
exists(Field f, Write w |
60+
f.hasQualifiedName(packagePath(), "Config", "AllowAllOrigins") and
61+
w.writesField(base, f, this) and
62+
this.getType() instanceof BoolType
63+
//and this.asExpr().(SliceLit).getAnElement().toString().matches("%null%")
64+
)
65+
}
66+
DataFlow::Node getBase() { result = base}
67+
}
68+
69+
class GinConfig extends Variable {
70+
SsaWithFields v;
71+
72+
GinConfig() {
73+
this = v.getBaseVariable().getSourceVariable() and
74+
exists(Type t | t.hasQualifiedName(packagePath(), "Config") | v.getType() = t)
75+
}
76+
SsaWithFields getV() { result = v}
77+
}
78+
79+
}
80+

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

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -69,22 +69,45 @@ module UntrustedToAllowOriginHeaderConfig implements DataFlow::ConfigSig {
6969
predicate isSink(DataFlow::Node sink) { isSinkHW(sink, _) }
7070
}
7171

72+
module UntrustedToAllowOriginConfigConfig implements DataFlow::ConfigSig {
73+
predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }
74+
75+
additional predicate isSinkWrite(DataFlow::Node sink, GinCors::AllowOriginsWrite w) {
76+
sink = w
77+
}
78+
79+
predicate isSink(DataFlow::Node sink) { isSinkWrite(sink, _) }
80+
}
81+
7282
/**
7383
* Tracks taint flowfor reasoning about when an `UntrustedFlowSource` flows to
7484
* a `HeaderWrite` that writes an `Access-Control-Allow-Origin` header's value.
7585
*/
7686
module UntrustedToAllowOriginHeaderFlow = TaintTracking::Global<UntrustedToAllowOriginHeaderConfig>;
7787

88+
module UntrustedToAllowOriginConfigFlow = TaintTracking::Global<UntrustedToAllowOriginConfigConfig>;
89+
7890
/**
7991
* Holds if the provided `allowOriginHW` HeaderWrite's parent ResponseWriter
8092
* also has another HeaderWrite that sets a `Access-Control-Allow-Credentials`
8193
* header to `true`.
8294
*/
83-
predicate allowCredentialsIsSetToTrue(AllowOriginHeaderWrite allowOriginHW) {
95+
predicate allowCredentialsIsSetToTrue(DataFlow::ExprNode allowOrigin) {
8496
exists(AllowCredentialsHeaderWrite allowCredentialsHW |
8597
allowCredentialsHW.getHeaderValue().toLowerCase() = "true"
8698
|
87-
allowOriginHW.getResponseWriter() = allowCredentialsHW.getResponseWriter()
99+
allowOrigin.(AllowOriginHeaderWrite).getResponseWriter() = allowCredentialsHW.getResponseWriter()
100+
)
101+
or
102+
exists(GinCors::AllowCredentialsWrite allowCredentialsGin |
103+
allowCredentialsGin.toString() = "true"
104+
|
105+
//flow only goes in one direction so fix this before PR
106+
allowCredentialsGin.getConfig() = allowOrigin.(GinCors::AllowOriginsWrite).getConfig()
107+
and
108+
not exists(GinCors::AllowAllOriginsWrite allowAllOrigins |
109+
allowAllOrigins.toString() = "true" //and DataFlow::localFlow(allowCredentialsGin.getBase(), allowAllOrigins.getBase())
110+
)
88111
)
89112
}
90113

@@ -93,10 +116,12 @@ predicate allowCredentialsIsSetToTrue(AllowOriginHeaderWrite allowOriginHW) {
93116
* UntrustedFlowSource.
94117
* The `message` parameter is populated with the warning message to be returned by the query.
95118
*/
96-
predicate flowsFromUntrustedToAllowOrigin(AllowOriginHeaderWrite allowOriginHW, string message) {
119+
predicate flowsFromUntrustedToAllowOrigin(DataFlow::ExprNode allowOriginHW, string message) {
97120
exists(DataFlow::Node sink |
98121
UntrustedToAllowOriginHeaderFlow::flowTo(sink) and
99-
UntrustedToAllowOriginHeaderConfig::isSinkHW(sink, allowOriginHW)
122+
UntrustedToAllowOriginHeaderConfig::isSinkHW(sink, allowOriginHW) or
123+
UntrustedToAllowOriginConfigFlow::flowTo(sink) and
124+
UntrustedToAllowOriginConfigConfig::isSinkWrite(sink, allowOriginHW)
100125
|
101126
message =
102127
headerAllowOrigin() + " header is set to a user-defined value, and " +
@@ -108,11 +133,17 @@ predicate flowsFromUntrustedToAllowOrigin(AllowOriginHeaderWrite allowOriginHW,
108133
* Holds if the provided `allowOriginHW` HeaderWrite is for a `Access-Control-Allow-Origin`
109134
* header and the value is set to `null`.
110135
*/
111-
predicate allowOriginIsNull(AllowOriginHeaderWrite allowOriginHW, string message) {
112-
allowOriginHW.getHeaderValue().toLowerCase() = "null" and
136+
predicate allowOriginIsNull(DataFlow::ExprNode allowOrigin, string message) {
137+
allowOrigin.(AllowOriginHeaderWrite).getHeaderValue().toLowerCase() = "null" and
113138
message =
114-
headerAllowOrigin() + " header is set to `" + allowOriginHW.getHeaderValue() + "`, and " +
139+
headerAllowOrigin() + " header is set to `" + allowOrigin.(AllowOriginHeaderWrite).getHeaderValue() + "`, and " +
115140
headerAllowCredentials() + " is set to `true`"
141+
or
142+
allowOrigin.(GinCors::AllowOriginsWrite).asExpr().(SliceLit).getAnElement().toString().matches("\"null%\"") and
143+
message =
144+
headerAllowOrigin() + " header is set to `" + allowOrigin.(GinCors::AllowOriginsWrite).asExpr().(SliceLit).getAnElement().toString() + "`, and " +
145+
headerAllowCredentials() + " is set to `true`"
146+
116147
}
117148

118149
/**
@@ -170,26 +201,26 @@ module FromUntrustedFlow = TaintTracking::Global<FromUntrustedConfig>;
170201
/**
171202
* Holds if the provided `allowOriginHW` is also destination of a `UntrustedFlowSource`.
172203
*/
173-
predicate flowsToGuardedByCheckOnUntrusted(AllowOriginHeaderWrite allowOriginHW) {
204+
predicate flowsToGuardedByCheckOnUntrusted(DataFlow::ExprNode allowOriginHW) {
174205
exists(DataFlow::Node sink, ControlFlow::ConditionGuardNode cgn |
175206
FromUntrustedFlow::flowTo(sink) and FromUntrustedConfig::isSinkCgn(sink, cgn)
176207
|
177208
cgn.dominates(allowOriginHW.getBasicBlock())
178209
)
179210
}
180211

181-
from AllowOriginHeaderWrite allowOriginHW, string message
212+
from DataFlow::ExprNode allowOrigin, string message
182213
where
183-
allowCredentialsIsSetToTrue(allowOriginHW) and
214+
allowCredentialsIsSetToTrue(allowOrigin) and
184215
(
185-
flowsFromUntrustedToAllowOrigin(allowOriginHW, message)
216+
flowsFromUntrustedToAllowOrigin(allowOrigin, message)
186217
or
187-
allowOriginIsNull(allowOriginHW, message)
218+
allowOriginIsNull(allowOrigin, message)
188219
) and
189-
not flowsToGuardedByCheckOnUntrusted(allowOriginHW) and
220+
not flowsToGuardedByCheckOnUntrusted(allowOrigin) and
190221
not exists(ControlFlow::ConditionGuardNode cgn |
191222
cgn.ensures(any(AllowedFlag f).getAFlag().getANode(), _)
192223
|
193-
cgn.dominates(allowOriginHW.getBasicBlock())
224+
cgn.dominates(allowOrigin.getBasicBlock())
194225
)
195-
select allowOriginHW, message
226+
select allowOrigin, message

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
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 `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+
* experimental
11+
* external/cwe/cwe-942
12+
* external/cwe/cwe-346
13+
*/
14+
15+
import go
16+
17+
// from GinCors::New n, GinCors::Config cfg, DataFlow::CallNode c
18+
// //where n.getACall() = c and c.getArgument(0) = cfg
19+
// where cfg.getAllowCredentials() = "*"
20+
// select cfg, "hello"
21+
22+
from GinCors::GinConfig gc
23+
select gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs(), "test"
24+
//where s.getAnElement().toString().matches("%https://foo.com%")
25+
//where a.getBase() = b.getBase()
26+
//select a.getBase(), b.getBase()

0 commit comments

Comments
 (0)