Encrypt credit card numbers with OS key store instead of Master Password

VERIFIED FIXED in Firefox 65

Status

()

enhancement
P1
normal
VERIFIED FIXED
8 months ago
2 months ago

People

(Reporter: timdream, Assigned: timdream)

Tracking

(Depends on 6 bugs)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 verified)

Details

(Whiteboard: [webpayments-reserve])

Attachments

(5 attachments, 1 obsolete attachment)

This patch creates SecureDataStore.jsm which has the same interface as
MasterPassword.jsm. It would redirect the function call based on the pref set.
This is an adaptor between nsIOSKeyStore.idl and MasterPassword.jsm. Most
of the detail lives in ensureLoggedIn(), which we will re-do over and over
given that the user can re-lock or delete the secret anytime from the OS
management UI.

Noted that OS Key Store has the concept of "recovery phrase" that we won't
be adopting here. The recovery phrase, together with our label, allow
the user to re-create the same key in OS key store.

Depends on D4498
These patches are based off bug 1399367. It _kinda_ of work with pref manually set to true and run a few basic mochitests.

I have a few questions in mind:

1. I don't know how I can mock the OS re-auth prompt. We don't do that in OS Key Store unit test either.

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/security/manager/ssl/tests/unit/test_oskeystore.js#120

I was thinking about adding a test pref that could overwrite reauth, but I would worry that could be a security risk (the pref that kids could flip to use parents' credit cards).

If this can be solved I would like to copy the entire formautofill test suite and make sure everything works with the pref either on or off.

2. I can see we ask for re-auth in a few different places.

https://searchfox.org/mozilla-central/search?q=decrypt(Sync%7C)%5C(.%2B%2C+true&case=true&regexp=true&path=

From the tests I can see one of which triggers when the user hits keydown to get the autofill results. This is a sub-optimal UI that I think we should avoid. Perhaps we should address them in another bug?
Priority: -- → P1
Whiteboard: [webpayments] → [webpayments-reserve]
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #3)
> I was thinking about adding a test pref that could overwrite reauth, but I
> would worry that could be a security risk (the pref that kids could flip to
> use parents' credit cards).

Here is an idea. What if the pref only exists in non-offical and Nightly builds? Would that be acceptible? The tests would just skip itself if the testing pref doesn't exist or flipped to false.
Another design issue I have is on the value of |isEnabled| property. I am not sure the best way to map that property to OS key store. It could affect behaviors like bug 1394075.
I tested my local patch a bit. There are various places where we should at least distinguish the case between decryption error v.s. user canceling the login dialog, and handle the former better.

Decryption error can happen when:

1) autofill-profiles.json corruption

for master password encryption, but it could happen to OS key store encryption because of:

2) User lost of their OS key store.
3) User moving their Firefox profile from one OS user profile or computer to another.

I am assuming that migration data from master password to OS key store is not in the scope of this bug. We should decide if we want to do that for Nightly user or not later.
Attachment #9004723 - Attachment description: Bug 1486954 - Allow switch between secure store → Bug 1486954 - Allow switch between secure stores
Attachment #9004723 - Attachment description: Bug 1486954 - Allow switch between secure stores → Bug 1486954 - Part I, Allow switch between secure stores
Attachment #9004736 - Attachment description: Bug 1486954 - Create OSKeyStore.jsm → Bug 1486954 - Part II, Create OSKeyStore.jsm
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> Here is an idea. What if the pref only exists in non-offical and Nightly
> builds? Would that be acceptible? The tests would just skip itself if the
> testing pref doesn't exist or flipped to false.

I decided to disable the pref even if NIGHTLY is true when MOZILLA_OFFICIAL=true. The tests as it currently written will not be run on our CIs since our builds there come with MOZILLA_OFFICIAL=true.
Flags: qe-verify+
QA Contact: hani.yacoub
I am still tweaking the tests to make sure they pass and don't interfere with MP tests.

Matt suggests that I should check whether the re-authentication parameter actually triggers an OS login dialog on all platforms and adjust the tests accordingly.
(In reply to Tim Guan-tin Chien [:timdream] from comment #6)
> I am assuming that migration data from master password to OS key store is
> not in the scope of this bug. We should decide if we want to do that for
> Nightly user or not later.

I think that handling that (or not) should be in-scope as I don't think we should support both old and new encryption methods in the meantime,
Attachment #9004736 - Attachment is obsolete: true
Attachment #9004723 - Attachment description: Bug 1486954 - Part I, Allow switch between secure stores → Bug 1486954 - Part I, Encrypt credit card numbers with OS key store
Attachment #9006704 - Attachment description: Bug 1486954 - Part III, Recover from decryption errors in some places → Bug 1486954 - Part II, Recover from decryption errors in some places
Given that the new store is always considered enabled, the not-enabled code
is now dead code. This patch removes them.

Depends on D5083
Patch updated; The patch right now doesn't do comment 9, i.e. tweaking tests on other OSes, so it's more like feedback? than review? right now. Feel free to send back if you would like to wait for me complete that.
Bad news:

I tried the patch on Windows 10 and Linux, and it appears asyncLock() and asyncUnlock() does not trigger an OS auth dialog. This means we will no longer ask for login on places that we had asked for master password, e.g. credit card management dialog, confirm filling of credit card # on the form.

I am not sure about this though given that the test here https://searchfox.org/mozilla-central/source/security/manager/ssl/tests/unit/test_oskeystore.js suggests otherwise... let me check.
OK. So for Linux it is using the NSS impl so if a master password is set, it will prompt for that. False alarm.

Let me check Windows again.
And yeah... asyncLock() doesn't trigger OS auth dialog in Windows.
Whiteboard: [webpayments-reserve] → [webpayments]
Whiteboard: [webpayments] → [webpayments-reserve]
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13)
> Bad news:
> 
> I tried the patch on Windows 10 and Linux, and it appears asyncLock() and
> asyncUnlock() does not trigger an OS auth dialog.

https://hg.mozilla.org/mozilla-central/rev/b5399f9627bc#l1.51

Over bug 1478668, "The Windows credential manager can't be locked.". lol.

We would need a decision here on if this is what we wanted for Windows users.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e81864c511d133f4a385d040f505be83f216b690

Looks like this needs more adjustments.
Comment on attachment 9008571 [details]
Bug 1486954 - Part II, Remove OSKeyStore.isEnabled

Matthew N. [:MattN] (PM me if requests are blocking you) has approved the revision.
Attachment #9008571 - Flags: review+
Mark, see https://phabricator.services.mozilla.com/D5083#160306 please. Is there an easy way to delete saved cards but have Sync bring them back? Does that mean just deleting the record from autofill storage without leaving a tombstone or do we have to update state in the autofill sync JSM too? I guess we would need to trigger a sync and/or mark as dirty too.
Flags: needinfo?(markh)
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #20)
> Mark, see https://phabricator.services.mozilla.com/D5083#160306 please. Is
> there an easy way to delete saved cards but have Sync bring them back? Does
> that mean just deleting the record from autofill storage without leaving a
> tombstone or do we have to update state in the autofill sync JSM too? I
> guess we would need to trigger a sync and/or mark as dirty too.

To make this work, we'll want to call `async resetClient()` on the engine - this acts as though the user disconnected and reconnected sync, meaning it will download all records and compare them to the records it already has - ie, a "first sync" of a new device. Note however that this deletes all sync metadata for all records, so if there are other pending changes to be synced, they'll be lost. This process doesn't seem to kill tombstones though, so it would be important to ensure that tombstones aren't left behind or we may treat the local deletion as more recent and sync that back up. You'd also want to make sure the observers don't fire for these changes, or sync will immediately start.

The process for doing this isn't that straight forward, but the code could look something like:

async function blah() {
  let xps = Cc["@mozilla.org/weave/service;1"]
            .getService(Ci.nsISupports)
            .wrappedJSObject;
  if (!xps.enabled) {
    return; // sync's disabled.
  }
  await xps.whenLoaded();
  let {Weave} =  ChromeUtils.import("resource://services-sync/main.js", {});
  let engine = Weave.Service.engineManager.get("creditcards");
  if (!engine || !engine.enabled) {
    return; // CC engine isn't enabled.
  }
  await engine.resetClient();
  // sync now in the background.
  Weave.Service.sync({"why": "cc-migration"}).catch(er => Cu.reportError("failed to sync, er));
}

But note that this is probably only safe if done quite soon after startup (ie, while xps.ready returns false), otherwise we need to deal with the fact that it may already be mid-sync for this engine - you'd probably need to poll for Weave.Service.lock() to return true - after it does, sync will not again start until it is unlocked. I guess to be truly reliable, you'd probably want to perform the entire deletion process while sync was locked, or there's a tiny chance sync will, for example, race to add a new remote record in between you deleting them and resetting sync.

Let me know if you need any more info.
Flags: needinfo?(markh)
Attachment #9006704 - Attachment is obsolete: true
Attachment #9008571 - Attachment description: Bug 1486954 - Part III, Remove OSKeyStore.isEnabled → Bug 1486954 - Part II, Remove OSKeyStore.isEnabled
MattN and I decided its best that we migrate the record in the storage, instead of relying on Sync to get the data back. We however agreed on not migrating the records if the user has MasterPassword turned on, just to avoid having the user to see the prompt on start-up (and having to handle the case when user cancels)

Here is the oppotunity in code that I think its doable:

1. When the FormAutofillStor Nightly initialized, it calls into _migrateRecord, and when the version mismatches, it will call into _stripComputedFields(record)

https://searchfox.org/mozilla-central/rev/807a37c670c093b6e5201841a7c5315ba67ba8d5/browser/extensions/formautofill/FormAutofillStorage.jsm#1186-1199

2. In _stripComputedFields(), we would have the oppotunity to do one-off decryption of the CC numbers previously encrypted by master password.

https://searchfox.org/mozilla-central/rev/807a37c670c093b6e5201841a7c5315ba67ba8d5/browser/extensions/formautofill/FormAutofillStorage.jsm#1616-1621

I will provide a patch for this.
Attachment #9006704 - Attachment is obsolete: false
Attachment #9006704 - Attachment description: Bug 1486954 - Part II, Recover from decryption errors in some places → Bug 1486954 - Part III, Recover from decryption errors in some places
Attachment #9006704 - Attachment description: Bug 1486954 - Part III, Recover from decryption errors in some places → Bug 1486954 - Part IV, Recover from decryption errors in some places
Comment hidden (obsolete)
Comment hidden (obsolete)
(In reply to Mark Hammond [:markh] from comment #21)

Thanks Mark. Since that seems like it has a bunch of caveats, how bad would it be if we just cleared `data.creditCards` in `_dataPostProcessor` if the file version was old? https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/browser/extensions/formautofill/FormAutofillStorage.jsm#1813

We don't care about Nightly users losing their saved credit cards but don't want to leave Sync in an inconsistent state. The current approach on Phabricator to migrate each record seems a bit more complicated than I was hoping for a migration that only affects Nightly.

Since the sync record metadata is stored on the records I guess the only issue is metadata not part of the records like services.sync.creditcards.* prefs. 

I'm really just looking for the easiest way to not mess up sync (either preserving or nuking the data) for the beta/nightly users who have had credit card Sync enabled (it was removed from Beta a while ago). Ideally in a way that doesn't depend on knowing about sync locks, readiness, etc. Any suggestions?
Flags: needinfo?(markh)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #27)
> The published patches should be free of test timeouts.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ad32528a1fa14805ffff82ab5f1288fc42ffceca

Review comments addressed. I can see one failure here

https://treeherder.mozilla.org/#/jobs?repo=try&author=timdream@gmail.com&selectedJob=205124581

not sure if this is an intermittent orange or not. I guess we will inherit intermittent failure from bug 1480199 so as long as it's not worse than that I will try to land this bug.
Ah, I guess I should wait for Mark to clarify comment 26.
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #26)
> (In reply to Mark Hammond [:markh] from comment #21)
> 
> Thanks Mark. Since that seems like it has a bunch of caveats, how bad would
> it be if we just cleared `data.creditCards` in `_dataPostProcessor` if the
> file version was old?

I guess it depends on whether you are happy to ignore those caveats. If you remove that data and reset the pref 'services.sync.creditcards.lastSync' is likely to do the right thing, except for those caveats - eg, if you do that during a sync, it's possible it will not do what you expect (ie, that sync will never recover that data)
Flags: needinfo?(markh)
I've previously discussed with Matt on not bumping the schema version (instead, tag the ciphertext w/ versions) so there won't be any complications with sync. We ended decide not to go with that. We may revisit that if there is too much unknown here.
Attachment #9004723 - Attachment description: Bug 1486954 - Part I, Encrypt credit card numbers with OS key store → Bug 1486954 - Part I, (Nighty-only feature) Encrypt credit card numbers with OS key store
We're landing this during the soft code freeze as we don't think it fits the criteria since Credit Card storage is only used on Nightly (not riding the trains).

Comment 34

6 months ago
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96a6e1ceb697
Part I, (Nighty-only feature) Encrypt credit card numbers with OS key store r=MattN
https://hg.mozilla.org/integration/autoland/rev/87e64652386d
Part II, Remove OSKeyStore.isEnabled r=MattN
https://hg.mozilla.org/integration/autoland/rev/27e9286503e8
Part III, Upgrade existing Nightly credit card records to OSKeyStore r=MattN
https://hg.mozilla.org/integration/autoland/rev/c895888bdddc
Part IV, Recover from decryption errors in some places r=MattN

Updated

6 months ago
Depends on: 1499766
Backed out 4 changesets (Bug 1486954) for hangs on Linux. a=backout

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=busted%2Ctestfailed%2Cexception%2Cusercancel%2Crunnable&revision=831b8aa6281e5e200d92be09a519d17f9367f5ca
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla64 → ---
Status: REOPENED → ASSIGNED
We had to disable it on linux because of bug 1486954.

Changes had made in initialization so that we don't call into credit card
AutofillRecords class.
Always set the pref to true in payment tests, also, switch the store name there (in Part I).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fca2570bbc4e4624d7bdd6df75588a22635199a1
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #40)
> also, switch the store name there (in Part I).

This is a pretty sad getcha given that without it we are cleaning up developers' Firefox credit card every time they run the tests!
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #41)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #40)
> > also, switch the store name there (in Part I).
> 
> This is a pretty sad getcha given that without it we are cleaning up
> developers' Firefox credit card every time they run the tests!

Could you elaborate? I guess you were talking about a problem in an earlier version of your commit, not in m-c? I assume you're talking about deleting the encryption key? Or are you talking about deleting saved cards?
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #42)
> Could you elaborate? I guess you were talking about a problem in an earlier
> version of your commit, not in m-c? I assume you're talking about deleting
> the encryption key? Or are you talking about deleting saved cards?

We are switching the key store name to a generated one in OSKeyStoreTestUtils.setup(). I made sure all test suites in FormAutofill call that, but I realized I didn't do it in payment browser chrome tests, which also add/remove credit cards.

But you are right, not switching the store name simply makes the profiles sharing the key, not the credit card records. Sorry about the noise. Context switch failure on my part.

The actual danger would be, if OSKeyStoreTestUtils.cleanup() is called without switching store name -- in that case we will be deleting the os key needed to decrypt developers' Firefox credit card records.
Attachment #9018428 - Attachment description: Bug 1486954 - Part V, Disable credit card fill on Linux and prevent it from initialize r=MattN → Bug 1486954 - Part V, Prevent credit card record from being accessed if not initialize r=MattN

Comment 44

6 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/024dfb5b2d67
Part I, Encrypt credit card numbers with OS key store. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/21162d81c6d8
Part II, Remove OSKeyStore.isEnabled. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/91cb3b099316
Part III, Upgrade existing Nightly credit card records to OSKeyStore. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c7e2bc2d63
Part IV, Recover from decryption errors in some places. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/740ead88dd20
Part V, Prevent credit card record from being accessed if not initialize r=MattN

Comment 45

6 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98061ccbb157
Fix browser/components/payments/test/browser/head.js eslint issue. r=me

Comment 47

6 months ago
I went through the comments, but I didn't know exactly how we could verify this bug manually?
Do you have any ideas what areas should we cover more exactly?
Flags: needinfo?(MattN+bmo)
I'll let Matt answer, but perhaps check that nothing changes (asides Master Password dialog no longer pops up)?
Yeah, just make sure autofill and web payments interactions with credit cards still work fine.
Flags: needinfo?(MattN+bmo)

Updated

6 months ago
Depends on: 1502082

Comment 50

6 months ago
I verified that Autofill and Web Payments interactions with credit cards work fine, and Master Password dialog no longer pops up on Firefox Nightly 65.0a1 (2018-10-25) on Windows 10 x 64, Mac OS X 10.13 and on Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.