466576 - Null deref [@ nsSVGTransformList::GetValueString] after failed appendItem

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

appendItem throws "Error: Permission denied for <file://> to create wrapper for object of class UnnamedClass" but sets up innerHTML for a crash.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch [Checkin: Comment 11 & 13]

Similar patch to bug 372046. Tells us there that we have a problem bug 372046 comment 7

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm confused. Why are {} and function(){} treated as valid values for arbitrary XPCOM interfaces? Shouldn't XPConnect be throwing before we get into C++?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In fact, I think allowing untrusted script to implement our DOM interfaces is, in almost all cases, a huge mistake. Trying to bulletproof our code to handle it is a fool's errand IMHO.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #2)
> I'm confused. Why are {} and function(){} treated as valid values for arbitrary
> XPCOM interfaces? Shouldn't XPConnect be throwing before we get into C++?

There are cases like event listeners when {} should be supported.
(Though usually it is { handleEvent: function (evt) {} })

We have had similar crashes before at least in SVG, found by some fuzzer IIRC.
DOM APIs convert parameters to nsINode or similar so those should be safe.
At least I haven't noticed any problematic code.

I thought it is very well known fact that {} can be used as an
implementation of a scriptable interface.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I know there are a few XPCOM/DOM interfaces which need to be implementable by client script. And I know functions should be usable for "function callback" DOM interfaces. But these should be the exception rather than the rule, shouldn't they?

> DOM APIs convert parameters to nsINode or similar so those should be safe.

How does that work? It looks to me like content code just calls NODE_FROM which does a static_cast from nsIDOMNode* to nsINode*. How do we know that's safe?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

NODE_FROM isn't used with nsIDOMNodes, only with nsINode/nsIContent/nsIDocument

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

insertBefore/appendChild/replaceChild/etc DOM methods QI the parameters to nsIContent, which is C++ only interface. Code which handles attribute nodes
uses nsIAttribute, not nsIDOMAttr, range code uses nsIRange, not nsIDOMRange...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 350063 [details] [diff] [review]
patch
[Checkin: Comment 11 & 13]

OK, I still think it's really poor to have to allow untrusted JS to implement DOM interfaces willy-nilly, but if I can't convince anyone of that, then I guess we need this.

Comment 9

[Robert Longson [:longsonr]]

Comment on attachment 350063 [details] [diff] [review]
patch
[Checkin: Comment 11 & 13]

The patch is similar in structure to bug 372046 which we've had in the codebase for a long time. Hopefully that means this patch will be low risk. The first attachment can become a crashtest.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We've thought in the past about only allowing certain DOM interfaces to be implemented, yes.  That would require some XPConnect changes, of course, and some DOM interfaces do need to be implementable by random JS objects, not all of which are functions.  So we'd have to do some annotation on a per-interface basis...

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 350063 [details] [diff] [review]
patch
[Checkin: Comment 11 & 13]

http://hg.mozilla.org/mozilla-central/rev/95220a88eb95

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 350063 [details] [diff] [review]
patch
[Checkin: Comment 11 & 13]

a191=beltzner

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 350063 [details] [diff] [review]
patch
[Checkin: Comment 11 & 13]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e613045aa8a9

Comment 14

[Aakash Desai [:aakashd]]

Seeing that there haven't been any duplicate sightings for this issue for 3
months, I'm going to go ahead and verify this unless someone has an issue with
that.

Comment 15

[Bob Clary [:bc] (inactive)]

1.9.0 
PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsSVGTransformList::GetValueString

Comment 16

[Daniel Veditz [:dveditz]]

we'll put blocking at 1.9.0.12 since we're winding down 1.9.0.11 and don't want to hound you, but we'll approve the patch for 1.9.0.11 if you want and can get to it (add the approval request flag). Code-freeze is Wednesday May 6.(In reply to comment #15)

> 1.9.0 
> PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address
> controls Code Flow starting at gklayout!nsSVGTransformList::GetValueString

This is one of the our code patterns that !exploitable gets wrong. We're getting a null deref because we've explicitly nulled out the value. !exploitable worries that if the null is there because it was unexpected then maybe it could just as easily be some other value and therefore be exploited. !exploitable is biased toward "no false negatives".

Comment 17

[Robert Longson [:longsonr]]

Comment on attachment 350063 [details] [diff] [review]
patch
[Checkin: Comment 11 & 13]

This has been in the trunk for a while and is in 3.1 beta 3. The patch applies cleanly to 3.0 and stops the testcase crashing there too.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 350063 [details] [diff] [review]
patch
[Checkin: Comment 11 & 13]

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 19

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

Verified for 1.9.0.12 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009070105 GranParadiso/3.0.12pre (.NET CLR 3.5.30729). Testcase no longer crashes as it does with 1.9.0.11.