From 8a0f28f43f5692fdd65ac5d136ee668843b0f92e Mon Sep 17 00:00:00 2001
From: Porcupiney Hairs <porcupiney.hairs@protonmail.com>
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/urlCheck.ql | 71 ++++++++++++++++++++++
 1 file changed, 71 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 0000000000000..d6090d4245ea4
--- /dev/null
+++ b/go/ql/src/experimental/CWE-639/urlCheck.ql
@@ -0,0 +1,71 @@
+/**
+ * @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%"]) }
+
+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 DataFlow::Node source, DataFlow::Node sink
+where Flow::flow(source, sink)
+select sink, source, sink, "This can lead to a Cross Origin Resource Sharing(CORS) policy bypass"