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

Question: Does it make sense to use Crumb with CORS? #23

Closed
findesk opened this issue Aug 28, 2014 · 10 comments
Closed

Question: Does it make sense to use Crumb with CORS? #23

findesk opened this issue Aug 28, 2014 · 10 comments
Labels
security Issue with security impact support Questions, discussions, and general support

Comments

@findesk
Copy link

findesk commented Aug 28, 2014

Hi,

I may be completely misunderstanding the code and/or XSRF protection in general but...

In 3.x.x. if CORS is enabled, Crumb never generates a crumb cookie. See:

!request.server.settings.cors) {

All requests pass through the plugin with no XSRF protection, even though I am using the plugin.

Does this mean that Crumb should not be used in an environment supporting CORS?

(For background, previously with Crumb 2.2.0, I had CORS set up, was using Crumb restful=false, and passing XSRF tokens in the payload. Which was working great. This no longer works with Crumb 3.x.x. With my same 2.2.0 setup, Crumb doesn't do anything. I was surprised to discover this. Again, perhaps I'm misunderstand XSRF protection when using CORS.)

Thanks
fd

@findesk findesk changed the title Does it make sense to use Crumb with CORS? Question: Does it make sense to use Crumb with CORS? Aug 28, 2014
@stongo
Copy link
Contributor

stongo commented Aug 28, 2014

Very good question!
CSRF Tokens are not needed when using CORS, as CORS adequately handles CSRF.
I'm about to add an Origin check. If the origin doesn't match the server
name and CORS is enabled, crumb validation will be bypassed. This should
handle instances when CORS is enabled, but same origin calls are still
made to the server.
The reason for not setting a crumb cookie when CORS is enabled to
prevent token leakage on cross site requests. Otherwise the crumb cookie
would be set on non-origin sites.

@stongo
Copy link
Contributor

stongo commented Aug 28, 2014

See #24

@findesk
Copy link
Author

findesk commented Aug 29, 2014

With an app at http://api.somedomain.com, if I have cors: { origin: ['http://app.mydomain.com'] } in my server config, can you describe a scenario where token leakage can occur?

If I'd hit http://api.somedomain.com from http://attacksite.com, wouldn't cors in hapi prevent the request, and therefore prevent the CSRF tokens from leaking?

(I'm assuming you can't spoof origin: http://stackoverflow.com/questions/21058183/whats-to-stop-malicious-code-from-spoofing-the-origin-header-to-exploit-cors)

Or are you working to protect against scenarios where cors: { origin: ['*'] } (i.e. hapi default.)

Just trying to understand the circumstances where token leak is at issue.

(With cors origin white listed, I could have other other cookies too, yar session cookie for example. So I'm trying to figure out...what is the harm in exposing CSRF tokens to my white list when other cookies, potentially quite precious cookies, could be exposed.)

@stongo
Copy link
Contributor

stongo commented Aug 29, 2014

I definitely agree it is a wider issue, and there is even more sensitive information that can be leaked with cookies, and I believe this is already on the radar for hapi core, as well as this issue for setting cookies during proxy hapijs/hapi#1813
Crumb went through a security audit, and the CORS token leakage was flagged as well as the proxy leakage.

@findesk
Copy link
Author

findesk commented Aug 29, 2014

Got it. Closing now.

Fyi...I had been taking guidance from:

https://www.owasp.org/index.php/HTML5_Security_Cheat_Sheet#Cross_Origin_Resource_Sharing

See bullet 4.

"Keep in mind that CORS does not prevent the requested data from going to an unauthenticated location. It's still important for the server to perform usual CSRF prevention."

@stongo
Copy link
Contributor

stongo commented Aug 29, 2014

In light of that owasp segment, I could leave crumb as is now, as in still performing crumb check on CORS requests. One can set a route to issue crumb tokens via a CORS route like https://github.com/hapijs/crumb/blob/master/example/restful.js#L13-L22
This would allow for CORS to still use CSRF Tokens and not leak tokens via cookies on every CORS request. Going to review with my security team before closing this.

@stongo stongo reopened this Aug 29, 2014
@stongo
Copy link
Contributor

stongo commented Sep 3, 2014

OK the conclusion of this upon review is that token leakage when CORS origin is set to specified hosts is acceptable, and the cookie should only be unset if cors.origin is set to '*'
See issue #25

@stongo stongo closed this as completed Sep 3, 2014
@tomsteele
Copy link
Contributor

@findesk The scenario where you wouldn't want to leak the token is this. Say you have http://api.mydomain.com, you would like a 3rd party http://foobar.com to access some CORS handlers, but not others. You wouldn't want to leak the CSRF token, because if you did that, well they could make requests to every route.

But we still need support actually allowing POST requests. And you're right, having CSRF protection on these routes is absolutely necessary, as hapi's cor's settings do not prevent simple http requests from being processed by handlers.

There are two ways I can see of handling this. Use the suggestion @stongo pointed to above. Which I think is fine, most frameworks just leak the tokens ;)

The other option, is to set an option of allow csrf origins. This is how sails handles this. https://github.com/balderdashy/sails-docs/blob/master/reference/sails.config/sails.config.csrf.md

@findesk
Copy link
Author

findesk commented Sep 11, 2014

@tomsteele

I think 25 suits.

I can't think of a scenario where one would set CSRF origins (like sails allows) different to CORS origins. So in Hapi, CORS origins should guard token leakage to foreign domains. (I'd hope!)

(Apologies for not replying sooner.)

@Marsup Marsup added support Questions, discussions, and general support and removed question labels Sep 21, 2019
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
security Issue with security impact support Questions, discussions, and general support
Projects
None yet
Development

No branches or pull requests

3 participants