1393745 - [Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Steve Chung [:steveck]]

We found that using crypto related API in content process will be busted in e10s mode, so we have to wrap up the decrypt API by process message manager in handler to make sure cipher string must be decrypted in parent process.

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1393745 - [Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler.

Review commit: https://reviewboard.mozilla.org/r/172626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172626/

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8901151 [details]
Bug 1393745 - [Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172626/diff/1-2/

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901151 [details]
Bug 1393745 - [Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler.

https://reviewboard.mozilla.org/r/172626/#review178122

::: browser/extensions/formautofill/FormAutofillHandler.jsm:286
(Diff revision 2)
>          try {
> -          profile["cc-number"] = await MasterPassword.decrypt(profile["cc-number-encrypted"], true);
> +          profile["cc-number"] = await this._decrypt(profile["cc-number-encrypted"], true);
>          } catch (e) {
>            if (e.result == Cr.NS_ERROR_ABORT) {
>              log.warn("User canceled master password entry");
>              return;
>            }
>            throw e;
>          }

The `try...catch` is unnecessary now, but you need to check if it returns an invalid value, which means the decryption is failed.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:576
(Diff revision 2)
>      }
>  
>      return data;
>    },
> +
> +  async _decrypt(cipherText, reAuth) {

nit: `reauth` to align with the API in MasterPassword.jsm. -- or you can alter the other one.

::: browser/extensions/formautofill/FormAutofillParent.jsm:227
(Diff revision 2)
>          win.openPreferences("panePrivacy", {origin: "autofillFooter"});
> +        break;
> +      }
> +      case "FormAutofill:GetDecryptedString": {
> +        let {cipherText, reAuth} = data;
> +        let string = await MasterPassword.decrypt(cipherText, reAuth);

You need a `try...catch` here and let it return an empty string (or null) when the decryption is failed.

Comment 4

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901151 [details]
Bug 1393745 - [Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler.

https://reviewboard.mozilla.org/r/172626/#review178150

::: browser/extensions/formautofill/test/unit/test_autofillFormFields.js:507
(Diff revision 2)
>          let form = doc.querySelector("form");
>          let formLike = FormLikeFactory.createFromForm(form);
>          let handler = new FormAutofillHandler(formLike);
>          let promises = [];
> +        // Replace the interal decrypt method with MasterPassword API
> +        handler._decrypt = MasterPassword.decrypt.bind(MasterPassword);

You should make this mock more accurate, e.g. returning an invalid value instead of throwing an error as `MasterPassword.decrypt` does -- unless you are going to make `handler._decrypt` throw an error as well.

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8901151 [details]
Bug 1393745 - [Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172626/diff/2-3/

Comment 6

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8901151 [details]
Bug 1393745 - [Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler.

https://reviewboard.mozilla.org/r/172626/#review178412

Thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a4d1c750d9a
[Form Autofill] Wrap up masterpassword decrypt API in FormAutofillHandler. r=lchang

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0a4d1c750d9a