1496736 - Firefox Loses Master Password on (RedHat) systems forcing usage of key3.db

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Phineas Fudrucker]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

Entered Master Password, shut down Firefox, and started Firefox the next morning.


Actual results:

Since last upgrade to 60.2.1 esr, master password is no longer there on startup.  I can recreate it but is is gone after next startup.


Expected results:

Master password should persist from one invocation to the next.

Comment 1

[Stefan [:StefanG_QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 20181001135620

I've tested this report on Ubuntu 18.04 x64 using esr 60.2.1 and 60.2.0. I did not manage to reproduce the mentioned behavior. When master password is enabled before the update it did not disappear after the update for me. 

Do you have any add-ons installed on your Firefox?

Do this happen every time if you close and reopen the browser?

Comment 2

[Phineas Fudrucker]

It doesn't vanish if I just shut down and start up immediately.  However, if I leave the browser shut down for more than about five minutes, the master password disappears.

I just upgraded to 60.2.2 esr this morning.  Unfortunately that did not cure this behavior.  I have the following addons installed and all are enabled.

Adblock Plus
Blur
Cisco WebEx Extension
DuckDuckGo Privacy Essentials
FoxyProxy Standard
No Script
Privacy Badger
Web Developer

I found that if I delete or empty logins.json, the master password does not go away between restarts.  However, as soon I save an account from the dialog box and shutdown, the master password will be gone the next time I start firefox.

Unless you have another suggestion, my next step is to test with all my addons disabled.  That will have to wait until tomorrow.

Comment 3

[Stefan [:StefanG_QA]]

I've re-tested this on different 'esr' versions using the above add-ons, but still cannot reproduce it. 

Are you still able to reproduce it in safe mode with all add-ons disabled?

Comment 4

[Phineas Fudrucker]

It may be this is Redhat specific. I see there is a report for the same behavior on Redhat bugzilla and they are testing a fix now.

Comment 5

[Nerijus Krukauskas]

(In reply to Phineas Fudrucker from comment #4)
> It may be this is Redhat specific. I see there is a report for the same
> behavior on Redhat bugzilla and they are testing a fix now.

Any chance you could paste a reference to RedHat bugzilla about this? I've got CentOS 7 with Firefox 60.2.2ESR and it looses master password and sync to Firefox Account along with all saved passwords. My other device is with CentOS 7 / Firefox 62 and it does not exhibit this behaviour.

Comment 6

[Phineas Fudrucker]

Here is the Redhat bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1633932

Comment 7

[Matthew N. [:MattN]]

Thanks. So IIUC, bug 1475775 deletes key3.db even if NSS is configured to still use key3.db? Is that correct? Where is this getting fixed?

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

We should probably at least make an effort to see if we're using the new DB before removing the old one.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1496736 - check if we actually have a new key DB before removing the old one r?jcj

In bug 1475775, we added code to remove the old NSS key DB if the user has set a
password on the grounds that the old DB could potentially be unencrypted and
contain secrets. However, we did so with the assumption that we were using the
new DB, which is not necessarily true when the system has been configured to
always use the old DB, as with some RedHat products. This patch checks for the
existence of the new DB before proceeding with deleting the old DB. Technically
this isn't sufficient, because the new DB could be present even if we're not
using it. However, we've already gone far into "this configuration isn't
supported" territory.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

Red Hat is going to fix that as a part of next Firefox ESR update this week:
https://bugzilla.redhat.com/show_bug.cgi?id=1633932#c27

Comment 11

[Ryan VanderMeulen [:RyanVM]]

If Red Hat is fixing it downstream for now, it sounds like we can wait for ESR 60.4 instead of trying to rush it into a dot release.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b9eaaf47037614e5a7204c62537f2a147306328

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d51f2e8955b
check if we actually have a new key DB before removing the old one r=jcj

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/2d51f2e8955b

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Might this be good for uplift to 64 beta or is it better to let this change wait for 65?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I'd like to see it uplifted, but I was also hoping to get QA verification on it before doing so.

Comment 17

[Rares Doghi, Desktop QA]

Hi Phineas, We are running into some problems mounting our Red Hat images on Virtual Machines and it might take a while but in order to speed things up can you please re-test this issue using our latest Firefox Nightly you can find it here: https://nightly.mozilla.org and let us know if the issue still occurs, it will help us a lot. Thanks

Comment 18

[Martin Stránský [:stransky] (ni? me)]

This bug should be fixed by firefox-60.3.0-1 package on Red Hat side.

Comment 19

[Phineas Fudrucker]

My workstation is still at CentOS 6 so I downloaded the nightly build (65.0a1) onto a CentOS 7 virtual and tested from there.  The master password now does seem to survive restarts including a restart after a reboot.

Comment 20

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I can imagine this same issue happening in more than just the "RedHat unexpectedly forced the old db format on Firefox", so yes, it would probably be good to uplift.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to Beta and ESR60 as-landed. Please nominate it for approval when you get a chance.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 9018758 [details]
bug 1496736 - check if we actually have a new key DB before removing the old one r?jcj

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1475775

User impact if declined: Users can lose passwords stored on systems that configure NSS in a way we don't expect.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a safer version of what we did in bug 1475775 (i.e. try to check that we successfully upgraded before removing the old db)

String changes made/needed: 

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Loss of stored passwords on some systems.

User impact if declined: Users can lose passwords stored on systems that configure NSS in a way we don't expect.

Fix Landed on Version: 65

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a safer version of what we did in bug 1475775 (i.e. try to check that we successfully upgraded before removing the old db)

String or UUID changes made by this patch:

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 9018758 [details]
bug 1496736 - check if we actually have a new key DB before removing the old one r?jcj

dataloss fix for some nss configurations, approved for 64.0b8

Comment 25

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1370c2a23ff7

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9018758 [details]
bug 1496736 - check if we actually have a new key DB before removing the old one r?jcj

OK for uplift to 60.4.0esr, seems worth fixing to avoid user data loss.

Comment 27

[u554753]

Verified on Cent OS x64
Version: 65.0a1
Build ID: 20181112100105

Comment 28

[u554753]

Verified on Cent OS x64
Version: 64.0b9
Build ID: 20181112164519

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/3bed863ee656

Comment 30

[Kai Engert [:KaiE:]]

The existence of file key4.db doesn't guarantee that the secret key contents of file key3.db have already been successfully migrated, see bug 1510212.

Comment 31

[Kai Engert [:KaiE:]]

Setting needinfo for keeler, just to ensure you see comment 30.

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Ok - thanks. So it sounds like NSS is the right place to fix this. Is there already a bug filed for addressing this?

Comment 33

[Kai Engert [:KaiE:]]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #32)

Ok - thanks. So it sounds like NSS is the right place to fix this. Is there
already a bug filed for addressing this?

I've finally filed it: bug 1561368