Closed Bug 1213510 Opened 9 years ago Closed 5 years ago

By default, hide the Sync credentials in the list of visible saved passwords

Categories

(Toolkit :: Password Manager, defect, P5)

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox69 --- disabled
firefox70 --- fixed

People

(Reporter: ckarlof, Unassigned)

References

Details

(Whiteboard: [passwords:management] [fxsync])

This user was a bit freaked out seeing it there: https://bugzilla.mozilla.org/show_bug.cgi?id=1202298#c5 Hiding them would also mitigate against users accidentally deleting the credentials. For debugging purposes, we could have an about:config flag to show them.
Flags: firefox-backlog+
I think this belongs in the password manager component. Arguably, it might be reasonable to simply hide sites starting with chrome://. Dolske, have I got the component right, and how do you think we should approach this?
Component: Sync → Password Manager
Flags: needinfo?(dolske)
Product: Firefox → Toolkit
Whiteboard: [fxsync]
I don't have a strong opinion on hiding Sync, since it's kind of special as being built into Firefox. I'm a little dubious about hiding everything chrome://, because you'd have no way of deleting/changing credentials from other random addons (especially after they're uninstalled). Maybe there's some value in considering some kind of UI for splitting "web logins" from "addon logins"?
Flags: needinfo?(dolske)
Whiteboard: [fxsync] → [passwords:management] [fxsync]

I would rather we moved the storage of the sync key somewhere else and use nsIOSKeyStore for encryption. Maybe it can be stored in signedInUser.json? That would also avoid the master password prompts. If the sync team wants to run with that feel free to move the bug back out of pwmgr. nsIOSKeyStore is only implemented for desktop though so that wouldn't work for Android but I don't know if that matters since Sync is implemented as a system service on Android.

There may still be some kinks to work out with nsIOSKeyStore since we didn't get around to shipping credit card storage past Nightly yet. I wouldn't say this path is 100% clear but I think it's the right direction.

Mark, what do you think about that?

Flags: needinfo?(markh)
Priority: P2 → P3

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

Matt and I had a bit of a chat in slack. I think the first problem we need to address is what problem was being solved by storing stuff in the login manager in the first place - we did this work in bug 1013064.

We are trying to protect against a local attacker - someone who has some level of access to a local Firefox install. The idea is: if local passwords are protected by a password, then it should be impossible to convince sync to download and decrypt those passwords from the server without knowing that password. So our solution was to put the sync keys in the login manager - the keys are only available when the master-password is unlocked - and when unlocked, all local passwords are also available.

So the end result is:

  • master-password locked means local passwords are not available, and keys needed to sync are not available.
  • master-password unlocked (or known) means both local passwords and sync keys are available locally - a local attacker would just extract the passwords directly from the local store rather than needing to go via the sync keys.

So at this stage, I'm not sure that using nsIOSKeyStore would meet our requirements - while the data would be encrypted, it's not encrypted using the master-password, but instead via an OS password. Currently, an attacker knowing the OS password but not the master-password still can't get access to the local passwords or the sync key.

So ultimately, it's seems important that we use the same encryption and protection mechanism for our sync keys as used for locally stored passwords. I'm not sure if there are any plans to change how local passwords are managed.

What probably would work (although I'm not sure how practical it is) is to arrange for the existing master-password/keyring code to do the encryption and decryption for us, but not use the login manager for storage - eg, we could store the encrypted data on disk, but we'd still end up with the same basic UX - a locked master password means no syncing.

(Another alternative, but which sounds like a can-of-worms from a security-review perspective, is to argue that having local passwords and the sync key behind 2 different passwords is ok)

Matt, does that make sense to you, and if so, do you have any suggestions or thoughts?

Thanks!

Flags: needinfo?(markh) → needinfo?(MattN+bmo)

A fly in this ointment is credit-cards. IIUC, they will be protected by nsIOSKeyStore - so we will be in a position that there are effectively 2 different passwords protecting two different types of valuable data exposed by Sync. I'm not sure what to make of this.

OK, that makes sense and differs from my my thinking and complicates a solution. We aren't shipping credit card autofill support now (bug 1521194) btw.

Flags: needinfo?(MattN+bmo)

Note that the Lockbox extension will hide all chrome:// logins so this will be handled there anyways. We aren't working on the current management UI now.

Priority: P3 → P5

Fixed by about:logins

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.