398886 - resetting a master password does not clear existing password manager logins

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Tracy]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7

I reset my master password, which cleared all my save passwords as well.
After doing so, the password manager stopped prompting me to save passwords for any websites that I had on the list before the reset.

Reproducible: Always

Steps to Reproduce:
1.set a master password
2.reset it
3.enter a password for a previous website.
Actual Results:  
the password manager did not open a prompt asking if I would like my password saved for the website.

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1

This fixes the obvious problem on trunk, in that when resetting the master password the stored logins are not cleared.

The only side effect of not clearing the stored logins *should* be that some undecryptable entries will lurk around, so I'm not completely what's happening with the reported problem.

Unfortunately resetting the master password is crashing on trunk right now, so I can't test this further just yet.

Comment 2

[Justin Dolske [:Dolske]]

Moving to NSS.

Also, note that this UI isn't exposed in FF so to test it you need to enter  chrome://pippki/content/resetpassword.xul in the URL bar.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I see no description of any NSS problem here.  Nothing in this bug 
describes NSS doing something it is not intended to do.

A user is not prompted for a master password when his key3.db has no 
master password.  

Resetting the master password is NOT the same thing as simply decrypting
the signons.txt file.  

Resetting the master password means erasing the key3.db and putting it 
in a state where it has no master password.   All encryption keys 
formerly in the key3.db are lost.  All passwords in signons.txt that were 
encrypted with the keys in the old key3db are effectively lost forever.  
Resetting the master password is to be done only when the master password
has been lost.  

It is possible to decrypt the entire signons.txt file, and preserve the (formerly encrypted) website passwords, provided that the master password 
is known (not lost).  But AFAIK, FireFox does not provide any UI by which 
the user may do so.  SeaMonkey does provide such UI, but FireFox does not.

If FireFox wishes to enforce a policy in which, after the master password
is reset, the user is then forced to set a new master password before the
password manager will remember any new site passwords, it is up to FireFox
to implement that, not NSS.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Giving this back to Justin.

Comment 5

[Kai Engert [:KaiE:]]

In general, when you reset your master password (and therefore throw away the secret key that was used to encrypt your stored passwords), then I'm ok to erase all such stored passwords.

In comment 0 you said, you reset the master password. How did you do that? I can't find how one does that in Firefox prefs. (I only know how to do it in SeaMonkey.)

Comment 6

[Justin Dolske [:Dolske]]

Kai: see comment #2: The "Reset MP" page is not exposed in the FF UI (we probably should...), but doing it this way is listed at both kb.mozillazine.org and support.mozilla.com.

Comment 7

[Kai Engert [:KaiE:]]

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

(In reply to comment #5)
> In comment 0 you said, you reset the master password. How did you do that? I
> can't find how one does that in Firefox prefs. (I only know how to do it in
> SeaMonkey.)

Oh, you answered that question in comment 2 already. 

So right now, this is actually core or SeaMonkey bug.

Because SeaMonkey gives a clear warning that all stored passwords will be lost, I'm ok with adding such a call to PSM (although I can't review whether the call itself is correct).

r=kengert


Someone who knows about login-manager should probably sr this patch.

Comment 8

[Kai Engert [:KaiE:]]

(In reply to comment #1)
> Unfortunately resetting the master password is crashing on trunk right now, so
> I can't test this further just yet.

Where does it crash? Do you have a stack?

Comment 9

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

(In reply to comment #8)
> (In reply to comment #1)
> > Unfortunately resetting the master password is crashing on trunk right now, so
> > I can't test this further just yet.
> 
> Where does it crash? Do you have a stack?

See bug 397178 (and bug 397296).

Comment 10

[Kai Engert [:KaiE:]]

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

As this is about password manager, I'd like to ask Mike if these calls look right.

(Note that "reset password" is not currently exposed in Firefox UI.)

Comment 11

[Mike Connor [:mconnor]]

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

so, this is ok for now.

The cleaner/extensible solution would be to somehow fire a notification for the various consumers to observe on reset, since anything like an extension using this won't be able to hack themselves into PSM (or might be noscript C++ interfaces for some unholy reason)

future fodder though.  Ideally we'd let users reset their MP in the UI (I thought we had this when I added MP support to the UI...)

Comment 12

[Justin Dolske [:Dolske]]

This is extremely low-risk, so if we can get it into M9 that would be handy.

Comment 13

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

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

a=endgame drivers for M9

Comment 14

[Justin Dolske [:Dolske]]

Checking in security/manager/pki/resources/content/resetpassword.js;
/cvsroot/mozilla/security/manager/pki/resources/content/resetpassword.js,v  <--  resetpassword.js
new revision: 1.10; previous revision: 1.9

Comment 15

[Reed Loden [:reed]]

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

>+    loginManger.removeAllLogins();

s/loginManger/loginManager/

Comment 16

[Justin Dolske [:Dolske]]

Oops, typo, fixed.

/cvsroot/mozilla/security/manager/pki/resources/content/resetpassword.js,v  <--  resetpassword.js
new revision: 1.11; previous revision: 1.10

Comment 18

[[e-mail]]

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

>+  try {
>+    var loginManager = Components.classes["@mozilla.org/login-manager;1"].
>+                       getService(Components.interfaces.nsILoginManager);
>+    loginManger.removeAllLogins();
>+  } catch (e) {
>+  }
>+
>   var pref = Components.classes['@mozilla.org/preferences-service;1'].getService(Components.interfaces.nsIPrefService);
>   if (pref) {
>     pref = pref.getBranch(null);
>     try {
>       if (pref.getBoolPref("wallet.crypto")) {
>         // data in wallet is encrypted, clear it
>         var wallet = Components.classes['@mozilla.org/wallet/wallet-service;1'];
>         if (wallet) {
[This code that did the equivalent for wallet can go, as wallet is dead.]

Comment 19

[Justin Dolske [:Dolske]]

Perhaps you should file a new bug instead of just dropping a comment into a bug that was fixed 2 1/2 years ago.