Fix * behavior to not create CORS vulnerability footgun.

[ejcx]

When a user specifies *, then * should be returned by the server in
the access-control-allow-origin header, not the origin header.

All implementations of the CORS standard that reflect the origin header
when * is specified are incorrect, because an Access-Control-Allow-Origin
header of '*' has a different meaning than a reflected Origin header. Refer
to Section 6.1 https://www.w3.org/TR/cors/. When * is set, Credentials
are not allowed to be used in an authenticated request.

What's the big deal?
If you set Allow Credentials to True and Origins to * with this library
then you have turned off SAMEORIGIN policy for your website, which is
unexpected behavior and....really bad.

[ejcx]

Upstream merged a fix: rs/cors#57 !!

[VojtechVitek]

Thanks for the contribution! We will review as soon as possible.

[c2h5oh]

Part of Section 6.1 https://www.w3.org/TR/cors/ that applies to this case:

If the resource supports credentials add a single Access-Control-Allow-Origin header, with the value of the Origin header as value, and add a single Access-Control-Allow-Credentials header with the case-sensitive string "true" as value.

Otherwise, add a single Access-Control-Allow-Origin header, with either the value of the Origin header or the string "*" as value.

Based on that reflecting Origin value in Access-Control-Allow-Credentials is always valid. Specification doesn't express preference for * over returning Origin value. It returning * was the recommended behavior SHOULD would have been used as per https://tools.ietf.org/html/rfc2119

Both handleActualRequest and handlePreflightRequest never return * - it's always Origin value, which I agree is a limitation, but is standard compliant

On the other hand your pull request would reverse it: * would be always returned when c.allowedOriginsAll == true && c.allowCredentials == false - standard compliant as well, but also limited to one of the valid responses. which is invalid when Access-Control-Allow-Credentials: true. I understand this is to prevent a misconfiguration by a user resulting in unsafe behavior, but it removes a valid, standard compliant use.

[ejcx]

It's standards compliant, but when I configure my CORS library for * I want it to return a * because it has a different meaning than reflecting the origin header.

Before this PR ACAO: * and ACAC: true turns off SAMEORIGIN policy. After this PR, it makes it safe. I've had CVEs issued for this exact bug several times now. The behavior is a vulnerability.

I updated the title for you =]

[c2h5oh]

Before this PR ACAO: * and ACAC: true turns off SAMEORIGIN policy. After this PR, it makes it safe.

1. It breaks perfectly valid use cases in which you want to receive/set cookies with XHR requests from any domain - it makes it impossible to turn off SAMEORIGIN policy if you want to.
   Header name Access-Control-Allow-Credentials implies this is about authentication/authorization, but it doesn't have to be - it could be any kind of state.

2. It's a backwards incompatible change for the sake of making it impossible for people to shoot themselves in a foot in a specific way, while removing a standards compliant, valid use.

3. HttpOnly cookie flag that you should be setting anyway already is a partial mitigation for people trying to shoot their own limbs off

1 & 2 are showstoppers for me.
I'm open to a solution that would add a allowCredentialsOnWildcardOrigin setting, but unless we're doing major release it would have to remain true preserving current behavior.

[ejcx]

I'm sorry but the list of CVEs and vulnerabilities the behavior causes is very very long... You're arguing for less security...

You can still turn off SAMEORIGIN policy using the AllowOriginFunc that's commented out in the README.md:

AllowOriginFunc:  func(r *http.Request, origin string) bool { return true },

If you want a list of some places that similar logic has caused security issues.

• https://github.com/captncraig/cors
• https://nodesecurity.io/advisories/148
• Rack CORS is fundamentally insecure. cyu/rack-cors#126

Quite frankly, CORS is really confusing and most people don't know the difference between Access-Control-Allow-Origin: * and a Access-Control-Allow-Origin: <origin>. It's really important to help them with opinionated and safe defaults. Also frankly, * reflecting all origins is not a safe default...

Additionally to address 3., HttpOnly has absolutely nothing to do with CORS. The HttpOnly bit will not mitigate any CORS issues whatsoever.

To top it off, there's no way to return Access-Control-Allow-Origin: * with this library before this PR which is an important configuration for CDNs and many other types of applications. The fact that setting the allowedOrigins to * doesn't return Access-Control-Allow-Origin: * is kind of an obvious cue that there is an issue.

No idea how you could be against this PR tbqh...

[c2h5oh]

Does it break compliance with the standard you're referring to in the pull request description?
Yes

Was the package compliant with that standard before your pull request?
Yes

Does it remove support for a behavior that is allowed by standard, supported by browsers and used in the real world?
Yes

Does it introduce a breaking change for those relying on this valid behavior?
Yes

Does it offer any way of reverting to the old behavior via a setting if you need it?
No

This change is not a clear win you describe it to be. I removes functionality and standard compliance because it can introduce a security vulnerability if misconfigured

[ejcx]

Lol... Man. Open source is a hoot.

[c2h5oh]

I suggested alternative solution that wouldn't break standard compliant behavior that has real world use. I would know, I'm in real world and I use it :P

Your answer is that I'm arguing for less security.

[IceflowRE]

Still no decision?

[hazcod]

ping @ejcx

[ejcx]

@hazcod what's the question for me? I'm not the maintainer of go-chi/cors

[hazcod]

Sorry, pinging @VojtechVitek
I completely agree with @ejcx that modifying the Origin header is a vulnerability in itself.

[pkieltyka]

Hey all, I've opted to take the security advice from @ejcx as this is a common and dangerous mistake. I've also taken the lead from the original author of this middleware at github.com/rs/cors -- if someone wants to allow a wildcard of origins and return that origin name, they should use the AllowOriginFunc handler and return true.

Resolved in b978ea8