390451 - Remembered passwords lost when changing Master Password

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Steve England [:stevee]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007073113 Minefield/3.0a7pre

1. New profile, start firefox
2. Tools > Options > Security: tick Use Master Password
3. Set a master password ("12345")
4. Go to https://gmail.google.com , enter username & password, click Sign In
5. Confirm dialog appears; click Remember
6. Tools > Options > Security > Passwords > Show Passwords. Observe the gmail logon is present.
7. Close firefox

8. Start firefox up.
9. Tools > Options > Security > Passwords > Show Passwords
10. Password Required dialog appears. Enter your master password ("12345")
11. Observe the gmail logon is present.
12. Click Change Master Password
13. Enter current password ("12345"), enter new password x 2 ("qwerty")
14. Password Change Succeeded dialog appears. Click OK.
15. Click Show Passwords. Observe the gmail logon is present.
16. Close firefox

17. Start firefox up
18. Tools > Options > Security > Passwords > Show Passwords
19. Password Required dialog appears. Enter your master password ("qwerty")

Expected:
Gmail logon/password is present

Actual:
OMG list is empty.

I'll try and find a regression range as soon as I can.

Comment 1

[Steve England [:stevee]]

Works: 20070723_1121_firefox-3.0a7pre.en-US.win32.zip
Broken: 20070723_1209_firefox-3.0a7pre.en-US.win32.zip

Checkins to module PhoenixTinderbox between 2007-07-23 11:21 and 2007-07-23 12:08 :
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1185214860&maxdate=1185217739

Comment 2

[Steve England [:stevee]]

So caused by bug 388403

Comment 3

[Kai Engert [:KaiE:]]

I can reproduce. I used SeaMonkey.

I had a fresh profile for testing. I had a gmail password stored, without encryption.

I enabled encryption (equivalent to firefox use a master password)

At this point I had an "empty" master password. At this point, access to the stored password was still possible.

I changed my master password. After restart, I confirm, password no longer remembered.

I changed the master password back to the original value (empty). After restart, the gmail password was back.


This tells me, the entries in the password-manager-list are not deleted, but they can no longer be decrypted.


My assumption was
- NSS db contains a symmetric key
- symmetric key is used to encrypt entries in password-manager-list
- master password protects symmetric key

My assmption was:
- when changing the master password, the symmetric stays the same
- the protection for the symmetric key gets updated to the new master password


As I regain access by reverting the master password change to the original value, I suspect the protection of the symmetric remains tied to the old master password value?

Another possibility, is the master password directly being used as the symmetric key?

Bob?

Comment 4

[Mike Schroepfer]

Marking to block M7 if there is a password dataloss issue this should block.

Comment 5

[Robert Relyea]

All the circumstantial evidents shows this to be in the new database code. Taking the bug.

Comment 6

[Robert Relyea]

Kai:

Master Password == database password.

When the database password is changed, the entries in the database need to be reencrypted with the new password.

For some reason that reencryption is not happenning. There is a test case in all.sh (which I added in this last round) to make sure that the entries really get reencrypted. So why is the test case succeeding and the field is failing? (something for me to ponder)...

bob

Comment 7

[Robert Relyea]

For those bit by this problem, the work around is to reset your master password back to the original. (this should allow you to access your keys).

Comment 8

[u88484]

Well since they aren't actually lost and it is simple to get them back, should the A7 release be held up because of this.

Comment 9

[Justin Dolske [:Dolske]]

So, changing the DB password (aka master password) isn't reencrypting the DB entries?

I'm a bit surprised that changing the master password back to the previous value is working, I thought there was a salt value being used when deriving the encryption key from the password -- changing the password ought to be changing that salt value as well. If it's not being changes, that's probably a fortunate bug.

As for blocking A7, I suspect there may be some existing bugs with the password manager dealing with login entries that can't be decrypted, as that case hasn't been well tested yet.

Comment 10

[Justin Dolske [:Dolske]]

Looking like blocking-1.9? got lost when moving to the NSS product. I'm not sure how the M7 drivers want to track this, though, since there's no corresponding target milestone for the Firefox 3 releases.

Comment 11

[Kai Engert [:KaiE:]]

(In reply to comment #10)
> Looking like blocking-1.9? got lost when moving to the NSS product. I'm not
> sure how the M7 drivers want to track this, though, since there's no
> corresponding target milestone for the Firefox 3 releases.

Maybe this bug should be reverted to a firefox-3 bug, and a separate bug on the nss product filed - blocking this one?

If the bug is in NSS, then the fix will require the following steps:

- patch NSS and produce a new NSS snapshot
  (to be tracked in separate nss bug)

- upgrade Mozilla trunk to pick up the new snapshot
  (to be tracked in this bug)

Comment 12

[Justin Dolske [:Dolske]]

I did a bit of testing to double check what's been found so far...

* Changing the master password does break password manager
* Enabling a master password when none was set (or disabling the master password if it was set) also breaks things. [Not surprising, as technically a master password is always used, the option to not use one just changes the master password to an empty string.]
* Reverting to the original password restores entries, as does changing back to the original master password enabled/disabled state.
* Reverting also works if there are intermediate master password changes and/or changes to the master password enabled/disabled state.

So, existing logins should be preserved although a user will certainly be confused.

I also tested adding a login. This fails, because it's unable to encrypt the username/password. Unfortunately this results in adding a bogus entry, such as:

https://www.google.com
Email
null
*Passwd
null
https://www.google.com

I don't /think/ that will harm anything, but it's certainly not desirable. Existing password entries are preserved unchanged.

One option to wallpaper over this would be to add a bit of code to disable changing the master password, or enabling/disabling it.

Comment 13

[Damon Sicore (:damons)]

What's the effort in disabling the option to change the master password?  

Comment 14

[gabe]

(In reply to comment #13)
> What's the effort in disabling the option to change the master password?  
> 

its holding up a milestone in this case Firefox 3.0 alpha 7 when that happens Firefox devs do odd things sometimes

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1 (disables UI only)

This patch disables the UI, so that a user can't change or enable/disable their master password. At least, not easily. An extension implementing such functionality on its own could still trigger the problem.

This disables both the prefs in the normal Firefox UI, as well as the password fields in the change/remove master password dialogs. [I've seen people reference using the chrome:// URLs directly in some cases.]

Comment 16

[Justin Dolske [:Dolske]]

Oh, for testing, the two chrome URLs are:

chrome://mozapps/content/preferences/changemp.xul

and

chrome://mozapps/content/preferences/removemp.xul

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 274848 [details] [diff] [review]
Patch for review, v.1 (disables UI only)

r=me

Comment 18

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Marking as blocking, need to get this fixed up ASAP after M7.

Comment 19

[Damon Sicore (:damons)]

approve1.9 M7 +

Yeah, land it.  Thanks Justin.  

Comment 20

[Justin Dolske [:Dolske]]

Attachment 274848 [details] [diff] checked in to trunk...

$ cvs commit browser/components/preferences/ toolkit/mozapps/preferences/
cvs commit: Examining browser/components/preferences
cvs commit: Examining toolkit/mozapps/preferences
Checking in browser/components/preferences/security.xul;
/cvsroot/mozilla/browser/components/preferences/security.xul,v  <--  security.xul
new revision: 1.17; previous revision: 1.16
done
Checking in toolkit/mozapps/preferences/changemp.xul;
/cvsroot/mozilla/toolkit/mozapps/preferences/changemp.xul,v  <--  changemp.xul
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/mozapps/preferences/removemp.xul;
/cvsroot/mozilla/toolkit/mozapps/preferences/removemp.xul,v  <--  removemp.xul
new revision: 1.4; previous revision: 1.3
done

Comment 21

[Steve England [:stevee]]

Should this be relnote'd to advise people this functionality is deliberately disabled, and that password-related extensions could potentially cause a problem?

Comment 22

[Justin Dolske [:Dolske]]

I updated the release notes...

http://wiki.mozilla.org/Firefox3/M7/ReleaseNotes#Firefox

A bug was found in NSS code, which results in master password changes being improperly handled. The preferences UI to change and enable/disable the master password has been disabled to avoid triggering the problem. (bug 390451). Extensions which perform these operations should not be used with M7. 

Comment 23

[Justin Dolske [:Dolske]]

Attachment: Patch for review, v.1 (oops)

Oops. The last patch wasn't sufficient... If you already have a master password set, the pref panel initialization was un-disabling the "Change Password" button. This patch comments out that code.

Tested both with a master password set, and not set. The button and checkbox now remain disabled in both cases.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Please turn this bug back into a PSM (or Firefox) bug, and (if necessary)
file a subordinate NSS bug that blocks this one.  The NSS bug should 
have an NSS QA assignee.

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 274865 [details] [diff] [review]
Patch for review, v.1 (oops)

$ cvs commit browser/components/preferences/
cvs commit: Examining browser/components/preferences
Checking in browser/components/preferences/security.js;
/cvsroot/mozilla/browser/components/preferences/security.js,v  <--  security.js
new revision: 1.12; previous revision: 1.11
done

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

According to comment 6, comment 8 and comment 12, there is no data loss.
The recovery is simple: go back to the original password.  
So I think thsi bug is merely major, not critical, and the dataloss keyword
should be removed.

Comment 27

[Robert Relyea]

We need to fix the NSS problems, there are more than one way to change the master password in Firefox. I'm working on this problem right now...

Comment 28

[Robert Relyea]

OK I have a patch I'm testing. I'll attach it shortly... Then I'll go find out why the NSS test I added to catch this problem wasn't catching it.

bob

Comment 29

[Robert Relyea]

RE salt and way regenerating an old password works:

Every database gets a new global salt value when that database is created. NSS does not change the salt value when changing passwords.

Each entry also has a salt value, unique to that entry. However if we don't update the entry, That salt value will also be preserved. So if you change your password back to the same password will end up generating the old key under the covers.

BTW the problem seems to be in the update point. When we try to update the entry, we aren't setting an update flag to true, to the database refuses to overwrite and existing entry.

bob

Comment 30

[Robert Relyea]

Attachment: Set the update flag when updating the key database.

Only needs one review to get into NSS tip...
Once in, I can create a new mozilla tag and the UI hacks can be removed.

bob

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 275029 [details] [diff] [review]
Set the update flag when updating the key database.

Sorry, Bob, this patch MUST be incomplete.  r-

>-	    rv = nsslowkey_StoreKeyByPublicKey(lg_getKeyDB(sdb), privKey, 
>-		&obj->dbKey, label, sdb );
>+	    rv = nsslowkey_StoreKeyByPublicKeyAlg(lg_getKeyDB(sdb), privKey, 
>+		&obj->dbKey, label, sdb, PR_TRUE );

According to 
http://lxr.mozilla.org/security/source/security/nss/lib/softoken/legacydb/keydb.c#1126
nsslowkey_StoreKeyByPublicKeyAlg has only 5 parameters, but this patch is 
adding a 6th argument to a call of that function. 

I'm guessing that somewhere you have more parts to this patch, parts that add
a 6th parameter to nsslowkey_StoreKeyByPublicKeyAlg in keydb.c and in 
http://lxr.mozilla.org/security/source/security/nss/lib/softoken/legacydb/lowkeyi.h#97
and adds a 6th argument to the other callers of that function at
http://lxr.mozilla.org/security/source/security/nss/lib/softoken/legacydb/lgcreate.c#669
and
http://lxr.mozilla.org/security/source/security/nss/lib/softoken/legacydb/lgcreate.c#867

Comment 32

[Robert Relyea]

Comment on attachment 275029 [details] [diff] [review]
Set the update flag when updating the key database.

Please check again. The function you pointed to in lxr is nsslowkey_StoreKeyByPublicKey, part of the patch is to change the function to nsslowkey_StoreKeyByPublicKeyAlg.
The former has 5 parameters the later has 6. (note the former calls the latter setting the 6th paramter to PR_FALSE).

I assure you that this is the complete patch.

(tested on both mozilla and in the nss trunk).

Comment 33

[Kai Engert [:KaiE:]]

Comment on attachment 275029 [details] [diff] [review]
Set the update flag when updating the key database.

I agree with Bob's patch.

I agree with Bob's assumption that Nelson might have overlooked that Bob is now calling a different function with 6 parameters.

(Both old and new function call the same internal worker function. While the old function used PR_FALSE as the 6th parameter, this other function uses the given 6th parameter)

Comment 34

[Robert Relyea]

Checked in:

2007-08-03 10:32	rrelyea%redhat.com 	mozilla/security/nss/lib/softoken/legacydb/lgattr.c 	1.4 	2/2  	

Bug 390451. NSS is not updating keys on password change.
r=kaie. 


and tagged NSS_3_12_ALPHA1B. Mozilla patch forthcoming..

Comment 35

[Robert Relyea]

Attachment: make sure keys are actually updated when changing passwords.

here's the mozilla patch, What magic, besides the review, what do we need to land this? I presume the 1.9 branch is closed for stablization. Also we should remove the UI hacks once it lands.

BTW the diff between NSS_3_12_ALPHA1A and NSS_3_12_ALPHA1B is (should be) on the the NSS patch attached to this bug (I've double checked, reviewers should also verify).

bob

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

OK, bob.  I didn't notice that you were also changing the function name
as well as the last argument. Sorry.

Comment 37

[Kai Engert [:KaiE:]]

(In reply to comment #35)
> BTW the diff between NSS_3_12_ALPHA1A and NSS_3_12_ALPHA1B is (should be) on
> the the NSS patch attached to this bug (I've double checked, reviewers should
> also verify).

Verified, the following command only lists the change from attachment 275029 [details] [diff] [review]

cvs rdiff -u -r NSS_3_12_ALPHA1A -r NSS_3_12_ALPHA1B mozilla/security/nss

Comment 38

[Kai Engert [:KaiE:]]

Comment on attachment 275143 [details] [diff] [review]
make sure keys are actually updated when changing passwords.

r=kengert

Comment 39

[Kai Engert [:KaiE:]]

(In reply to comment #35)
> What magic, besides the review, what do we need to
> land this? I presume the 1.9 branch is closed for stablization. Also we should
> remove the UI hacks once it lands.

The tinderbox is open currently. http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox

It says: "** README: M8 checkin rules: http://tinyurl.com/yu77oy ** Current threat level: Yellow (blockers already plus'd are auto-approved)"

As this bug has blocking1.9+ I guess we're fine to check in?

Comment 40

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

(In reply to comment #39)
> As this bug has blocking1.9+ I guess we're fine to check in?

Yes, you're fine to check in.

Comment 41

[Kai Engert [:KaiE:]]

I tested Bob's patch, it works for me, and checked it in.

Now we must revert the UI patch.

Comment 42

[Klaus Strebel]

I installed Alpha 7 on monday, first didn't realize the stored passwords were not found. Needed a day to find out about this ( shame on me ).
Now i tried this ( with alpha7 and also the minefield 8 of today ). I deleted the fx3 profile, copyied over my fx2 profile. On Fx3 start, the profile is updated ... the master password is set to empty (?) ... i changed the password via Options->Advanced->SecurityDevices->ChangedPassword to same i used in the fx2 profile ... restart ... the stored passwords list is still empty :-(.
Any further hints, ideas, logging to switch on or such ?

Comment 43

[Robert Relyea]

Is there a master password set in your fx2 profile? (if you start ff2, do you have a master password?)

Also, when you copied your profile, did you make sure to copy key3.db?

If you start off with no keys, then you are experiencing a different problem.

Comment 44

[Klaus Strebel]

Yepp, i have a master password set in ff2. I do a 'xcopy /s/e/d profile_fx2 profile_fx3' so: yes, the key3.db is copied.

Which role do the signon play in this game, i found my fx2 signon.SignonFileName pointing to 99556210.s which doesn't exist anymore (???, it did, it is still in my T'bird profile which i derived from the fx profile some years ago ;-) ). I just copy it over and give it another try with the lates nightly build ...

Comment 45

[Klaus Strebel]

Ok, here's what i did now ( Minefield is Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080815 Minefield/3.0a8pre ):

1. xcopy /s/e/d profile_fx2 profile_fx3
2. started minefield with 'C:\Programme\mozilla.org\Firefox3\firefox.exe -Profile D:\Data\mozilla\profile_fx3'
3. Profile is migrated, extensions are getting checked, browser opens - not asking for master password.
4. changed/set master password via Options->Advanced->SecurityDevices->Software Security Devices->Changed Password... to originally used in ff2
5. exit the browser and restart
6. giving 'new' original master password
7. display stored password -> list is empty

Unfortunately ( well, indeed not ;-) ) i'll be on holidays 'till August 19th, but i'll stay tuned 'till 18:00 CET :-).

Comment 46

[Justin Dolske [:Dolske]]

Attachment: Patch to revert temporary wallpaper

This patch reverts my previous two wallpaper patches, which disabled the Firefox UI for changing the MP in order to prevent triggering the NSS bug (which is now fixed).

Comment 47

[Justin Dolske [:Dolske]]

Wallpaper revert checked in. Since Relyea owns this as an NSS bug I won't change the resolution, but AFAIK this can be closed as FIXED now.

Klaus: Please file a separate bug for your issue.

Checking in browser/components/preferences/security.js;
/cvsroot/mozilla/browser/components/preferences/security.js,v  <--  security.js
new revision: 1.13; previous revision: 1.12
done
Checking in browser/components/preferences/security.xul;
/cvsroot/mozilla/browser/components/preferences/security.xul,v  <--  security.xul
new revision: 1.18; previous revision: 1.17
done
Checking in toolkit/mozapps/preferences/changemp.xul;
/cvsroot/mozilla/toolkit/mozapps/preferences/changemp.xul,v  <--  changemp.xul
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/mozapps/preferences/removemp.xul;
/cvsroot/mozilla/toolkit/mozapps/preferences/removemp.xul,v  <--  removemp.xul
new revision: 1.5; previous revision: 1.4
done

Comment 48

[Robert Relyea]

Thanks! There is one last thing I want to get before closing the bug, and that is fixing the nss tests to catch this kind of problem in the future.

patch for the test is part of the patch in bug 392522. As soon as that is in I'll close this bug.

bob

Comment 49

[Robert Relyea]

The NSS test is now checked in. NSS tinderbox should identify any password update failures in the future.

Comment 50

[Samuel Sidler (old account; do not CC)]

v.fixed

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre