1579512 - about:logins allows trivial bypasses of Master Password to reveal plaintext passwords if it is logged in (unlocked)

Status: VERIFIED FIXED

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

Description

[Matthew N. [:MattN]]

STR:

1. Set a master password if you don't already have one and restart Fx
2. Load a login page with a saved login and enter the MP to unlock it (log in).
3. Open about:logins with some already saved logins
4. See that the password for a login is masked
5. Right-click on the masked characters and choose Inspect Element

Expected result:
The password shouldn't be revealed as I didn't re-enter the Master Password

Actual result:
I can see the password in plaintext

This STR wasn't possible with the old UI as you couldn't access the plaintext passwords with the inspector alone, you would have to run code in the console. If we allow such a trivial bypass of MP we may as well remove the feature IMO. I can see viral articles showing how to perform this "hack". Dan, do you agree? The in-content prefs version of passwordManager.xul already had the problem of exposing logins with the web console but the window popup mode didn't have that problem.

[Tracking Requested - why for this release]: We need to at least make a decision on whether we care about this "regression" for the new UI shipping in 70.

Ideally we wouldn't send passwords to the page when a MP is enabled until they need to be in plaintext.
I have been wanting to re-architect login storage to do lazy decryption of passwords until they are actually needed as plaintext but that hasn't been prioritized yet. I think that would have helped here.

Some easier potential workarounds:

• Don't set .value/@value with the real password when it isn't revealed, use a string of the same length instead.
• Require re-auth of MP on about:logins page load though that has TOCTOU problems.
  • This makes MP more annoying if you have to enter it to load the page then again to copy/reveal.

Comment 1

[Matthew N. [:MattN]]

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

• Don't set .value/@value with the real password when it isn't revealed, use a string of the same length instead.

I'm going to look at implementing this.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

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

Some easier potential workarounds:

• Don't set .value/@value with the real password when it isn't revealed, use a string of the same length instead.

We would also need to force auth when moving to edit mode, otherwise we would have to figure out how to merge the faked value of the input with the actual password.

Comment 3

[Matthew N. [:MattN]]

Attachment: Bug 1579512 - Don't include the real password in the markup of about:logins when masked. r=jaws

Comment 4

[Matthew N. [:MattN]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)

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

Some easier potential workarounds:

• Don't set .value/@value with the real password when it isn't revealed, use a string of the same length instead.

We would also need to force auth when moving to edit mode, otherwise we would have to figure out how to merge the faked value of the input with the actual password.

Good point. I'll look into that.

Comment 5

[Daniel Veditz [:dveditz]]

Responding to comment 0: yeah, the current behavior is not good. Are there any active plans to re-think the password manager behavior globally any time soon? Or should we just fix this spot on its own now? I'm guessing we all want it to be nicer but there are no current plans because not that many people use the feature.

I acknowledge I'm more of an edge case, but I find the current behavior of having to input my master password to reveal each field kind of annoying when I'm trying to find the "right" password on a site where I've saved several (again, acknowledging the edge case here). I wouldn't mind forcing a re-login of the master password to show the page contents in the first place (e.g. the current UX if you haven't already logged into the MPw) and then don't ask the password again for individual passwords. This would have the advantage that if you are a MPw user, snoops can't easily see your password protected sites just by loading the about:logins page. And if a snoop cancels out the MPw prompt then passwords are locked and even the console trick won't work. While using the page MPw users would have the same experience as non-MPw users instead of a more annoying one.

On the other hand their passwords stay vulnerable as long as the tab stays open, but maybe that won't be so much a surprise if it feels like they're "unlocking" the page when they enter it each time. Might even make MPw users feel safer.

Don't set .value/@value with the real password when it isn't revealed, use a string of the same length instead.

That's fine too. It doesn't even need to be a string of the same length: just make everything 15 characters long like a generated password. Or use the default value "hunter2" as an easter egg for anyone who does try this trick.

Comment 6

[Matthew N. [:MattN]]

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

Responding to comment 0: yeah, the current behavior is not good. Are there any active plans to re-think the password manager behavior globally any time soon? Or should we just fix this spot on its own now? I'm guessing we all want it to be nicer but there are no current plans because not that many people use the feature.

"Master password" was on a tentative roadmap that was shared last week so it's possible.

I acknowledge I'm more of an edge case, but I find the current behavior of having to input my master password to reveal each field kind of annoying when I'm trying to find the "right" password on a site where I've saved several (again, acknowledging the edge case here).

Yeah, I agree. We could also keep it unlocked for a time interval but if that's not communicated to users then they may complain that we're not secure if we don't ask for re-entry sometimes.

On the other hand their passwords stay vulnerable as long as the tab stays open, …

This was also my concern and I feel like we would need a way to communicate this to users if we can't do something automatically.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/971fa9f17d78ee029febdab245115e30aeaa8590
https://hg.mozilla.org/mozilla-central/rev/971fa9f17d78

Comment 8

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

Do you want to test/verify this first before uplift to beta? (If you are intending uplift)

Comment 9

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

Cosmin, can you help find someone to verify?

Comment 10

[Cosmin Muntean [:cmuntean], Ecosystem QA]

I have verified this issue on the latest Nightly 71.0a1 (2019-09-11) build on Windows 10 x64, Windows 7 x64, Mac 10.14 and Arch Linux 4.14.

• After following the steps provided in comment 0, the password is no longer visible on the "value" attribute.
• After following the steps provided in comment 0, if the "password" value of the "type" attribute is changed with the "text" value, empty space is displayed instead of the password.

I have also verified this issue without a master password set and I got the same results.

@Matthew are there any other scenarios that need to be verified?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/f56f3e4f6702f2c3eaff5641459b9beac5e6af31

Comment 12

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

jaws - This should have had a security rating and maybe sec approval and an uplift request before landing on beta.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

I'm sorry. This is a fix for a feature that hasn't been released yet so there shouldn't be concern about exposing an issue to users that are unprotected. I assumed this uplift fell within the blanket uplift approval that we had for the Lockwise work up to and including beta 8.

Comment 14

[Matthew N. [:MattN]]

(In reply to Liz Henry (:lizzard) from comment #12)

jaws - This should have had a security rating and maybe sec approval and an uplift request before landing on beta.

I tried to rate it when I filed it but it seems like there is a gray area in the criteria… IMO it falls between low and moderate but I'm not sure which one since MP isn't exactly a security feature… there are many other ways to bypass this snooping protection after the initial unlock.

Comment 15

[Cosmin Muntean [:cmuntean], Ecosystem QA]

I have verified this issue on the latest Beta 70.0b9 (Build ID: 20190923154733) build on Windows 7 x64, macOS 10.14 and Arch Linux 4.12.

However, it seems that now the behavior was changed after the patch from bug 1582534 landed on Beta 70.0b9 and Nightly 71.0a1.
In the latest versions of Beta and Nightly, even if you have a master password set you can reveal the password without entering the master password using the following steps:

1. Set a master password if you don't already have one and restart the browser.
2. Navigate to a login page and enter the MP to unlock it.
3. Open about:logins and select any saved password.
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" keyr.

• The password is revealed in the login item.

Before the patch from bug 1582534 landed, empty spaces were displayed instead of the password after following the steps above.
@Matt, should I log a separate issue for this behavior and mark this one as verified?

Comment 16

[Matthew N. [:MattN]]

That's expected behaviour for now IMO and also applies to autofilled passwords on any website. I filed this bug because it was even easier as it didn't require changing anything, simple inpsecting the element and the password would appear in plaintext.

Out of curiousity I just compared with Chrome and they do use space characters like I did for their masked password before the OS re-auth dialog. In theory we could try fix that approach to not cause data loss but I didn't debug it to understand what was happening.

Filing a bug on this would be useful to track for the future as I think there is some room for improvement, such as not sending the password to the content process until the MP was entered for that specific login.

Comment 17

[Cosmin Muntean [:cmuntean], Ecosystem QA]

Thanks Matt! I have logged the described behavior in bug 1584126.

Considering this I will mark this issue as Verified - Fixed and will track the described behavior in bug 1584126.