frankie567 / starlette-csrf

Starlette middleware implementing Double Submit Cookie technique to mitigate CSRF
MIT License
63 stars 5 forks source link

sensitive_cookies and preventing login CSRF #13

Closed bkis closed 1 year ago

bkis commented 1 year ago

Hi again! Sorry for flooding you with issues lately, but I happen to work on a project using your fine suite of tools, so I stumble upon some things from time to time.

This time it's the following: starlette-csrf seems to work as expected in the context of my project, but there is one thing I think would be very useful but isn't possible ATM. The part of the OWASP Cheat Sheet you linked to that talks about double submit cookies has a passage that says

When a user visits (even before authenticating to prevent login CSRF), the site should generate a (cryptographically strong) pseudorandom value and set it as a cookie on the user's machine separate from the session identifier.

(emphasis by me) I may be misunderstanding login CSRF. I mean, there is no auth cookie before login. But the article makes it sound like it's a thing. So if my question misses the point, please help me out to understand it right :)

Now what I found is that starlette-csrf has a sensitive_cookies argument that defines the cookies that should trigger the CSRF check. This is very useful for me, as I want to allow cookie-based auth for my web client as well as JWT-based auth for external use of my server API. So I set sensitive_cookies to include the name of my authentication cookie (I use FastAPI-Users, as you know) to only enforce CSRF-checks when my auth cookie comes with a request via a method considered "unsafe".

The problem is: By doing this, I cannot prevent login CSRF anymore, because my auth cookie is only present in the client (web browser) after login. So calls to my login route won't have CSRF protection. There is exempt_urls to ignore calls to certain paths, but is there a way to enforce CSRF protection on requests to certain paths (like asgi-csrf's always_protect) even if the "sensitive cookie" is not present in the request? If there isn't (which I assume, as it's not documented), wouldn't that be a good addition to starlette-csrf's functionality?

As always: Thank you for your work!

bkis commented 1 year ago

I just now realize that enforcing CSRF checks on certain routes would conflict with the idea of sensitive_cookies in the first place. Because we'd then also enforce CSRF checks when a JWT (bearer) auth is used instead of cookies. Maybe I just really misunderstood what is meant by "login CSRF".

frankie567 commented 1 year ago

Well, login CSRF could indeed be a thing. They explain it how here: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#login-csrf The attack is a bit circumvoluted, but it may happen.

So, regarding the middleware, I suppose we're indeed introducing a risk regarding login CSRF with sensitive_cookies. So yes, the solution could be to have a similar always_protect mechanism as asgi-csrf, which would take precedence over sensitive_cookies.

bkis commented 1 year ago

Yeah and my concerns from above are nonsense, too, because the cookie-based login and the bearer/jwt-based login would have different API paths anyway. So we could say always_protect={"/auth/cookie/login"} and another route /auth/jwt/login route would be working fine, without CSRF checks.

frankie567 commented 1 year ago

Implemented in v2.0.0: https://github.com/frankie567/starlette-csrf/releases/tag/v2.0.0

bkis commented 1 year ago

I just tested the new release with required_urls - works like a charm 👍