Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rack CORS is fundamentally insecure. #126

Closed
ejcx opened this issue Sep 27, 2016 · 9 comments
Closed

Rack CORS is fundamentally insecure. #126

ejcx opened this issue Sep 27, 2016 · 9 comments

Comments

@ejcx
Copy link

ejcx commented Sep 27, 2016

Rack CORS has a fundamental problem. Setting origin: '*' should return a "Access-Control-Allow-Origin: *" and should not reflect the origin header.

Defaulting to Access Control Allow Credentials: true is also inherently insecure, and when combined with this unexpected reflection of the origin header means thousands of sites have "turned off" SAMEORIGIN policy.

Why should it be like this?
Because origin:* means that Credentials should never be passed to the cross origin site. The way you are doing it, developers who mean to set Access-Control-Allow-Origin: * (safely) are getting unexpected unsafe behavior that allows their credentials to be sent cross origin.

What should happen instead?
Origin: "" should return an Access-Control-Allow-Origin: header, instead of a reflected origin header.

It should not be possible to reflect the origin header, as there is no real use case for this that is not covered by Access-Control-Allow-Origin: *.

@ejcx
Copy link
Author

ejcx commented Sep 27, 2016

Just to give a little more detail.

https://github.com/cyu/rack-cors/blob/master/lib/rack/cors.rb#L359
At 359, It will return "*" only if credentials is turned off, but credentials is on by default.

Credentials should be off by default, because people setting a "*" policy without knowing the default are reflecting the Origin header with Credentials true.

This is not intuitive at all, and I'm seeing thousands of sites make this mistake.

@cesc1989
Copy link

Agreed. I've been recently fighting an issue with CORS and preflight requests over an Nginx server and learnt that Access-Control-Allow-Credentials: true can't go along with Access-Control-Allow-Origin: * because an error is raised:

Response to preflight request doesn't pass access control check: A wildcard '*' cannot be used in the 'Access-Control-Allow-Origin' header when the credentials flag is true. Origin '' is therefore not allowed access. The credentials mode of an XMLHttpRequest is controlled by the withCredentials attribute.

Issue #95 also asks about setting Access-Control-Allow-Credentials: true by default.

@jensvoid
Copy link

jensvoid commented Jul 2, 2017

Stumbled over this one when i wanted to file an issue for exactly the same thing.

Rack::Cors maps origins '*' into reflecting arbitrary origins; this is dangerous, because developers would think that "*" behaves according to the spec: mostly harmless because it cannot be used to make to make 'credentialed' requests; this leads to origin reflection with Access-Control-Allow-Credentials headers if the example configuration is used.

Had been looking for reasons of CORS misconfiguration in the scope of an analysis for the Alexa top 1m websites when is stumbled over Rack::Cors often being used in vulnerable websites. As it seems, lots of sites actually apply the example config or trust "*" to behave according to the spec.

Should really be fixed.

@peret
Copy link
Contributor

peret commented Jul 13, 2017

I opened a PR for this. The overall goal of this PR is to make the default/example configuration more secure.

This is a breaking change for:

  1. Users who use the default configuration and do rely on the insecure behavior for some reason.
  2. Users who explicitly set credentials: true in combination with a wildcard origin.

Disclaimer: Even with the PR it's still possible to have an insecure configuration with this gem, e.g. by specifying a super-generic Regex like /(.*)/ as the origin. However, the example config will be secure.

I would love feedback on the fix and whether this is the best way to go. Either way, let's move this thing forward quickly, please :)

@cyu
Copy link
Owner

cyu commented Jul 15, 2017

Thanks @ejcx @peret and everyone else for doing the heavy lifting on this. I'm working on putting this and a couple of other breaking fixes together for a 1.0.

Is there any posts that thoroughly discusses this issue? I want to add it to the readme.

@kimroen
Copy link

kimroen commented Jul 15, 2017

Here's a post that might be helpful: http://blog.portswigger.net/2016/10/exploiting-cors-misconfigurations-for.html

@jensvoid
Copy link

jensvoid commented Jul 15, 2017

Our large scale analysis of CORS misconfigurations blogpost (mentioning Rack::Cors a major cause):
http://web-in-security.blogspot.de/2017/07/cors-misconfigurations-on-large-scale.html

@cyu cyu closed this as completed Jul 15, 2017
@ejcx
Copy link
Author

ejcx commented Aug 3, 2017

@jensvoid . Your research, inspired by James Kettle's, is inspired by my research of the alexa top 1m.... I did this over 1.5 years ago now!

Funny how things repeat:
https://ejj.io/misconfigured-cors/

@ejcx
Copy link
Author

ejcx commented Aug 3, 2017

Also @peret way to take the initiative. I'm not rubyist and couldn't figure it out.

elithrar pushed a commit to gorilla/handlers that referenced this issue Nov 1, 2017
There's no reason to allow for a server to reflect all origin headers.
This has caused numerous security problems in the past.
 - cyu/rack-cors#126
 - https://nodesecurity.io/advisories/148
 - captncraig/cors@cc1cf75

Some helpful blog posts on the topic:
 - https://ejj.io/misconfigured-cors/
 - http://blog.portswigger.net/2016/10/exploiting-cors-misconfigurations-for.html
camallen pushed a commit to zooniverse/panoptes that referenced this issue Nov 20, 2019
* [Security] Bump rack-cors from 0.4.1 to 1.0.6

Bumps [rack-cors](https://github.com/cyu/rack-cors) from 0.4.1 to 1.0.6. **This update includes a security fix.**
- [Release notes](https://github.com/cyu/rack-cors/releases)
- [Changelog](https://github.com/cyu/rack-cors/blob/master/CHANGELOG.md)
- [Commits](cyu/rack-cors@v0.4.1...v1.0.6)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* security update to fix spec to not reflect origin header

https://github.com/cyu/rack-cors/blob/master/CHANGELOG.md#security-1

cyu/rack-cors#126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants