1639795 - Use a friendlier name for the OSKeyStore Keychain item

Status: VERIFIED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Matthew N. [:MattN]]

Chrome uses "Chrome Safe Storage" whereas we are restricted to org.mozilla.nss.keystore.* but this can be shown to users in some cases and would probably confuse them since "Firefox" would be at the end e.g. org.mozilla.nss.keystore.firefox. I think it would be good to make this friendlier from the get-go to avoid having to do an error-prone migration later (where a user denying access at that point would be a problem).

Filing this to track for bug 1527746, it's not a priority yet.

Comment 1

[Adam Roach [:abr]]

Matt -- do you have an STR and/or screenshot that demonstrates the circumstances under which this ends up being displayed to users?

Comment 2

[Matthew N. [:MattN]]

Attachment: Get Info on a Keychain Item

The easiest way is to delete Firefox*.app from the Keychain entry in Keychain Access.app and then trigger the dialog to appear again with extensions.formautofill.reauth.enabled = true and then editing a saved card.

The more common user STR would be:

1. Enable CC autofill and extensions.formautofill.reauth.enabled in two different channels of Firefox (two different profiles and binaries)
2. Save a credit card in one
3. Try to edit the card in the other .app

Comment 3

[Matthew N. [:MattN]]

https://searchfox.org/mozilla-central/search?q=org.mozilla.nss.keystore&path=&case=false&regexp=false are places to change

Comment 4

[Matthew N. [:MattN]]

Attachment: Get Info on a Chrome Safe Storage Keychain Item

One option could be to not enforce a prefix and allow each app to specify its own string. We may also want to use two separate strings for Account Name vs. Service Name like Chrome does.

Comment 5

[Matthew N. [:MattN]]

Attachment: What the user sees in this failure

Comment 6

[Matthew N. [:MattN]]

Then because of bug 1499241 and how macOS works you will see multiple of these prompts in a row even if you grant access with Allow. If the user uses Always Allow then both Firefox can access the same key.

Comment 8

[Adam Roach [:abr]]

Digging into this, it looks like the bulk of the changes required here are in the PSM module, which is where the "org.mozilla.nss.keystore.*" prefix is enforced. I'm tagging Dana Keeler in as the module owner (and I believe main implementor) for that module.

Dana: my specific question is: given the UX papercuts demonstrated in the screenshots above, does it make sense to keep the policy that requires key identifiers to be prefixed in this way? (Alternately: do you know of a way to display a more friendly name to the user in the dialogs shown above while maintaining the current policy?)

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I think it's fine to change how this works. If I recall correctly, the original motivation behind that prefix was to use something that nothing else would be using, but my understanding is that really only applies in the case where NSS is the backend storing the secrets (and in any case there's nothing actually stopping something else from using that label).
My only concern here is that if anything has actually used this secret storage API yet, we'll need to migrate the keys.

Comment 10

[Matthew N. [:MattN]]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #9)

My only concern here is that if anything has actually used this secret storage API yet, we'll need to migrate the keys.

Or we make that consumer add the prefix to their string. I don't know of any consumers in m-c that have shipped but maybe TB or some fork has used it… I don't think we really need to worry about them since they can easily add the old prefix back.

Comment 11

[Adam Roach [:abr]]

I did a quick run through the codebase, and agree with MattN's assessment: The six methods in OSKeyStore that use this prefix string are only currently called via IDL, and the only user of nsIOSKeyStore is OSKeyStore.jsm. That class -- or, at least its relevant methods -- are only called from FormAutofillStorage.jsm and its tests (which only use it for credit card numbers).

So, Dana, to be clear: would you be okay if I just pulled mLabelPrefix out of OSKeyStore completely and passed the caller-provided label as-is to the underlying OS-specific keystore? We know that this will break any existing stored credit cards, but since we haven't deployed the feature yet, we're willing to deal with that break.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

That sounds good to me. Please also update the documentation to make it clear that it's the caller's responsibility to not use duplicate labels (which actually could always happen, but anyway).

Comment 13

[Adam Roach [:abr]]

Attachment: Bug 1639795: Update keystore name to be user-friendly r?keeler,zbraniecki,MattN

Comment 14

[Adam Roach [:abr]]

Attachment: Screen Shot 2020-06-08 at 16.14.40 .png

Holly -- I'd like your feedback on the exact string we'll use here. My current patch calls it {productname} Encrypted Storage (so "Firefox Encrypted Storage" for release builds, "Firefox Nightly Encrypted Storage" for official Nightly builds, and "Nightly Encrypted Storage" for unbranded local builds). This appears in a couple of places in the macOS key management interface. Neither of these are part of the normal flow of adding, editing, or using a stored credit card. Users will only encounter them if they go looking in the Mac "Keychain Access" application, or if they have gone into that application and deauthorized Firefox from using the "Firefox Encrypted Storage" key.

Comment 15

[Adam Roach [:abr]]

Attachment: Screen Shot 2020-06-08 at 16.16.03 .png

Here's an example of how the string looks in the "Keychain Access" application.

Comment 16

[Adam Roach [:abr]]

Okay, there's a complicated discussion going on over on the Phabricator patch that may well change how we need to handle this, so I'm removing the ni? for now.

Comment 17

[Adam Roach [:abr]]

Attachment: Bug 1639795: Part II - "Migrate" older cards by deleting them. r?MattN

Depends on D78610

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d8e1ff06427
Update keystore name to be user-friendly r=MattN,keeler
https://hg.mozilla.org/integration/autoland/rev/69cc1de6f754
Part II - "Migrate" older cards by deleting them. r=MattN

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2d8e1ff06427
https://hg.mozilla.org/mozilla-central/rev/69cc1de6f754

Comment 20

[Cristian Fogel [:cfogel]]

Verified with 79.0b4 on macOS 10.15.5.