WIP: Go: CORS Bypass due to incorrect checks #16813

porcupineyhairs · 2024-06-23T15:59:25Z

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

porcupineyhairs · 2024-06-23T16:01:00Z

Please don't review this yet. I need some help writing QL for this. I will raise this over slack.

ghsecuritylab · 2024-06-24T00:10:21Z

Hello porcupineyhairs 👋
You have submitted this pull request as a bug bounty report in the github/securitylab repository and therefore this pull request has been put into draft state to give time for the GitHub Security Lab to assess the PR. When GitHub Security Lab has finished assessing your pull request, it will be marked automatically as Ready for review. Until then, please don't change the draft state.

In the meantime, feel free to make changes to the pull request. If you'd like to maximize payout for your this and future submissions, here are a few general guidelines, that we might take into consideration when reviewing a submission.

the submission models widely-used frameworks/libraries
the vulnerability modeled in the submission is impactful
the submission finds new true positive vulnerabilities
the submission finds very few false positives
code in the submission is easy to read and will be easy to maintain
documentation is written clearly, highlighting the impact of the issue it finds and is written without grammatical or other errors. The code samples clearly show the vulnerability
the submission includes tests, change note etc.

Please note that these are guidelines, not rules. Since we have a lot of different types of submissions, the guidelines might vary for each submission.

Happy hacking!

porcupineyhairs · 2024-07-01T09:00:25Z

@Kwstubbs This query is working and can be tested. However, for some reason, the select statement does not give the results in a proper form for Codeql to properly generate paths. I will take this up with the go team. In the mean time, please feel free to review and continue with the bounty application.

go/ql/src/experimental/CWE-639/urlCheck.ql

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 ```

porcupineyhairs · 2024-07-03T14:50:21Z

go/ql/src/experimental/CWE-639/urlCheck.ql

+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)
+    )
+  }
+}


@owen-mc This does not seem to do what I need it to do. I tried quick-eval for just these lines. I can see they identify the source correct and the sinks correctly but the moment I run a dataflow query, it fails to see the flow. I can see there is no modification done to the source before the sink. So, i don't know what could be missing here.

Please can you give me an example of a path you'd expect to see? I've downloaded the db you provided in the bounty issue.

@owen-mc I am trying to detect this

I get a different result from you - when I quick-eval this class on the current db for that repo I get 6 results - all the uses of origin in the lines you linked it.

Yes, I can see the origin's too but I don't see them when I run the full data flow query.

Well, GorillaOriginFuncSource seems to be giving the expected results. I think the problem is in MaybeOrigin.

MaybeOrigin correctly detects the vuln for the other project db.

owen-mc · 2024-07-05T12:44:24Z

go/ql/src/experimental/CWE-639/urlCheck.ql

+        or
+        v.getAWrite() = w and mayBeCors(v.getName())
+      |
+        w = r.getASuccessor*().asInstruction()


This line seems off - why would the write instruction be a data flow successor of a remote flow source? I could imagine the rhs of the write instruction might be. But that also gives me no results.

owen-mc · 2024-07-05T12:46:33Z

go/ql/src/experimental/CWE-639/urlCheck.ql

+      exists(Write w, Variable v |
+        mayBeCors(w.getLhs().getName())
+        or
+        v.getAWrite() = w and mayBeCors(v.getName())


This is a pattern that causes awful performance. In the first part of the or you don't constraint v at all, so it is allowed to be any Variable, which produces a large number of results. You should rewrite it like this, so that there are never any unconstrained variables.

Suggested change

exists(Write w, Variable v |

mayBeCors(w.getLhs().getName())

or

v.getAWrite() = w and mayBeCors(v.getName())

exists(Write w |

mayBeCors(w.getLhs().getName())

or

exists(Variable v |

v.getAWrite() = w and mayBeCors(v.getName())

)

owen-mc · 2024-07-05T12:55:22Z

go/ql/src/experimental/CWE-639/urlCheck.ql

+      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 is the same pattern that produces awful performance that I mentioned above, with the twist that a, which is unconstrained in the first part of the or, is actually used later on. Since argumentOf includes the receiver and all arguments, I think you probably want something like this:

Suggested change

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()])

exists(DataFlow::CallNode c, DataFlow::ArgumentNode a |

a = r.getASuccessor*() and

a.argumentOf(c.asExpr(), _)

|

mayBeCors([a.getStringValue(), c.getTarget().getName()])

Note that this matches the receiver and all arguments of any function whose name satisfies mayBeCors. I assume this is intended, though it doesn't match the comment.

This change makes the result in goZero disappear.

The result in goZero starts at this part of the code: r.Header.Get(originHeader). The "receiver or argument whose name could suggest it has something to do with cors" is the argument originHeader. But taint doesn't flow to that argument (it's a constant string) . The reason we get a result is that there is flow from a remote flow source to the receiver of this call, r.Header (in fact it is itself a remote flow source).

Maybe I haven't fully understood what this code is meant to do (ignoring the variable write side for now). My current understanding is "find any argument or a receiver whose name could suggest it has something to do with cors and which has taint flowing to it from a remote flow source - this is the remote flow source".

porcupineyhairs · 2024-07-05T13:12:27Z

go/ql/src/experimental/CWE-639/urlCheck.ql

+      mc.getTarget().hasQualifiedName("strings", "HasSuffix") and
+      a = mc.getArgument(1) and
+      // should not match ".domain.com"
+      not a.asExpr().(StringLit).getExactValue().matches(".%") and


I think you are testing it against the non-vulnerable version. There are two databases. One named playWithDockerDb and the other named playWithDockerVulnDb. The first should not get a detection as it is with the correct fix. The second is the one with the actual vulnerability. The line you refer should prevent all strings starting with a . as non-vulnerable. This is expected.

porcupineyhairs · 2024-07-05T19:35:52Z

@owen-mc I have made some changes to the query. The query now detects both the vulnerabilities correctly. However, I can see duplicate results for the playwithdocker project.

owen-mc · 2024-07-08T15:24:15Z

go/ql/src/experimental/CWE-639/urlCheck.ql

+      c.getTarget().hasQualifiedName("github.com/gorilla/handlers", "AllowedOriginValidator") and
+      c.getArgument(0).asExpr() = f
+    |
+      DataFlow::localFlow(DataFlow::parameterNode(f.getParameter(0)), this)


Suggested change

DataFlow::localFlow(DataFlow::parameterNode(f.getParameter(0)), this)

this = DataFlow::parameterNode(f.getParameter(0))

There is no point saying data flows from X to Y, and Y is the source - you should just make X the source. This gets rid of the duplicates.

owen-mc · 2024-07-08T15:42:22Z

go/ql/src/experimental/CWE-639/urlCheck.ql

+/**
+ * An argument to a Gorilla's OriginValidator Function taken as a source
+ */
+class GorillaOriginFuncSource extends RemoteFlowSource::Range {


Suggested change

class GorillaOriginFuncSource extends RemoteFlowSource::Range {

class GorillaOriginFuncSource extends DataFlow::Node {

I don't think you actually want this to extend RemoteFlowSource::Range. Since this is a .ql file, it can't be imported by any other files, so the only effects are within this file. Looking at the uses of RemoteFlowSource in this file, the only effect is that MaybeOrigin can include nodes from GorillaOriginFuncSource. But as MaybeOrigin is only used in node instanceof MaybeOrigin or node instanceof GorillaOriginFuncSource, this makes no difference.

porcupineyhairs · 2024-07-08T20:04:37Z

@owen-mc Changes done. MRVA showed be alerts in 3 projects. One of them was a false positive which has been fixed. So, there are two projects with alerts right now and they look valid to me.

go/ql/src/experimental/CWE-639/urlCheck.ql

+
+bindingset[s]
+private predicate mayBeCors(string s) {
+  s.toLowerCase().matches(["%origin%", "%cors%"]) and not s.toLowerCase().matches(["%original%"])


go/ql/src/experimental/CWE-639/urlCheck.ql

+  }
+}
+
+private module UrlFlow implements DataFlow::ConfigSig {


github-actions bot added the Go label Jun 23, 2024

porcupineyhairs mentioned this pull request Jun 23, 2024

Go: CORS Bypass due to incorrect checks github/securitylab#834

Open

2 tasks

porcupineyhairs force-pushed the goCorsBypass branch from 3aacc85 to 8a0f28f Compare July 1, 2024 08:56

ghsecuritylab marked this pull request as ready for review July 1, 2024 11:22

ghsecuritylab requested a review from a team as a code owner July 1, 2024 11:22

owen-mc reviewed Jul 1, 2024

View reviewed changes

go/ql/src/experimental/CWE-639/urlCheck.ql Outdated Show resolved Hide resolved

owen-mc reviewed Jul 3, 2024

View reviewed changes

go/ql/src/experimental/CWE-639/urlCheck.ql Outdated Show resolved Hide resolved

porcupineyhairs force-pushed the goCorsBypass branch 2 times, most recently from 12d3eb4 to cfb9aba Compare July 3, 2024 14:45

porcupineyhairs force-pushed the goCorsBypass branch from cfb9aba to 71fd955 Compare July 3, 2024 14:47

porcupineyhairs commented Jul 3, 2024

View reviewed changes

owen-mc reviewed Jul 5, 2024

View reviewed changes

porcupineyhairs commented Jul 5, 2024

View reviewed changes

Include changes from review

5b84c0c

owen-mc reviewed Jul 8, 2024

View reviewed changes

Include changes from review

883fd00

github-advanced-security bot found potential problems Jul 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Go: CORS Bypass due to incorrect checks #16813

WIP: Go: CORS Bypass due to incorrect checks #16813

porcupineyhairs commented Jun 23, 2024

porcupineyhairs commented Jun 23, 2024

ghsecuritylab commented Jun 24, 2024

porcupineyhairs commented Jul 1, 2024

porcupineyhairs Jul 3, 2024

owen-mc Jul 5, 2024

porcupineyhairs Jul 5, 2024

owen-mc Jul 5, 2024

porcupineyhairs Jul 5, 2024

owen-mc Jul 5, 2024

porcupineyhairs Jul 5, 2024

owen-mc Jul 5, 2024

owen-mc Jul 5, 2024

owen-mc Jul 5, 2024

porcupineyhairs Jul 5, 2024

owen-mc Jul 5, 2024

porcupineyhairs Jul 5, 2024

porcupineyhairs commented Jul 5, 2024

owen-mc Jul 8, 2024

owen-mc Jul 8, 2024

porcupineyhairs commented Jul 8, 2024

	DataFlow::localFlow(DataFlow::parameterNode(f.getParameter(0)), this)
	this = DataFlow::parameterNode(f.getParameter(0))

	class GorillaOriginFuncSource extends RemoteFlowSource::Range {
	class GorillaOriginFuncSource extends DataFlow::Node {

WIP: Go: CORS Bypass due to incorrect checks #16813

Are you sure you want to change the base?

WIP: Go: CORS Bypass due to incorrect checks #16813

Conversation

porcupineyhairs commented Jun 23, 2024

porcupineyhairs commented Jun 23, 2024

ghsecuritylab commented Jun 24, 2024

porcupineyhairs commented Jul 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porcupineyhairs commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

porcupineyhairs commented Jul 8, 2024