WIP: Go: CORS Bypass due to incorrect checks

[porcupineyhairs]

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

[ghost]

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

[ghsecuritylab]

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!

[ghost]

@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.

[ghost]

@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.

[ghost]

@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.

[Kwstubbs]

@porcupineyhairs Hey porcupiney, we have gone ahead and analyzed the results and although the query is working well, the context of the results mean they are not vulnerabilities. We see the value of this query and would like you to add the HasPrefix sink in addition to qualify for the bounty. Please check to ensure that that the origin does not end with ":" (ex: "https://127.0.0.1" is unsafe, but "https://127.0.0.1:" is safe) and that there aren't two ":" in the origin ( ex: "https://127.0.0.1:3000 is safe). Additionally, please check to makes sure there is no ! in front of the string.HasPrefix call if possible.

if !string.HasPrefix(origin, "whatever") { // ignore these 

if you have any more suggestions, please let me know. I have considered checking for string.Contains as well but that seems be wrought with FPs that would tough to program around.

[owen-mc]

Note that we already have modelling to combine all the different ways of checking if a string has a given prefix into one handy class: StringOps::HasPrefix. Here are the existing examples of it being used: here here and here.

[ghost]

@Kwstubbs I will take the prefix case up soon. Also,

context of the results mean they are not vulnerabilities

I haven't tried writing an exploit for these. But from the face of it, these should be exploitable right?

[ghost]

@Kwstubbs I am still waiting for your reply. I have included some changes from the review.

As for the prefix case, can you point me to a real world example where this could be a vulnerability?

[Kwstubbs]

@porcupineyhairs These two results are from CORS frameworks that check the config as a comparison. From what I can see, they properly use both hasPrefix and hasSuffix to ensure that no vulnerabilities occurs. (If a user puts in something like test*.com in the config, then that is a vulnerability created by the user, not really the fault of the CORS framework). Due to this, I don't have any results to properly evaluate. I ran this query on 2000+ repos and didn't get any results. I suggested to include prefix checks because it makes sense to put the suffix and prefix checks into the same query, and it would hopefully give some more results. Here is an example of one of my vulnerabilities that come from this exact pattern.