From 71fd9551d5baaf1ab5c528ec70f5b4ddcc9c3568 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Sun, 23 Jun 2024 21:28:39 +0530 Subject: [PATCH 1/3] 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 ``` --- go/ql/src/experimental/CWE-639/urlCheck.ql | 90 ++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 go/ql/src/experimental/CWE-639/urlCheck.ql diff --git a/go/ql/src/experimental/CWE-639/urlCheck.ql b/go/ql/src/experimental/CWE-639/urlCheck.ql new file mode 100644 index 000000000000..0ef3721d1c0d --- /dev/null +++ b/go/ql/src/experimental/CWE-639/urlCheck.ql @@ -0,0 +1,90 @@ +/** + * @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 + +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; + +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" From 5b84c0ca4e82339784d4e3a734debb2c090564f0 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Sat, 6 Jul 2024 01:01:51 +0530 Subject: [PATCH 2/3] Include changes from review --- go/ql/src/experimental/CWE-639/urlCheck.ql | 41 ++++++++++++---------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/go/ql/src/experimental/CWE-639/urlCheck.ql b/go/ql/src/experimental/CWE-639/urlCheck.ql index 0ef3721d1c0d..7f9efef19272 100644 --- a/go/ql/src/experimental/CWE-639/urlCheck.ql +++ b/go/ql/src/experimental/CWE-639/urlCheck.ql @@ -34,35 +34,37 @@ class GorillaOriginFuncSource extends RemoteFlowSource::Range { } } -private class MaybeOrigin extends RemoteFlowSource { +private class MaybeOrigin extends DataFlow::Node { 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()) + exists(Write w | + // Take `origin` in `origin := r.Header.Get(header)` as a source. + mayBeCors(w.getLhs().getName()) and + TaintTracking::localTaint(r, w.getRhs()) | - w = r.getASuccessor*().asInstruction() + this = r //or this.asInstruction() = w ) or - // Any argument or a receiver whose name could suggest it has something to do with cors. + // Take the receiver, call or call arguments as source when any of their names match `maybeCors` exists(DataFlow::CallNode c, DataFlow::ArgumentNode a | - c.getArgument(_) = r.getASuccessor*() - or - c.getReceiver() = r.getASuccessor*() and - a.argumentOf(c.asExpr(), _) - | + TaintTracking::localTaint(r, a) and + a.argumentOf(c.asExpr(), _) and mayBeCors([a.getStringValue(), c.getTarget().getName()]) + | + // When argument or receiver is `maybeCors` take them as origin source. + // When the call name is `maybeCors` take the result as origin source + // (this = c.getResult(_) or this = a) + this = r ) - | - this = r ) } } private module UrlFlow implements DataFlow::ConfigSig { - predicate isSource(DataFlow::Node node) { node instanceof MaybeOrigin } + predicate isSource(DataFlow::Node node) { + node instanceof MaybeOrigin or node instanceof GorillaOriginFuncSource + } predicate isSink(DataFlow::Node node) { exists(DataFlow::CallNode mc, DataFlow::ArgumentNode a | @@ -70,9 +72,12 @@ private module UrlFlow implements DataFlow::ConfigSig { 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) + not ( + a.asExpr().(StringLit).getExactValue().matches(".%") + or + exists(AddExpr w | w.getLeftOperand().getStringValue().matches(".%") | + DataFlow::localFlow(DataFlow::exprNode(w), a) + ) ) | mc.getArgument(0) = node From 883fd00ac5979e90e4c6d415944b236c83795fb2 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Tue, 9 Jul 2024 01:32:50 +0530 Subject: [PATCH 3/3] Include changes from review --- go/ql/src/experimental/CWE-639/urlCheck.ql | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/go/ql/src/experimental/CWE-639/urlCheck.ql b/go/ql/src/experimental/CWE-639/urlCheck.ql index 7f9efef19272..cef6093f4a65 100644 --- a/go/ql/src/experimental/CWE-639/urlCheck.ql +++ b/go/ql/src/experimental/CWE-639/urlCheck.ql @@ -13,12 +13,14 @@ import go bindingset[s] -private predicate mayBeCors(string s) { s.toLowerCase().matches(["%origin%", "%cors%"]) } +private predicate mayBeCors(string s) { + s.toLowerCase().matches(["%origin%", "%cors%"]) and not s.toLowerCase().matches(["%original%"]) +} /** * An argument to a Gorilla's OriginValidator Function taken as a source */ -class GorillaOriginFuncSource extends RemoteFlowSource::Range { +class GorillaOriginFuncSource extends DataFlow::Node { GorillaOriginFuncSource() { exists(FuncDef f, DataFlow::CallNode c | // Find a func passed to `AllowedOriginValdiator` as a validator. @@ -29,7 +31,7 @@ class GorillaOriginFuncSource extends RemoteFlowSource::Range { c.getTarget().hasQualifiedName("github.com/gorilla/handlers", "AllowedOriginValidator") and c.getArgument(0).asExpr() = f | - DataFlow::localFlow(DataFlow::parameterNode(f.getParameter(0)), this) + this = DataFlow::parameterNode(f.getParameter(0)) ) } }