1584126 - The password of a saved login can be revealed with devtools without re-entering the master password if one is set

Status: VERIFIED FIXED

(Firefox :: about:logins, defect, P2)

Description

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Attachment: password revealed without entering master password.gif

[Affected Versions]:

• Nightly 71.0a1
• Beta 70.0b9

[Affected Platforms]

• All Windows
• All Mac
• All Linux

[Prerequisites]

• Have multiple logins saved.
• Have a master password set.

[Steps to reproduce]:

1. Open the latest Nightly build with the profile from prerequisites.
2. Navigate to a login page and enter the MP to unlock it (eg: "https://www.facebook.com/").
3. Open about:logins and select any saved password from the login list.
4. Right-click on the password and select the "Inspect Element" option.
5. From the Inspector tool change the input's argument from "password" to "text" and press "Ente" key.
6. Observe the password from login-item.

[Expected results]:

• The password is not revealed.

[Actual results]:

• The password is revealed.

[Notes]:

• Attached a screen recording with the issue.

Comment 1

[Daniel Veditz [:dveditz]]

Like the other similar bug, this would be prevented by asking for the master password always when you load about:logins, rather than trying to wait until you look at individual items.

Comment 2

[Matthew N. [:MattN]]

(In reply to Daniel Veditz [:dveditz] from comment #1)

Like the other similar bug, this would be prevented by asking for the master password always when you load about:logins, rather than trying to wait until you look at individual items.

Right, but that is more annoying for users who weren't going to copy/reveal any of the passwords so there is a tradeoff. I guess we can see how common that case is with the event telemetry.

Comment 3

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

Unless you think this should block the feature for 70, I'll assume at this point we're aiming to fix it for 71.
But we still have beta 13 and 14 next week and I would still take a patch for 70.

Comment 4

[Matthew N. [:MattN]]

I'm not planning to fix this for 70.

Comment 5

[Pascal Chevrel:pascalc]

Matt, do you intend to fix this in beta 71? thanks

Comment 6

[Matthew N. [:MattN]]

Mass removing [skyline] and [passwords:management] from about:logins bugs which are no longer useful.

Comment 7

[Matthew N. [:MattN]]

(In reply to Pascal Chevrel:pascalc from comment #5)

Matt, do you intend to fix this in beta 71? thanks

No, probably not since we need UX input and/or architectural changes.

Comment 8

[Matthew N. [:MattN]]

I found that this was reported publicly in the (somewhat) wrong component in bug 1580929. Should we dupe that one hear (since it has more info) and open this one up?

Comment 9

[Pascal Chevrel:pascalc]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

I found that this was reported publicly in the (somewhat) wrong component in bug 1580929. Should we dupe that one hear (since it has more info) and open this one up?

Thanks, let's move it to th 72 queue then.

Comment 10

[Daniel Veditz [:dveditz]]

It makes sense to dupe 1580929 here given there's more discussion here.

Unhiding is fine with me. We just had another duplicate report of this sent to the security@ mail alias.

Comment 12

[jscher2000]

Could we have an explicit "log out" or "Re-lock" button on the about:logins page to re-secure the Password Manager?

Currently, the only way I think you can log out -- other than by ending your session -- is to cancel out of the Master Password dialog. So clicking any "Show password" icon and then clicking Cancel (or pressing the Esc key) in the dialog is a workaround; the page will be re-secured if reloaded. But this is not intuitive or easy to discover (except by accident).

So a button to log out and clear the contents would be helpful for those Master Password users who don't want to leave the page "unlocked."

Comment 16

[Matthew N. [:MattN]]

(Quoting Matthew N. [:MattN] from comment #2)

(In reply to Daniel Veditz [:dveditz] from comment #1)

Like the other similar bug, this would be prevented by asking for the master password always when you load about:logins, rather than trying to wait until you look at individual items.

Right, but that is more annoying for users who weren't going to copy/reveal any of the passwords so there is a tradeoff. I guess we can see how common that case is with the event telemetry.

Katie are you fine with what comment 1 proposes as a solution until we finish bigger decisions about MP? I don't want this to blow up since it's a regression for this UI (even though autofill in every browser has this issue too).

Comment 17

[katieC]

Yes, asking for the user to enter their MP upon loading about:logins makes sense (rather than waiting until a user chooses to show a password). If a user has chosen to set a MP, then this step feels like a feature of how it protects all logins rather than an annoyance/bug.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 19

[Daniel Veditz [:dveditz]]

If anyone's counting duplicates we just got another report of this issue mailed to the security@ alias. Clearly people think this is a security problem and don't understand that they've already unlocked everything if they've entered the master password once already. As Matt points out this has implications for password fill as well. There's no way to "log out" of the master password--via official UX, anyway. If you know the trick you can do it by trying to view a password and not entering a valid password, but that's a non-obvious ugly hack.

Maybe we need a locked/un-locked button in the toolbar, kind of like the keychain one for MacOS. That's probably a terrible suggestion, but maybe on the FxA toolbar menu there could be a "Unlock passwords" | "Log-out of Master Password" toggle menu item grouped with the existing "Logins and Passwords" that only shows for people with master passwords.

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #2)

Right, but that is more annoying for users who weren't going to copy/reveal any of the passwords so there is a tradeoff. I guess we can see how common that case is with the event telemetry.

Is it common for people to go into about:logins just to see the site list? Isn't a list of sites and usernames kind of sensitive on its own?

Comment 20

[Matthew N. [:MattN]]

FYI I did spend multiple hours working on a fix but it gets quite complicated when you have to handle multiple about:logins being open at once and the existing code is not at all friendly to re-locking once its opened. I also had discussions with UX about possible solutions but haven't had a chance to get back to this due to an unclear path forward that is feasible to implement while there are also other competing priorities.

Comment 21

[Matthew N. [:MattN]]

Attachment: Bug 1584126 - Factor showNotification out of showNotifications in AboutLoginsParent. r=sfoster

Comment 22

[Matthew N. [:MattN]]

Attachment: Bug 1584126 - Move master password about:logins notification options to a constant. r=sfoster

Depends on D57691

Comment 23

[Matthew N. [:MattN]]

Attachment: Bug 1584126 - Allow filtering by password value in about:logins when MP is enabled. r=sfoster

The master password is now required before loading the page so we can filter by its value without disclosing new information.

Depends on D57692

Comment 24

[Matthew N. [:MattN]]

Attachment: Bug 1584126 - Remove AboutLoginsUtils.masterPasswordEnabled. r=sfoster

Depends on D57693

Comment 25

[Matthew N. [:MattN]]

The above patches are just prerequisite cleanups, not the main patch which is still WIP.

Comment 26

[Matthew N. [:MattN]]

Attachment: Bug 1584126 - Disconnect password input when it's not editable and require master password when entering edit mode. r=sfoster

Depends on D58128

Comment 27

[Matthew N. [:MattN]]

Attachment: Bug 1584126 - Fix tests to handle the disconnected password input in about:logins. r=sfoster

Depends on D58129

Comment 28

[Matthew N. [:MattN]]

Attachment: Bug 1584126 - Add tests for the disconnected and display password input. r=sfoster

Depends on D58130

Comment 29

[Matthew N. [:MattN]]

Requiring the MP before loading the page was too big of a change for something we would ideally uplift and would be inconsistent with how the old password management UI worked and how Chrome works so for now I'm just addressing the specific issue in this bug summary: access to the plaintext password via the password input element in the Inspector of developer tools before re-entering the master password. There will still be other ways to get the plaintext out of the page, just like the same can be done from the Browser Console/Toolbox once MP has already been unlocked.

(In reply to Daniel Veditz [:dveditz] from comment #19)

If anyone's counting duplicates we just got another report of this issue mailed to the security@ alias. Clearly people think this is a security problem and don't understand that they've already unlocked everything if they've entered the master password once already. As Matt points out this has implications for password fill as well. There's no way to "log out" of the master password--via official UX, anyway. If you know the trick you can do it by trying to view a password and not entering a valid password, but that's a non-obvious ugly hack.

Technically there is a button buried in the Security Devices preferences sub-dialog (chrome://pippki/content/device_manager.xhtml) for this but it's still a few clicks as you have to choose Software Security Device on sidebar then choose "Log Out".

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #2)

Right, but that is more annoying for users who weren't going to copy/reveal any of the passwords so there is a tradeoff. I guess we can see how common that case is with the event telemetry.

Is it common for people to go into about:logins just to see the site list? Isn't a list of sites and usernames kind of sensitive on its own?

It is for me and both our old UI and Chrome support this use case. I just checked with telemetry and the number of actions that would currently require a MP is only 65% of the number of times about:logins was loaded so it seems like it is fairly common. This analysis was for all users, not specific to users with MP enabled.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/812145c6e714
Disconnect password input when it's not editable and require master password when entering edit mode. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/d16058ad1ced
Fix tests to handle the disconnected password input in about:logins. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/06976e586b36
Add tests for the disconnected and display password input. r=sfoster

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/812145c6e714
https://hg.mozilla.org/mozilla-central/rev/d16058ad1ced
https://hg.mozilla.org/mozilla-central/rev/06976e586b36

Comment 32

[Cosmin Muntean [:cmuntean], Ecosystem QA]

I have verified this issue and is no longer reproducible. Tested on the latest Nightly 73.0a1 build (Build ID: 20200103094738) on Windows 7 x64, Mac OS 10.15 and Ubuntu 18.04 x64.

• The password of a saved login can no longer be revealed with devtools without re-entering the master password.
• The master password is required when entering edit mode.

Comment 34

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 35

[Matthew N. [:MattN]]

I personally don't think this deserves an advisory as this protection isn't really security protection, it's to make it harder for people snooping and this was an known limitation of the fix in bug 1579512. If you think we need one then it should clarify that the bypass is only through the use of developer tools.

Comment 36

[Tom Ritter [:tjr]]

I was really leaning against it but wasn't certain and figured I'd write it up and see if anyone said anything.