The logout process does not correctly work in the scenario where a user has attempted to change their password, but they failed. Scenario: 1. User logs in and goes to Preferences->Name and Password 2. The user enters the correct old password and then enters two new passwords which are NOT the same (or less than the minimum password length of 3) 3. This generates an error message about the password. 4. The user clicks the logout link without successfully changing their password. Under these conditions the following problems exist 1. At step #3 the HTTP response will include a new Bugzilla_logincookie for the user even though the user's password update failed. 2. After the user logs out (step 4), the original Bugzilla_logincookie and the new Bugzilla_logincookie from step 3 both still work. They have been cleared from the user's browser by a set-cookie response in step4, but the cookies are still valid on the server and can be used by manually updating the brower's cookies. 3. If the user continues to fail the password change process as described in step 2, multiple valid cookies will be created. All of these cookies remain valid even after the user logs out. Notes: 1. If a user successfully changes their password, then the logout works correctly and all previous cookies are destroyed on the server side. 2. If the user fails the original password check of the change password then this problem doesn't occur. Risk: 1. Increased risk for a user's session to be stolen since the session IDs are still valid even after the user clicks ""logout"" 2. An attacker could create a script for the failed password action which would result in a flood the database with numerous Bugzilla_logincookie values. Remediation: This is an unexpected behavior for the error case when a user incorrectly changes their password. The code should be updated so that a new Bugzilla_logincookie is only generated if the user successfully updates their password. A failed password update attempt should not result in the generation of a new Bugzilla_logincookie. Relevant code: http://mxr.mozilla.org/bugzilla/source/userprefs.cgi#78

(In reply to comment #0) > 2. After the user logs out (step 4), the original Bugzilla_logincookie and the > new Bugzilla_logincookie from step 3 both still work. They have been cleared > from the user's browser by a set-cookie response in step4, but the cookies are > still valid on the server The new logincookie is deleted from the DB and so is not valid anymore. Only the old one remains. > 3. If the user continues to fail the password change process as described in > step 2, multiple valid cookies will be created. All of these cookies remain > valid even after the user logs out. You cannot do everything with these logincookies as you need to be on a computer with the same IP address to use them (unless the session wasn't restricted to a given IP address). > Risk: > 1. Increased risk for a user's session to be stolen since the session IDs are > still valid even after the user clicks ""logout"" The risk is not higher as closing your browser without logging out, or when your network connection is flaky and you have to reconnect all the time because your IP address changes. And changing your password, and failing to do so, is pretty rare anyway. Note that bug 320726 would help protecting users from this problem about multiple logincookies. > 2. An attacker could create a script for the failed password action which would > result in a flood the database with numerous Bugzilla_logincookie values. Pretty minor issue. The attacker just needs to run heavy queries to put a high load on the DB, and then can save as many queries as he wants to to fill the namedqueries table. And that's just one example. > This is an unexpected behavior for the error case when a user incorrectly > changes their password. The code should be updated so that a new > Bugzilla_logincookie is only generated if the user successfully updates their > password. A failed password update attempt should not result in the generation > of a new Bugzilla_logincookie. I agree.

For the record, userprefs.cgi uses Bugzilla_password, with Bugzilla_login as a hidden field, so that a user not using cookies doesn't need to enter his password twice, see bug 45918 (implemented by justdave in Bugzilla 2.14). We could either revert this change (probably not ideal as it worked fine since 2001), or first try to use cookies to log in, else fall back to given credentials. The 2nd proposal seems to be a better alternative.

Here we go. This implements bug 45918 in a better way, avoiding the problem described in this bug. I tested my patch with cookies enabled/disabled, and it's working correctly in all cases. The existing logincookies is reused rather than creating a new one.

Use $user->cryptpassword as a minor additional cleanup.

mkanat and I agree that this is not a security bug, but simply a bug fix. I will only take it for 3.6 and higher.

Comment on attachment 435709 [details] [diff] [review] patch, v1.1 Cool. Looks great and works fine.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified userprefs.cgi modified template/en/default/account/prefs/account.html.tmpl Committed revision 7101. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/3.6/ modified userprefs.cgi modified template/en/default/account/prefs/account.html.tmpl Committed revision 7060.