887098 - (CVE-2013-1713) InstallTrigger can use the wrong principal when validating URI loads

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Cody Crews]

Attachment: pdffspoc.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803

Steps to reproduce:

I held a reference to the eval function of another window before loading the new pdf.js implementation in that window, and then used that reference to show that components/extensions implementing their own security checks can potentially allow unsafe actions to be performed from content.


Actual results:

Since the pdf.js implementation has access to resource:// and file:// URLs, it was possible to use InstallTrigger to issue a warning to the error console/firebug demonstrating that it actually uses the principal of the new pdf viewer when performing its own security checks which happen here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions-content.js#152

This is a potential concern in other javascript components that use the document of the window they're accessible from to perform checks against URLs before performing sensitive actions, and could also potentially be used to bypass the same origin policy and other all around nastiness.  I had some success before firefox 21 installing webapps for other domains with this technique but didnt see it as a very big concern since access to navigator.mozApps.mgmt has since been properly handled which means that any DOMRequest object returned from such an install will be a cross compartment wrapper and deny access.  In this instance I showed that InstallTrigger gains local filesystem access in the warning that results.


Expected results:

I should have received the normal exception that results from passing a file:/// URL to InstallTrigger.install from content.  In this instance this is the fault of the InstallTrigger object so I'm sure there's other code that this should raise concerns about.

Comment 1

[Daniel Veditz [:dveditz]]

Somewhere between sec-moderate and sec-high, going with sec-high because bholley thinks this flaw could be extended to do worse things.

Comment 2

[Daniel Veditz [:dveditz]]

bholley checked other places that similarly call checkLoadURIWithPrincipal() from chrome JS and the other functions appear to be grabbing the correct principal safely. At the moment only InstallTrigger is using an outer-window reference.  But this is a lurking footgun that could bite us with new features in the future as well.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Cache nodePrincipal when InstallTrigger is bound. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1

I wrote exactly this patch locally before I noticed you taking this :)

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 767912 [details] [diff] [review]
Tests. v1

Can you put the test in http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/mochitest/ instead? This isn't an xpconnect bug.

Comment 7

[Bobby Holley (:bholley)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #6)
> Can you put the test in
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/mochitest/ instead? This isn't an xpconnect bug.

Sure.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Tests. v2

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1

I think gavin's review here is probably sufficient.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Somewhat. The patch makes it clear what the issue is, but there are a number of steps required to exploit it. The testcase as-filed here isn't even a full exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The tests do, for sure. The patch just makes it clear that we're getting the wrong answer by waiting too long to check the principal.

Which older supported branches are affected by this flaw?

all.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backports should be trivial.

How likely is this patch to cause regressions; how much testing does it need?

Lowish risk, I think. But InstallTrigger is generally a funky kind of thing that has the potential to break weird addon configurations in unexpected ways.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 767941 [details] [diff] [review]
Tests. v2

Don't you need to copy file_empty.html too?

nit: you should use test() rather than !!exec()

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Before I give sec-approval, how far back does this go? Also, can we please put the tests in a separate patch to be checked in later unless this only affects Firefox 23 and higher?

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Nevermind about the tests. Clearly I didn't read fully. :-) I see the second patch now. First question still stands. I assume by "all," you mean "dawn of time?"

Comment 14

[Bobby Holley (:bholley)]

(In reply to Al Billings [:abillings] from comment #13)
> Nevermind about the tests. Clearly I didn't read fully. :-) I see the second
> patch now. First question still stands. I assume by "all," you mean "dawn of
> time?"

Since the dawn of any currently-supported release, for sure. The most recent blame in that code is January 2011.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Tests. v3 r=gavin

Good catch on file_empty.html gavin. Fixing that as well as s/exec/test/ and
carrying over review.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1

sec-approval+ for trunk. Please don't check in tests until at least six weeks after we ship this to the public. We should nominate branch patches for this.

Comment 17

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f16e696428be

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: security bugs
Fix Landed on Version: Just pushed to inbound
Risk to taking this patch (and alternatives if risky): Low-ish risk. No alternatives.  
String or UUID changes made by this patch: None

Comment 19

[Bobby Holley (:bholley)]

Note to self - tests are in local branch |installtrigger_prin|.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f16e696428be

Comment 21

[Alex Keybl [:akeybl]]

(In reply to Bobby Holley (:bholley) from comment #18)
> Risk to taking this patch (and alternatives if risky): Low-ish risk. No
> alternatives.  

Please provide QA with pointers of how to verify that there have not been add-on install regressions here. Thanks!

Comment 22

[Bobby Holley (:bholley)]

(In reply to Alex Keybl [:akeybl] from comment #21)
> (In reply to Bobby Holley (:bholley) from comment #18)
> > Risk to taking this patch (and alternatives if risky): Low-ish risk. No
> > alternatives.  
> 
> Please provide QA with pointers of how to verify that there have not been
> add-on install regressions here. Thanks!

This fix changes the validation we do on XPI uris when deciding whether the page is allowed to install-prompt that XPI. So we should just make sure that all the use cases we support for addon installation (both in terms of the entity doing the install and the location of the XPI, remote or local) work as expected.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a991bca0ae09
https://hg.mozilla.org/releases/mozilla-beta/rev/d97c3cb4ed9a
https://hg.mozilla.org/releases/mozilla-esr17/rev/9000b2962aa2

Comment 25

[u279076]

Ioana, can you please make sure this gets verified fixed?

Comment 26

[Ioana (away)]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

I tested the following: installed the top 12 most popular addons on addons.mozilla.org (restart and restartless), installed a couple of addons from other sites, installed a couple of addons that were previously saved on my machine, installed local addon by drag and drop on browser, installed local addon with "Install Add-on from file", installed addons from the "Get Add-ons" Addons Manager panel, installed twice remote and local addons, installed after cancelling installation.

Please let me know if you think of anything else I should cover.

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

Ioana, can you verify that this is fixed on ESR 17.0.8 and update the bug? Thank you.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/17.0.8esr-candidates/build1/

Comment 28

[Ioana (away)]

Tested on Firefox 17.0.8 ESR along the lines detailed in comment 26:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20100101 Firefox/17.0 (20130729214632)

Comment 29

[Ioana (away)]

Verified as fixed on Firefox 24 too:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0

Comment 30

[Alex Keybl [:akeybl]]

Per bholley, B2G is actually unaffected

Comment 31

[Ioana (away)]

Verified as fixed on Firefox 25 beta 1 (20130917123208):
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0

Tested as described in comment 26.

Comment 32

[Bobby Holley (:bholley)]

Landed tests, which are green locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dac30c9a28

Comment 33

[Carsten Book [:Tomcat]]

landed on cnetral https://hg.mozilla.org/mozilla-central/rev/d8dac30c9a28