Closed Bug 1337314 Opened 7 years ago Closed 7 years ago

[Form Autofill] Handle credit card number encryption while normalizing

Categories

(Toolkit :: Form Manager, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: steveck, Assigned: steveck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M4][ETA:7/28])

Attachments

(1 file, 2 obsolete files)

Ideally we should leverage the logic applied in login manager, no matter it's encrypt w/wo master password. We could reference crypto-SDR.js[1] to access the encrypt/decrypt API if user have master password, or doing some check like passwordManager did[2] if user didn't set.

[1]https://dxr.mozilla.org/mozilla-central/rev/af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177/toolkit/components/passwordmgr/crypto-SDR.js
[2]https://dxr.mozilla.org/mozilla-central/rev/af8a2573d0f1e9cc6f2ba0ab67d7a702a197f177/toolkit/components/passwordmgr/content/passwordManager.js#701
It's just a minor note that bug 1271851 might change the behavior in crypto-SDR. It's unlikely to use modern crypto and keep the original master password behavior if we don't decouple master password from PSM the in bug 1271851 first.
Whiteboard: [form autofill:MVP] → [form autofill:M3]
We could also reference the password manager's storage[1] to figure out how it works.

[1] https://dxr.mozilla.org/mozilla-central/rev/0b77ed3f26c5335503bc16e85b8c067382e7bb1e/toolkit/components/passwordmgr/storage-json.js
We had a meeting to discuss Master Password behaviour and decided that we won't filter credit card numbers and will show unmasked credit card numbers for users with a master password set (regardless of whether it's logged in or not). Upon selecting a row to fill a credit card, the MP dialog will be forced to appear and upon successful login the credit card will be filled. Users without a master password will have the whole CC number previewed and will support live-filtering of credit card numbers in autocomplete.
Whiteboard: [form autofill:M3] → [form autofill:M4]
Assignee: nobody → schung
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:7/21]
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review163268

::: browser/extensions/formautofill/FormAutofillUtils.jsm:304
(Diff revision 3)
> +  /**
> +   * Check if masterpassword is set.
> +   *
> +   * @returns {boolean} True if masterpassword is set and false if it isn't.
> +   */
> +  get masterPasswordEnabled() {

It's mainly for preferences subdoalog that it'll need to know whether "show number" button should be displayed or not.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:330
(Diff revision 3)
> +    if (!forceRelogin && token.isLoggedIn()) {
> +      return true;
> +    }
> +
> +    // If master password is disabled
> +    if (!this.masterPasswordEnabled) {

I'm not quite sure placing this checking inside the `masterPasswordLogin` is a good idea(although passwordmgr did the same)... The good thing is caller won't need to know that the mp is set or not, but mp is not logged in(token.isLoggedIn will return false) if mp is not set. I'm worry that some people might be misled that masterPasswordLogin return true means token is logged in.

::: browser/extensions/formautofill/test/unit/test_masterPasswordUtils.js:20
(Diff revision 3)
> +  description: "Without masterpassword set",
> +  masterpassword: "", // "" means no masterpassword
> +  mpEnabled: false,
> +}];
> +
> +add_task(async function test_masterPasswordLogin() {

I can't find a way to verify the masterPasswordLogin with prompt displayed cases because it'll return error directly in the test. I guess we could only verify this in mochitest.
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review163898

I remember that Matt once said we should avoid using the modules of "login-manager" so we won't be affected by any refactoring plan of that. Could you try to use `nsISecretDecoderRing` directly for encryption and decryption?
Attachment #8886550 - Flags: review?(lchang)
(In reply to Luke Chang [:lchang] from comment #12)
> Comment on attachment 8886550 [details]
> Bug 1337314 - Part 1: Implement masterpassword related functions in utils.
> 
> https://reviewboard.mozilla.org/r/157366/#review163898
> 
> I remember that Matt once said we should avoid using the modules of
> "login-manager" so we won't be affected by any refactoring plan of that.
> Could you try to use `nsISecretDecoderRing` directly for encryption and
> decryption?

Hi Matt, if we can make sure that using nsISecretDecoderRing directly for encryption/decryption always after token logged in, could we assume that we could control all the prompt UI in utils masterPasswordLogin? But I think it might have a problem if we want to handle the prompt all by ourselves because of multiple mp prompt case. It looks like both login manager and FormUtils will need to fire/observe the UI state changes. Would you think it would be more reasonable that moving crypto out of password manager for general mp login purpose, since it is already in Services.logins?
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review165256

::: browser/extensions/formautofill/FormAutofillUtils.jsm:348
(Diff revision 6)
> +   * Display the masterPassword login prompt no matter it's logged in or not.
> +   *
> +   * @returns {boolean} Ture if it's logged in or no masterPassword set and false
> +   *                    if it's still not logged in(prompt canceled or other error).
> +   */
> +  masterPasswordRelogin() {

This part is synced from password manager masterPasswordLogin, but I'm not sure why it didn't check prompt UI busy like login manager. Do you think it's fine without waiting for other prompt if it's forced login?
Whiteboard: [form autofill:M4][ETA:7/21] → [form autofill:M4][ETA:7/28]
Comment on attachment 8886976 [details]
Bug 1337314 - Encrypt card number while normallizing field.

https://reviewboard.mozilla.org/r/157734/#review165638
Attachment #8886976 - Flags: review?(lchang) → review+
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review166012

::: browser/extensions/formautofill/FormAutofillUtils.jsm:302
(Diff revision 6)
> +  _decoderRing: null,
> +  _utfConverter: null,

Do we still need these two?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:397
(Diff revision 6)
> +   * @param   {boolean} forceRelogin True if we want to force the prompt shows up
> +   *                    no matter it's logged in or not.
> +   * @returns {string} encrypted string, or throws an exception if there was a
> +   *                   problem.
> +   */
> +  encrypt(plainText, forceRelogin = false) {

I can't think of any case that we want to force it to relogin while doing the encryption. I prefer to make it simple until we need it.

::: browser/extensions/formautofill/test/unit/test_masterPasswordUtils.js:36
(Diff revision 6)
> +    token.initPassword(testcase.masterpassword);
> +    FormAutofillUtils.initCrypto();
> +    ok(FormAutofillUtils._decoderRing);
> +    ok(FormAutofillUtils._utfConverter);
> +
> +    token.login(/* force */ false);

Could you comment on what `/* force */ false` is for?
Attachment #8886550 - Flags: review?(lchang)
Comment on attachment 8888741 [details]
Bug 1337314 - Part 0: Implement waitForMasterPasswordDialog for multiple dialog case.

https://reviewboard.mozilla.org/r/159772/#review167224

::: toolkit/components/passwordmgr/LoginHelper.jsm:723
(Diff revision 2)
> +  async waitForMasterPasswordDialog() {
> +    return new Promise((resolve) => {
> +      log.debug("Deferring the MasterPassword dialog because ui is still busy.");

Hmm… in my mind this method would include the uibusy check so consumers never needed to know about it. Then if uibusy was true this method would re-focus the MP dialog. I'll take a closer look at this later though
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review167218

Here are some quick comments

::: browser/extensions/formautofill/FormAutofillUtils.jsm:311
(Diff revision 7)
> +  /**
> +   * Initialize the token/decoderRing/utfConverter for encryption/decryption.
> +   *
> +   */
> +  initCrypto() {
> +    if (this._token.needsUserInit) {
> +      log.debug("Initializing key3.db with default blank password.");
> +      this._token.initPassword("");
> +    }
> +  },
> +
> +  get _token() {
> +    let tokendb = Cc["@mozilla.org/security/pk11tokendb;1"]

Did you consider putting all the crypto methods on a nested object/namespace just to keep some more organization. e.g. FormAutofillUtils.crypto.init

The crypto property could be an alias to top-level Crypto object in the file.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:315
(Diff revision 7)
> +  initCrypto() {
> +    if (this._token.needsUserInit) {
> +      log.debug("Initializing key3.db with default blank password.");
> +      this._token.initPassword("");
> +    }
> +  },

Personally I would prefer if more of this was outside of autofill code to be shared with sync and pwmgr but I guess that affects landing/uplift.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:338
(Diff revision 7)
> +  get masterPasswordEnabled() {
> +    return this._token.hasPassword;
> +  },
> +
> +  /**
> +   *

Nit: Delete the empty line

::: browser/extensions/formautofill/FormAutofillUtils.jsm:373
(Diff revision 7)
> +    return token.isLoggedIn();
> +  },
> +
> +  /**
> +   *
> +   * Decrypts stringthat references method in crypto-SDR module(without ui logic control).

missing space
Comment on attachment 8888741 [details]
Bug 1337314 - Part 0: Implement waitForMasterPasswordDialog for multiple dialog case.

https://reviewboard.mozilla.org/r/159772/#review168590

Clearing review for now since I think the helper should handle checking uiBusy.
Attachment #8888741 - Flags: review?(MattN+bmo)
Attachment #8886976 - Flags: review+ → review?(lchang)
Comment on attachment 8888741 [details]
Bug 1337314 - Part 0: Implement waitForMasterPasswordDialog for multiple dialog case.

https://reviewboard.mozilla.org/r/159772/#review168702

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:198
(Diff revision 4)
>        return;
>      }
>  
>      // If we're currently displaying a master password prompt, defer
>      // processing this form until the user handles the prompt.
>      if (Services.logins.uiBusy) {

Even I moved the uiBusy checking in the helper, I still keep the checking because the behavior is still different in ui busy/idle. For example, even if it's not logged in yet, it will trigger searchAndDedupeLogins instead of returning the empty logins directly if showMasterPassword is true and ui is idle. If we don't check the ui state and only wait for the login state, we'll miss this part.
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review168720

::: browser/extensions/formautofill/FormAutofillUtils.jsm:90
(Diff revision 10)
> +   *                    no matter it's logged in or not.
> +   * @returns {string} decrypted string, or throws an exception if there was a
> +   *                   problem.
> +   */
> +  async decrypt(cipherText, forceRelogin = false) {
> +    if (Services.logins.uiBusy) {

Here I still check the ui state before waitForMasterPasswordDialog. I think we should not show the prompt again if user just logged in from previous prompt, even it's forceRelogin case. wdyt?
Comment on attachment 8886976 [details]
Bug 1337314 - Encrypt card number while normallizing field.

https://reviewboard.mozilla.org/r/157734/#review169124

::: browser/extensions/formautofill/ProfileStorage.jsm:1442
(Diff revision 10)
>  }
>  
>  class CreditCards extends AutofillRecords {
>    constructor(store) {
>      super(store, "creditCards", VALID_CREDIT_CARD_FIELDS, VALID_CREDIT_CARD_COMPUTED_FIELDS, CREDIT_CARD_SCHEMA_VERSION);
> +    FormAutofillUtils.crypto.init();

Is it possible to delay the `init` until we need it, e.g. put it in `normalizeCCNumberFields`.

::: browser/extensions/formautofill/ProfileStorage.jsm:1465
(Diff revision 10)
>  
>      return hasNewComputedFields;
>    }
>  
>    _normalizeFields(creditCard) {
> -    // Fields that should not be set by content.
> +    // Check if cc-number is normalized.

nit: It would be clearer to explicitly mention `normalizeCCNumberFields` should be called first.

::: browser/extensions/formautofill/ProfileStorage.jsm:1508
(Diff revision 10)
>        }
>      }
>    }
> +
> +  /**
> +   * Normalize credit card number related field for saving.

nit: Mention that this function should always be called before adding/updating credit card records.

::: browser/extensions/formautofill/test/unit/test_transformFields.js:484
(Diff revision 10)
>    // Empty
>    {
>      description: "Empty credit card",

nit: The description needs to update as it's not empty now. e.g. "No normalizable field".
Attachment #8886976 - Flags: review+
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review170664

::: browser/extensions/formautofill/FormAutofillUtils.jsm:22
(Diff revision 10)
> +XPCOMUtils.defineLazyModuleGetter(this, "LoginHelper",
> +                                  "resource://gre/modules/LoginHelper.jsm");
> +
> +let Crypto = {
> +  /**
> +   * Initialize the token/decoderRing/utfConverter for encryption/decryption.

nit: `decoderRing` and `utfConverter` are no longer initialized here, aren't they?
Attachment #8886550 - Flags: review?(lchang) → review+
Depends on: 1388238
Comment on attachment 8888741 [details]
Bug 1337314 - Part 0: Implement waitForMasterPasswordDialog for multiple dialog case.

https://reviewboard.mozilla.org/r/159772/#review170904

I moved this commit to bug 1388238 since I think we should start it as MasterPassword.jsm as we thought about before.

::: toolkit/components/passwordmgr/LoginHelper.jsm:726
(Diff revision 4)
> +   *          or false if canceled.
> +   */
> +  async waitForMasterPasswordDialog() {
> +    return new Promise((resolve) => {
> +      if (!Services.logins.uiBusy) {
> +        log.debug("Return current login state directly since mp ui is not busy.");

I think this log message is a bit confusing and long. How about:
```js
log.debug("waitForMasterPasswordDialog: Dialog isn't showing. isLoggedIn:", Services.logins.isLoggedIn)
```

::: toolkit/components/passwordmgr/LoginHelper.jsm:731
(Diff revision 4)
> +        log.debug("Return current login state directly since mp ui is not busy.");
> +        resolve(Services.logins.isLoggedIn);
> +        return;
> +      }
> +
> +      log.debug("Deferring the MasterPassword dialog because ui is still busy.");

```js
log.debug("waitForMasterPasswordDialog: Observing the  open dialog");
```

::: toolkit/components/passwordmgr/LoginHelper.jsm:737
(Diff revision 4)
> +      let observer = {
> +        QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
> +                                               Ci.nsISupportsWeakReference]),
> +
> +        observe(subject, topic, data) {
> +          log.debug("Got notification from MasterPassword dialog:", topic);

```js
log.debug("waitForMasterPasswordDialog: Got notification:", topic);
```

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:199
(Diff revision 4)
> -      log("deferring sendLoginDataToChild for", formOrigin);
> -      let self = this;
> +      let isLoggedIn = await LoginHelper.waitForMasterPasswordDialog();
> +      if (!isLoggedIn) {

Since we don't have good MP test coverage at the moment due to bug 1269039 I change my mind and think we should reduce our risk for autofill by leaving LoginManagerParent.jsm alone for now and filing a follow-up pwmgr bug to use your new helper. That new bug should depend on bug 1269039.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 4)
> -          self.sendLoginDataToChild(showMasterPassword, formOrigin, actionOrigin,
> -                                    requestId, target);

Just to double-check, did you intentionally delete this line? I thought you were saying you didn't want to unify the code paths at this time?
Attachment #8888741 - Flags: review?(MattN+bmo)
Comment on attachment 8886550 [details]
Bug 1337314 - Part 1: Implement masterpassword related functions in utils.

https://reviewboard.mozilla.org/r/157366/#review171008

I also moved this to bug 1388238 updated with renames. Please check if it's fine.
Attachment #8886550 - Flags: review?(MattN+bmo)
Comment on attachment 8886976 [details]
Bug 1337314 - Encrypt card number while normallizing field.

https://reviewboard.mozilla.org/r/157734/#review171010

Hi Steve,

Please rebase this on bug 1388238 after reviewing that it's fine.

I guess a different bug will handle decryption? If so, please update the summary of this bug. For the decryption bug, please include mochitest-bc tests for cases where the dialog is already showing.

::: browser/extensions/formautofill/test/unit/test_creditCardRecords.js
(Diff revision 10)
> -  // TODO: Uncomment this after decryption lands (bug 1337314).
> -  // filter = {info: {fieldName: "cc-number"}, searchString: "11"};
> -  // creditCards = profileStorage.creditCards.getByFilter(filter);
> -  // do_check_eq(creditCards.length, 1);
> -  // do_check_credit_card_matches(creditCards[0], TEST_CREDIT_CARD_2);

Maybe I missed it in my skim but you should have a test that confirms that the encrypted CC number matches a call to the .encrypt method.

::: browser/extensions/formautofill/test/unit/test_transformFields.js:512
(Diff revision 10)
>    {
>      description: "Has only the split name fields",
>      creditCard: {
>        "cc-given-name": "John",
>        "cc-family-name": "Doe",
> +      "cc-number": "1234123412341234", // cc-number won't be verified

As I said in my other comment, why can't we verify it?
Summary: [Form Autofill] Handle credit card number encryption/decryption → [Form Autofill] Handle credit card number encryption while normoalizing
Attachment #8888741 - Attachment is obsolete: true
Attachment #8886550 - Attachment is obsolete: true
No longer blocks: 1379600
Status: NEW → ASSIGNED
Summary: [Form Autofill] Handle credit card number encryption while normoalizing → [Form Autofill] Handle credit card number encryption while normalizing
Patch rebased and mochitest error fixed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d15f5dd1966
Encrypt card number while normallizing field. r=lchang
https://hg.mozilla.org/mozilla-central/rev/2d15f5dd1966
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.