jurgen Reporter
	Description • 6 years ago

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180704192850

Steps to reproduce:

In my existing firefox 61.0.4 (Ubuntu), I added a master password to protect my passwords.
Next, I used dumpzilla (https://github.com/Busindre/dumpzilla) to extract the passwords from my closed firefox browser.


Actual results:

All my passwords got decrypted without asking for my master password.
I'm assuming protection happens on UI level but not on data level.


Expected results:

I would expect a master password to encrypt the json file using the master password so it can't be opened without knowledge of that master password.

	jurgen Reporter
	Comment 1 • 6 years ago

apologies, it's Firefox 61.0.1 Quantum (64 bits) and not .0.4

	:Gijs (he/him)
	Comment 2 • 6 years ago

(In reply to jurgen from comment #0)
> User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:61.0) Gecko/20100101
> Firefox/61.0
> Build ID: 20180704192850
> 
> Steps to reproduce:
> 
> In my existing firefox 61.0.4 (Ubuntu), I added a master password to protect
> my passwords.
> Next, I used dumpzilla (https://github.com/Busindre/dumpzilla) to extract
> the passwords from my closed firefox browser.

Did you restart Firefox? How old is the user profile?

FWIW, https://github.com/Busindre/dumpzilla/blob/master/dumpzilla.py#L415-L427 has decryption code that specifically requests a master password, so no, the master password is not just a UI level protection.

It's possible there were old files in the profile you tested with (we moved password storage to a JSON file from a sqlite one) that the tool was able to use to recover passwords. Can you check if the profile folder (go to about:support in Firefox, then click the button to open the folder next to "Profile folder") if it has a signons.sqlite file?

I guess the other possibility is that we don't immediately re-encrypt files, or that if you kept Firefox open, the new state wasn't yet flushed to disk to overwrite the old one - I'm not familiar enough with our pw manager implementation myself to know for sure. Maybe Matt can add detail about how that's supposed to work.

	jurgen Reporter
	Comment 3 • 6 years ago

Hi,

thanks for the feedback - glad to be wrong (I was surprised that I didn't find the files encrypted)
So after rebooting, the passwords and usernames are encrypted in the json file.
Still leaking the visited sites, but I guess that's acceptable.

	jurgen Reporter
	Comment 4 • 6 years ago

... and after opening my browser, I could extract my passwords again without entering my password to dumpzilla (I could even access it with another account using a different tty login).

When trying to recreate the bug with a new profile, it didn't work and it asked the master password as expected.

On my existing (incrementally upgraded for over multiple years) account, no master password is needed and I see my passwords in plain text.

I have not found the signons.sqlite file in my profile folder.

	:Gijs (he/him)
	Comment 5 • 6 years ago

Jurgen, thanks for the quick updates. I'm not sure I follow completely at this point. Can you confirm that you're seeing the following:


- on your regular profile, with Firefox running, dumpzilla gets the passwords without prompting you for a password (presumably at this point you've given Firefox your master password when it started?)
- on that same profile, once you close Firefox, dumpzilla can't get passwords (or maybe it can, but it can't anymore after a reboot? Please clarify. :-) )

- on a new profile, where you save a password "behind" a master password, dumpzilla can't get the passwords either when Firefox is running or when it isn't - at this point, when testing what happens when Firefox is running, have you given Firefox the master password for that new profile?


(Basically, my current hypothesis is that while Firefox is running *and* you've already given it your master password, the NSS library which dumpzilla relies on directly (exact paths are stored in the python file) knows the master password (presumably stores it in memory?) and can thus decrypt your passwords without asking you for the master password. But I can't quite tell from your description if that holds up or not.)

	jurgen Reporter
	Comment 6 • 6 years ago

So I have 2 users:
1. jurgen (my old ye faithful account that has been passed on since Ubuntu 12.04 and FF3 or so)
2. test (newly created for this test)

both are in the sudo'ers group.

This is after a fresh reboot.

Step 1: just work as jurgen (with master password). I have closed FF. On tty2 log in with test - sudo su. Now run dumpzilla on the profile under jurgen. I gain access to all passwords without entering master password.

Step 2: work as test (first login, so completely virgin account) - I created an account on Wikipedia and stored my username and password. Now I went to settings and added a master password. Log in to tty2 with jurgen - sudo su. Now run dumpzilla on the profile under test. Access to passwords failed.

I'd like to supply with screenshots or recording, but it's a bit hard. Don't know how to record TTY2.

	:Gijs (he/him)
	Comment 7 • 6 years ago

OK. I don't really know what's going on then, and it's a bit hard for me to try to reproduce - in part because you yourself can't reproduce on a clean profile and we don't know what's different about your main profile, and in part because the tool doesn't seem to work well on macOS. Mind you, the output is confusing anyway, because it says:


[ERROR] Error decoding passwords: libnss not found (/Applications/Firefox\ Nightly.app/Contents/MacOS/libnss3.dylib)


(I altered the path to be correct because plain `libnss3.dylib` also didn't work, but this didn't make any difference)

and yet it also says:

===============================================================================================================
== Total Information
==============================================================================================================

Total Decode Passwords     : 1
Total Passwords            : 1

even though the output does not in fact contain my (fake) saved password. I assume:

User field: identifier
Password field: password

would normally contain the actual password and username, or something?


Hopefully MattN has more ideas.

	Matthew N. [:MattN]
	Comment 8 • 6 years ago

(In reply to jurgen from comment #4)
> I have not found the signons.sqlite file in my profile folder.

Can you please double/triple check this? Note that basename of the files is different signons vs. logins. Make absolutely sure you're looking at the correct profile folder, the same one as dumpzilla is.

The issue may actually be about key3.db vs. key4.db… When did you setup a Master Password in your default profile? If you move key3.db out of the profile folder can dumpzilla still decrypt the logins?

	jurgen Reporter
	Comment 10 • 6 years ago

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #8)
> (In reply to jurgen from comment #4)
> > I have not found the signons.sqlite file in my profile folder.
> 
> Can you please double/triple check this? Note that basename of the files is
> different signons vs. logins. Make absolutely sure you're looking at the
> correct profile folder, the same one as dumpzilla is.

See above reply.

> 
> The issue may actually be about key3.db vs. key4.db… When did you setup a
> Master Password in your default profile? If you move key3.db out of the
> profile folder can dumpzilla still decrypt the logins?

When I move key3.db out of the profile, it indeed can't access the passwords anymore and asks for a master password.

	Matthew N. [:MattN]
	Comment 11 • 6 years ago

(In reply to jurgen from comment #10)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #8)
> > (In reply to jurgen from comment #4)
> > > I have not found the signons.sqlite file in my profile folder.
> > 
> > Can you please double/triple check this? Note that basename of the files is
> > different signons vs. logins. Make absolutely sure you're looking at the
> > correct profile folder, the same one as dumpzilla is.
> 
> See above reply.

OK, great.

> > 
> > The issue may actually be about key3.db vs. key4.db… When did you setup a
> > Master Password in your default profile?

What was the answer to this question?

> If you move key3.db out of the
> > profile folder can dumpzilla still decrypt the logins?
> 
> When I move key3.db out of the profile, it indeed can't access the passwords
> anymore and asks for a master password.

OK, so I think the issue is that you setup a master password after the conversion to key4.db (looks like it was Firefox 58?) and we haven't yet cleaned up key3.db files. This bug could track that cleanup if that's the case.

David, does that make sense? My understanding is that having an old key*.db without a master password would be enough to decrypt logins since we don't re-encrypt the saved data when a MP is enabled or disabled.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 12 • 6 years ago

Yes - if a user adds a master password after upgrading to 58, their logins will still be decryptable as long as key3.db exists. We can probably clean up those old files at this point (key3.db, cert8.db, and secmod.db).

	:Gijs (he/him)
	Comment 13 • 6 years ago

What code does the migration from key3 to key4? Would it be easy to delete the old file as part of that at this point? And should we backport any such change to esr60 to avoid this issue persisting on ESR for another year or so?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 14 • 6 years ago

NSS performs the migration internally. We could probably add code to NSS to delete the old DBs as part of migration, but that wouldn't help anyone who has already migrated.

	:Gijs (he/him)
	Comment 15 • 6 years ago

(In reply to [:keeler] (use needinfo) from comment #14)
> NSS performs the migration internally. We could probably add code to NSS to
> delete the old DBs as part of migration, but that wouldn't help anyone who
> has already migrated.

OK. At what point in browser startup is the migration performed? We can add a migration step to nsBrowserGlue, but we don't normally use it for this type of thing, and if it races with the migration then the obvious result would be dataloss... :-(

	Matthew N. [:MattN]
	Comment 16 • 6 years ago

It would be great if we had bug 1455707 implemented before we delete the old files. Usually we leave the old files around for a while to support downgrades.

I think we should do the following:
a) Delete key3.db (if it exists) when a master password is changed/enabled. That solves this bug and should be backported to ESR.

b) Delete all of the old files (key3.db, cert8.db, and secmod.db) for the next ESR (to support downgrades between ESR) in an upcoming release… I don't think we need to rush on this. Should we only do this for Firefox (e.g. migrateUI) or do we another another place to do this that won't incur a startup perf hit from file I/O every startup? Or does this need to be in PSM for all PSM consumers? We should at least notify downstream consumers about this issue I guess? This affects who should implement it.

(In reply to :Gijs (he/him) from comment #13)
> Would it be easy to delete the old file as part of that at this point?

That will break the downgrade case so I don't think it's the best idea.

> And should we backport any such
> change to esr60 to avoid this issue persisting on ESR for another year or so?

I think it would be safest to only backport a fix to delete key3.db if a master password is changed/enabled. This won't help ESR users who already make a MP change after ESR60 got installed but since that is still very new we probably have time to fix this before may people are affected since less than 1% of users use a MP.

David, what do you think about this proposal and where do you think we should implement parts (a) and (b) in the code since that affects what team implements it and which component this should end up in.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 17 • 6 years ago

(In reply to :Gijs (he/him) from comment #15)
> OK. At what point in browser startup is the migration performed? We can add
> a migration step to nsBrowserGlue, but we don't normally use it for this
> type of thing, and if it races with the migration then the obvious result
> would be dataloss... :-(

It happens when we call NSS_Initialize [0] as part of initializing nsINSSComponent (nsNSSComponent [1]).

[0] https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/security/certverifier/NSSCertDBTrustDomain.cpp#1147
[1] https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/security/manager/ssl/nsNSSComponent.cpp#1988

For better or worse, currently it's synchronous, so if you first instantiate the service (and it successfully initializes NSS in read/write mode, which isn't guaranteed...) then you won't race.

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #16)
> I think we should do the following:
> a) Delete key3.db (if it exists) when a master password is changed/enabled.
> That solves this bug and should be backported to ESR.

This seems reasonable (although there's the issue of if a user switches back to an older ESR, things like saved passwords and imported certificates might not work).

> b) Delete all of the old files (key3.db, cert8.db, and secmod.db) for the
> next ESR (to support downgrades between ESR) in an upcoming release… I don't
> think we need to rush on this. Should we only do this for Firefox (e.g.
> migrateUI) or do we another another place to do this that won't incur a
> startup perf hit from file I/O every startup? Or does this need to be in PSM
> for all PSM consumers? We should at least notify downstream consumers about
> this issue I guess? This affects who should implement it.

I mean, we're already accessing all of these files synchronously at startup anyway right now (see bug 1370516), so it probably won't be that much more of a performance hit.

We should at least mention this to the NSS team. Their stance may be "yes, definitely the old DBs should be deleted" or it may be "no, we can't risk the loss of user data". That said, I think Firefox may need/want to make a different decision than NSS does on this.

> (In reply to :Gijs (he/him) from comment #13)
> > Would it be easy to delete the old file as part of that at this point?
> 
> That will break the downgrade case so I don't think it's the best idea.
> 
> > And should we backport any such
> > change to esr60 to avoid this issue persisting on ESR for another year or so?
> 
> I think it would be safest to only backport a fix to delete key3.db if a
> master password is changed/enabled. This won't help ESR users who already
> make a MP change after ESR60 got installed but since that is still very new
> we probably have time to fix this before may people are affected since less
> than 1% of users use a MP.
> 
> David, what do you think about this proposal and where do you think we
> should implement parts (a) and (b) in the code since that affects what team
> implements it and which component this should end up in.

I think doing both a and b sounds good. There is already some code in nsNSSComponent that does a similar thing [2], so maybe it makes sense to put both there.

[2] https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/security/manager/ssl/nsNSSComponent.cpp#1570

	jurgen Reporter
	Comment 18 • 6 years ago

So the work-around for people like me, would be to manually delete key3.db if I want to prioritize security over backward compatibility. That's right?


Would it be a sensible approach to put this decision in a GUI?

[x] Maintain backward compatibilty (decreasing security)
[ ] Increase security (breaks compatibility with FF older than XXX)

This way, the option is left to the user? (ideally, these options only appear in the master password dialog box)

	Matthew N. [:MattN]
	Comment 19 • 6 years ago

(In reply to jurgen from comment #18)
> So the work-around for people like me, would be to manually delete key3.db
> if I want to prioritize security over backward compatibility. That's right?

Yes

> Would it be a sensible approach to put this decision in a GUI?

We could but IMO that would be too much work (initial and maintenance) for a feature that less than 1% of users use.

	jurgen Reporter
	Comment 20 • 6 years ago

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #19)
> (In reply to jurgen from comment #18)
> > So the work-around for people like me, would be to manually delete key3.db
> > if I want to prioritize security over backward compatibility. That's right?
> 
> Yes
Thanks. Feeling safer already.

> 
> > Would it be a sensible approach to put this decision in a GUI?
> 
> We could but IMO that would be too much work (initial and maintenance) for a
> feature that less than 1% of users use.
Too bad... well, I guess people being critical about safety use something external like keepass and the ones not critical about it... well, they just prefer the ease of it.
Anyways, thanks for implementing it.

I'd like to set out a little blog post on this topic and the workaround. Seeing the timeline, I'm assuming that's okay to do that soonish?

	Matthew N. [:MattN]
	Comment 21 • 6 years ago

(In reply to jurgen from comment #20)
> > We could but IMO that would be too much work (initial and maintenance) for a
> > feature that less than 1% of users use.
> Too bad... well, I guess people being critical about safety use something
> external like keepass and the ones not critical about it... well, they just
> prefer the ease of it.
> Anyways, thanks for implementing it.
> 
> I'd like to set out a little blog post on this topic and the workaround.
> Seeing the timeline, I'm assuming that's okay to do that soonish?

Please wait until the bug has been fixed on the release channel so that users are no longer affected. At that point I'm not sure it would be useful to blog about… it would only help ESR users who changed/enabled a MP since ESR60.

Moving this to PSM since that's where the original bug was and it sounds like this will get fixed in security code, not Toolkit.

(In reply to [:keeler] (use needinfo) from comment #17)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #16)
> > I think we should do the following:
> > a) Delete key3.db (if it exists) when a master password is changed/enabled.
> > That solves this bug and should be backported to ESR.
> 
> This seems reasonable (although there's the issue of if a user switches back
> to an older ESR, things like saved passwords and imported certificates might
> not work).

Yeah, though I think if a user is changing or setting a MP then they would prefer their data was no longer accessible without a MP.

> > b) Delete all of the old files (key3.db, cert8.db, and secmod.db) for the
> > next ESR (to support downgrades between ESR) in an upcoming release… I don't
> > think we need to rush on this. Should we only do this for Firefox (e.g.
> > migrateUI) or do we another another place to do this that won't incur a
> > startup perf hit from file I/O every startup? Or does this need to be in PSM
> > for all PSM consumers? We should at least notify downstream consumers about
> > this issue I guess? This affects who should implement it.
> 
> I mean, we're already accessing all of these files synchronously at startup
> anyway right now (see bug 1370516), so it probably won't be that much more
> of a performance hit.
> 
> We should at least mention this to the NSS team. Their stance may be "yes,
> definitely the old DBs should be deleted" or it may be "no, we can't risk
> the loss of user data". That said, I think Firefox may need/want to make a
> different decision than NSS does on this.

Who is going to reach out to them? I'm not sure of the right people to talk to…

> > (In reply to :Gijs (he/him) from comment #13)
> > > Would it be easy to delete the old file as part of that at this point?
> > 
> > That will break the downgrade case so I don't think it's the best idea.
> > 
> > > And should we backport any such
> > > change to esr60 to avoid this issue persisting on ESR for another year or so?
> > 
> > I think it would be safest to only backport a fix to delete key3.db if a
> > master password is changed/enabled. This won't help ESR users who already
> > make a MP change after ESR60 got installed but since that is still very new
> > we probably have time to fix this before may people are affected since less
> > than 1% of users use a MP.
> > 
> > David, what do you think about this proposal and where do you think we
> > should implement parts (a) and (b) in the code since that affects what team
> > implements it and which component this should end up in.
> 
> I think doing both a and b sounds good. There is already some code in
> nsNSSComponent that does a similar thing [2], so maybe it makes sense to put
> both there.
> 
> [2]
> https://searchfox.org/mozilla-central/rev/
> b0275bc977ad7fda615ef34b822bba938f2b16fd/security/manager/ssl/nsNSSComponent.
> cpp#1570

David, who should get this prioritized and likely find an owner? I think we should fix this sooner rather than later so we don't need to blog about it and cause a press cycle.

	jurgen Reporter
	Comment 22 • 6 years ago

As a final (and completely off-topic) note: thanks to you guys! It was an honour being amongst you guys. I really appreciate all the work you're doing. I was impressed of how quick the response time on this was, even with it being in the weekend!

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 24 • 6 years ago

Matt - let me know if that's the behavior you were expecting (I would have asked for review from you, but I don't know what your phabricator username is).

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 25 • 6 years ago

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #21)
> > We should at least mention this to the NSS team. Their stance may be "yes,
> > definitely the old DBs should be deleted" or it may be "no, we can't risk
> > the loss of user data". That said, I think Firefox may need/want to make a
> > different decision than NSS does on this.
> 
> Who is going to reach out to them? I'm not sure of the right people to talk
> to…

I filed bug 1476434.

	Phabricator Automation
	Comment 26 • 6 years ago

Comment on attachment 8992768 [details]
bug 1475775 - clean up old NSS DB file after upgrade if necessary r?franziskus,mattn

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.

https://phabricator.services.mozilla.com/D2202

	Matthew N. [:MattN]
	Comment 27 • 6 years ago

Sorry, I've been busy… I'll try look tomorrow.

(In reply to [:keeler] (use needinfo) from comment #25)
> (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from
> comment #21)
> > > We should at least mention this to the NSS team. Their stance may be "yes,
> > > definitely the old DBs should be deleted" or it may be "no, we can't risk
> > > the loss of user data". That said, I think Firefox may need/want to make a
> > > different decision than NSS does on this.
> > 
> > Who is going to reach out to them? I'm not sure of the right people to talk
> > to…
> 
> I filed bug 1476434.

FYI I can't access that bug but I also don't know if it matters.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 28 • 6 years ago

I cc'd you - in any case it's basically this bug but specific to NSS.

	Phabricator Automation
	Comment 29 • 6 years ago

Comment on attachment 8992768 [details]
bug 1475775 - clean up old NSS DB file after upgrade if necessary r?franziskus,mattn

Matthew N. [:MattN] (PM if requests are blocking you) has approved the revision.

https://phabricator.services.mozilla.com/D2202

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 30 • 6 years ago

Thanks for the reviews!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8419bd0fc4664db6b43aaeaf09adace6908fa17c

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 31 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/26ac31d53e50217dff8829e6d9bae18c7e36b812

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 32 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/26ac31d53e50

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 6 years ago

This grafts cleanly to Beta. Is it worth considering for uplift?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 34 • 6 years ago

Comment on attachment 8992768 [details]
bug 1475775 - clean up old NSS DB file after upgrade if necessary r?franziskus,mattn

Approval Request Comment
[Feature/Bug causing the regression]: bug 783994
[User impact if declined]: If a user has saved passwords but hadn't set a master password before upgrading to Firefox 58, and then subsequently sets a master password, it is possible to recover the user's saved passwords without knowing their master password. This is probably fairly rare, though - the vast majority (99%) of users don't set a master password ever.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: the fix we went with was to check for and remove the old key database if the user has a master password set. Since most users don't have a password, this is a no-op for most users.
[String changes made/needed]: none

	Julien Cristau [:jcristau]
	Comment 35 • 6 years ago

Comment on attachment 8992768 [details]
bug 1475775 - clean up old NSS DB file after upgrade if necessary r?franziskus,mattn

deleting old unprotected nss db if we have set a password; let's get this in 62.0b16

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/1e05e8939a18

	jurgen Reporter
	Comment 37 • 6 years ago

Just a small privacy question... as soon as this bug gets published, could someone please remove the TREE list in the feedback (comment 9), as it discloses quite some personal information about my account (e.g. what password manager I'm using and sites I seem to have visited recently).

Thanks,
Jurgen

	:Gijs (he/him)
	Comment 38 • 6 years ago

(In reply to jurgen from comment #37)
> Just a small privacy question... as soon as this bug gets published, could
> someone please remove the TREE list in the feedback (comment 9), as it
> discloses quite some personal information about my account (e.g. what
> password manager I'm using and sites I seem to have visited recently).
> 
> Thanks,
> Jurgen

We can't delete comments on bugzilla, but we can mark them private (meaning they'll only be visible to people who have security clearance on bugzilla). I'll do this pre-emptively now so it doesn't get forgotten.

	jurgen Reporter
	Comment 39 • 6 years ago

Thanks!

	:Gijs (he/him)
	Comment 41 • 6 years ago

Matt, is there a bug on file to make ESR60 delete key3.db if/when the user sets a MP where one was not previously set?

	Matthew N. [:MattN]
	Comment 42 • 6 years ago

IIUC, that would be doing what this bug did but on ESR60. IMO this should have been considered for esr60 but RyanVM marked it as wontfix in comment 33 but I'm not sure what the rationale was. I don't think we need a new bug… we can request uplift here.

> status-firefox-esr60: affected → wontfix

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 6 years ago

We don't normally backport sec-low bugs to ESR releases. If you feel strongly that we should consider doing so in this bug, feel free to nominate the patch and make your case.

	jurgen Reporter
	Comment 44 • 6 years ago

Given the relative ease of resolution and the impact for people affected (it is really REALLY simple to get those passwords) - I thought especially in an ESR, which people tend to use on security centric distros like Kali Linux and Parrot OS ... this feels like a critical feature (especially since the implementation of the crypto is really solid) to end up being cheated about.

The only reason for the low CVE rating, is the judgement call that the MP is a rarely used feature. This kind of behavior will consolidate people into not using it (as it appears to be not reliable). So if you yourself believe that using the MP is a good thing, you should agree that it being rock solid is not a detail.

Not sure as how or where to nominate the patch, but I really want to make a case here. Otherwise you could as well just leave out that feature (since only less than 1% of the users use it) and recommend people use an external password manager.

	Al Billings [:abillings - ex-MoCo]
	Comment 45 • 6 years ago

(In reply to jurgen from comment #44)
> it is really REALLY simple to get those passwords

How so? People would need local system access and to be logged into the account in question. This is not a drive by web exploit that can be done by a malicious site. This requires malware or your local account to be hacked by someone.

> - I thought especially in an
> ESR, which people tend to use on security centric distros like Kali Linux
> and Parrot OS ... this feels like a critical feature (especially since the
> implementation of the crypto is really solid) to end up being cheated about.

 Are those people likely to have a third party access their system or have a vulnerability that gives local file access to their accounts? Your example is security centric distros so this seems unlikely.

As a general rule, we don't backport sec-low rated issues to ESR branches. Ryan's point is that if you or someone makes a patch that applies cleanly to ESR60 and nominates the patch for acceptance, he's willing to listen to an argument to apply it.

	Daniel Veditz [:dveditz]
	Comment 46 • 6 years ago

Setting the flags to actually renominate per comment 44.

	Matthew N. [:MattN]
	Comment 47 • 6 years ago

We shouldn't just consider this for nomination from the security perspective but also take into account that it's a form of regression that the Master Password can be bypassed so easily.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 48 • 6 years ago

Comment on attachment 8992768 [details]
bug 1475775 - clean up old NSS DB file after upgrade if necessary r?franziskus,mattn

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: this is a pretty easy way for users to think they've encrypted their passwords when in fact there's a plain-text copy of the encryption key sitting in their profile directory, which is not a great situation. We've already had one duplicate filed on this.
User impact if declined: users may be surprised to find that their passwords can be decrypted when they thought they had password-protected them.
Fix Landed on Version: 63, uplifted to 62
Risk to taking this patch (and alternatives if risky): fairly low - it seems to be working without issue so far
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

	Ryan VanderMeulen [:RyanVM]
	Comment 49 • 6 years ago

Comment on attachment 8992768 [details]
bug 1475775 - clean up old NSS DB file after upgrade if necessary r?franziskus,mattn

Approved for ESR 60.2.1.

	Ryan VanderMeulen [:RyanVM]
	Comment 50 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/300efdbc9fe1

	Paul Stauffer
	Comment 51 • 6 years ago

It appears that this fix caused data loss for one of my users.  User is on CentOS 7, and was upgraded from 60.2.0 ESR to 60.2.1 ESR on Sep 29.  She has a very old profile dir, has a master password set, and reports having last changed the master password 5-6 years ago.  After the update to 60.2.1 ESR, the next time she opened Firefox, she noted that it no longer prompted her for her master password, the "Use a master password" option in Preferences was no longer checked, and all of her saved logins are gone.  Instead there is now just a single entry for Site: chrome://FirefoxAccounts (Firefox Accounts credentials), with a long hexadecimal username, and Password: {"version":1,"accountData":{}}.

We do have backups of her home directory from before the upgrade, which we could restore data from, but I am unclear on how to proceed in this case.  If I simply replace her ~/.mozilla with the older copy and relaunch Firefox, on the first run her MP is back and her saved logins are present, but after closing and restarting Firefox they're gone again.  Using the old ~/.mozilla but manually deleting key3.db before launching Firefox again results in no MP and no saved logins.

FWIW, all of the following files were present in her profile directory prior to the upgrade:
# ls -l key3.db secmod.db cert8.db signons.sqlite logins.json
-rw-------. 1 500 500 688128 Sep  4 08:23 cert8.db
-rw-------. 1 500 500  16384 Sep  4 08:23 key3.db
-rw-------. 1 500 500  82886 Sep 26 11:01 logins.json
-rw-------. 1 500 500  16384 Oct 12  2014 secmod.db
-rw-r--r--. 1 500 500 327680 May 11  2015 signons.sqlite

From this and reading the thread above, my interpretation is that her saved logins were successfully migrated from signons.sqlite to logins.json at some point in the past (2015?), but her MP was still saved in an old format (key3.db? secmod.db?) and never migrated to the new format.  Does that interpretation sound correct?  Given this scenario, how can I go about restoring this user's saved login data, and/or converting her MP to the new format?

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 52 • 6 years ago

Given a directory "dir" containing cert8.db, key3.db, and secmod.db (and not cert9.db, key4.db, or pkcs11.txt), try running:

certutil -K -X -d dir

You should be asked for the password for that database. (It might say "no keys found", but you can ignore that.)
If that succeeds, cert9.db, key4.db, and pkcs11.txt should have been created in that directory. Move those to the Firefox profile and remove the old cert8.db, key3.db, and secmod.db files from the Firefox profile. (This is assuming your version of NSS is recent enough - from https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.35_release_notes I think 3.35 should be sufficient).

	Paul Stauffer
	Comment 53 • 6 years ago

That certutil command does prompt for the password, and does say "no keys found", but it does not create any of the files you indicated.  I've looked at the man page for certutil, and it's not clear to me why those options would convert anything or create new files; can you confirm this was the syntax you intended?

laptop:~/.mozilla/firefox/loejs653.default$ ls cert*db key* secmod.db pkcs11.txt
ls: cannot access pkcs11.txt: No such file or directory
cert8.db  key3.db  secmod.db
laptop:~/.mozilla/firefox/loejs653.default$ cd ..
laptop:~/.mozilla/firefox$ certutil -K -X -d loejs653.default/
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
Enter Password or Pin for "NSS Certificate DB":
certutil: no keys found
laptop:~/.mozilla/firefox$ cd loejs653.default/
laptop:~/.mozilla/firefox/loejs653.default$ ls cert*db key* secmod.db pkcs11.txt
ls: cannot access pkcs11.txt: No such file or directory
cert8.db  key3.db  secmod.db

FWIW, this is with nss-tools-3.36.0-7.el7_5.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave) Assignee
	Comment 54 • 6 years ago

Yes, that's the intended syntax. Newer versions of NSS *should* automatically upgrade the old dbm databases to sqlite when they're opened read/write (which is what -X should do). Maybe try with a newer version of NSS? (I'm using nss-3.39.0-1.0.fc28)

	Orion Poplawski
	Comment 55 • 6 years ago

On EL7 in order to get NSS/certutil to create the new sql databases you need to do:

export NSS_DEFAULT_DB_TYPE=sql

but I still haven't figured out how to migrate a pre 60.2.1 profile - though I may be working with a bad initial profile.

	Phil Durbin
	Comment 56 • 6 years ago

I am in the same situation as Comment 51 ; I have tried the solution suggested in comment 53 using export NSS_DEFAULT_DB_TYPE=sql suggested in comment 55. That created the files expected.I copied the new files: key4.db pkcs11.txt cert9.db into place  and deleted the old ones  key3.db secmod.db cert8.db. On start up that worked but (after trial and error) 
I found that firefox is creating key3.db cert8.db and secmod.db files when I add a new entry to the password manager and exit. Then I have to delete the newly created  key3.db cert8.db and secmod.db files to get things to work.

	Phil Durbin
	Comment 57 • 6 years ago

Turns out the trigger to make it work after following the instructions is to have the env variable NSS_DEFAULT_DB_TYPE=sql set before exucuting firefox .If it is not set it keeps recreating key3.db cert8.db and secmod.db files . Adding a new entry to the password manager has nothing to do with it.

	Paul Stauffer
	Comment 58 • 6 years ago

Confirming that setting NSS_DEFAULT_DB_TYPE=sql before running certutil seems to have worked.  New files were generated; removed old files.  Based on Phil's notes in comment 56 and comment 57 above, also added "export NSS_DEFAULT_DB_TYPE=sql" to user's .bash_profile and had her log in again.  Running firefox now preserves the MP, saved logins remain accessible, and the old versions of the files are not recreated.