-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
WIP: Go: CORS Bypass due to incorrect checks
Most Go frameworks provide a function call where-in you can pass a handler for testing origins and performing CORS checks. These functions typically check for the supllied origin in a list of valid origins. This behaviour is mostly fine but can lead to issues when done incorrectly. for example, consider the code snippets below https://github.com/zeromicro/go-zero/blob/5c9fae7e6258fd66d026793e7cb03ba9955e3dee/rest/internal/cors/handlers.go#L79-L91 https://github.com/play-with-docker/play-with-docker/blob/7188d83f867cbc201aef4b0597ac5f868c1971f3/handlers/bootstrap.go#L71-L80 In both these cases, the checks are implemented incorrectly and can lead to a CORS bypass resulting in CVE-2023-28109 and CVE-2024-27302. This PR aims to add a query, and its corresponding qhelp and tests for detecting the same vulnerability. The databases to verify the same can be downloaded from ``` https://file.io/OQX8Q3H3hMd4 https://filetransfer.io/data-package/wAfSEvZu#link ```
- Loading branch information
1 parent
b779341
commit 12d3eb4
Showing
2 changed files
with
120 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/** | ||
* @name Incorrect check on url | ||
* @description If a CORS policy is configured to accept an origin value obtained from the request data, it can lead to a policy bypass. | ||
* @kind path-problem | ||
* @problem.severity warning | ||
* @id go/cors-bypass | ||
* @tags security | ||
* experimental | ||
* external/cwe/cwe-942 | ||
* external/cwe/cwe-346 | ||
*/ | ||
|
||
import go | ||
import codeql.dataflow.TaintTracking | ||
|
||
bindingset[s] | ||
private predicate mayBeCors(string s) { s.toLowerCase().matches(["%origin%", "%cors%"]) } | ||
|
||
from FuncDef f, DataFlow::CallNode c, DataFlow::Node sink | ||
where | ||
// // gh.AllowedOriginValidator(func(origin string) bool { | ||
f.getParameter(0).getType() instanceof StringType and | ||
// and | ||
f.getNumParameter() = 1 and | ||
// // and | ||
c.getTarget().hasQualifiedName("github.com/gorilla/handlers", "AllowedOriginValidator") and | ||
c.getArgument(0).asExpr() = f and | ||
DataFlow::localFlow(DataFlow::parameterNode(f.getParameter(0)), sink) | ||
select sink |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
/** | ||
* @name Incorrect check on url | ||
* @description If a CORS policy is configured to accept an origin value obtained from the request data, it can lead to a policy bypass. | ||
* @kind path-problem | ||
* @problem.severity warning | ||
* @id go/cors-bypass | ||
* @tags security | ||
* experimental | ||
* external/cwe/cwe-942 | ||
* external/cwe/cwe-346 | ||
*/ | ||
|
||
import go | ||
import codeql.dataflow.TaintTracking | ||
|
||
bindingset[s] | ||
private predicate mayBeCors(string s) { s.toLowerCase().matches(["%origin%", "%cors%"]) } | ||
|
||
/** | ||
* An argument to a Gorilla's OriginValidator Function taken as a source | ||
*/ | ||
class GorillaOriginFuncSource extends RemoteFlowSource::Range { | ||
GorillaOriginFuncSource() { | ||
exists(FuncDef f, DataFlow::CallNode c | | ||
// Find a func passed to `AllowedOriginValdiator` as a validator. | ||
// The string parameter supplied to the validator is a remote controlled string supplied in the origin header. | ||
// `gh.AllowedOriginValidator(func(origin string) bool{})` | ||
f.getParameter(0).getType() instanceof StringType and | ||
f.getNumParameter() = 1 and | ||
c.getTarget().hasQualifiedName("github.com/gorilla/handlers", "AllowedOriginValidator") and | ||
c.getArgument(0).asExpr() = f | ||
| | ||
DataFlow::localFlow(DataFlow::parameterNode(f.getParameter(0)), this) | ||
) | ||
} | ||
} | ||
|
||
private class MaybeOrigin extends RemoteFlowSource { | ||
MaybeOrigin() { | ||
exists(RemoteFlowSource r | | ||
// Any write where the variables name could suggest it has something to do with cors. | ||
exists(Write w, Variable v | | ||
mayBeCors(w.getLhs().getName()) | ||
or | ||
v.getAWrite() = w and mayBeCors(v.getName()) | ||
| | ||
w = r.getASuccessor*().asInstruction() | ||
) | ||
or | ||
// Any argument or a receiver whose name could suggest it has something to do with cors. | ||
exists(DataFlow::CallNode c, DataFlow::ArgumentNode a | | ||
c.getArgument(_) = r.getASuccessor*() | ||
or | ||
c.getReceiver() = r.getASuccessor*() and | ||
a.argumentOf(c.asExpr(), _) | ||
| | ||
mayBeCors([a.getStringValue(), c.getTarget().getName()]) | ||
) | ||
| | ||
this = r | ||
) | ||
} | ||
} | ||
|
||
private module UrlFlow implements DataFlow::ConfigSig { | ||
predicate isSource(DataFlow::Node node) { node instanceof MaybeOrigin } | ||
|
||
predicate isSink(DataFlow::Node node) { | ||
exists(DataFlow::CallNode mc, DataFlow::ArgumentNode a | | ||
// Get a call to `strings.HasSuffix(origin, allowedDomain)` | ||
mc.getTarget().hasQualifiedName("strings", "HasSuffix") and | ||
a = mc.getArgument(1) and | ||
// should not match ".domain.com" | ||
not a.asExpr().(StringLit).getExactValue().matches(".%") and | ||
not exists(AddExpr w | w.getLeftOperand().getStringValue().matches(".%") | | ||
DataFlow::localFlow(DataFlow::exprNode(w), a) | ||
) | ||
| | ||
mc.getArgument(0) = node | ||
) | ||
} | ||
} | ||
|
||
private module Flow = TaintTracking::Global<UrlFlow>; | ||
|
||
private import Flow::PathGraph | ||
|
||
from Flow::PathNode source, Flow::PathNode sink | ||
where Flow::flowPath(source, sink) | ||
select sink.getNode(), source, sink, | ||
"This can lead to a Cross Origin Resource Sharing(CORS) policy bypass" |