1569847 - Store breach indicator meta-data for logins

Status: RESOLVED FIXED

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

Description

[Luke Crouch [:groovecoder]]

In addition to showing “red alert” breach indicators on saved logins with passwords older than breaches, we should show “yellow alert” indicators on saved logins that are re-using those breached passwords.

But, when we launch the first indicators, users will (hopefully) change their passwords, and we will lose the ability to know which other logins were re-using the breached password.

So, I’d like to land some code now that will add a new property to saved logins - something like login.reusingBreachedPassword - that we can set to true on the first run of getBreachesForLogins, and then set to false when the user changes the password.

Note: before setting the value to false we will need to make sure the user didn’t change the password to some other breached password - i.e., neither a password of a login that shows up in getBreachesForLogins nor a password of a login that has its own reusingBreachedPassword field as true.

In addition, Bug 1569846 will need a way to store the "dismissed" state of breach alerts for logins.

:MattN - can you describe how we might add this meta-data for saved logins? In Slack you mentioned storing this meta-data separately from the other login data?

Comment 1

[Matthew N. [:MattN]]

(In reply to Luke Crouch [:groovecoder] from comment #0)

In addition to showing “red alert” breach indicators on saved logins with passwords older than breaches, we should show “yellow alert” indicators on saved logins that are re-using those breached passwords.

My main concern is that we don't actually know that those passwords are breached… we only know that the saved password is older than the breach date for that site… they could have changed their password to a newer one before the breach happened but didn't save that in Firefox. I realize that it's rare for users to willingly change their password but I think it's not that rare that users forget one and use a reset flow which forces a new one. Could we let the user check the password in the HIBP data before actually telling them to change that password for all accounts they re-used it on.

So, I’d like to land some code now that will add a new property to saved logins - something like login.reusingBreachedPassword - that we can set to true on the first run of getBreachesForLogins, and then set to false when the user changes the password.

Having to do the accounting to update the boolean seems like it is error-prone. It would be better to use a timestamp of the breach instead of a boolean so we don't have to pre-compute this, just compare the date with the timePasswordChanged timestamp (though note that changing the username now edits that timestamp in the last few years and we prompt to add a username upon login if it is stored as blank but one was submitted).

Note: before setting the value to false we will need to make sure the user didn’t change the password to some other breached password - i.e., neither a password of a login that shows up in getBreachesForLogins nor a password of a login that has its own reusingBreachedPassword field as true.

This is one reason why I think it may be easier to store possibly breached passwords (encrypted) in a separate data structure. Possibly still in logins.json but not part of the logins array. Then we can compare passwords to this hopefully smallish Set in order to show indicators and we don't even need to store a date. We would have to update that Set when a new breach is added and could maybe do that in getBreachesForLogins on some interval. Due to my main concern I wonder if we then need a management UI for this set in case of false-positives?

In addition, Bug 1569846 will need a way to store the "dismissed" state of breach alerts for logins.

Should this Sync across devices? Storing a timestamp of when it was dismissed would probably be fine (the timestamp is necessary to handle a future breach on the same domain). If we store this on the login it's easier to sync but we also have to be careful to make sure the Sync code will merge records properly if a login changes on a device which doesn't have them (e.g. iOS, Android, old Fx desktop, etc.) and we'd probably need to ask the Sync team to help with that, at least for testing our changes.

:MattN - can you describe how we might add this meta-data for saved logins? In Slack you mentioned storing this meta-data separately from the other login data?

Similar to above, it depends if you want the data to Sync and whether we are storing properties on a login or keeping a set of potentially breached passwords as I suggested above. I guess we should first decide on that. Do you agree that storing a set of potentially breached passwords is easier to maintain than a flag on each login? With my suggestion the set is only used for presentation of warnings in management UI and doesn't need to be kept in sync with other properties.

Comment 2

[Luke Crouch [:groovecoder]]

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

My main concern is that we don't actually know that those passwords are breached… we only know that the saved password is older than the breach date for that site… they could have changed their password to a newer one before the breach happened but didn't save that in Firefox. I realize that it's rare for users to willingly change their password but I think it's not that rare that users forget one and use a reset flow which forces a new one. Could we let the user check the password in the HIBP data before actually telling them to change that password for all accounts they re-used it on.

I defer to product & UX on this. :betsymi, rgaddis, ssage - the only thing we know in this case is that this saved password was in a breach. What do you think is a proper UX (if any) to show in that case?

We would need to be careful about prompting the user to check the password in HIBP. There was a recently published attack on the privacy of of the Pwned Passwords API: https://cablej.io/blog/k-anonymity/ Since it relies on correlating IP address of successive API requests, it will help to proxy any such requests thru our own server. There maybe still be some correlation attack to execute based solely on the timing, but that seems much lower risk of being effective.

This is one reason why I think it may be easier to store possibly breached passwords (encrypted) in a separate data structure. Possibly still in logins.json but not part of the logins array. Then we can compare passwords to this hopefully smallish Set in order to show indicators and we don't even need to store a date. We would have to update that Set when a new breach is added and could maybe do that in getBreachesForLogins on some interval. Due to my main concern I wonder if we then need a management UI for this set in case of false-positives?

Similar to above, it depends if you want the data to Sync and whether we are storing properties on a login or keeping a set of potentially breached passwords as I suggested above. I guess we should first decide on that. Do you agree that storing a set of potentially breached passwords is easier to maintain than a flag on each login? With my suggestion the set is only used for presentation of warnings in management UI and doesn't need to be kept in sync with other properties.

I like this idea to keep a separate set of breached saved passwords. Definitely sounds easier to maintain - updating it in getBreachesForLogins on some interval. :MattN - is there some example code that does something like this already?

In addition, Bug 1569846 will need a way to store the "dismissed" state of breach alerts for logins.

Should this Sync across devices? Storing a timestamp of when it was dismissed would probably be fine (the timestamp is necessary to handle a future breach on the same domain). If we store this on the login it's easier to sync but we also have to be careful to make sure the Sync code will merge records properly if a login changes on a device which doesn't have them (e.g. iOS, Android, old Fx desktop, etc.) and we'd probably need to ask the Sync team to help with that, at least for testing our changes.

I'll needinfo? :lnorton and the same UX folks to discuss the best UX for this in https://bugzilla.mozilla.org/show_bug.cgi?id=1569846.

Comment 3

[Matthew N. [:MattN]]

(In reply to Luke Crouch [:groovecoder] from comment #2)

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

My main concern is that we don't actually know that those passwords are breached… we only know that the saved password is older than the breach date for that site… they could have changed their password to a newer one before the breach happened but didn't save that in Firefox. I realize that it's rare for users to willingly change their password but I think it's not that rare that users forget one and use a reset flow which forces a new one. Could we let the user check the password in the HIBP data before actually telling them to change that password for all accounts they re-used it on.

I defer to product & UX on this. :betsymi, rgaddis, ssage - the only thing we know in this case is that this saved password was in a breach. What do you think is a proper UX (if any) to show in that case?

My point was that we don't know that the password which is currently saved in Firefox is the one that was in a breach, even if its date is older than the breach. The user could have changed it before the breach happened but not saved it in Firefox e.g. if they used another browser or password manager.

This is one reason why I think it may be easier to store possibly breached passwords (encrypted) in a separate data structure. Possibly still in logins.json but not part of the logins array. Then we can compare passwords to this hopefully smallish Set in order to show indicators and we don't even need to store a date. We would have to update that Set when a new breach is added and could maybe do that in getBreachesForLogins on some interval. Due to my main concern I wonder if we then need a management UI for this set in case of false-positives?

Similar to above, it depends if you want the data to Sync and whether we are storing properties on a login or keeping a set of potentially breached passwords as I suggested above. I guess we should first decide on that. Do you agree that storing a set of potentially breached passwords is easier to maintain than a flag on each login? With my suggestion the set is only used for presentation of warnings in management UI and doesn't need to be kept in sync with other properties.

I like this idea to keep a separate set of breached saved passwords. Definitely sounds easier to maintain - updating it in getBreachesForLogins on some interval. :MattN - is there some example code that does something like this already?

I think you could store the data in logins.json still so you can access it from storage-json.js using this._store.data.myVariableName… like here.
You could expose it via a method on nsILoginManagerStorage or you can add a new IDL file for this and have storage-json.js implement that new interface as well.

There is also an example of encryption in that file: https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/toolkit/components/passwordmgr/storage-json.js#599-601

If you're asking about doing something on an interval then I think the mechanism for doing that in Firefox isn't great idle-daily and won't react fast enough for this case. Doesn't RemoteSettings give us a way to be notified of updates? If so, I think it would be best to use that to update our breached password store.

Comment 4

[Luke Crouch [:groovecoder]]

My point was that we don't know that the password which is currently saved in Firefox is the one that was in a breach, even if its date is older than the breach. The user could have changed it before the breach happened but not saved it in Firefox e.g. if they used another browser or password manager.

Ah, I see. FWIW 1Password says it like this: "This website was affected by a security breach since you last changed your password. Change your password to keep your account safe." If we wanted to be the most technically correct, we could say "This website was affected by a security breach since this password was last changed. Change your password to keep your account safe."

I still defer to UX and product to make that call.

In the meantime, I'll looking into the storage-json and the Remote Settings notifications so we can land the code for this while we work out the final UX. Thanks!

Comment 5

[Betsy Mikel [:betsymi]]

Ah, I see. FWIW 1Password says it like this: "This website was affected by a security breach since you last changed your password. Change your password to keep your account safe." If we wanted to be the most technically correct, we could say "This website was affected by a security breach since this password was last changed. Change your password to keep your account safe."

Our string on breached logins is similar to the one 1pass uses: Passwords were leaked or stolen from this website since you last updated your login details. Change your password to protect your account. [Learn more about this breach]

:groovercoder, can you confirm my understanding of this bug and what we might surface to users?

• User is (likely) in website breach A and uses password X. User sees the above red prompt to change their pw and learn more about the breach
• User may or may not change that pw. It really doesn't matter for this particular use case.
• Firefox knows user also uses that same password X in websites B, C, and D where are saved in their saved logins
• You'd like to surface a yellow message on login details for websites B, C, and D that essentially says "This is an unsafe password because it's been in a breach for a different website. You should change it here, too."

I am also thinking through the legal requirements of such a message. It could seem creepy to users that Firefox "knows" their passwords. So we'd need to be clear about what we know and what we don't.

Comment 6

[Luke Crouch [:groovecoder]]

Attachment: Bug 1569847 store breached passwords as "vulnerable" r=mattn

Comment 7

[Luke Crouch [:groovecoder]]

Attachment: Bug 1569847 use wrappedJSObject until we do full IDL support

Depends on D40889

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5f7672e2d2
Store breached passwords as potentially "vulnerable". r=MattN

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d3776e4a61a
Skip browser_breachAlertDismissals.js in debug or verify. DONTBUILD

Comment 10

[Cristina Coroiu [:ccoroiu]]

Backed out 2 changesets (bug 1569847) for browser-chrome failures at browser/components/aboutlogins/tests/browser/browser_breachAlertDismissals.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa5a2f0bcdc6b747bc8ff88db9481439198b42a5

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1b5f7672e2d2be7a2e6b4b17dbc681f79c43b558

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=263491918&repo=mozilla-inbound&lineNumber=16218

This failed on opt also.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e87e49589e
Store breached passwords as potentially "vulnerable". r=MattN

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/b2e87e49589e

Comment 13

[Alexandru Michis [:malexandru]]

Backed out changeset b2e87e49589e (Bug 1569847) for causing failures in browser_breachAlertDismissals.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/7e382148d47f8f09f5395c014bb621e8f55cc0a1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=614c7ff5c423c779450d2c4f24e3cc18364deaa9&tochange=8abf2e05abaf82065731f9976f1a68fa57d4c9f0&searchStr=browser%2Cchrome

• Failure at browser_breachAlertDismissals.js:
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=263612500&repo=autoland&lineNumber=3767

• Failure at AboutLoginsParent.jsm:415:41:
  https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=263597705&repo=autoland&lineNumber=12148

Comment 14

[Matthew N. [:MattN]]

Ugh, it ended up being one extra slash in the JSM path that was the problem but it only reproduced in packaged builds so that's why it failed in automation but not locally unless you package… NS_ERROR_UNEXPECTED wasn't a descriptive error at all…

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b4a8d99dca60fdaa194f932ec2a355906c9a949

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4954c2f6267b
Store breached passwords as potentially "vulnerable". r=MattN

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4954c2f6267b