813906 - (CVE-2013-0758) Content can access chrome-privileged pages using plugin objects

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Mariusz Mlynski]

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.

Comment 1

[Mariusz Mlynski]

Attachment: 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.

Comment 2

[Mariusz Mlynski]

Comment on attachment 683937 [details]
Exploit

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

Comment 3

[Benjamin Smedberg]

Is the source for loader.swf available?

Comment 4

[Mariusz Mlynski]

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>

Comment 5

[Benjamin Smedberg]

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.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Will look into this tomorrow.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Mochitest

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

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.

Comment 9

[Georg Fritzsche [:gfritzsche]]

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

Comment 10

[Boris Zbarsky [: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.

Comment 11

[Daniel Veditz [:dveditz]]

(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

Comment 12

[Mariusz Mlynski]

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

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Unify base URI usage

Comment 14

[Georg Fritzsche [:gfritzsche]]

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

Comment 15

[Benjamin Smedberg]

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 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Attachment: Unify base URI usage

Adress review comments.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

r=me.  Thanks!

Comment 19

[Georg Fritzsche [:gfritzsche]]

(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

Comment 20

[Benjamin Smedberg]

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?

Comment 21

[Georg Fritzsche [:gfritzsche]]

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

Comment 22

[Georg Fritzsche [:gfritzsche]]

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 23

[Benjamin Smedberg]

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

Comment 24

[Georg Fritzsche [:gfritzsche]]

Attachment: Unify base URI usage, beta version

Comment 25

[Georg Fritzsche [:gfritzsche]]

Attachment: 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.

Comment 26

[Georg Fritzsche [:gfritzsche]]

Should we target 17 ESR with this?

Comment 27

[Benjamin Smedberg]

Yes.

Comment 28

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

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.

Comment 29

[Georg Fritzsche [:gfritzsche]]

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

Comment 30

[Georg Fritzsche [:gfritzsche]]

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

Comment 31

[Georg Fritzsche [:gfritzsche]]

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

Comment 32

[Georg Fritzsche [:gfritzsche]]

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.

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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

Comment 35

[Georg Fritzsche [:gfritzsche]]

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

Comment 36

[Boris Zbarsky [:bzbarsky]]

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

Seems fine.

Comment 37

[Georg Fritzsche [:gfritzsche]]

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

Comment 38

[Alex Keybl [:akeybl]]

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

Comment 39

[Georg Fritzsche [:gfritzsche]]

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

Looking into it.

Comment 40

[Georg Fritzsche [:gfritzsche]]

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.

Comment 41

[Georg Fritzsche [:gfritzsche]]

Attachment: 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.

Comment 43

[Georg Fritzsche [:gfritzsche]]

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

https://hg.mozilla.org/releases/mozilla-esr10/rev/33e4424595d6

Comment 44

[Georg Fritzsche [:gfritzsche]]

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.

Comment 45

[Georg Fritzsche [:gfritzsche]]

Attachment: Unify base URI usage, ESR 10 version

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

Comment 46

[Georg Fritzsche [:gfritzsche]]

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

Comment 47

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

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.

Comment 48

[Georg Fritzsche [:gfritzsche]]

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

Comment 49

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

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

Comment 50

[Jan Horak [:jhorak]]

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

Comment 51

[Georg Fritzsche [:gfritzsche]]

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

Comment 52

[Georg Fritzsche [:gfritzsche]]

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

Comment 53

[Georg Fritzsche [:gfritzsche]]

Attachment: 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.

Comment 54

[Alex Keybl [:akeybl]]

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

Comment 55

[Georg Fritzsche [:gfritzsche]]

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

Comment 56

[Georg Fritzsche [:gfritzsche]]

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

Comment 57

[Georg Fritzsche [:gfritzsche]]

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

Comment 58

[Ed Morley [:emorley]]

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

Comment 59

[Georg Fritzsche [:gfritzsche]]

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