Add query to detect CORS misconfiguration by gagliardetto · Pull Request #542 · github/codeql-go

gagliardetto · 2021-05-22T16:18:57Z

This PR add a new experimental query that looks for misconfigured CORS headers that are too widely permissive on the accepted origins (i.e. set the Access-Control-Allow-Origin to unsafe values) while also allowing the inclusion of cookies on the request (i.e. Access-Control-Allow-Credentials: true).

CWE-942: Permissive Cross-domain Policy with Untrusted Domains

gagliardetto · 2021-05-22T16:29:11Z

I'm running a scan on lgtm.com to see how many FPs there are because of checks on the user-provided values (scenarios like if isValidOrigin(origin)); will then update the query.

smowton · 2021-05-27T14:41:48Z

Are you done updating this?

smowton · 2021-05-27T14:42:59Z

Also I note we already have CORS-related queries for JavaScript and Java; is this intended as a port of either one, or was it written from scratch?

gagliardetto · 2021-05-27T14:47:24Z

The only missing part is constraints to avoid false positives (in I-don't-know-yet-what scenarios), and I haven't gotten to that yet. I wrote the query from scratch. Didn't think to take a look at implementations for other languages 😅

gagliardetto · 2021-06-03T10:59:29Z

I scanned 3.743 projects, and got 39 results.

Some of them were false positives, so I added a flag check that helps get rid of them.

Some of the rest of the 39 are technically true positives, but do not lead to a security vulnerability scenario.

smowton · 2021-06-03T13:16:55Z

Are you declaring this ready to review?

gagliardetto · 2021-06-03T13:17:40Z

Yes, I am.

PR ready for review.

gagliardetto · 2021-06-03T13:18:30Z

I'll write today the issue for https://github.com/github/securitylab

smowton · 2021-06-03T13:20:44Z

Aha, excellent -- in that case know that the seclab now do the early review stages (though I imagine since this is a well-known vuln category and produces good results, it'll come back to us in short order). I'll review once they're done commenting.

gagliardetto · 2021-06-08T12:10:13Z

Will they run the query on all go projects?

JarLob · 2021-06-09T11:02:38Z

Hi @gagliardetto,
There is a safety catch for

Access-Control-Allow-Origin: *
Access-Control-Allow-Credentials: true

Browsers won't allow credentials in this case. You need to fix your tests in several places.

Regarding the Example:,
We encourage security researchers to responsibly report all found 0days and accept old CVEs and past vulnerable source revisions if they are detected by submitted queries.
In this case it can be a TP if a custom authentication header is not always required, but also can be a FP. Did you try to contact the maintainers?
Also you may consider filtering the cases like this to reduce FP rate.

gagliardetto · 2021-06-10T16:24:07Z

Hi @gagliardetto,
There is a safety catch for
Access-Control-Allow-Origin: *
Access-Control-Allow-Credentials: true
Browsers won't allow credentials in this case. You need to fix your tests in several places.

Thanks! I somehow managed to forget that part...

gagliardetto · 2021-06-10T16:35:01Z

Regarding the Example:,
We encourage security researchers to responsibly report all found 0days and accept old CVEs and past vulnerable source revisions if they are detected by submitted queries.
In this case it can be a TP if a custom authentication header is not always required, but also can be a FP. Did you try to contact the maintainers?
Also you may consider filtering the cases like this to reduce FP rate.

Can we put that in the all-for-one.md Issue template, suggesting a clear alternative method for researchers to communicate the one useful result found by your query to the seclab? I found dozens of results, but I don't have the time or resources to go after them (otherwise I would have opened a Bug Slayer ticket instead of the All For One.)

I tried to pick the most ambiguous and difficult result to exploit. Still, the query is public and can be run by anyone, so that doesn't really "protect" from disclosure. But I do understand that making things extra easy is not really a good thing.

JarLob · 2021-06-10T18:46:09Z

Great, I saw you have created github/securitylab#382

Imho, we expect people to provide some recent CVE (not too old, it still has to be relevant 😅), that could have been found with the query. That's why it can be a source snapshot from the past. Usual path is to look for advisories and say hey, my query would find the vuln in that revision before the fix. Finding a vuln in existing project is OK too, but you have to at least try to do a coordinated disclosure before dropping it publicly. It boils down to: is it real vulnerability that happens in real apps (proof) or just a made up assumption like if you pass untrusted input to CompileAndRun it may lead to RCE (but it doesn't really happen).
Just my Imho.

gagliardetto · 2021-06-10T19:08:48Z

provide some recent CVE [...] that could have been found with the query

Oh, I always thought that I had to provide CVEs that I had found and created thanks to the query.

JarLob · 2021-06-10T19:14:03Z

If you find at least 4 your name is Bug Slayer :D

gagliardetto · 2021-06-10T19:28:12Z

is it real vulnerability that happens in real apps (proof) or just a made up assumption

I like that phrasing; I'll add that.

gagliardetto · 2021-06-10T19:36:44Z

Finding a vuln in existing project is OK too, but you have to at least try to do a coordinated disclosure before dropping it publicly.

You are absolutely right.

Daily realities sometimes diverge from the rightful and ideal path.

I want to make sure the security lab can still catch them.

smowton · 2021-07-01T12:29:00Z

+
+  override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { any() }


This can be very expensive. Instead of computing flow to absolutely any expression then filtering in isGuardedByCheckOnUntrusted, do your filtering here (i.e., first find if-conditions that control header writes, then enumerate interesting descendents of those if-conditions, which are either string-suffix checks or map-membership checks or equality test operands), then check for untrusted flow.

Does it make more sense with how I updated it now? @smowton

Yes, much better

smowton · 2021-07-01T12:29:25Z

+        operand = child or
+        operand = child.(LorExpr).getAnOperand() or
+        operand = child.(LandExpr).getAnOperand()


Surely getAChildExpr() applied one more time takes care of cases 2 and 3?

Not sure I follow what you mean here..

I mean, you already use getAChildExpr*(), so why do you need to special-case LorExpr and LandExpr?

You're right, it work without them.

Wait, actually it doesn't

Nope, my bad. Removed too much. It's all good.

gagliardetto · 2021-07-09T09:37:54Z

Ping @smowton

gagliardetto · 2021-07-15T13:07:08Z

All good now? @smowton

smowton · 2021-07-15T16:51:45Z

+/**
+ * Holds if the provided `dst` is also destination of a `UntrustedFlowSource`.
+ */
+predicate flowsToGuardedByCheckOnUntrusted(HTTP::HeaderWrite allowOriginHW) {


Define a class for HeaderWrites that use the allow-origin key, and use that type for every allowOriginHW variable. Perhaps similarly allow-credentials HeaderWrites.

Oh that's a lot more elegant. Love it.

Co-authored-by: Chris Smowton <smowton@github.com>

gagliardetto · 2021-07-16T14:46:37Z

Is this good to go?

gagliardetto · 2021-07-16T18:33:19Z

Thank you @smowton !

Add query to detect CORS misconfiguration

f261f34

gagliardetto requested a review from a team as a code owner May 22, 2021 16:18

Add missing newline

924e445

gagliardetto added 3 commits May 22, 2021 18:32

Add allowOriginIsWildcardOrNull predicate

9d1f13f

Cleanup

74f8f1d

Improve qhelp doc

8525c58

gagliardetto added 2 commits June 3, 2021 10:50

Convert header values to lowercase before comparing

56e99b6

Add flag checks

4662358

gagliardetto mentioned this pull request Jun 3, 2021

[go]: Add query for detecting CORS misconfiguration github/securitylab#379

Closed

1 task

Wildcard origin does not allow Access-Control-Allow-Credentials: true

824b5a4

Reduce false positives

c0f195b

smowton reviewed Jul 1, 2021

View reviewed changes

Don't use any() as sink

66bd56f

Remove special cases inside if

92e0f02

smowton reviewed Jul 15, 2021

View reviewed changes

gagliardetto and others added 2 commits July 16, 2021 00:42

Apply suggestions from code review

e92738a

Co-authored-by: Chris Smowton <smowton@github.com>

Add AllowOriginHeaderWrite and AllowCredentialsHeaderWrite classes

52b650a

smowton reviewed Jul 16, 2021

View reviewed changes

Comment thread ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated

Comment thread ql/src/experimental/CWE-942/CorsMisconfiguration.ql Outdated

smowton force-pushed the cors-misconfig branch from dbc280a to 4e82fd9 Compare July 16, 2021 09:50

use hasFlowTo where possible

87afdae

smowton force-pushed the cors-misconfig branch from 4e82fd9 to 87afdae Compare July 16, 2021 13:38

smowton approved these changes Jul 16, 2021

View reviewed changes

smowton merged commit b03513b into github:main Jul 16, 2021

intrigus-lgtm mentioned this pull request Sep 27, 2021

Codeql to detect CORS misconfiguration in go webapp github/codeql#9303

Open


		override predicate isSource(DataFlow::Node source) { source instanceof UntrustedFlowSource }

		override predicate isSink(DataFlow::Node sink) { any() }

Conversation

gagliardetto commented May 22, 2021

Uh oh!

gagliardetto commented May 22, 2021

Uh oh!

smowton commented May 27, 2021

Uh oh!

smowton commented May 27, 2021

Uh oh!

gagliardetto commented May 27, 2021 via email

Uh oh!

gagliardetto commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smowton commented Jun 3, 2021

Uh oh!

gagliardetto commented Jun 3, 2021

Uh oh!

gagliardetto commented Jun 3, 2021

Uh oh!

smowton commented Jun 3, 2021

Uh oh!

gagliardetto commented Jun 8, 2021

Uh oh!

JarLob commented Jun 9, 2021

Uh oh!

gagliardetto commented Jun 10, 2021

Uh oh!

gagliardetto commented Jun 10, 2021

Uh oh!

JarLob commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gagliardetto commented Jun 10, 2021

Uh oh!

JarLob commented Jun 10, 2021

Uh oh!

gagliardetto commented Jun 10, 2021

Uh oh!

gagliardetto commented Jun 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gagliardetto commented Jul 9, 2021 via email

Uh oh!

gagliardetto commented Jul 15, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gagliardetto commented Jul 16, 2021

Uh oh!

gagliardetto commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gagliardetto commented Jun 3, 2021 •

edited

Loading

JarLob commented Jun 10, 2021 •

edited

Loading