Mariusz Mlynski Reporter
	Description • 12 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.

	Mariusz Mlynski Reporter
	Comment 1 • 12 years ago

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.

	Mariusz Mlynski Reporter
	Comment 2 • 12 years ago

Comment on attachment 683937 [details]
Exploit

Uh, jar spoils things again... Run it unpacked.

	Benjamin Smedberg
	Comment 3 • 12 years ago

Is the source for loader.swf available?

	Mariusz Mlynski Reporter
	Comment 4 • 12 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>

	Benjamin Smedberg
	Comment 5 • 12 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 6 • 12 years ago

Will look into this tomorrow.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 7 • 12 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 8 • 12 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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 9 • 12 years ago

Boris, do you happen to know what the proper behaviour here should be so that we can unify that URL resolving?

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 12 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 11 • 12 years ago

(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

	Mariusz Mlynski Reporter
	Comment 12 • 12 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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 14 • 12 years ago

The above patches pass the mochitests locally, any suggestions on how to sneak them through a try run?

	Benjamin Smedberg
	Comment 15 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 12 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 17 • 12 years ago

Adress review comments.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 12 years ago

Comment on attachment 686684 [details] [diff] [review]
Unify base URI usage

r=me.  Thanks!

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 19 • 12 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

	Benjamin Smedberg
	Comment 20 • 12 years ago

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?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 21 • 12 years ago

I can't think of something that's not involving chrome etc., will have to recheck tomorrow.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 22 • 12 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.

	Benjamin Smedberg
	Comment 23 • 12 years ago

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).

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 25 • 12 years ago

[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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 26 • 12 years ago

Should we target 17 ESR with this?

	Benjamin Smedberg
	Comment 27 • 12 years ago

Yes.

	Al Billings [:abillings - ex-MoCo]
	Comment 28 • 12 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 29 • 12 years ago

Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/ab23919fe636

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 30 • 12 years ago

Turns out that [leave open] is not correct for this situation.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 31 • 12 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 32 • 12 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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 33 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 12 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 35 • 12 years ago

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).

	Boris Zbarsky [:bzbarsky]
	Comment 36 • 12 years ago

Comment on attachment 689291 [details] [diff] [review]
Unify base URI usage, ESR 17 version

Seems fine.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 37 • 12 years ago

https://hg.mozilla.org/releases/mozilla-esr17/rev/9fbe111deff5

	Alex Keybl [:akeybl]
	Comment 38 • 12 years ago

(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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 39 • 12 years ago

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

Looking into it.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 40 • 12 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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 41 • 12 years ago

[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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 42 • 12 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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 43 • 12 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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 44 • 12 years ago

Fixed nullptr/nsnull bustage.
https://tbpl.mozilla.org/?tree=Try&rev=74889ea752f5

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 45 • 12 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

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 46 • 12 years ago

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.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 47 • 12 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.

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 48 • 12 years ago

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

	Jan Horak [:jhorak]
	Comment 49 • 12 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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 50 • 12 years ago

Right, apparently it's just ESR10 that's missing nsPluginInstanceOwner::mContent initialization, 17+ should be unaffected.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 51 • 12 years ago

So - akeybl, lsblakk, i don't think we're still doing releases for ESR10, are we?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 52 • 12 years ago

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.

	Alex Keybl [:akeybl]
	Comment 53 • 12 years ago

(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).

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 54 • 12 years ago

Pushed test to inbound:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/fba52fc5161a

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 55 • 12 years ago

Unused variable fixup:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/230f42e5e26d

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 56 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 57 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/7ec124ace4cb

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 58 • 12 years ago

No further incidents on trunk, so landed on ESR17:
https://hg.mozilla.org/releases/mozilla-esr17/rev/440bd308471f