Logout form #275

kantorii · 2020-02-15T09:45:20Z

Add the option to logout only with POST request (possibly using CSRF Tokens).
At the same time, allows specifying a form for the logout page.

This may answer #112 , although I'm not sure if @golyalpha was only talking about the JSON API or removing logout functionality from the GET request (which #79 does not do).

Render `logout_form` at `logout` endpoint, if set. Extract and update translation. Translate `Logout` to Japanese.

jwag956 · 2020-02-15T15:00:54Z

I don't quite understand the rationale for this PR. POST /logout has worked since June (as mentioned in #112). Why add all this mechanism for a form that doesn't contain anything?

I do appreciate your effort and PR - it seems pretty complete - in general it is best to file an issue defining the bug or feature enhancement. Then we can discuss how to proceed. Thanks.

kantorii · 2020-02-16T10:02:08Z

Yes, this might be more of a proposal rather than a solution.

I switched from the original Flask-Security to this in the hope of obtaining a framework that was more secure. Thanks to you, I believe this to be true. ("Secure" here means robust against various kinds of attacks.)

The motivation behind this PR is to make the framework more robust.

Maybe I should explain an imaginary wiki service. The wiki service allows images to be embedded in the wiki pages by writing a link. (e.g. [img]https://example.com/image.jpg[/img] which gets translated to <img src="https://example.com/image.jpg">)

Some say a browser may prefetch some content, which I believe was the motivation behind feature(view): add POST logout for proper json #79 . So, there is currently a way to logout the user with a POST request.
A mischievous user of the wiki service may include the /logout URL as an image component on a wiki page, therefore logging out all users opening the page (e.g. [img]/logout[/img] ). The mischievous user may realize the depth of damage that he (or she) caused, but he can't even remove the link, because he gets logged out himself. The solution to prevent this is to remove GET functionality from a logout endpoint, or just provide a page for logging out without actually logging out the user. (This is not addressed in feature(view): add POST logout for proper json #79.)
The same kind of attack can be implemented on a totally separate web site (as in https://hackerone.com/reports/54610 ).
It would be nice if I didn't have to use Javascript to logout a user, hence the motivation to provide a logout form with a GET request on /logout.
If I do that, I could even use CSRF tokens to make it harder for imposters.
Yes, a form with no content other than CSRF token would not be user-friendly. Maybe I should have put "Are you sure you want to logout?" in the security/logout_user.html template. (I was assuming other developers would override it anyway with a Cancel button. I see some financial institutions provide such a page with advertisements.)

jwag956 · 2020-02-16T15:24:10Z

Thank you for the detailed proposal. I like the security fix to not allow GET for /logout - so at a minimum we should fix that.

While this PR adds a lot of boilerplate - it does have the advantage of making all FS endpoints consistent which I like.

So - lets proceed - I will do a review - there are still a few things necessary to fully integrate this in..

Thanks for switching over and taking the time and effort to contribute.

jwag956

Some additional work to actually get this all integrated.

As I mentioned in my prior comment - I think the key fix here is to provide a way to not allow GET to logout. That could be fixed very simply by providing a SECURITY_LOGOUT_METHODS=["GET", "POST"] (for backwards compat) and then at some point we can change the default... I think most applications can easily figure out how to create an in line form... Your choice.

jwag956 · 2020-02-17T01:10:47Z

flask_security/views.py

@@ -211,6 +211,20 @@ def logout():
    return redirect(get_post_logout_redirect())


+def logout_with_form():
+    """View function which handles a logout with a form."""
+    form = _security.logout_form(request.form)


Please make sure and test that this works with JSON. Doesn't look like it will.

jwag956 · 2020-02-17T01:14:16Z

flask_security/views.py

+
+    if form.validate_on_submit():
+        response = logout()
+        assert not current_user.is_authenticated


No asserts in production code. Also - this whole method should probably follow more closely how logout() works - including json, redirects, checking if authenticated, tf_clean_session, etc.

It might make sense to simply have a single endpoint - and change behavior based on whether _security.logout_form is set?? Would that require less duplicate code?

Thanks for the wait. Here is my first attempt to resolve this comment.
kantorii@63e83a8

logout() now performs similarly to the original logout(), because indeed it calls the original function.

I wasn't sure in what situations someone would specify a logout form when using json. Or, did you intend for the json API and html API to coexist?

jwag956 · 2020-02-17T01:15:47Z

flask_security/views.py

+        return response
+
+    return _security.render_template(
+        config_value("LOGOUT_USER_TEMPLATE"), logout_user_form=form, **_ctx("logout")


context processors have to be created and registered - please do so and be sure to test it.

kantorii · 2020-02-18T02:38:18Z

Thanks for checking it out. I'm glad you understood what I wrote.

Please give me some time. I'll try to follow what you said.

Remove `assert` from `logout()` (There is still one in `two_factor_verify_password()`)

jwag956 · 2020-02-27T00:50:26Z

So - I don't like conflating having a form or not with which methods are accepted. To put it another way - GET that results in logout seems like a small but serious security issue. In the next release or 2 I would like to change the default so that GET (by default at least) never logs off, only POST does (regardless of forms or not).
Besides CSRF (which I am not certain is actually interesting) there isn't a reason to have a form at all. And if we think CSRF is worth the code - it is equally important for JSON as form input. I know this makes things way more complex (which is why I am not certain it is worth it to have CSRF for logout).

A few other things - logout.html but I didn't see a logout in forms.py??

I don't want to discourage folks spending effort to make all this better - I also know we all get invested in code we have written. Would you consider scaling back and putting off adding a form and just addressing the issue of a configuration variable that disables GET?

If you really want to continue down this path - I will do a more complete review.
As for your question - JSON is really just a different format - internally everything is manipulated as a form. So if we think CSRF on logout is important - then it should work with JSON as well.

kantorii · 2020-02-27T11:23:23Z

I don't intend to force any implementation on everybody. (Actually, I'm not sure POST only as default would be acceptable, since it'll break almost all the web sites that use Flask-Security-Too, unless utmost security should be the default.) So long as I can disable /logout altogether, I'll be able to implement my own, so that would be my minimum.

It's really up to the owner of this software to decide, because it will certainly make the framework a little more difficult to understand in full. Ultimately, it comes down again to the trade off between ease of use and security.

What I do want is at least one person who agrees with what I do, especially someone who can review my code.

So here is a link with some information for you to see if you agree with implementing CSRF for logout.
https://security.stackexchange.com/questions/62769/should-login-and-logout-action-have-csrf-protection

jwag956 · 2020-02-27T14:57:12Z

Thanks for the link - I went through all those when I re-did the CSRF support around 6 months ago. I mostly use OWASP:
https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#login-csrf

as a guide and it doesn't mention it at all. It does mention not using GET for mutating operations (which I think logout is in some respect). And your link seemed to be neutral or negative on logout CSRF protection. Given that, and the complexity, and there are more important/useful security fixes that we should be adding to Flask-Security - as owner I think this is more complexity than the security issue warrants.

I do think that the much simpler fix - namely a configuration that disallows GET (but defaults to GET and POST) would be a nice addition and close a more interesting security hole.

I believe today you can define your own LOGOUT_URL, and use the logout_user exposed method to actually log out the user and therefore easily implement your own logout form etc.

kantorii · 2020-02-27T23:09:25Z

I'm ok with your decision, because I agree with following OWASP practices.

But I do want to make sure we interpret the StackExchange link in the same way. There are currently 5 answers, and to me all the answers but one seem to be positive towards CSRF on logout. The answer that is negative says it's going to be exploitable anyway with self XSS, but social engineering is out of the scope of this pull request. The others are positive or start out negative with an edit making it positive. I'm going to ignore the answers that seem to confuse CSRF with POST and the answers related to DoS, which leaves the problem of combining logout and phishing: https://www.tomsguide.com/us/lastpass-phishing-attacks,news-22139.html

Also, if you really want "logout on POST only" as default, then providing a web page with the GET request may be your choice. Most web sites use GET requests for logging out, so the change in implementation won't break these sites. (Although functional, the logout web page will look ugly).

If your conclusion doesn't change with the comments above, I'll go ahead with implementing SECURITY_LOGOUT_METHODS. But I need to make sure that setting LOGOUT_URL actually gets me what I want (disabling the default /logout altogether), because it seems to me that it will cause a conflict in registering routes. In that case, I'll disable /logout if SECURITY_LOGOUT_METHODS is empty.

jwag956 · 2020-02-27T23:30:48Z

Of course - redefining LOGIN_URL doesn't actually accomplish what you need - I answered that before my second cup of coffee. I think your solution - if LOGOUT_METHODS==None - then don't register - seems simple.

I might be skipping over parts of the SE article - but it seems like CSRF on login and only POST for logout seems to pretty much mitigate almost all the attacks.

I do agree that alas, there are HUGE number of sites - including all older Flask-Security sites that only support logout with GET.

Thanks for your help with this.

kantorii · 2020-03-02T07:33:48Z

Now that #282 is out, I think this Pull Request can be closed but not merged.

kantorii added 2 commits February 15, 2020 15:03

Add logout_form argument to Security initializer.

faca5f3

Render `logout_form` at `logout` endpoint, if set. Extract and update translation. Translate `Logout` to Japanese.

Update documentation.

847de81

jwag956 requested changes Feb 17, 2020

View reviewed changes

Single endpoint for logout

63e83a8

Remove `assert` from `logout()` (There is still one in `two_factor_verify_password()`)

kantorii mentioned this pull request Mar 2, 2020

HTTP request methods for Logout #282

Merged

jwag956 closed this Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logout form #275

Logout form #275

kantorii commented Feb 15, 2020

jwag956 commented Feb 15, 2020 •

edited

Loading

kantorii commented Feb 16, 2020 •

edited

Loading

jwag956 commented Feb 16, 2020

jwag956 left a comment

jwag956 Feb 17, 2020

jwag956 Feb 17, 2020

kantorii Feb 26, 2020 •

edited

Loading

jwag956 Feb 17, 2020

kantorii commented Feb 18, 2020

jwag956 commented Feb 27, 2020

kantorii commented Feb 27, 2020

jwag956 commented Feb 27, 2020

kantorii commented Feb 27, 2020 •

edited

Loading

jwag956 commented Feb 27, 2020

kantorii commented Mar 2, 2020

Logout form #275

Logout form #275

Conversation

kantorii commented Feb 15, 2020

jwag956 commented Feb 15, 2020 • edited Loading

kantorii commented Feb 16, 2020 • edited Loading

jwag956 commented Feb 16, 2020

jwag956 left a comment

Choose a reason for hiding this comment

jwag956 Feb 17, 2020

Choose a reason for hiding this comment

jwag956 Feb 17, 2020

Choose a reason for hiding this comment

kantorii Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

jwag956 Feb 17, 2020

Choose a reason for hiding this comment

kantorii commented Feb 18, 2020

jwag956 commented Feb 27, 2020

kantorii commented Feb 27, 2020

jwag956 commented Feb 27, 2020

kantorii commented Feb 27, 2020 • edited Loading

jwag956 commented Feb 27, 2020

kantorii commented Mar 2, 2020

jwag956 commented Feb 15, 2020 •

edited

Loading

kantorii commented Feb 16, 2020 •

edited

Loading

kantorii Feb 26, 2020 •

edited

Loading

kantorii commented Feb 27, 2020 •

edited

Loading