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

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, enhancement, P1)

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1486954 - Part I, (Nighty-only feature) Encrypt credit card numbers with OS key store

This patch creates SecureDataStore.jsm which has the same interface as
MasterPassword.jsm. It would redirect the function call based on the pref set.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1486954 - Part II, Create OSKeyStore.jsm

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

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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?

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1486954 - Part IV, Recover from decryption errors in some places

Depends on D4500

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 10

[Matthew N. [:MattN]]

(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,

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1486954 - Part II, Remove OSKeyStore.isEnabled

Given that the new store is always considered enabled, the not-enabled code
is now dead code. This patch removes them.

Depends on D5083

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

And yeah... asyncLock() doesn't trigger OS auth dialog in Windows.

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Patch updated to include reauth tests with master password prompt on Linux.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e81864c511d133f4a385d040f505be83f216b690

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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 19

[Matthew N. [:MattN]]

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.

Comment 20

[Matthew N. [:MattN]]

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.

Comment 21

[Mark Hammond [:markh] [:mhammond]]

(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.

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 23

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1486954 - Part III, Upgrade existing Nightly credit card records to OSKeyStore

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am having trouble update the existing diff because it complains about cycle dependencies. It looks like I cannot reorder the patches in both Phabricator and local branch with histedit and expect them to work. Fun.

https://github.com/mozilla-conduit/review/blob/b358c59af9f3a5ddf200c0a8597e4d999ea8fbdd/moz-phab#L40-L45

This leaves me no choice but to abandon the existing changesets.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

... and apparently it worked after I removed the "Depend on ..." line in the message on Phabricator. So Phabricator recognizes both the commit message AND the related parent/child revisions...

Comment 26

[Matthew N. [:MattN]]

(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?

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The published patches should be free of test timeouts.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad32528a1fa14805ffff82ab5f1288fc42ffceca

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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=[e-mail]&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.

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ah, I guess I should wait for Mark to clarify comment 26.

Comment 30

[Mark Hammond [:markh] [:mhammond]]

(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)

Comment 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

More review comment addressed

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6e9852910f04f65122a29d50d87da606df8fe08

Comment 33

[Matthew N. [:MattN]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 35

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/96a6e1ceb697
https://hg.mozilla.org/mozilla-central/rev/87e64652386d
https://hg.mozilla.org/mozilla-central/rev/27e9286503e8
https://hg.mozilla.org/mozilla-central/rev/c895888bdddc

Comment 36

[Eliza Balazs [:ebalazs_]]

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

Comment 37

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1486954 - Part V, Prevent credit card record from being accessed if not initialize r=MattN

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.

Comment 38

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=870e41f69556b588a3fc3edadc1ebc325985f319&selectedJob=206468877

Comment 39

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f22e37d84daaa2828a262115fc4d305366c45802

Comment 40

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 41

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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!

Comment 42

[Matthew N. [:MattN]]

(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?

Comment 43

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 44

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98061ccbb157
Fix browser/components/payments/test/browser/head.js eslint issue. r=me

Comment 46

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/024dfb5b2d67
https://hg.mozilla.org/mozilla-central/rev/21162d81c6d8
https://hg.mozilla.org/mozilla-central/rev/91cb3b099316
https://hg.mozilla.org/mozilla-central/rev/f5c7e2bc2d63
https://hg.mozilla.org/mozilla-central/rev/740ead88dd20
https://hg.mozilla.org/mozilla-central/rev/98061ccbb157

Comment 47

[Hani Yacoub, Desktop QA]

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?

Comment 48

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I'll let Matt answer, but perhaps check that nothing changes (asides Master Password dialog no longer pops up)?

Comment 49

[Matthew N. [:MattN]]

Yeah, just make sure autofill and web payments interactions with credit cards still work fine.

Comment 50

[Hani Yacoub, Desktop QA]

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.