elixir-plug / plug

Compose web applications with functions
https://hex.pm/packages/plug
Other
2.84k stars 583 forks source link

CSRF-Token is reusable #504

Closed smoes closed 7 years ago

smoes commented 7 years ago

We recently had a pentest running and one security flaw that was reported is that CSRF-Tokens can be reused over multiple requests. I inspected it via get_session("_csrf_token") which yields the same value for multiple request of the same form. Although, the latter embedded CSRF token seems to differ each request, when using an already used CSRF-Token a form can be validated again and again.

I took a quick look at Plug.CSRFProtection, but was not able to fully understand the interplay with phoenix.

Is it possible to invalidate an CSRF-Token after it has been used once?

josevalim commented 7 years ago

Could they please expand on the concerns of the CSRF token being reused? If we change the CSRF token per request, it means that the user can never have two tabs open on the same website. For example, imagine they are filling a larger form and then decided to do some actions on the website. Such would expire the CSRF token.

Furthermore, I am not aware of any framework that builds a CSRF token per use. Instead all of them use a masking mechanism to guarantee enough variation on the rendered contents.

Therefore, we would be willing to support one CSRF token per use but I would like to know which kind of attacks it aims to prevent.

ericmj commented 7 years ago

Here are some related discussions on the subject: http://security.stackexchange.com/questions/22903/why-refresh-csrf-token-per-form-request http://stackoverflow.com/questions/8655817/csrf-protection-do-we-have-to-generate-a-token-for-every-form

It seems like generating a token per session is generally fine but there needs to be a way to manually invalidate the token.

josevalim commented 7 years ago

A way to delete the token already exists: https://github.com/elixir-lang/plug/blob/master/lib/plug/csrf_protection.ex#L111

smoes commented 7 years ago

Thank you @josevalim and @ericmj for your feedback. Especially after reading the linked discussions I totally agree with you on having one token is fine. However, since the pentesters complained about it, our customer wants that to be changed. I will talk to the pentesters and ask them about their concerns. If there are valid points I will get back here and throw them in ;)

smoes commented 7 years ago

The reason the pentesters categorize that as a security flaw is the following (I am citing them, I will not evaluate their reasoning):

Since the CSRF-Token is only connected to the Session-Cookie but not to a specific form, a potential attacker may generate valid CSRF-Tokens using reverse-engineering. Using phishing mails and/or manipulated websites everything can be done with the user's permissions, then.

josevalim commented 7 years ago

Given the CSRF token is encrypted inside the session, I don't believe someone can generate a valid CSRF token. If they do, they will be able to generate ANY session information, and that's much worse than reverse engineering CSRF tokens (as they would be able to log in as any user).

I will close this for now as it seems that, if you want one token per page, you can probably call delete_csrf_token after protect_from_forgery. Let me know if such solution is not enough. Thank you for the clarification!