From 12d3eb4a6eca2eb86a77e3f32f7f66ac6b55d1f1 Mon Sep 17 00:00:00 2001 From: Porcupiney Hairs Date: Sun, 23 Jun 2024 21:28:39 +0530 Subject: [PATCH] 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/temp.ql | 29 +++++++ go/ql/src/experimental/CWE-639/urlCheck.ql | 91 ++++++++++++++++++++++ 2 files changed, 120 insertions(+) create mode 100644 go/ql/src/experimental/CWE-639/temp.ql create mode 100644 go/ql/src/experimental/CWE-639/urlCheck.ql diff --git a/go/ql/src/experimental/CWE-639/temp.ql b/go/ql/src/experimental/CWE-639/temp.ql new file mode 100644 index 0000000000000..335eafa369576 --- /dev/null +++ b/go/ql/src/experimental/CWE-639/temp.ql @@ -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 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 0000000000000..728ad790cafdd --- /dev/null +++ b/go/ql/src/experimental/CWE-639/urlCheck.ql @@ -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; + +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"