384524 - Passwords still filled in on web sites after logged out of Software Security Device

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Michael Kraft [:morac]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070614 Minefield/3.0a6pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070614 Minefield/3.0a6pre

This occurs in Gecko/20070614 Minefield/3.0a6pre.

If a master password is set and the user logs in with the master password, Firefox will continue to fill in passwords on web pages even after the user logs out of the Software Security Device.  In addition the "Show Passwords" option shows all stored sites and usernames (asking to show the passwords still prompts for the master password).

The Software Security device is logged out since attempting to save a new password prompts the user for the master password which, if entered, logs back into the Software Security device.

Reproducible: Always

Steps to Reproduce:
1. Set a master password
1. Go to any web site (I used google) and enter a username and password
2. When Firefox asks to store it, do so and enter the master password.
3. Go to Tools->Options->Advanced->Security Devices->Software Security Device and click the "Log out" button.
4. Go to the same web site above.
Actual Results:  
The username and password are automatically filled in despite the Software Security Device being logged out.

Expected Results:  
The username and password should not have been filled in, at least not without prompting for the master password to log back into the Software Security Device.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

This is a regression, right? Sounds like it might have been caused by the password manager rewrite.

Comment 2

[Michael Kraft [:morac]]

Yes, it is a regression as the problem does not exist in Firefox 2.0.0.4.
I also tested FIPS mode and the problem exists with both FIPS enable and disabled.

Based on another bug I just filed (bug 384525), it looks like the password data is loaded only once per browser session, but I'm not positive.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Please confirm before nominating for blocking ...

Comment 4

[Daniel Veditz [:dveditz]]

*sigh*

This time checking the confirm box for real and renominating.

Comment 5

[Justin Dolske [:Dolske]]

I think the patch for bug 381164 will probably fix this.

Comment 6

[Daniel Veditz [:dveditz]]

Hopefully it will, but please don't dupe this -- it's an important enough case to justify separate verification.

Comment 7

[Michael Kraft [:morac]]

I'm still seeing this issue in the 2007071205 nightly despite bug 381164 being fixed.  

In addition, I also noticed that if you sign out and the log into a web site, even if the master password prompt pop up is canceled, the username and password are still stored.

The way it works now, the _decrypt function in storage-Legacy.js happily returns the decrypted information despite the security device being logged out.

The issue appears to be in the nsISecretDecoderRing component (or something it calls) since it should not be decrypting and returning data if the user logs out of the security device.

Comment 8

[Mike Connor [:mconnor]]

Missing the freeze, moving out.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

This should fix the problem, pwmgr was effectively caching the decrypted strings as a side effect of decrypting them for the caller.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 276595 [details] [diff] [review]
Patch for review, v.1

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js

>+            var [decryptedLogin, userCanceled] =
>+                        this._decryptLogins([logins[i]]);

Use [[decryptedLogin], userCanceled], turn the length check below into a null check, and remove the [0] below?

>+            if (decryptedLogin.length != 1)

>+            if (decryptedLogin[0].equals(login)) {

>     _decryptLogins : function (logins) {
>+        const nsLoginInfo = new Components.Constructor(
>+            "@mozilla.org/login-manager/loginInfo;1", Ci.nsILoginInfo, "init");

I think you should just remove the login object cloning, per discussion on IRC.

r=me with those addressed.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch with review comments addressed, v.1

Patch with review comments addressed, checked in.

Checking in toolkit/components/passwordmgr/src/storage-Legacy.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/src/storage-Legacy.js,v  <--  storage-Legacy.js
new revision: 1.11; previous revision: 1.10
done