Bug 813906 (CVE-2013-0758)

Content can access chrome-privileged pages using plugin objects

VERIFIED FIXED in Firefox 18

Status

()

Core
Plug-ins
P1
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Mariusz Mlynski, Assigned: gfritzsche)

Tracking

({csectype-priv-escalation, sec-critical})

17 Branch
mozilla20
csectype-priv-escalation, sec-critical
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox18+ verified, firefox19+ verified, firefox20+ verified, firefox-esr1018+ verified, firefox-esr1718+ verified)

Details

(Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+], URL)

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
It is possible to open chrome-privileged pages via plugin instances cloned into anonymous content of the SVG "use" element.

The root of the problem lies in the method nsPluginInstanceOwner::GetURL, which may resolve a relative path against a chrome URI and pass it to the OnLinkClick handler method. In theory, such URI should be rejected earlier in nsPluginHost::GetURLWithHeaders, where DoURLLoadSecurityCheck is called, but there's a discrepancy between how nsPluginHost::DoURLLoadSecurityCheck and nsIContent::GetBaseURI derive the baseURI. This inconsistency allows a malicious website to load an arbitrary chrome page in a frame and use it to escalate privileges in drive-by download attacks.
(Reporter)

Comment 1

5 years ago
Created attachment 683937 [details]
Exploit

Arbitrary code execution using the COW bypass from bug 813901. Once the page is loaded, it should open C:\Windows\System32\calc.exe on Windows or alert Components.classes on other systems.

Please note that this exploit is remote-only, don't run it from the file system.
(Reporter)

Updated

5 years ago
Attachment #683937 - Attachment mime type: application/octet-stream → application/java-archive
(Reporter)

Comment 2

5 years ago
Comment on attachment 683937 [details]
Exploit

Uh, jar spoils things again... Run it unpacked.
Attachment #683937 - Attachment mime type: application/java-archive → application/zip
Flags: sec-bounty?
Is the source for loader.swf available?
Keywords: sec-critical
(Reporter)

Comment 4

5 years ago
There you go:

<mx:Application xmlns:mx="http://www.adobe.com/2006/mxml" preinitialize="flash.external.ExternalInterface.call('f');navigateToURL(new URLRequest('browser.xul'),'n')"></mx:Application>
Thank you.

gfritzsche, could you create a mochitest for this using the testplugin? It's not clear to me that the plugin must call f() (it looks like the page could set the base.href directly). But in either case, the NPN_GetURL("browser.xul", "n") is what we really need to test. That should really fail directly.
Assignee: nobody → georg.fritzsche
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P1
(Assignee)

Comment 6

5 years ago
Will look into this tomorrow.
(Assignee)

Comment 7

5 years ago
Created attachment 684752 [details] [diff] [review]
Mochitest

Mochitest for the issue. Both |f()| and the |<use />| pattern are required for this test to fail.
(Assignee)

Comment 8

5 years ago
For the URL security check we resolve it directly via the (safe) document base URI:
http://hg.mozilla.org/mozilla-central/annotate/3c67034ba39c/dom/plugins/base/nsPluginHost.cpp#l3206

From nsPluginInstanceOwner::GetURL() we use nsIContent::GetBaseURI():
http://hg.mozilla.org/mozilla-central/annotate/3c67034ba39c/dom/plugins/base/nsPluginInstanceOwner.cpp#l540

... which ends up picking a different (still chrome) base URI here:
http://hg.mozilla.org/mozilla-central/annotate/3c67034ba39c/content/base/src/FragmentOrElement.cpp#l306

What i don't know is which one actually is working as intended.
(Assignee)

Comment 9

5 years ago
Boris, do you happen to know what the proper behaviour here should be so that we can unify that URL resolving?
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Generally, if you have a URI that's meant to be relative to a node you should use nsIContent::GetBaseURI.  That will take into account things like xml:base and whatever the svg:use mess is, iirc.
Flags: needinfo?(bzbarsky)
Attachment #683937 - Attachment mime type: application/zip → application/java-archive
(In reply to Mariusz Mlynski from comment #2)
> Uh, jar spoils things again... Run it unpacked.

If you give the attachments the content-type "application/java-archive" you can run the PoC from bugzilla using a jar: URL. The one in the comment probably won't be linkified correctly but hopefully the one in the bug's URL field will.
jar:https://bugzilla.mozilla.org/attachment.cgi?id=683937!/exploit.html
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
tracking-firefox18: --- → +
tracking-firefox19: --- → +
tracking-firefox20: --- → +
Keywords: csec-priv-escalation
(Reporter)

Comment 12

5 years ago
(In reply to Daniel Veditz [:dveditz] from comment #11)
> (In reply to Mariusz Mlynski from comment #2)
> > Uh, jar spoils things again... Run it unpacked.
> 
> If you give the attachments the content-type "application/java-archive" you
> can run the PoC from bugzilla using a jar: URL.

And I did that, but bmo does two 302 redirects on every GET request for an attachment, which tends to break my testcases. In this particular example, I have to refresh the loaded jar URL to make the exploit work, and it only works if I do a cached refresh (F5) -- the initial load and an uncached refresh (Ctrl + F5) always fail, because GET requests for the swf file are redirected (as can be seen in the web console) and things go awry for god knows what reason. If you run it from a server that doesn't do all this 302 magic, the exploit should trigger every single time.
(Assignee)

Comment 13

5 years ago
Created attachment 685607 [details] [diff] [review]
Unify base URI usage
Attachment #685607 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Attachment #684752 - Flags: review?(benjamin)
(Assignee)

Comment 14

5 years ago
The above patches pass the mochitests locally, any suggestions on how to sneak them through a try run?
Attachment #685607 - Flags: review?(benjamin) → review?(bzbarsky)
Comment on attachment 684752 [details] [diff] [review]
Mochitest

I think we can land this in public by filing a new bug and just making this a correctness issue and not mentioning that you could also do cross-domain loading with it. To do that, modify this testcase so that instead of

<base href="chrome://browser/content/">

it's all same-domain like

<base href="base1">

And then you don't even need specialpowers for this testcase, you can just check window.frame1.contentWindow.location.
Comment on attachment 685607 [details] [diff] [review]
Unify base URI usage

>   // get the full URL of the document that the plugin is embedded

Please fix this comment.

>+    rv = NS_MakeAbsoluteURI(absUrl, aURL, owner->GetBaseURI().get());

That leaks the base URI.  Having to use .get() is supposed to be a red flag... ;)

The rest looks fine, but I'd like to see the leak fixed.
Attachment #685607 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 17

5 years ago
Created attachment 686684 [details] [diff] [review]
Unify base URI usage

Adress review comments.
Attachment #685607 - Attachment is obsolete: true
Attachment #686684 - Flags: review?(bzbarsky)
Comment on attachment 686684 [details] [diff] [review]
Unify base URI usage

r=me.  Thanks!
Attachment #686684 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

5 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> To do that, modify this testcase so that instead of
> 
> <base href="chrome://browser/content/">
> 
> it's all same-domain like
> 
> <base href="base1">
> 
> And then you don't even need specialpowers for this testcase, you can just
> check window.frame1.contentWindow.location.

Hm, i don't see a way to get a covering test that way. We'd need something that:
 * is same-domain
 * doesn't trigger the security check before the patch (but does afterwards), due to this:
   http://hg.mozilla.org/mozilla-central/annotate/3c67034ba39c/dom/plugins/base/nsPluginHost.cpp#l3206

The actual URL loading part already resolves the correct way:
http://hg.mozilla.org/mozilla-central/annotate/3c67034ba39c/dom/plugins/base/nsPluginInstanceOwner.cpp#l540
Oh I see, well hrm. Could we do it the other way around and have the test *reject* a load that it should have allowed?
(Assignee)

Comment 21

5 years ago
I can't think of something that's not involving chrome etc., will have to recheck tomorrow.
(Assignee)

Comment 22

5 years ago
From nsScriptSecurityManager::CheckLoadURIWithPrincipal(a,b,0) we are limited to triggering this via the base URLs being:
 * cross-domain
 * cross-some-schemes (http <-> chrome, file, javascript, ...)
... unless i'm missing some other approach.
Comment on attachment 684752 [details] [diff] [review]
Mochitest

ok. We cannot commit the patch until after this has been released, though. Only the patch should be committed (and should have a very generic commit message which doesn't point to the security issue).
Attachment #684752 - Flags: review?(benjamin) → review+
(Assignee)

Comment 24

4 years ago
Created attachment 688432 [details] [diff] [review]
Unify base URI usage, beta version
(Assignee)

Comment 25

4 years ago
Created attachment 688434 [details] [diff] [review]
Unify base URI usage

[Security approval request comment]
How easily can the security issue be deduced from the patch?
The ability to avoid cross-domain security checks isn't directly discernible, but can possibly deduced by someone sufficiently motivated or sufficiently knowledgeable of the code base.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comments or checkins don't and the test will only be checked in after release.

Which older supported branches are affected by this flaw?
firefox17+.

If not all supported branches, which bug introduced the flaw?
[not applicable]

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports are not required, the beta branch only required a trivial adjustment for a header.

How likely is this patch to cause regressions; how much testing does it need?
It is unlikely to cause regressions as this fixes the URL resolving for a relatively obscure code path (base URL lookup for plugins inside svg elements etc.). I completed local test runs for the affected branches.
Attachment #686684 - Attachment is obsolete: true
Attachment #688434 - Flags: sec-approval?
(Assignee)

Comment 26

4 years ago
Should we target 17 ESR with this?
tracking-firefox-esr17: --- → ?
Yes.
Comment on attachment 688434 [details] [diff] [review]
Unify base URI usage

sec-approval+. Please don't check in the test until after a fixed version ships.
Attachment #688434 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 29

4 years ago
Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab23919fe636
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
(Assignee)

Comment 30

4 years ago
Turns out that [leave open] is not correct for this situation.
Flags: in-testsuite?
Whiteboard: [leave open]
(Assignee)

Comment 31

4 years ago
https://hg.mozilla.org/mozilla-central/rev/ab23919fe636
(Assignee)

Comment 32

4 years ago
Comment on attachment 688434 [details] [diff] [review]
Unify base URI usage

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is sec-crit.

User impact if declined: 
Drive-by exploit for privilege escalation

Fix Landed on Version:
20

Risk to taking this patch (and alternatives if risky): 
Relatively low risk as this fixes the URL resolving for a relatively obscure code path (base URL lookup for plugins inside svg elements etc.). I completed local test runs for the affected branches.

String or UUID changes made by this patch: 
None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
[not applicable]

User impact if declined: 
See above.

Testing completed (on m-c, etc.): 
Landed on m-c without incidents, completed local test runs. Automated test coverage will have to land after the fix is released.

Risk to taking this patch (and alternatives if risky): 
See above.

String or UUID changes made by this patch: 
None.
Attachment #688434 - Flags: checkin+
Attachment #688434 - Flags: approval-mozilla-esr17?
Attachment #688434 - Flags: approval-mozilla-beta?
Attachment #688434 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
status-firefox20: affected → fixed
Target Milestone: --- → mozilla20
Comment on attachment 688434 [details] [diff] [review]
Unify base URI usage

Will track for the ESR17 that ships with 18 - please go ahead and land as per https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
Attachment #688434 - Flags: approval-mozilla-esr17?
Attachment #688434 - Flags: approval-mozilla-esr17+
Attachment #688434 - Flags: approval-mozilla-beta?
Attachment #688434 - Flags: approval-mozilla-beta+
Attachment #688434 - Flags: approval-mozilla-aurora?
Attachment #688434 - Flags: approval-mozilla-aurora+
status-firefox-esr17: --- → affected
tracking-firefox-esr17: ? → 18+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f9e048ca23dc
https://hg.mozilla.org/releases/mozilla-beta/rev/1cb7d7111d57

I attempted to land this on esr17 but had to back it out for build bustage.

e:/builds/moz2_slave/m-esr17-w32/build/dom/plugins/base/nsPluginHost.cpp(3126) : error C2039: 'GetBaseURI' : is not a member of 'nsIPluginInstanceOwner'

e:/builds/moz2_slave/m-esr17-w32/build/dom/plugins/base/nsPluginHost.cpp(3139) : error C2039: 'GetDOMElement' : is not a member of 'nsIPluginInstanceOwner'

e:/builds/moz2_slave/m-esr17-w32/build/dom/plugins/base/nsPluginHost.cpp(3249) : error C2660: 'nsNPAPIPluginInstance::GetOwner' : function does not take 0 arguments
status-firefox18: affected → fixed
status-firefox19: affected → fixed
(Assignee)

Comment 35

4 years ago
Created attachment 689291 [details] [diff] [review]
Unify base URI usage, ESR 17 version

Sorry about that.

bz, do you have any problems with this adoption for ESR17? Those casts are admittedly ugly but safe and i would have to partially backport bug 797100 otherwise (which would touch additional files).
Attachment #689291 - Flags: review?(bzbarsky)
Comment on attachment 689291 [details] [diff] [review]
Unify base URI usage, ESR 17 version

Seems fine.
Attachment #689291 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 37

4 years ago
https://hg.mozilla.org/releases/mozilla-esr17/rev/9fbe111deff5
status-firefox-esr17: affected → fixed
(In reply to Georg Fritzsche [:gfritzsche] from comment #37)
> https://hg.mozilla.org/releases/mozilla-esr17/rev/9fbe111deff5

Would you mind preparing a patch for ESR10? If it's the same one as for the other branches, a=akeybl.
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 18+
(Assignee)

Comment 39

4 years ago
(In reply to Alex Keybl [:akeybl] from comment #38)
> Would you mind preparing a patch for ESR10?

Looking into it.
(Assignee)

Comment 40

4 years ago
The original exploit doesn't affect ESR10. The underlying issue here (inconsistent resolving of the base URI) already exists in ESR10, but it ends up
with a different (safe) base URI after the security check in nsIContent::GetBaseURI():

https://hg.mozilla.org/releases/mozilla-esr10/file/164e6a42a414/content/base/src/nsGenericElement.cpp#l1493
(that method moved and changed slightly)

In ESR10 it starts with the non-chrome base URI, finds 1 xml:base attribute (the chrome URI), rejects that one via the security manager and hence returns the non-chrome URI.

This however still looks pretty exploitable, as only the last xml:base is checked against the security manager. This could again be a different base URI than the document base URI that nsPluginHost::DoURLLoadSecurityCheck() checked.
(Assignee)

Comment 41

4 years ago
Created attachment 690358 [details] [diff] [review]
Unify base URI usage, ESR 10 version

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
(sec crit)

User impact if declined: 
Possible privilege escalation due to incosistent URI resolving. Note that there is currently no known exploit for ESR10 as lined out in comment 40.

Fix Landed on Version: 
17+, ESR17

Risk to taking this patch (and alternatives if risky): 
Relatively low risk as this fixes the URL resolving for a relatively obscure code path (base URL lookup for plugins inside svg elements etc.). I completed local test runs.

String or UUID changes made by this patch: 
None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #690358 - Flags: approval-mozilla-esr10?
(Assignee)

Updated

4 years ago
status-firefox-esr10: affected → checkin-pending

Updated

4 years ago
Flags: sec-bounty? → sec-bounty+
(Assignee)

Updated

4 years ago
Attachment #689291 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #688432 - Flags: checkin+
Attachment #690358 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 43

4 years ago
Comment on attachment 690358 [details] [diff] [review]
Unify base URI usage, ESR 10 version

https://hg.mozilla.org/releases/mozilla-esr10/rev/33e4424595d6
Attachment #690358 - Flags: checkin+
(Assignee)

Comment 44

4 years ago
Comment on attachment 690358 [details] [diff] [review]
Unify base URI usage, ESR 10 version

Backed out for build bustage:
https://hg.mozilla.org/releases/mozilla-esr10/rev/175bcce07cd1

Sorry, built fine locally.
Attachment #690358 - Flags: checkin+
(Assignee)

Comment 45

4 years ago
Created attachment 692315 [details] [diff] [review]
Unify base URI usage, ESR 10 version

Fixed nullptr/nsnull bustage.
https://tbpl.mozilla.org/?tree=Try&rev=74889ea752f5
Attachment #690358 - Attachment is obsolete: true
(Assignee)

Comment 46

4 years ago
Comment on attachment 692315 [details] [diff] [review]
Unify base URI usage, ESR 10 version

Looks like the try build environment doesn't work for ESR10 anymore (configure failures).
The nsnull/nullptr fix should have been all that's needed, pushed to ESR10: https://hg.mozilla.org/releases/mozilla-esr10/rev/b894c9c7239d
Attachment #692315 - Flags: checkin+
(Assignee)

Updated

4 years ago
status-firefox-esr10: checkin-pending → fixed
In regressing this bug, the new behavior does not seem clear to me.

All ESR builds I that I've tested so far - 10.0.9, 10.0.10 and 10.0.11 (12-16-2012) behave identically. The iframe in the PoC populates with a page from safe.com, but nothing else happens. The error console has a lot of errors, including many of these, as expected:

Security Error: Content at <my_test_domain> may not load or link to chrome://browser/content/browser.xul.

But, the ESR builds don't appear to vulnerable to the exploit.

If I run the PoC in latest (12-16-2012) 19/20 nightly, I get an empty iframe, and the error console has just one instance of the above error. This seems ideal.

If I run the PoC in the current 17.0.1 release, I see the exploit in action, and the error console does not have the above errors.

So, the question I have - was ESR ever affected? I see no indication thus far that it was, or that the latest change makes a difference. 

Regardless, current ESR shows no exploit, so that's good. I'd just like to make sure we're happy with its behavior.
(Assignee)

Comment 48

4 years ago
(In reply to Matt Wobensmith from comment #47)
> So, the question I have - was ESR ever affected? 

No, the exploit doesn't work on ESR10. A similar approach might work though (see comment 40), thats why ESR10 approval was requested.
Georg, thank you, and I'm sorry for missing that. :\

Looks good.

Confirmed exploit on release Firefox 17.0.1.
Confirmed exploit on nightly beta 2012-11-21, Firefox18.0a2
Confirmed exploit on nightly release 2012-11-21, Firefox19.0a1
Verified fixed on build 2012-12-16, Firefox 10.0.11 ESR
Verified fixed on build 2012-12-16, Firefox 17.0.1 ESR
Verified fixed on build 2012-12-16, Firefox 18.0
Verified fixed on build 2012-12-16, Firefox 19.0a2
Verified fixed on build 2012-12-16, Firefox 20.0a1
Status: RESOLVED → VERIFIED
status-firefox-esr10: fixed → verified
status-firefox18: fixed → verified
status-firefox19: fixed → verified
status-firefox20: fixed → verified
status-firefox-esr17: fixed → verified
Whiteboard: [adv-main18+][adv-esr17+][adv-esr10+]
Alias: CVE-2013-0758

Comment 50

4 years ago
With Firefox 10.0.12 ESR we've hit this issue:
https://bugzilla.redhat.com/show_bug.cgi?id=907388 
(see backtrace).
Looks like mContent is not initialized when calling nsPluginInstanceOwner::GetBaseURI
(Assignee)

Comment 51

4 years ago
Right, apparently it's just ESR10 that's missing nsPluginInstanceOwner::mContent initialization, 17+ should be unaffected.
(Assignee)

Comment 52

4 years ago
So - akeybl, lsblakk, i don't think we're still doing releases for ESR10, are we?
(Assignee)

Comment 53

4 years ago
Created attachment 710150 [details] [diff] [review]
Fix missing initialization

In case we are not doing another release but you need a patch: I've looked through the usage of GetBaseURI() and fixing the initialization should be fine.
(In reply to Georg Fritzsche [:gfritzsche] from comment #52)
> So - akeybl, lsblakk, i don't think we're still doing releases for ESR10,
> are we?

Nope, no planned releases for ESR10 at this point. Only chemspills if necessary (in the next week).
Group: core-security
(Assignee)

Comment 55

4 years ago
Pushed test to inbound:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/fba52fc5161a
(Assignee)

Comment 56

4 years ago
Unused variable fixup:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/230f42e5e26d
(Assignee)

Comment 57

4 years ago
Relanded after try run (yay, -Werror platforms):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec124ace4cb
https://hg.mozilla.org/mozilla-central/rev/7ec124ace4cb
(Assignee)

Comment 59

4 years ago
No further incidents on trunk, so landed on ESR17:
https://hg.mozilla.org/releases/mozilla-esr17/rev/440bd308471f
(Assignee)

Updated

4 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.