Bug 887098 (CVE-2013-1713)

InstallTrigger can use the wrong principal when validating URI loads

VERIFIED FIXED in Firefox 23

Status

Core Graveyard
Installer: XPInstall Engine
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: Cody Crews, Assigned: bholley)

Tracking

({sec-high})

22 Branch
mozilla25
x86_64
Windows 7
sec-high
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox22 wontfix, firefox23+ verified, firefox24+ verified, firefox25+ verified, firefox-esr1723+ verified, b2g1823+ unaffected)

Details

(Whiteboard: [adv-main23+][adv-esr1708+])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Attachment #767559 - Attachment mime type: text/plain → text/html
Somewhere between sec-moderate and sec-high, going with sec-high because bholley thinks this flaw could be extended to do worse things.
Status: UNCONFIRMED → NEW
Component: Untriaged → Installer: XPInstall Engine
Ever confirmed: true
Keywords: sec-high
Product: Firefox → Core
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.
Assignee: nobody → bobbyholley+bmo
Summary: Certain privileged javascript components can be tricked into allowing content to perform actions that could potentially compromise user security. → InstallTrigger can use the wrong principal when validating URI loads
Created attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1
Attachment #767910 - Flags: review?(mrbkap)
Attachment #767910 - Flags: review?(dtownsend+bugmail)
Created attachment 767912 [details] [diff] [review]
Tests. v1
Attachment #767912 - Flags: review?(mrbkap)
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 :)
Attachment #767910 - Flags: review?(dtownsend+bugmail) → review+
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.
(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.
Created attachment 767941 [details] [diff] [review]
Tests. v2
Attachment #767912 - Attachment is obsolete: true
Attachment #767912 - Flags: review?(mrbkap)
Attachment #767941 - Flags: review?(gavin.sharp)
Comment on attachment 767910 [details] [diff] [review]
Cache nodePrincipal when InstallTrigger is bound. v1

I think gavin's review here is probably sufficient.
Attachment #767910 - Flags: review?(mrbkap)
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.
Attachment #767910 - Flags: sec-approval?
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()
Attachment #767941 - Flags: review?(gavin.sharp) → review+
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?
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?"
(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.
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.
Attachment #767941 - Attachment is obsolete: true
Attachment #768106 - Flags: review+
status-b2g18: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
status-firefox-esr17: --- → affected
tracking-b2g18: --- → ?
tracking-firefox23: --- → +
tracking-firefox24: --- → +
tracking-firefox25: --- → +
tracking-firefox-esr17: --- → +
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.
Attachment #767910 - Flags: sec-approval? → sec-approval+
Flags: in-testsuite?
https://hg.mozilla.org/integration/mozilla-inbound/rev/f16e696428be
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
Attachment #767910 - Flags: approval-mozilla-esr17?
Attachment #767910 - Flags: approval-mozilla-beta?
Attachment #767910 - Flags: approval-mozilla-aurora?
Note to self - tests are in local branch |installtrigger_prin|.
https://hg.mozilla.org/mozilla-central/rev/f16e696428be
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox25: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(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!
tracking-b2g18: ? → 23+
tracking-firefox-esr17: + → 23+
Keywords: qawanted, verifyme
(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.
Attachment #767910 - Flags: approval-mozilla-esr17?
Attachment #767910 - Flags: approval-mozilla-esr17+
Attachment #767910 - Flags: approval-mozilla-beta?
Attachment #767910 - Flags: approval-mozilla-beta+
Attachment #767910 - Flags: approval-mozilla-aurora?
Attachment #767910 - Flags: approval-mozilla-aurora+
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
status-firefox23: affected → fixed
status-firefox24: affected → fixed
status-firefox-esr17: affected → fixed
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Ioana, can you please make sure this gets verified fixed?
Keywords: qawanted
QA Contact: ioana.budnar

Comment 26

4 years ago
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.
status-firefox23: fixed → verified
Whiteboard: [adv-main23+][adv-esr1708+]
Alias: CVE-2013-1713
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

4 years ago
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)
status-firefox-esr17: fixed → verified

Comment 29

4 years ago
Verified as fixed on Firefox 24 too:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0
status-firefox24: fixed → verified
Per bholley, B2G is actually unaffected
status-b2g18: affected → unaffected

Comment 31

4 years ago
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.
Status: RESOLVED → VERIFIED
status-firefox25: fixed → verified
Keywords: verifyme
Landed tests, which are green locally:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8dac30c9a28
landed on cnetral https://hg.mozilla.org/mozilla-central/rev/d8dac30c9a28
Flags: in-testsuite? → in-testsuite+
Group: core-security
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.