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

CSRF protection can be bypassed #77

Closed
willemvermeer opened this issue Nov 24, 2020 · 10 comments
Closed

CSRF protection can be bypassed #77

willemvermeer opened this issue Nov 24, 2020 · 10 comments

Comments

@willemvermeer
Copy link

Hi again,

Another question about the CSRF protection. It can be bypassed by forging a request that contains the same value for both the X-XSRF-TOKEN header and the XSRF-TOKEN cookie value. Any value will do since the only check that is now performed in randomTokenCsrfProtection is for those two values to be equal and non-empty.

Would it be somehow possible to conform to the OWASP recommendations described here what do you think would be the best approach?

Thanks,
Willem

@adamw
Copy link
Member

adamw commented Nov 26, 2020

It's been a while since I last looked into this so excuse me if I'm slightly out of date :).

As far as I remember, CSRF attacks are when a user submits a request by clicking a link or even visiting a malicious site. Last I checked (but this could have been 2 years ago), being able to submit anything in the header was sufficient to determine that a request is not a CSRF attack, as setting a header requires running a script (and if a script can be run, than we are dealing with XSS, which is a more serious problem but not in scope here).

But of course new attack vectors might have surfaced, and double-submit cookies might no longer be sufficient. I don't have the capacity right now to investigate, so maybe you could share some recommendations on an implementation that would be more secure? And how an attack against the current one can be carried out?

Thanks!

@willemvermeer
Copy link
Author

Hi Adam,

Thanks for getting back on this one. The issue was detected during a security test by an external company.

On the OWASP site there are some recommendations on how to mitigate this risk: herehttps://owasp.org/www-community/attacks/csrf
What might be possible is to define a new CsrfCheckMode that ties the SessionManager and CsrfManager together, such that the CsrfManager can reuse the session cookie value to create a secure hash instead of the current SessionUtil.randomString(60)
I might be able to help out by working on a PR but would first like to know if you think this would be valuable addition to the library?
Thanks, Willem

@mszczygiel
Copy link

mszczygiel commented Dec 3, 2020

Hi Willem,
per OWASP recommendations using secure hash of session cookie is discouraged. Suggested approach is to either encrypt or to use HMAC of the token as a cookie. For sure it would be valuable addition to the library, so PR is welcome :) Looks like HMAC may be simpler to implement, but any of the solutions recommended by OWASP is fine.

@willemvermeer
Copy link
Author

willemvermeer commented Dec 21, 2020 via email

@willemvermeer
Copy link
Author

Hi @adamw / @mszczygiel ,

I created a PR with a minimal implementation of the suggested CSRF improvement.
How do I move forward? I have no access rights to the repository to push a PR?

Thanks,
Willem

@adamw
Copy link
Member

adamw commented Dec 21, 2020

@willemvermeer great! you should fork the repository to your account, push the branch, then you should be able to open a PR against the original one (this one :) )

@willemvermeer
Copy link
Author

Hi @adamw happy new year :-) this is a gentle nudge to take a look at the PR I submitted for this issue - comments are appreciated when you can find some time, thanks!
See: #79

@adamw
Copy link
Member

adamw commented Jan 5, 2021

@willemvermeer thanks, taking a look now. Sorry it took so long, but we've been mostly away for Christmas / New Year :)

@willemvermeer
Copy link
Author

Thanks @adamw ; I addressed your comments in a followup commit. And no apologies required for taking a Xmas break :-)

@adamw
Copy link
Member

adamw commented Jan 20, 2021

@willemvermeer I suppose this can be closed now?

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

3 participants