The default bug view has changed. See this FAQ.

Remembered passwords lost when changing Master Password

VERIFIED FIXED in 3.12

Status

NSS
Libraries
P1
critical
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: stevee, Assigned: Robert Relyea)

Tracking

({dataloss, regression, relnote})

trunk
3.12
dataloss, regression, relnote
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.

Updated

10 years ago
Flags: blocking-firefox3?

Updated

10 years ago
Flags: in-litmus?
(Reporter)

Comment 1

10 years ago
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
Blocks: 388403
(Reporter)

Comment 2

10 years ago
So caused by bug 388403

Updated

10 years ago
OS: Windows 2000 → All

Updated

10 years ago
Hardware: PC → All

Comment 3

10 years ago
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

10 years ago
Marking to block M7 if there is a password dataloss issue this should block.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M7
(Assignee)

Comment 5

10 years ago
All the circumstantial evidents shows this to be in the new database code. Taking the bug.
Assignee: nobody → rrelyea
Component: Password Manager → Libraries
Flags: in-litmus?
Flags: blocking-firefox3+
Product: Firefox → NSS
Target Milestone: Firefox 3 M7 → 3.12
Version: Trunk → 3.12
(Assignee)

Comment 6

10 years ago
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
(Assignee)

Comment 7

10 years ago
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

10 years ago
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.
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.
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.
Flags: blocking1.9?

Comment 11

10 years ago
(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)
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.
What's the effort in disabling the option to change the master password?  

Comment 14

10 years ago
(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
Created attachment 274848 [details] [diff] [review]
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.]
Attachment #274848 - Flags: review?(gavin.sharp)
Oh, for testing, the two chrome URLs are:

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

and

chrome://mozapps/content/preferences/removemp.xul
Comment on attachment 274848 [details] [diff] [review]
Patch for review, v.1 (disables UI only)

r=me
Attachment #274848 - Flags: review?(gavin.sharp) → review+
Marking as blocking, need to get this fixed up ASAP after M7.
Flags: blocking1.9? → blocking1.9+
approve1.9 M7 +

Yeah, land it.  Thanks Justin.  
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
Keywords: relnote

Updated

10 years ago
Attachment #274848 - Attachment is obsolete: true
(Reporter)

Comment 21

10 years ago
Should this be relnote'd to advise people this functionality is deliberately disabled, and that password-related extensions could potentially cause a problem?
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. 
Created attachment 274865 [details] [diff] [review]
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.
Attachment #274865 - Flags: review?(vladimir)
Attachment #274865 - Flags: review?(vladimir) → review+
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 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
Attachment #274865 - Attachment is obsolete: true
QA Contact: password.manager → libraries
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.
Priority: -- → P1
Version: 3.12 → trunk
(Assignee)

Comment 27

10 years ago
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...

(Assignee)

Comment 28

10 years ago
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
(Assignee)

Comment 29

10 years ago
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
(Assignee)

Comment 30

10 years ago
Created attachment 275029 [details] [diff] [review]
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
Attachment #275029 - Flags: superreview?(kengert)
Attachment #275029 - Flags: review?(nelson)
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
Attachment #275029 - Flags: review?(nelson) → review-
(Assignee)

Comment 32

10 years ago
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).
Attachment #275029 - Flags: review- → review?(nelson)

Comment 33

10 years ago
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)
Attachment #275029 - Flags: superreview?(kengert) → review+
(Assignee)

Comment 34

10 years ago
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..
(Assignee)

Comment 35

10 years ago
Created attachment 275143 [details] [diff] [review]
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
Attachment #275143 - Flags: review?(kengert)
OK, bob.  I didn't notice that you were also changing the function name
as well as the last argument. Sorry.

Comment 37

10 years ago
(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

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

r=kengert
Attachment #275143 - Flags: review?(kengert) → review+

Comment 39

10 years ago
(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?
Attachment #275029 - Flags: review?(nelson)
(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

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

Now we must revert the UI patch.

Comment 42

10 years ago
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 ?
(Assignee)

Comment 43

10 years ago
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

10 years ago
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

10 years ago
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 :-).
Created attachment 277805 [details] [diff] [review]
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).
Attachment #277805 - Flags: review?(gavin.sharp)
Attachment #277805 - Flags: review?(gavin.sharp) → review+
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
(Assignee)

Comment 48

10 years ago
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
(Assignee)

Comment 49

10 years ago
The NSS test is now checked in. NSS tinderbox should identify any password update failures in the future.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
v.fixed

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.