softwaremill / akka-http-session

Web & mobile client-side akka-http sessions, with optional JWT support
https://softwaremill.com/open-source/
Apache License 2.0
440 stars 58 forks source link

CSRF protection can be bypassed #77

Closed willemvermeer closed 3 years ago

willemvermeer commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

per OWASP recommendations using secure hash of session cookie is discouraged. Suggested approach is to either encrypt the cookie 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.

Picking up on this one: how I could see this working would be:

This way the CSRF cookie value is not bound to the session ID yet for an attacker it is impossible to generate a valid CSRF cookie value because they don't know the server secret.

Do you think this is a valid approach? If yes I can try to submit a PR with an implementation.

On Thu, Dec 3, 2020 at 11:32 AM Michał notifications@github.com wrote:

Hi Willem, per OWASP recommendations https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#double-submit-cookie using secure hash of session cookie is discouraged. Suggested approach is to either encrypt the cookie 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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/softwaremill/akka-http-session/issues/77#issuecomment-737853686, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIHMP2XNATXY3GMLEY3U23SS5SK5ANCNFSM4UBESQBA .

willemvermeer commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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: https://github.com/softwaremill/akka-http-session/pull/79

adamw commented 3 years ago

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

willemvermeer commented 3 years ago

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

adamw commented 3 years ago

@willemvermeer I suppose this can be closed now?