805024 - (CVE-2013-0752) Crash when XML binding (XBL) includes another binding which contains SVG

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Sviatoslav Chagaev]

Attachment: ff_xbl_svg_crash.zip

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010144125

Steps to reproduce:

The XBL file contains two XML bindings, "pie_chart" and "pie_chart_segment".
Both contain SVG. When you insert "pie_chart_segment" into "pie_chart" with the help of the <children/> XBL tag -- Firefox crashes.

Reproducible: always.

How to reporoduce:
1. Install 'Remote XUL Manager' addon, so you can open local XUL files https://addons.mozilla.org/en-US/firefox/addon/remote-xul-manager/
2. Add a <file> record in Remote XUL Manager's allowed URL list
3. Unpack ff_xbl_svg_crash.zip
4. Open test.xul

Firefox will crash.

Comment 1

[Robert Longson [:longsonr]]

Can you post the about:crashes report ID please? starts bp-

Comment 2

[Alice0775 White]

bp-d66694d3-cc47-43da-9008-adfb82121024
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/f2b2b99108a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517030029
Crash:
http://hg.mozilla.org/mozilla-central/rev/895e12563245
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517110214
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2b2b99108a2&tochange=895e12563245


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/37f2536e975e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516204627
Crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2c3647738e81
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516230826
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37f2536e975e&tochange=2c3647738e81

In local build:
Last Good: d3b11e443f04
First Bad: 05a339620439

Triggered by:
05a339620439	Jonathan Watt — Bug 734082 - Compute and store bounds and visual overflow bounds for both SVG leaf and container frames. r=roc.

Comment 3

[Sviatoslav Chagaev]

(In reply to Robert Longson from comment #1)
> Can you post the about:crashes report ID please? starts bp-

FF 16.0.1, Windows 7 32bit: https://crash-stats.mozilla.com/report/index/bp-8f975b99-b30b-4d6e-b401-e9ebd2121025

FF 16.0.1, Fedora Linux 17 64bit: https://crash-stats.mozilla.com/report/index/bp-e3acc15d-9848-4dd0-8225-eba722121025

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

Comment 5

[Daniel Holbert [:dholbert]]

I think this is security-sensitive.

HasValidDimensions() and GetAnimatedTransformList() are both virtual functions, and we're calling them on objects of the wrong type, so we're making a bogus virtual-function call.  That could be exploitable.

Comment 6

[Daniel Holbert [:dholbert]]

(*

Comment 7

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> I think this is security-sensitive.
> 
> HasValidDimensions() and GetAnimatedTransformList() are both virtual
> functions, and we're calling them on objects of the wrong type, so we're
> making a bogus virtual-function call.  That could be exploitable.

It would be except that you can only trigger it using xul and remote xul is disabled so we get away with it.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 681701 [details] [diff] [review]
patch

>diff --git a/layout/svg/nsSVGContainerFrame.cpp b/layout/svg/nsSVGContainerFrame.cpp
>--- a/layout/svg/nsSVGContainerFrame.cpp
>+++ b/layout/svg/nsSVGContainerFrame.cpp
>@@ -93,17 +93,18 @@ nsSVGDisplayContainerFrame::Init(nsICont
>   return rv;
> }
> 
> NS_IMETHODIMP
> nsSVGDisplayContainerFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
>                                              const nsRect&           aDirtyRect,
>                                              const nsDisplayListSet& aLists)
> {
>-  if (!static_cast<const nsSVGElement*>(mContent)->HasValidDimensions()) {
>+  if (mContent->IsSVG() && 

Trailing whitespace: -------^   (remove)

r=me with that fixed.  May want to hold off on the testcase... I'll defer to dveditz on that.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #7)
> It would be except that you can only trigger it using xul and remote xul is
> disabled so we get away with it.

It is disabled by default, but we do have ways of letting users enable it:
  https://developer.mozilla.org/en-US/docs/Using_Remote_XUL

If you had e.g. an enterprise environment who had enabled it for a specific whitelisted origin, someone could post a nasty XUL file on that origin and successfully exploit this.  It's a bit contrived, but worth considering.

Comment 10

[Robert Longson [:longsonr]]

Comment on attachment 681701 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch? pretty difficult,  from the code change, pretty easy if I include the testcase as a crashtest in the patch though.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The crashtest certainly would!

Which older supported branches are affected by this flaw? Anything from 15 onwards. However you'd need to enable remote xul to make this remotely exploitable so 99.999% of users won't be affected.

If not all supported branches, which bug introduced the flaw? Bug 734082

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? They'll all be the same.

How likely is this patch to cause regressions; how much testing does it need? Pretty unlikely, it tests for whether we're going to do the bad thing that crashes us and skips it. It's a localised and simple test.

Comment 11

[Jonathan Watt [:jwatt]]

When you check in, can you add comments before the IsSVG() checks noting why we're checking that, since it's not obvious given that the frame is nsSVGDisplayContainerFrame.

Comment 12

[Jonathan Watt [:jwatt]]

Seems like we shouldn't be creating the nsSVGDisplayContainerFrame if the content isn't SVG.

Comment 13

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> When you check in, can you add comments before the IsSVG() checks noting why
> we're checking that, since it's not obvious given that the frame is
> nsSVGDisplayContainerFrame.

We'll that'll make even the code change obvious. We'll see what security says I guess!

(In reply to Jonathan Watt [:jwatt] from comment #12)
> Seems like we shouldn't be creating the nsSVGDisplayContainerFrame if the
> content isn't SVG.

We actually create an nsSVGGenericContainerFrame for the xul content, which is how it's designed to work. nsSVGGenericContainerFrame is derived from nsSVGDisplayContainerFrame though which means nsSVGDisplayContainerFrame can't assume it's content is an svg element.

Comment 14

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

Comment on attachment 681701 [details] [diff] [review]
patch

Sec-approval+.

Please don't check in the test until after a release with this issue goes live. We don't want to expose ourselves.

You should probably prepare a patch for earlier releases that are affected.

It looks like ESR10 is unaffected, right?

Comment 15

[Robert Longson [:longsonr]]

(In reply to Al Billings [:abillings] from comment #14)
> 
> It looks like ESR10 is unaffected, right?

Correct.

Comment 16

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9bf32419896

Comment 17

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #13)
> We actually create an nsSVGGenericContainerFrame for the xul content, which
> is how it's designed to work. nsSVGGenericContainerFrame is derived from
> nsSVGDisplayContainerFrame though which means nsSVGDisplayContainerFrame
> can't assume it's content is an svg element.

Ah, gotcha. Thanks.

Comment 18

[Robert Longson [:longsonr]]

(In reply to Jonathan Watt [:jwatt] from comment #11)
> When you check in, can you add comments before the IsSVG() checks noting why
> we're checking that, since it's not obvious given that the frame is
> nsSVGDisplayContainerFrame.

I'll do that as a follow up once this has rolled out.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Robert Longson [:longsonr]]

Comment on attachment 681701 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 734082
User impact if declined: only remote xul users affected by security hole
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none

Comment 21

[Daniel Holbert [:dholbert]]

Comment on attachment 681701 [details] [diff] [review]
patch

(canceling feedback request, as comment 14 answered my question about the testcase)

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 681701 [details] [diff] [review]
patch

If this can be landed before Monday 11/19 then it will be on Aurora (18) before merge to Beta.  Otherwise, re-nom for Beta.

Comment 23

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b236d7245d62

Comment 24

[Alex Keybl [:akeybl]]

wontfixing for ESR17 given the user impact in comment 20.

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to Alex Keybl [:akeybl] from comment #24)
> wontfixing for ESR17 given the user impact in comment 20.

I humbly suggest reconsidering this.  It sounds like you're basing the decision on the assumption that ESR users aren't likely to have remote XUL turned on, but I'd contest that.

I think (?) enterprise webapps (perhaps written a while ago) are one of the only remaining "legitimate" usages of remote XUL.  So, enterprise ESR users are actually *more* likely than the general user-base to have remote XUL turned on.

So I think it's worth considering our ESR userbase as potentially-vulnerable, and given the low risk of this change, I think it's worth taking this on ESR.

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> I think (?) enterprise webapps (perhaps written a while ago) are one of the
> only remaining "legitimate" usages of remote XUL.

Here are some citations for that claim, from some quick Googling:

"there is just a handful of people using it [XUL] for specialist web apps and intranet stuff."
from a random mozillazine poster here:
http://forums.mozillazine.org/viewtopic.php?f=38&t=2066135

...and here's one example of one such enterprise intranet webapp:
"Qualoupe LIMS application will not run in versions of Firefox 4 and above with its default settings, you have to enable remote XUL."
http://www.twofold-software.com/site/2011/07/enabling-remote-xul/

Comment 27

[Alex Keybl [:akeybl]]

(In reply to Daniel Holbert [:dholbert] from comment #26)
> (In reply to Daniel Holbert [:dholbert] from comment #25)
> > I think (?) enterprise webapps (perhaps written a while ago) are one of the
> > only remaining "legitimate" usages of remote XUL.
> 
> Here are some citations for that claim, from some quick Googling:
> 
> "there is just a handful of people using it [XUL] for specialist web apps
> and intranet stuff."
> from a random mozillazine poster here:
> http://forums.mozillazine.org/viewtopic.php?f=38&t=2066135
> 
> ...and here's one example of one such enterprise intranet webapp:
> "Qualoupe LIMS application will not run in versions of Firefox 4 and above
> with its default settings, you have to enable remote XUL."
> http://www.twofold-software.com/site/2011/07/enabling-remote-xul/

I stand corrected - sounds like ESR users are even more vulnerable. Thanks for pointing this out Daniel.

We'll want to land this on mozilla-esr17 as well.

Comment 28

[Daniel Holbert [:dholbert]]

Thanks -- landed on esr17:
 https://hg.mozilla.org/releases/mozilla-esr17/rev/35378151cb68

Comment 29

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

This looks like a sec-moderate to me. Anyone have any comments on this? I need a rating for the advisory.

Comment 30

[Daniel Holbert [:dholbert]]

I think this is sec-high. The keywords description page says "sec-high" means "Exploitable web vulnerabilities that can lead to the targeted compromise of a small number of users."

That sounds like an accurate description of this bug. (It's exploitable, but it only affects people w/ remote XUL turned on, which is a small number of users.)

So I'd recommend sec-high.

Comment 31

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

Ok. We often mitigate downwards for things that are pref'd off, etc. but I'll go with sec-high here.