Closed Bug 390451 Opened 17 years ago Closed 17 years ago

Remembered passwords lost when changing Master Password

Categories

(NSS :: Libraries, defect, P1)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: stevee, Assigned: rrelyea)

References

(
URL
)

Details

(Keywords: dataloss, regression, relnote)

Attachments

(3 files, 2 obsolete files)

Set the update flag when updating the key database.
669 bytes, patch
KaiE
: review+
Details | Diff | Splinter Review
make sure keys are actually updated when changing passwords.
554 bytes, patch
KaiE
: review+
Details | Diff | Splinter Review
Patch to revert temporary wallpaper
5.03 KB, patch
Gavin
: review+
Details | Diff | Splinter Review 
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.
Flags: blocking-firefox3?
Flags: in-litmus?
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
So caused by bug 388403
OS: Windows 2000 → All
Hardware: PC → All
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?

Marking to block M7 if there is a password dataloss issue this should block.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M7
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
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
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).


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?
(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?  


(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
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
Attachment #274848 - Attachment is obsolete: true
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.
Attached patch Patch for review, v.1 (oops) (obsolete) — — Splinter Review 
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)
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
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...


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
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
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-
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 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+
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..

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.
(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 on attachment 275143 [details] [diff] [review]
make sure keys are actually updated when changing passwords.

r=kengert
Attachment #275143 - Flags: review?(kengert) → review+
(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.
I tested Bob's patch, it works for me, and checked it in.

Now we must revert the UI patch.

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 ?
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.
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 ...
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 :-).
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

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
The NSS test is now checked in. NSS tinderbox should identify any password update failures in the future.
Status: NEW → RESOLVED
Closed: 17 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.

Attachment

General

Creator:
Created:
Updated:
Size: