Bug #7736
closed
Change to prevent unauthenticated requests for CSRF modified login behaviour as well
Description
The change that went into Foreman as 4e3a7e7a2a5 included definition of handle_unverified_request which in turn affected not just the logout functionality, but login as well. The net effect is that it's now not possible to log in to Foreman WebUI via its logon form at /users/login without GETing that URL first, to get the authenticity_token.
Since issue #6999 was attempting to only change the logout behaviour, I'd call this a regression, unless there is a specific reason why the exception should be raised for the unauthenticated login page where just resetting the session and proceeding with the request was enough.
Updated by Dominic Cleal over 9 years ago
- Subject changed from Change 4e3a7e7a2a542435686a667773eafc73c92e557b for issue 6999 modified login behaviour as well to Change to prevent unauthenticated requests for CSRF modified login behaviour as well
- Category changed from Web Interface to Security
Updated by Dominic Cleal over 9 years ago
- Related to Bug #6999: CVE-2014-3590 - User logout susceptible to CSRF attack added
Updated by Dominic Cleal over 9 years ago
Thanks for the report. Did this affect one of the login features you added? (I'm thinking the form intercept method perhaps?)
Updated by Dominic Cleal over 9 years ago
- translation missing: en.field_release set to 22
Updated by Jan Pazdziora over 9 years ago
Dominic Cleal wrote:
Thanks for the report. Did this affect one of the login features you added? (I'm thinking the form intercept method perhaps?)
It did affect my tests of that feature. It's fairly easy to change the tests but I'm concerned that there was a wholesale change in behaviour even if the issue/commit message only point to a very narrow case (logout). Hence this issue, to spark a discussion about the intended behaviour in all the other cases.
Updated by Dominic Cleal over 9 years ago
- Status changed from New to Rejected
- translation missing: en.field_release deleted (
22)
Yeah, the commit message, as is typical for us, was overly narrow. We realised the scope was broader than logout only, as any cross-domain request that was made missing CSRF tokens would result in the user being "logged out" by way of their session being invalidated.
Looking at what we do during the login process, we reset the user's session immediately before we go any further, as part of the fix for another CVE (#4457). So as not to leave another case where a user could be attacked and get their session deliberately reset, I think we need to retain the CSRF authenticity check on login.
I think we can say this is deliberate and we're preventing an identical attack on the login route.