Closed Bug 1308261 Opened 8 years ago Closed 8 years ago

Add hash to mozAddonManager.createInstall() options

Categories

(Toolkit :: Add-ons Manager, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 + fixed
firefox51 + fixed
firefox52 --- fixed

People

(Reporter: aswan, Assigned: aswan)

Details

Attachments

(1 file)

AddonManager installs can be given the hash of the xpi to be installed that gets verified after downloading but before actually installing an extension.  mozAddonManager should expose this as well.
Comment on attachment 8798679 [details]
Bug 1308261 Add hash to options for mozAddonManager.createInstall()

https://reviewboard.mozilla.org/r/84106/#review82876

::: dom/webidl/AddonManager.webidl:51
(Diff revision 1)
>    Promise<void> cancel();
>  };
>  
>  dictionary addonInstallOptions {
>    required DOMString url;
> +  DOMString hash;

`DOMString? hash`

::: toolkit/mozapps/extensions/test/browser/browser_webapi_install.js:211
(Diff revision 1)
> -  is(addons[0], null, "Addon was uninstalled");
> +    is(addons[0], null, "Addon was uninstalled");
> -}));
> +  });
> +}
> +
> +add_task(makeRegularTest({url: XPI_URL}, "a basic install works"));
> +add_task(makeRegularTest({url: XPI_URL, hash:XPI_SHA}, "install with hash works"));

Nit: `hash: XPI_SHA`

::: toolkit/mozapps/extensions/test/browser/browser_webapi_install.js:225
(Diff revision 1)
>          error: null,
>        },
>      }
>    ];
>  
> -  yield testInstall(browser, XPI_URL, steps, "canceling an install works");
> +  yield testInstall(browser, {url:XPI_URL}, steps, "canceling an install works");

Nit: `url: XPI_URL` (same for the changes below)
Attachment #8798679 - Flags: review?(kmaglione+bmo) → review+
Should this be required?
[Tracking Requested - why for this release]:
At some point, Test Pilot will being using the mozAddonManager API to install its add-on.  As described in bug 1308251, that process is currently broken when certain other extensions that MITM outgoing SSL connections are installed.  The underlying AddonManager allows a checksum to be supplied for an add-on installation, since that checksum provides an end-to-end check on the thing being downloaded, it relaxes other rules about a secure download channel.  The patch for this bug exposes that capability up to mozAddonmanager, which will allow us to make Test Pilot installation work even when these MITM'ing extensions are present.
(In reply to Andy McKay [:andym] from comment #3)
> Should this be required?

It is not required in the underlying AddonManager API, I don't see a good reason to make it any different for mozAddonManager.
Comment on attachment 8798679 [details]
Bug 1308261 Add hash to options for mozAddonManager.createInstall()

https://reviewboard.mozilla.org/r/84106/#review82934

::: dom/webidl/AddonManager.webidl:51
(Diff revision 2)
>    Promise<void> cancel();
>  };
>  
>  dictionary addonInstallOptions {
>    required DOMString url;
> +  DOMString? hash;

You should probably just make this:

    DOMString? hash = null;
  
so the "not passed" case doesn't need to be worried about on the callee side, since semantically you want null and not passed to act the same anyway.  A comment about what null means (and that null and "" are semantically equivalent, because afaict they are) would be a good idea too.

That's all assuming you do in fact want null to mean "no hash" as opposed to getting stringified to "null".

r=me on the webidl changes with that.
Comment on attachment 8798679 [details]
Bug 1308261 Add hash to options for mozAddonManager.createInstall()

https://reviewboard.mozilla.org/r/84106/#review82942
Attachment #8798679 - Flags: review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c6ac0ce7b39
Add hash to options for mozAddonManager.createInstall() r=bz,kmag
Andy, Chuck, Can you please file bugs to begin using this in AMO and Test Pilot?
Flags: needinfo?(charmston)
Flags: needinfo?(amckay)
https://hg.mozilla.org/mozilla-central/rev/0c6ac0ce7b39
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tracking to avoid some issues with system addons and MITM software.  Please fill the uplift request, Ritu will decide whether to take this for 50.
Flags: needinfo?(aswan)
Comment on attachment 8798679 [details]
Bug 1308261 Add hash to options for mozAddonManager.createInstall()

Approval Request Comment
[Feature/regressing bug #]:
mozAddonManager

[User impact if declined]:
This patch allows AMO and testpilot to add an additional layer of protection against a potential MITM attacker modifying addons to be installed.  If uplift is declined, we won't have that additional protection.

[Describe test coverage new/current, TreeHerder]:
Tests are included in this patch

[Risks and why]: 
see above

[String/UUID change made/needed]:
none
Flags: needinfo?(aswan)
Attachment #8798679 - Flags: approval-mozilla-beta?
Attachment #8798679 - Flags: approval-mozilla-aurora?
Comment on attachment 8798679 [details]
Bug 1308261 Add hash to options for mozAddonManager.createInstall()

This is needed to make TestPilot experiments more safe, stabilized on Nightly for a week, Aurora51+, Beta50+
Attachment #8798679 - Flags: approval-mozilla-beta?
Attachment #8798679 - Flags: approval-mozilla-beta+
Attachment #8798679 - Flags: approval-mozilla-aurora?
Attachment #8798679 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.