CVE-2014-0090 - Session fixation, new session IDs are not generated on login
|Assigned To:||Dominic Cleal|
|Target version:||Sprint 21|
|Found in release:||Pull request:|
|Velocity based estimate||-|
Since new session id is not generated every time users log in, authentication can be bypassed through session fixation attacks in the situation where attackers are able to fixate another user's session id. Once users log in with the session id, attackers could also access the whole site with the user's privilege.
At host A, get a new session_id by accessing /users/login with any existing cookie removed.
At host B, access /users/login through http proxy. Intercept a request and delete Cookie header if exists. Intercept its response and
modify _session_id in Set-cookie header with the one got in host A.
At host B, access /users/login and verify if the injected _session_id is using in Cookie header.
At host B, log in with admin(or any user) account.
At Host A, verify if the session is considered as authenticated.
User at host A can access the application bypassing authentication
The session ID should be always changed when users log in.
#5 Updated by Dominic Cleal over 3 years ago
Please just upload patches here, thanks.
#6 Updated by Dominic Cleal over 3 years ago
Review comments of v1 patch:
1. Should this only be in the
request.post? branch, so we're not cycling sessions for every render of the login page?
2. This should also be in
3. This introduces a regression with saved items within the session (see session_expiry in app controller), they're deleted as the session is reset.
4. Are the API controllers vulnerable too, since they also use the
#7 Updated by Joseph Magen over 3 years ago
changes in v2 patch attached
1. moved to request.post?
2. added to extlogin.
3. saved original_uri. good catch
4. API not effected. It doesn't create a new session session. Either the authentication must be part of the request or the it checks if there is an existing user session (by UI) already exists.
#8 Updated by Dominic Cleal over 3 years ago
3. the session_expiry handler also saves the current taxonomy - maybe we should put this into a shared method so we don't have discrepancies
4. my understanding is that not creating new session IDs is the issue here. The security bug is that we take the session ID from the request, then escalate its privileges by assigning a current user so an attacker with a copy of the session ID (or who planted it into a user's request) gets escalated privileges.
The solution in the non-API controllers is to always generate new session IDs when we authenticate and escalate privileges. Since we do this on every request in the API, I'm not sure what the equivalent would be. Generating a new session on every request sounds expensive. Can we disable session handling in these controllers, or stop the authentication concern from storing escalated user details in the session when inside API controllers?
#9 Updated by Dominic Cleal over 3 years ago
4. Ah, perhaps it's already the case that the API controllers don't touch the session: https://github.com/theforeman/foreman/blob/0f7d219a4a65cd795eecd05117b08511d9025de2/test/functional/api/base_controller_subclass_test.rb#L44-L47
The only minor issue is that the base controller test doesn't test in the case where login=true, only login=false.
#10 Updated by Joseph Magen over 3 years ago
updated to include location_id, organization_id is saved session.
The API doesn't "login". Either the it users the session from a currently logged in user, or it needs to pass authentication credentials in the request. So, I don't see how the API touches this bug.
#11 Updated by Dominic Cleal over 3 years ago
- Status changed from Ready For Testing to Pending
ACK, tests well. Thanks Joseph! I think we can release this tomorrow.
I agree that the API's unaffected, but the API does use the same Foreman::Authorization concern to process basic authentication as the main application so I thought that was vulnerable. It sets User.current, but it doesn't change the session because api_request? is true (see link above).
#12 Updated by Dominic Cleal over 3 years ago
- Status changed from Pending to Ready For Testing
I take that back, the tests are failing (test/functional/users_controller_test.rb).
The test failures expose a bigger problem, which is that under SSO authentication (which is triggered under any request protected by the require_login filter), the issue is still present. The authenticate method in the Foreman::Controller::Authentication concern will set session[:user], but the session isn't reset first.
#14 Updated by Joseph Magen over 3 years ago
Are we referring to this line in the code regarding SSO
This calls authenticate! for each provider.
I haven't worked with the SSO code, so I think it will be more efficient if Marek can take a look at this issue (as I think he wrote the SSO code). I don't have apache installed locally to test it.
#16 Updated by Dominic Cleal over 3 years ago
- File 0001-fixes-4457-Session-fixation-new-session-IDs-are-not-.patch added
Attached a new patch for review, which resets the session when the user is authenticated via UsersController#login and also via Foreman::Controller::Authentication (used for SSO), but not when it's an API request (where we don't store the user in the session).
#18 Updated by Dominic Cleal over 3 years ago
Apologies, missed a file from the commit.
#20 Updated by Dominic Cleal over 3 years ago
As per our IRC conversation, this works for me. When testing expiry, set the idle_timeout setting to '1' and wait a minute for the session to expire. Deleting your session ID isn't the same as a time-based expiry - it's only on a time-based expiry that we know the user is still the same one, and are able to restore their context.
#22 Updated by Greg Sutcliffe over 3 years ago
I've reproduced the issue locally and tested Dominic's patch, which correctly fixes the issue. I've also tested that the session expiry correctly stores the user's current taxonomy scope, so it's restored when logging back in.
Good to merge from my perspective.
#23 Updated by Dominic Cleal over 3 years ago
Yes, I don't think you're testing the right thing, and even before this patch, what you're testing wouldn't work. On session expiry (that is, the time limit by the idle_timeout setting) then the current context is preserved with this patch.
If you delete your session ID then you're testing something else. We can only restore original_uri in that case because the browser is making a new request for it, while the browser doesn't provide anything in the request about its taxonomy context. We could store this in a cookie instead, but this is unrelated to the session fixation issue and I don't think I've caused a regression.
I do however need an ack from another maintainer to proceed, thanks.