Last Comment Bug 887098 - (CVE-2013-1713) InstallTrigger can use the wrong principal when validating URI loads
(CVE-2013-1713)
: InstallTrigger can use the wrong principal when validating URI loads
Status: VERIFIED FIXED
[adv-main23+][adv-esr1708+]
: sec-high
Product: Core Graveyard
Classification: Graveyard
Component: Installer: XPInstall Engine (show other bugs)
: 22 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla25
Assigned To: Bobby Holley (busy)
: Ioana (away)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-25 19:26 PDT by Cody Crews
Modified: 2015-12-11 07:21 PST (History)
11 users (show)
abillings: sec‑bounty+
cbook: in‑testsuite+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
+
verified
+
verified
+
verified
23+
verified
23+
unaffected


Attachments
pdffspoc.html (1.35 KB, text/html)
2013-06-25 19:26 PDT, Cody Crews
no flags Details
Cache nodePrincipal when InstallTrigger is bound. v1 (1.82 KB, patch)
2013-06-26 11:48 PDT, Bobby Holley (busy)
gavin.sharp: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
abillings: sec‑approval+
Details | Diff | Review
Tests. v1 (2.56 KB, patch)
2013-06-26 11:49 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
Tests. v2 (2.54 KB, patch)
2013-06-26 13:00 PDT, Bobby Holley (busy)
gavin.sharp: review+
Details | Diff | Review
Tests. v3 r=gavin (3.02 KB, patch)
2013-06-26 18:19 PDT, Bobby Holley (busy)
bobbyholley: review+
Details | Diff | Review

Description Cody Crews 2013-06-25 19:26:45 PDT
Created attachment 767559 [details]
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] 2013-06-26 11:03:41 PDT
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] 2013-06-26 11:05:37 PDT
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 (busy) 2013-06-26 11:48:54 PDT
Created attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1
Comment 4 Bobby Holley (busy) 2013-06-26 11:49:10 PDT
Created attachment 767912 [details] [diff] [review]
Tests. v1
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-06-26 11:56:16 PDT
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: gavin@gavinsharp.com] 2013-06-26 11:58:04 PDT
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 (busy) 2013-06-26 13:00:29 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 (busy) 2013-06-26 13:00:43 PDT
Created attachment 767941 [details] [diff] [review]
Tests. v2
Comment 9 Bobby Holley (busy) 2013-06-26 13:01:08 PDT
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 (busy) 2013-06-26 13:03:45 PDT
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: gavin@gavinsharp.com] 2013-06-26 14:31:51 PDT
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] 2013-06-26 16:16:59 PDT
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] 2013-06-26 16:18:01 PDT
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 (busy) 2013-06-26 18:15:12 PDT
(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 (busy) 2013-06-26 18:19:25 PDT
Created attachment 768106 [details] [diff] [review]
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] 2013-06-26 18:42:53 PDT
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 18 Bobby Holley (busy) 2013-06-26 21:43:44 PDT
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 (busy) 2013-06-26 21:44:07 PDT
Note to self - tests are in local branch |installtrigger_prin|.
Comment 20 Ed Morley [:emorley] 2013-06-27 04:24:39 PDT
https://hg.mozilla.org/mozilla-central/rev/f16e696428be
Comment 21 Alex Keybl [:akeybl] 2013-06-28 14:08:02 PDT
(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 (busy) 2013-06-28 17:30:13 PDT
(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 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-07-09 09:52:22 PDT
Ioana, can you please make sure this gets verified fixed?
Comment 26 Ioana (away) 2013-07-12 06:32:17 PDT
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:] 2013-07-30 14:34:36 PDT
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) 2013-07-31 07:44:50 PDT
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) 2013-08-07 04:32:48 PDT
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] 2013-08-20 10:41:03 PDT
Per bholley, B2G is actually unaffected
Comment 31 Ioana (away) 2013-09-20 04:43:15 PDT
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 (busy) 2013-11-20 16:05:46 PST
Landed tests, which are green locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dac30c9a28
Comment 33 Carsten Book [:Tomcat] 2013-11-21 05:51:03 PST
landed on cnetral https://hg.mozilla.org/mozilla-central/rev/d8dac30c9a28

Note You need to log in before you can comment on or make changes to this bug.