-
Notifications
You must be signed in to change notification settings - Fork 311
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
Add force_new parameter to generate_csrf #354
base: main
Are you sure you want to change the base?
Conversation
Security best practices indicate CSRF tokens should be unique for each user session. Adding a force_new parameter to generate_csrf enables users to generate new CSRF tokens between user sessions with minimal code changes or understanding of CSRF generation internals required.
Codecov Report
@@ Coverage Diff @@
## master #354 +/- ##
==========================================
+ Coverage 99.67% 99.67% +<.01%
==========================================
Files 18 18
Lines 911 919 +8
Branches 74 74
==========================================
+ Hits 908 916 +8
Misses 3 3
Continue to review full report at Codecov.
|
As an aside, is there an upcoming release planned for Flask-WTF at some point in the future? |
We recently had to roll this behavior ourselves in a project using flask-wtf. In some cases, failing to rotate a CSRF token on website login can lead to CSRF fixation attacks, whereby an attacker knowingly sets the victim's CSRF token before the victim logs in. Once the victim has logged in, and the CSRF token doesn't rotate, the attacker can force the victim to make state-changing requests to a web application without their knowledge and against their will. This feature would be greatly appreciated! |
I don't understand why this is necessary. If a user clicks "log out", the logout route should clear the session, at which point a new token would be generated next time anyway. Same with if a user logs in and the session isn't empty. Teaching users to call this function with this argument in these cases is exactly as much work, but less comprehensive, than teaching them to do |
I'm also not clear how you'd expect this to fix session fixation, as it doesn't prevent an attacker reusing a previous session cookie as long as it's not expired. The way you protect against that is with HTTPS, not with CSRF. Note that Flask's secure cookie already prevents tampering with the data within the cookie. |
This doesn't fix session fixation, it fixes the possibility of CSRF fixation: where an attacker knows, or is able to set, the victim's CSRF token. The OWASP CSRF cheat sheet recommends CSRF tokens are unique to each user session.
Imagine the following scenario:
This attack scenario becomes far worse if other factors lead to the attacker being able to set cookies on the victim's browser:
The second attack is obviously much worse, as it doesn't require physical access to the machine, but I think we would both agree that failing to rotate a CSRF token on login can have serious security implications. |
Could you provide an example of how this change would be used to prevent that? Scenario one is more comprehensively addressed by just clearing the session on login and logout. The session cookie is not vulnerable to step one in scenario two because it is securely signed. |
An example of an exploitable scenario:
Note that even if the session id changes, if the CSRF token does not rotate upon principal change (anonymous -> authorized user), the attacker will not have access to the authenticated session but will now have knowledge of the CSRF token and can use this to execute privileged commands as the user. |
Regarding sessions being cleared on logout: you're right that logging out should clear the session for a user. This isn't the case for Flask-Login, however, which only removes user ID and session ID from the session object, if they exist (link: https://github.com/maxcountryman/flask-login/blob/master/flask_login/utils.py#L192).
By default, Flask sessions are signed cookies containing the session data, which means without a backing interface via Flask-Session or another extension the session object is already not being invalidated in practice for what I imagine are a large number of production environments. Given that the defaults for Flask-Login and Flask together already lead to session cookies that aren't wholly rotated, and Flask itself doesn't provide logic for explicit session invalidation, it seems reasonable to include token rotation as an option in `generate_csrf`.
… On Jan 10, 2019, at 18:19, Christopher Tarquini ***@***.***> wrote:
An example of an exploitable scenario:
I own blogglr.com and provide blogs to my users
Users can control the content of username.blogglr.com
Attacker tricks the user into visiting their blog and set their session/csrf token cookie to a known value (attackers own session/csrf cookie) with the domain set .blogglr.com. Setting the cookie path can also be used to force higher precedence than existing session/csrf cookies
If the User logs in while using this attacker known session/csrf token, the session becomes authenticated and the attacker can now use this knowledge to execute CSRF attacks against the user
Note that even if the session id changes, if the CSRF token does not rotate upon principal change (anonymous -> authorized user), the attacker will not have access to the authenticated session but will now have knowledge of the CSRF token and can use this to execute privileged commands as the user.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
That is addressed by calling |
It does make it easier for users to safeguard their applications without having to change the behavior of their application for users that using In order to make their application safe and maintain the behavior they expect from flask login where only the user-id and session-id are cleared they'd need to:
Alternatively, they would have to rely internals to rotate the CSRF token.
It looks like it should be okay as long as you call session.clear before calling |
tl;dr I don't disagree with you that |
It sounds like a @app.route("/logout")
def logout():
clear_csrf()
logout_user()
return redirect(url_for("index")) I personally haven't encounterd an app where logging out would still want to preserve something in the session, and if it does it's probably more appropriate for Redis or the database. In the majority of cases where that's not needed, @app.route("/logout")
def logout():
logout_user()
for key in session:
if key not in keys_to_keep_for_my_app:
del session[key]
return redirect(url_for("index")) |
The motivation for adding this as a parameter to Providing this is less about good session management within Flask and more about good CSRF token management within Flask and other Flask extensions. Flask's documentation demonstrates popping values off the session during logout (see here), while Flask-Login only mutates the fields it's concerned with on login and logout, so adding the ability to rotate only the token in the session allows for improved CSRF protection without deviating from common practice within the Flask community. |
One example would be a shopping cart, where the user can add items to their cart when logged out but need to login to pay. You'd want to maintain the shopping cart in the transition to an authenticated session. I can imagine a few other scenarios as well (session-bound preferences like sort order, filters for searches, etc).
I actually was about to suggest making this a separate function as well! I think it makes it more clear what's going on, especially since I'd also add that in either case, that in order to ensure the function is used properly, it's essential that the the clearing function ( I'd recommend that the CSRF token only be cleared from the session if |
It's also nice that any authentication libraries that are aware of Plus if you add the safe-guard against rotating after already calling As an aside, I apologize if I came off as short in my earlier comments, the caffeine crash is hitting hard 😅 . All your work on this project is much appreciated |
A Either way works for me; the package has been great to use so far. |
Security best practices indicate CSRF tokens should be unique for each user session. Adding a
force_new
parameter togenerate_csrf
enables users to generate new CSRF tokens between user sessions with minimal code changes or understanding of CSRF generation internals required.