Closed Bug 1486954 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Form Autofill, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox65 --- verified

People

(Reporter: timdream, Assigned: timdream)

References

(Depends on 4 open bugs)

Details

(Whiteboard: [webpayments-reserve])

Attachments

(5 files, 1 obsolete file)

No description provided.
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.
Depends on: 1494478
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+
Blocks: 1481971
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
Blocks: 1429265
(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)
Depends on: 1498403
(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.
Depends on: 1499241
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).
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
Depends on: 1499766
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.
Depends on: 1500893
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
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
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
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)
Depends on: 1502082
Depends on: 1502173
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+
Depends on: 1515970
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: