Closed Bug 1532377 Opened 5 years ago Closed 4 years ago

Avoid saving a munged password in login storage

Categories

(Toolkit :: Password Manager, enhancement, P2)

Product:
Toolkit
Component:
Password Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla77
Tracking Flags:
Tracking Status
firefox77 --- verified

People

(Reporter: MattN, Assigned: severin)

References

Details

(Whiteboard: [passwords:capture-UI])

User Story

In this first version, we consider that a munged password is one that consists only in repeats of characters `*`, `•`, or `.`.

Attachments

(1 file)

Bug 1532377 - Don't save munged passwords in login storage;r?MattN
47 bytes, text/x-phabricator-request
Details | Review

Address some of the issues from bug 1530814 related to the wrong value being saved for the password.

One short-term solution could be to just open the existing doorhanger with dismissed:true to allow the user to type in the correct password.

status-firefox67: affected → ---
Depends on: 1543449

Moving this to P3 since we didn't find a common pattern to address yet other than bug 1543449 (which is P2)

Priority: P2 → P3

:Yoric reports that his bank (URL pending) tried to submit "*" for each character of his password in bug 1584279. That should be an easy case to fix I think.

Priority: P3 → P2

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

:Yoric reports that his bank (URL pending) tried to submit "*" for each character of his password in bug 1584279. That should be an easy case to fix I think.

My bank is http://www.banquepopulaire.fr.

If the right place to fix this is around https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginManagerParent.jsm#615-622, I'll be happy to provide a patch for that specific case.

Flags: needinfo?(MattN+bmo)

I was thinking it should go at https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/toolkit/components/passwordmgr/LoginManagerContent.jsm#1134 with its own return and log. That way it will just skip that password field and potentially still find the real one earlier in tree order. You can then add unit tests to https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/test/unit/test_getPasswordFields.js

I guess there is a potential edge case if a user's password really is a series of only asterisks though… I guess those users will have to manually save in about:logins? According to https://haveibeenpwned.com/Passwords it seems like passwords containing a series of asterisks are not unheard of :(

Now I wonder if we should re-open bug 1584279 and have it only handle not overwriting a saved password with a series of asterisks? That seems much safer. Do you agree?

Flags: needinfo?(MattN+bmo)

Or if the real password is in another password field in the page then we could have getPasswordFields remove it only if it find another password field that meets the min. length?

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

Now I wonder if we should re-open bug 1584279 and have it only handle not overwriting a saved password with a series of asterisks? That seems much safer. Do you agree?

Yeah, that does feel safer.

Flags: needinfo?(MattN+bmo)

A number of websites munge the password, for instance by replacing it with
literals ********. This patch changes LoginManager{Child, Parent} to ensure
that we do not display "do you want to save"/"do you want to update" for
such passwords and that we conversely do not save them.

In this first version, we consider that a munged password is one that consists
only in repeats of characters * or in repeats of characters •.

Assignee: nobody → dteller

Oh, well, I guess I wrote something :)

Thanks, I'll take a look soon… just trying to deal with some Fx72 stuff first.

Status: NEW → ASSIGNED

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #8)

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

Now I wonder if we should re-open bug 1584279 and have it only handle not overwriting a saved password with a series of asterisks? That seems much safer. Do you agree?

Yeah, that does feel safer.

I would need to look at the patch closer but are you addressing this bug or bug 1584279 after all?

Flags: needinfo?(MattN+bmo)

We should also fix the case from bug 1277172 (all period characters) in this bug if it's straightforward. In can be in a separate commit.

Assignee: dteller → severin.mozilla
Blocks: 1277172
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/27e376421401
Don't save munged passwords in login storage;r=MattN

https://hg.mozilla.org/mozilla-central/rev/27e376421401

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox77: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Flags: qe-verify+
User Story: (updated)

Matthew, can you please provide some steps to reproduce this issue. Thank you.

Flags: needinfo?(MattN+bmo)

Sure, hope this helps.

STR

Expect

  • Password manager should not offer to save the the password.
Flags: needinfo?(MattN+bmo)

The site you provided is not working temporarily. I used another site: https://retail.onlinesbi.com/retail/login.htm, but the prompt for saving the password is still displayed on the latest Nightly 77.0a1 using Windows 10 x64.

https://retail.onlinesbi.com/retail/login.htm looks like a different case (which we'll need to open a bug for). This bug is about preventing the PM from saving "********" passwords, but https://retail.onlinesbi.com is intentionally confusing the manager and stuffing it with incorrect but readable characters.

From earlier investigation, the case for this munging bug is really rare. We've only managed to repro it in the wild at https://www.banquepopulaire.fr/se-connecter/identifier.

My apologies Oana, I think the link I gave you might be bad. This is the process I used to get to the login page:

Your URL should then be https://www.banquepopulaire.fr/se-connecter/identifier, and you can follow the STR from comment 17.

Sorry again for the mistake!

Flags: needinfo?(oana.botisan)

I managed to reproduce the issue using an older version of Nightly from 2019-03-04.
I retested everything using latest Nightly 77.0a1 on Windows 10 x64, Mac OS 10.11 and Ubuntu 18.04 x64. The issue is not reproducing anymore.
Thank you, Rudie, for all your help.

Status: RESOLVED → VERIFIED
status-firefox77: fixed → verified
Flags: qe-verify+
Flags: needinfo?(oana.botisan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: