1356292 - Installing add-ons from non-whitelisted sites no longer gives the option to allow the install if the addon load is redirected

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Normally when installing from a non-whitelisted site we give a doorhanger offering allow or don't allow options to the user. This was broken by bug 1353975 and now the install is just blocked.

Comment 1

[Dave Townsend [:mossop]]

As an example go to https://github.com/mozilla/onboard/releases/ and click the XPI to install.

Comment 2

[:Gijs (he/him)]

Looks like we get a null principal as the principalToInherit here:

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/toolkit/mozapps/extensions/addonManager.js#198

from here:

https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/toolkit/mozapps/extensions/amContentHandler.js#49-52


Shouldn't this always be using the triggering principal?

Comment 3

[Boris Zbarsky [:bzbarsky]]

The changes in bug 1353975 set the "principal to inherit" to a nullprincipal when an HTTP redirect happens for a document load.  

Looking at the testcase in comment 1, the link points to https://github.com/mozilla/onboard/releases/download/v1.0.0-rc8/onboard-v1.xpi which redirects to <https://github-cloud.s3.amazonaws.com/releases/85563262/d64b0a48-1eed-11e7-82d1-218bfdee45ea.xpi?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAISTNZFOVBIJMK3TQ%2F20170413%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20170413T173153Z&X-Amz-Expires=300&X-Amz-Signature=44bdceab6c287701ee53e10ded0b7b235ecc664f35e95e6bdfd83556559c90ea&X-Amz-SignedHeaders=host&actor_id=0&response-content-disposition=inline%3B%20filename%3Donboard-v1.xpi&response-content-type=application%2Fx-xpinstall> so it's hitting the "document load being redirected" codepath.

Why is this amContentHandler code examining the principalToInherit at all in cases when it's not relevant (as here; http:// doesn't use the principalToInherit)?  Looks like it dates back to the fix for bug 1297338, but before then it was just using the triggeringPrincipal... Is the idea to determine "which page requested the install" or something?  If so, should the relevant principal here be the one for https://github.com/mozilla/onboard/releases/ or the one for https://github.com/mozilla/onboard/releases/download/v1.0.0-rc8/onboard-v1.xpi (which may have been on a different site, but wasn't in this case)?  The latter is what decided where to actually redirect to and hence where the addon is really coming from....

Comment 4

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3)
> Why is this amContentHandler code examining the principalToInherit at all in
> cases when it's not relevant (as here; http:// doesn't use the
> principalToInherit)?  Looks like it dates back to the fix for bug 1297338,
> but before then it was just using the triggeringPrincipal... Is the idea to
> determine "which page requested the install" or something?  If so, should
> the relevant principal here be the one for
> https://github.com/mozilla/onboard/releases/ or the one for
> https://github.com/mozilla/onboard/releases/download/v1.0.0-rc8/onboard-v1.
> xpi (which may have been on a different site, but wasn't in this case)?  The
> latter is what decided where to actually redirect to and hence where the
> addon is really coming from....

We care about the page that originally initiated the install regardless of redirects, i.e the original link. If a page links to an AMO url that redirects to the real XPI to install we want to compare the original page's principal, not AMO's.

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK.  So...

Normally the "triggering principal" would be the right thing in that situation.  The only reason it's not is because of bug 1297338 comment 50 and bug 1297338 comment 55: we have tests for what happens when an XPI url is loaded from the URL bar, and those tests expect "the page that initiated the install" to be the page that happened to be loaded when the user hit enter on the pasted XPI link, afaict.  And I'm not sure that's a reasonable expectation to start with.

We did things this way because the ways principalToInherit was used were very limited at first.  But now it's being used in more different ways, so we should probably revisit this decision, and perhaps the substance of the browser_bug553455.js test.  Or at least the test_urlbar() parts of it.

Comment 6

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> OK.  So...
> 
> Normally the "triggering principal" would be the right thing in that
> situation.  The only reason it's not is because of bug 1297338 comment 50
> and bug 1297338 comment 55: we have tests for what happens when an XPI url
> is loaded from the URL bar, and those tests expect "the page that initiated
> the install" to be the page that happened to be loaded when the user hit
> enter on the pasted XPI link, afaict.  And I'm not sure that's a reasonable
> expectation to start with.

I'm not sure if I'm just restating what you already understand here, but installing add-ons by entering an XPI url into the address bar has been broken for some time, something that ideally we would fix but was apparently hard to do at the time. I believe it was bug 1042699 that broke that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> but installing add-ons by entering an XPI url into the address bar has been broken for some time

I did not know that, in fact.  ;)

Given that, what is test_urlbar() testing?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> > but installing add-ons by entering an XPI url into the address bar has been broken for some time
> 
> I did not know that, in fact.  ;)
> 
> Given that, what is test_urlbar() testing?

It's testing that it is blocked with the error message that we expect.

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK.  Why does/should it get blocked?  That is, what is the actual security check we perform?  Why did that security check stop working right when we started using "system" as the triggering principal for loads via the URL bar (which is what necessitated the change to using principalToInherit to make the test happy)?

That is, loads via the URL bar currently end up looking, for purposes of that test, as if they came from the page that was loaded when the URL bar load was kicked off, right?  But we don't want to block installs from pages, but _do_ want to block them from the url bar, sounds like, so I'm a bit confused as to what's being tested, exactly.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 53. I'm not clear yet here what behavior we expect  and whether this is a 53 release blocking issue.

Comment 11

[Andy McKay]

Comment 0 is still valid and this should be a release blocker.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Yeah seems release blocking. I will hold the 53 RC 3 build. 
We are at some risk now of slipping the release date.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

ESR52 would also be affected.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

I think Boris is already mentioning everything that's relevant.

I used the STRs from comment 0:
* go to https://github.com/mozilla/onboard/releases/
* click the XPI to install.
* Result: Nightly prevented addon installation
* Expected: Addon installation should be allowed

Just for the sake of completeness and to avoid false expectations, here are the values in the loadInfo:
  channelURI: https://github.com/mozilla/onboard/releases/download/v1.0.0-rc8/onboard-v1.xpi
  loadingPrincipal: nullptr
  triggeringPrincipal: https://github.com/mozilla/onboard/releases/
  principalToInherit: https://github.com/mozilla/onboard/releases/
  contentPolicyType: 6
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, 
  initalSecurityChecksDone: yes
  enforceSecurity: yes

and after the redirect:

doContentSecurityCheck {
  channelURI: https://github-cloud.s3.amazonaws.com/releases/85563262/d64b0a48-1eed-11e7-82d1-218bfdee45ea.xpi?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAISTNZFOVBIJMK3TQ%2F20170413%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20170413T190658Z&X-Amz-Expires=300&X-Amz-Signature=cffef173263b01b99b72808c138772b1ba94801991b9bc494f9aec0be39c667b&X-Amz-SignedHeaders=host&actor_id=0&response-content-disposition=inline%3B%20filename%3Donboard-v1.xpi&response-content-type=application%2Fx-xpinstall
  loadingPrincipal: nullptr
  triggeringPrincipal: https://github.com/mozilla/onboard/releases/
  principalToInherit: NullPrincipal
  contentPolicyType: 6
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, 
  initalSecurityChecksDone: yes
  enforceSecurity: yes

Probably it's best if we understand what test_urlBar() within browser_bug553455.js is testing precisely and then we can take it from there.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #9)
> OK.  Why does/should it get blocked?  That is, what is the actual security
> check we perform?  Why did that security check stop working right when we
> started using "system" as the triggering principal for loads via the URL bar
> (which is what necessitated the change to using principalToInherit to make
> the test happy)?
> 
> That is, loads via the URL bar currently end up looking, for purposes of
> that test, as if they came from the page that was loaded when the URL bar
> load was kicked off, right?  But we don't want to block installs from pages,
> but _do_ want to block them from the url bar, sounds like, so I'm a bit
> confused as to what's being tested, exactly.

The ideal is that urls initiated from the url bar bypass the whitelist and start the install without the whitelist prompt. Before bug 1042699 we did that by seeing that the internalReferer of the load was null. Bug 1042699 necessitated switching to checking the triggeringPrincipal for loads and if it was a null principal banning the install. This had the side effect of blocking installs from the url bar where the triggeringPrincipal was also null. We punted on fixing that because it was more important to fix the security issue than maintaining that particular feature. I added a test verifying that url bar installs did what we now "expected".

In retrospect I probably should have commented in that test explaining that it wasn't really what we wanted to happen because it sounds like in bug 1297338 we might have made installs from the url bar work, seen it broke the test and so assumed it wasn't something we wanted and fixed that. Is that a fair summary? I clearly should have read the bug more thoroughly before reviewing :(

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Just chatted with :mossop on IRC. The expected behavior:

* when pasting "https://github.com/mozilla/onboard/releases/download/v1.0.0-rc9/onboard-v1.xpi" into the address bar?
-> Install should be allowed

* when clicking the link for the *.xpi on "https://github.com/mozilla/onboard/releases/"?
-> check github.com against our whitelist and (as it is not in there by default) we should show a doorhanger asking the user if they want to allow the install or not

Comment 17

[Boris Zbarsky [:bzbarsky]]

> it sounds like in bug 1297338 we might have made installs from the url bar work

Yep.  Specifically, after that bug, loads from the url bar have a system principal as triggering principal.  Per bug 1042699 comment 37 that would make them work.

So it sounds to me like we should do the following:

1) Back out https://hg.mozilla.org/mozilla-central/diff/57d473c3ca22/toolkit/mozapps/extensions/amContentHandler.js

2) Back out https://hg.mozilla.org/mozilla-central/rev/a3056e9ae41a

3) Fix the browser_bug553455.js by adjusting the test expectation.

Is that about right?

Christoph or I (or anyone else) can obviously do #1 and #2 quite easily.  Could use help on #3.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #17)
> > it sounds like in bug 1297338 we might have made installs from the url bar work
> 
> Yep.  Specifically, after that bug, loads from the url bar have a system
> principal as triggering principal.  Per bug 1042699 comment 37 that would
> make them work.
> 
> So it sounds to me like we should do the following:
> 
> 1) Back out
> https://hg.mozilla.org/mozilla-central/diff/57d473c3ca22/toolkit/mozapps/
> extensions/amContentHandler.js
> 
> 2) Back out https://hg.mozilla.org/mozilla-central/rev/a3056e9ae41a
> 
> 3) Fix the browser_bug553455.js by adjusting the test expectation.
> 
> Is that about right?
> 
> Christoph or I (or anyone else) can obviously do #1 and #2 quite easily. 
> Could use help on #3.

I'm working on #3 now.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1356292_addon_redirect_problem.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #17)
> So it sounds to me like we should do the following:
> 
> 1) Back out
> https://hg.mozilla.org/mozilla-central/diff/57d473c3ca22/toolkit/mozapps/
> extensions/amContentHandler.js
> 
> 2) Back out https://hg.mozilla.org/mozilla-central/rev/a3056e9ae41a
> 
> 3) Fix the browser_bug553455.js by adjusting the test expectation.
> 
> Is that about right?

That's what I thought actually, and I suppose this is what the attached patch is doing, or have I missed something? Anyway, when looking at the expected behavior for:

> * when pasting
> "https://github.com/mozilla/onboard/releases/download/v1.0.0-rc9/onboard-v1.
> xpi" into the address bar?
> -> Install should be allowed

Actual result:
[Nightly has prevented this site from installling an unverified addon.]

> * when clicking the link for the *.xpi on
> "https://github.com/mozilla/onboard/releases/"?
> -> check github.com against our whitelist and (as it is not in there by
> default) we should show a doorhanger asking the user if they want to allow
> the install or not

Actual result:
Nightly prevented this site from asking you to install software on your computer.
[Don't Allow] [Allow]

--> click allow

Nightly has prevented this site from installling an unverified addon.


That doesn't sound right in the end, does it? What have I missed?

Comment 20

[Dave Townsend [:mossop]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19)
> Created attachment 8858126 [details] [diff] [review]
> bug_1356292_addon_redirect_problem.patch
> 
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> decent message, automatic r-) from comment #17)
> > So it sounds to me like we should do the following:
> > 
> > 1) Back out
> > https://hg.mozilla.org/mozilla-central/diff/57d473c3ca22/toolkit/mozapps/
> > extensions/amContentHandler.js
> > 
> > 2) Back out https://hg.mozilla.org/mozilla-central/rev/a3056e9ae41a
> > 
> > 3) Fix the browser_bug553455.js by adjusting the test expectation.
> > 
> > Is that about right?
> 
> That's what I thought actually, and I suppose this is what the attached
> patch is doing, or have I missed something? Anyway, when looking at the
> expected behavior for:
> 
> > * when pasting
> > "https://github.com/mozilla/onboard/releases/download/v1.0.0-rc9/onboard-v1.
> > xpi" into the address bar?
> > -> Install should be allowed
> 
> Actual result:
> [Nightly has prevented this site from installling an unverified addon.]

This is the right behaviour. This add-on is unsigned so you need to flip xpinstall.signatures.required to false for the install to proceed beyond the signing check.

> > * when clicking the link for the *.xpi on
> > "https://github.com/mozilla/onboard/releases/"?
> > -> check github.com against our whitelist and (as it is not in there by
> > default) we should show a doorhanger asking the user if they want to allow
> > the install or not
> 
> Actual result:
> Nightly prevented this site from asking you to install software on your
> computer.
> [Don't Allow] [Allow]
> 
> --> click allow
> 
> Nightly has prevented this site from installling an unverified addon.
> 
> 
> That doesn't sound right in the end, does it? What have I missed?

Same again.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8858126 [details] [diff] [review]
bug_1356292_addon_redirect_problem.patch

r=me

Comment 22

[Dave Townsend [:mossop]]

Attachment: testcase change

This changes test_urlbar to behave the way we want, that is that XPIs entered on the url bar are whitelisted and install proceeds.

Comment 23

[Dave Townsend [:mossop]]

We should probably add a testcase for the unwhitelisted install that gets redirect case that caught this in the first place, might take me a little longer to do that.

Comment 24

[Mike Kaply [:mkaply]]

This is happening with Amazon as well. See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1356388

Can we verify they work after the fix as well please?

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8858129 [details] [diff] [review]
testcase change

Review of attachment 8858129 [details] [diff] [review]:
-----------------------------------------------------------------

It would be nice to create functions for repeated fragments of code but I think that's outside the scope of what's needed immediately.
r=me with the `Services.perms.remove()` call removed.

::: browser/base/content/test/general/browser_bug553455.js
@@ +733,5 @@
> +    is(notification.getAttribute("label"),
> +             "XPI Test will be installed after you restart " + gApp + ".",
> +             "Should have seen the right message");
> +
> +    let installs = yield getInstalls();

AddonManager methods now do promises natively so this could be `yield AddonManager.getAllInstalls();` but keeping I guess keeping it consistent with the rest of this file is probably better.

@@ +737,5 @@
> +    let installs = yield getInstalls();
> +    is(installs.length, 1, "Should be one pending install");
> +    installs[0].cancel();
> +
> +    Services.perms.remove(makeURI("http://example.com/"), "install");

I don't think this belongs here?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1197636189f2
Switch back to triggering principal checks for add-on install permission checking to allow url bar triggered installs to proceed. r=bz, r=aswan

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to do the uplift request now? We can approve it after this looks ok on m-c.
 
Andrei, can your team verify the case from bug 1356388? Thanks!

Comment 28

[Dave Townsend [:mossop]]

Attachment: combined patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug 1353975 broke this on ESR 52 so we should probably take the fix there too.
User impact if declined: Users will be unable to install add-ons from certain websites
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low risk, well tested
String or UUID changes made by this patch: None

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8858152 [details] [diff] [review]
combined patch

Looks good on inbound enough to where let's just get this onto m-r for the RC3 build.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/189ed7b2306f

We should definitely land a test for this still. In the mean time, manual QA will have to suffice.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

This needs a rebased patch for ESR52. Dave, please go ahead and push it when ready so we can get the respin going as soon as possible.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d345b657d381

Comment 33

[Dave Townsend [:mossop]]

Pushed: https://hg.mozilla.org/releases/mozilla-esr52/rev/04e42829d1150bee2f197092746c124435c278bb

Comment 34

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/1197636189f2

Comment 35

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using:
- lastpass add-on (from https://www.lastpass.com/?lpInstall)
- amazon add-on (from https://www.amazon.com/gp/BIT/ref=bit_v2_surl_a?bitCampaignCode=A0027)
- noscript add-on (from https://noscript.net/) 

Verified using latest Nightly that the add-ons are successfully installed and the correct message is displayed so the user can Allow or Don't Allow the install, using Firefox 53.0rc-build6 across platforms (Ubuntu 16.04 64bit, macOS 10.12.4, Windows 10 64bit and Windows 7 64bit).

Comment 37

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The issue was reproduced on 55.0a1 (2017-04-13), using the STR from comment 0, comment 14 and the add-ons from comment 35. We can confirm that it is verified fixed on 52.1.0esr-build3 (20170417065206), using Windows 7 x64, Windows 10 x64, macOS 10.12 and Ubuntu 14.04 x64: the add-ons are successfully installed and the doorhanger offering Allow or Don't Allow options to the user is properly displayed.