Closed Bug 1287125 Opened 4 years ago Closed 4 years ago

Lock down mozAddonManager to install only listed add-ons

Categories

(Toolkit :: Add-ons Manager, defect, P1)

48 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 - fix-optional
firefox49 + fixed
firefox50 --- fixed

People

(Reporter: andy+bugzilla, Assigned: aswan)

References

Details

(Whiteboard: triaged)

Attachments

(1 file)

I think the install method of mozAddonManager will take any URL for the install. Firefox will enforce that the add-on is signed (in 48). However we did relax signing of add-ons so that any unlisted add-on will automatically be signed and not reviewed.

Because the review policy is different, we should probably ensure that if you can XSS AMO and then inject a different URL into the install method, that the URL is for a listed add-on on AMO. i.e., one that has been reviewed.

The only way I can see us distinguishing listed vs unlisted is by the URLs. Can we do a simple check in install that the URL coming in conforms to one of AMO or the AMO CDN? And similarly for the dev and stage end points.

Wladimir Palant and others reported this in London and chatted to me on IRC about it so not sure if there's a bounty there, but wanted to make sure he gets credit. 

Because 48 is coming pretty fast now, I wanted to get this in today.
Who can provide the definitive pattern for what URLs should be accepted?
Flags: needinfo?(amckay)
For prod its:

https://addons.mozilla.org
https://addons.cdn.mozilla.net

For stage its:

https://addons.allizom.org
https://addons-stage-cdn.allizom.org

For dev its:

https://addons-dev.allizom.org
https://addons-dev-cdn.allizom.org

Just going to needinfo Jason to verify those and also to let ops know that changing the CDN domains in the future wouldn't be great.
Flags: needinfo?(amckay) → needinfo?(jthomas)
(In reply to Andy McKay [:andym] from comment #2)
> For prod its:
> 
> https://addons.mozilla.org
> https://addons.cdn.mozilla.net
> 
> For stage its:
> 
> https://addons.allizom.org
> https://addons-stage-cdn.allizom.org
> 
> For dev its:
> 
> https://addons-dev.allizom.org
> https://addons-dev-cdn.allizom.org
> 
> Just going to needinfo Jason to verify those and also to let ops know that
> changing the CDN domains in the future wouldn't be great.

r+
Flags: needinfo?(jthomas)
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64624/diff/1-2/
Attachment #8771500 - Attachment description: bug 1287125 wip → bug 1287125 Lock down mozAddonManager.install()
Attachment #8771500 - Flags: review?(rhelmer)
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64624/diff/2-3/
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

https://reviewboard.mozilla.org/r/64624/#review61698

lgtm as-is, couple nits you can take or leave.

::: toolkit/mozapps/extensions/AddonManager.jsm:2911
(Diff revision 3)
>      createInstall(target, options) {
> -      return new Promise((resolve) => {
> +      return new Promise((resolve, reject) => {
> +        try {
> +          let host = Services.io.newURI(options.url, null, null).host;
> +          if (WEBAPI_INSTALL_HOSTS.includes(host)) {
> +            // good

I think it's correct as-is but the comment-only `if` block looks weird... would explit `resolve` here work instead?

The usual style I see in browser and toolkit is to have separate `if` blocks and early return in the case of a bad condition, for instance:

```
if (!WEBAPI_INSTALL_HOSTS.includes(host)) {
  throw new Error(`${host} not permitted`);
}

if (...) {
  throw ...;
}

// anything that reaches here has not thrown or returned early so must be good
```

::: toolkit/mozapps/extensions/test/browser/browser_webapi_install.js:243
(Diff revision 3)
>    is(addons[0], null, "The addon was not installed");
>  
>    ok(AddonManager.webAPI.installs.size > 0, "webAPI is tracking the AddonInstall");
>  }));
>  
> +add_task(function test_badhost() {

This test looks good, should there be one to make sure the testing pref works ok?

Also maybe a test which passes in a bad URL to make sure that `Services.io.newURI` throws and the message is rejected and right `err.message` happens (not to test `newURI` itself, just to make sure the exception+rejection handling is all OK).
Attachment #8771500 - Flags: review?(rhelmer) → review+
https://reviewboard.mozilla.org/r/64624/#review61698

> I think it's correct as-is but the comment-only `if` block looks weird... would explit `resolve` here work instead?
> 
> The usual style I see in browser and toolkit is to have separate `if` blocks and early return in the case of a bad condition, for instance:
> 
> ```
> if (!WEBAPI_INSTALL_HOSTS.includes(host)) {
>   throw new Error(`${host} not permitted`);
> }
> 
> if (...) {
>   throw ...;
> }
> 
> // anything that reaches here has not thrown or returned early so must be good
> ```

Sorry my comment was a bit contradictory - also I see why you don't want to explicitly `resolve` here, there's more work to do below.

I think the separate `if` blocks with early return for bad conditions would be easier to understand quickly than `if/else if` with empty comment blocks.
[Tracking Requested - why for this release]:
The mozAddonManager API is new in Firefox 48.  It is only available to addons.mozilla.org (AMO) but this patch adds more defense-in-depth to the install() method in the API to ensure that it cannot be mis-used to install addons that don't come from AMO.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fbf164ef9e70
Lock down mozAddonManager.install() r=rhelmer
https://hg.mozilla.org/mozilla-central/rev/fbf164ef9e70
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Hi Andrew, do you have any idea what made the test added fails after it got merged around? e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=31918920&repo=mozilla-inbound
Flags: needinfo?(aswan)
This is too late for such changes in 48. Please submit an uplift request to 49.
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #12)
> Hi Andrew, do you have any idea what made the test added fails after it got
> merged around? e.g.
> https://treeherder.mozilla.org/logviewer.html#?job_id=31918920&repo=mozilla-
> inbound

backed this out now from central since this failures are perma failures now :(
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/0fbdcd21fad7
Backed out changeset fbf164ef9e70 for perma failing in browser_webapi_install.js after merged
Should this be Re-Opened since it was backed out >?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #12)
> Hi Andrew, do you have any idea what made the test added fails after it got
> merged around? e.g.
> https://treeherder.mozilla.org/logviewer.html#?job_id=31918920&repo=mozilla-
> inbound

I've seen this before as an intermittent failure, I don't think it has to do with this patch:
"ImportError: No module named pygtk"

I'll see if I can find out where that failure is coming from.
It got backed out for this:
> TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_webapi_install.js | Wrong error message for invalid download url - Got false, expected true
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #18)
> It got backed out for this:
> > TEST-UNEXPECTED-FAIL | toolkit/mozapps/extensions/test/browser/browser_webapi_install.js | Wrong error message for invalid download url - Got false, expected true

Yes I realized that finally :) Pretty sure this test passed on try, hm. I will see if I can repro locally.
If we lock down to listed add-ons only, this means that it won't work for your newly authorised test pilot add-ons. Do all the add-ons for test pilot come off one domain we could add to this patch?
Flags: needinfo?(cprice)
(In reply to Andy McKay [:andym] from comment #20)
> Do all the add-ons for test pilot
> come off one domain we could add to this patch?
No. We don't have any hard rules on where the XPI files are hosted.
Flags: needinfo?(cprice)
We *could* host everything in one place, we just didn't have a good reason to require it before. This seems to qualify as a good reason ^_^

:clouserw says[1] he's in favor of test pilot hosting its addons, and that we have ops approval to use a blessed S3 bucket.

I'm not sure if a specific URL was discussed with ops, so I'll just flag Wil to see if there's a domain picked out already

[1] http://logs.glob.uno/?c=testpilot#c16902
Flags: needinfo?(wclouser)
Flags: needinfo?(amckay)
Forgot to ask: andy, how quickly do we need to come up with a domain name?
I want to get this uplifted into 49 so pretty soon would be good, doesn't mean you have to the domain working, just decide on one for when you want to start using mozAddonManager. Could you just put S3 behind a proxied part of testpilot.mozilla.org, for example testpilot.mozilla.org/files or something similar?
Flags: needinfo?(amckay)
If a point release comes out it would be nice to get it in and its been baking on aurora for a little while. It still only affects the discovery pane, so should be low risk... someone suggested a good way to do that was to set it for fix-optional for 48, my apologies.
https://reviewboard.mozilla.org/r/64624/#review61698

> Sorry my comment was a bit contradictory - also I see why you don't want to explicitly `resolve` here, there's more work to do below.
> 
> I think the separate `if` blocks with early return for bad conditions would be easier to understand quickly than `if/else if` with empty comment blocks.

I initially wrote it like that but we can't actually return early, if the host isn't in the default set of hosts, we need to check if the test flag is enabled and if so, if the host is in the list of tests hosts, before concluding that this host isn't allowed.
Expressed that way, the logic felt even gnarlier to me than the way it is now.

> This test looks good, should there be one to make sure the testing pref works ok?
> 
> Also maybe a test which passes in a bad URL to make sure that `Services.io.newURI` throws and the message is rejected and right `err.message` happens (not to test `newURI` itself, just to make sure the exception+rejection handling is all OK).

Yep, I'll add those additional tests.  I need to push a new version with the failures that led to the back-out corrected so I'll add some new tests then, but I'm struggling to reproduce the failures :(
Let's add testpilot.firefox.com for the domain.  Like you said, we could proxy or readfile through from there and it will match the add-on install whitelist domain we have.  Anything else you need us to do?  Thanks for giving us the heads up!
Flags: needinfo?(wclouser)
aswan, could you add in testpilot.firefox.com too please?
Sorry!  For completeness:

Prod:  testpilot.firefox.com
Stage: testpilot.stage.mozaws.net
Dev:   testpilot.dev.mozaws.net
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64624/diff/3-4/
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64624/diff/4-5/
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

Re-requesting review for new changes, which include:
- rewrote the logic for checking a url, hopefully for clarity
- added a test for a malformed url

I wanted to add a test that the permission is working correctly (i.e., that you can't install from a test domain if the permission isn't set) but we use the same flag for the presence of the api and enabling test domains, so you can't actually use the api to try it out if the flag isn't set...

Also I've done several try runs and re-triggers and have been unable to reproduce the failures that led to the original back-out.  I have changed the error reporting a bit so if this does recur, hopefully we'll be one step closer to figuring out why.
Flags: needinfo?(aswan)
Attachment #8771500 - Flags: review+ → review?(rhelmer)
Because this bug was getting a bit messy, I broke out testpilot into a seperate bug 1288178.
Attachment #8771500 - Flags: review?(rhelmer) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e25f1a122d9d
Lock down mozAddonManager.install() r=rhelmer
https://hg.mozilla.org/mozilla-central/rev/e25f1a122d9d
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1288480
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

Approval Request Comment
[Feature/regressing bug #]: mozAddonManager
[User impact if declined]: Low severity security issue: if an attacker compromised addons.mozilla.org, they could use mozAddonManager to install undesired addons.
[Describe test coverage new/current, TreeHerder]: Tests are included in the patch
[Risks and why]: Low risk, the change only affects the mozAddonManager API, which is only available on addons.mozilla.org
[String/UUID change made/needed]: none
Attachment #8771500 - Flags: approval-mozilla-aurora?
Comment on attachment 8771500 [details]
bug 1287125 Lock down mozAddonManager.install()

This patch fixes a security issue for installing addons that don't come from AMO. Take it in aurora.
Attachment #8771500 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1303735
See Also: → 1325524
You need to log in before you can comment on or make changes to this bug.