search

maxcountryman / flask-seasurf

SeaSurf is a Flask extension for preventing cross-site request forgery (CSRF).
http://readthedocs.org/docs/flask-seasurf/
Other
190 stars 49 forks source link

wrong comparison, making it potentially pointless #20

Closed ThomasWaldmann closed 11 years ago

ThomasWaldmann commented 11 years ago

i think the csrf token must not just be stored in a cookie.

A cookie comes from the client. The cookie name is known and the client (attacker) can (at least potentially) put any value it likes in there. Maybe not easily (different domain) , but browsers could have bugs also...

Then, you compare that value against another value (the csrf token form field or http header value) that also comes from same client.

So, if client just puts same value at both places, it can do any csrf it likes.

If the csrf token is not read from the cookie, but from the session (which either comes from a file on the server or from a signed SecureCookie), the client can not tamper it.

See also there: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_%28CSRF%29_Prevention_Cheat_Sheet

And there: https://docs.djangoproject.com/en/dev/ref/contrib/csrf/ """ Limitations

Subdomains within a site will be able to set cookies on the client for the whole domain. By setting the cookie and using a corresponding token, subdomains will be able to circumvent the CSRF protection. The only way to avoid this is to ensure that subdomains are controlled by trusted users (or, are at least unable to set cookies). Note that even without CSRF, there are other vulnerabilities, such as session fixation, that make giving subdomains to untrusted parties a bad idea, and these vulnerabilities cannot easily be fixed with current browsers. """

alanhamlett commented 11 years ago

Signing the token cookie solves this. Not an issue.

The csrf token SHOULD be stored in the cookie for scaling.

ThomasWaldmann commented 11 years ago

alan, I agree about signing the cookie used for the token (the flask session cookie is signed btw).

not sure why you say it is good for scaling to have it in a separate cookie, can you add more detail? i mean the session (if existing) is usually loaded anyway, so adding another value to it doesn't make a real difference.

maxcountryman commented 11 years ago

Storing a signed token on the client is fine: it means the client can't tamper with the value, it can scale (since it's not server-side), and using the vary header means certain AJAX may make use of it.

ThomasWaldmann commented 11 years ago

Max, why did you just close this? If we all agree that it should be a signed cookie (and not, like it is now, a normal cookie), this issue should be kept open until someone implements that change.

maxcountryman commented 11 years ago

I don't see an issue here. The token is generated on the backend and as that must agree with whatever the client presents that means things should be okay. The client isn't going to be successful at changing that value clientside or predicting what the value will be serverside. So what's the issue here?

ThomasWaldmann commented 11 years ago

the current code is comparing a token X that the client submits as a (unsigned) cookie with a token Y that the client submits as a form field (or http header). note: you did it differently in the early changesets in this repo.

remember the old security rule "don't trust whatever comes from the client"? you are basing a security decision on the server on equality of 2 things both coming from the client (X == Y).

this is working ok as long as the client software can be considered secure and working as expected. but considering the amount of browser bugs we see all the time, this causes a bad feeling in my stomach (esp. since one of the values could be stored in either a signed, untamperable cookie or on the server in the session without a problem).

maxcountryman commented 11 years ago

You're confused. The token is generated in the extension not clientside. Go through the code again if you still don't understand.

ThomasWaldmann commented 11 years ago

it doesn't matter (what you think) where they were generated.

it only matters what inputs you use in your security decision and where they come from.

if both are coming from the client, they (under weird, unforseen circumstances) could both have been generated on the client.

maxcountryman commented 11 years ago

You don't need to be worried here. Django has the same "flaw" and yet there are exactly zero instances of documented exploits against its CSRF protection.

ThomasWaldmann commented 11 years ago

/me leaves, singing "don't worry, be happy".