426520 - crash [@ ParseXMLSource / jsxml.c:1978]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Adam Hauner]

I'm running nightly of Firefox wit Jesse's JS fuzzer and Firefox is repeatably crashing in ParseXMLSource. I'm running fuzzer in browser, so I can't provide exact crashing testcase, but Firefox crash every time in few minutes.

Used JS fuzzer version:
https://bugzilla.mozilla.org/attachment.cgi?id=240710
in HTML wrapper.

Signature	ParseXMLSource	
UUID		311476e1-00b4-11dd-b302-001a4bd43ef6	
Time		2008-04-02 05:56:09-07:00	
Uptime		1692	
Product		Firefox	
Version		3.0pre	
Build ID	2008040112	
OS		Windows NT	
OS Version	5.1.2600 Service Pack 2
CPU		x86	
CPU Info	GenuineIntel family 6 model 14 stepping 8
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x0

Some reports:
http://crash-stats.mozilla.com/report/index/21e5f14c-00b6-11dd-ab47-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/7d973863-00b5-11dd-9cde-001a4bd43e5c
http://crash-stats.mozilla.com/report/index/f3a062b5-00b4-11dd-8dfd-001a4bd43e5c
http://crash-stats.mozilla.com/report/index/311476e1-00b4-11dd-b302-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/8e43ff21-00b4-11dd-b784-001a4bd43e5c
http://crash-stats.mozilla.com/report/index/47f6af38-00b4-11dd-8b99-001a4bd43e5c
http://crash-stats.mozilla.com/report/index/f5bfa15e-00b6-11dd-9513-001a4bd43ef6
http://crash-stats.mozilla.com/report/index/e1cf3e47-00b6-11dd-980e-001a4bd43ef6

Comment 1

[Jesse Ruderman]

Attachment: reduced testcase (crashes Firefox when loaded)

Strangely, this testcase only crashes in the browser, not in the standalone js shell.

Comment 2

[Jesse Ruderman]

Confirmed on Mac.

Comment 3

[Jesse Ruderman]

Btw, if you set browser.dom.window.dump.enabled to true and run Firefox from the command line, you can see what jsfunfuzz is doing (and therefore reproduce and reduce) even with a nightly build.

Comment 4

[Adam Hauner]

Regression window is between 2008030206 and 2008030305:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030206 Minefield/3.0b4pre
- doesn't crash

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030305 Minefield/3.0b4pre
- crash

Comment 5

[Damon Sicore (:damons)]

Crowder, can you take a look at this and make a blocking call here?  Historically, we've been pushing these to 1.9.0.x, but I'm not sure how bad this is.

Comment 6

[Brian Crowder]

These all look like null-pointer derefs, so not sec-sensitive.  We should fix, but not block.

Comment 7

[Brian Crowder]

Dropping off my list for now, since it is not a blocker.  Will pick up again later, if no one else has gotten it.

Comment 8

[Igor Bukanov]

This is a critical bug that allows to present to XML code any object as if it has Namespace class leading to memory corruption and other nasty features. 

Consider the script from the test case:

undefined = {};

with (this) { 
  throw <x/>; 
}

Here XML construction code eventually calls the js_GetDefaultXMLNamespace function when a with object is on scope chain. The function then loops over scope chain calling OBJ_GET_PROPERTY(cx, obj, JS_DEFAULT_XML_NAMESPACE_ID, &v) on each scope.

Now, JS_DEFAULT_XML_NAMESPACE_ID is a special id. The code assumes that only global objects from the embedding would see it and handle it properly. But that OBJ_GET_PROPERTY code violates this since with can delegates OBJ_GET_PROPERTY to any script-accessible object.

What happens in this particular case is that OBJ_GET_PROPERTY is applied to the global this. In the browser this is not the global object (which knows how to deal with JS_DEFAULT_XML_NAMESPACE_ID), but rather its outer view with code implementing OBJ_GET_PROPERTY in XPC_XOW_GetOrSetProperty from XPCCrossOriginWrapper.cpp. That function eventually calls JS_GetPropertyById on the global object but before that it converts any id value into a real id using JS_ValueToId. 

Now, since JS_DEFAULT_XML_NAMESPACE_ID is just JSVAL_VOID, that call converts JS_DEFAULT_XML_NAMESPACE_ID into the id corresponding to the string "undefined". Since the script overwrites in the global with an object, that object will be eventually returned to JS_DEFAULT_XML_NAMESPACE_ID and treated as a namespace object.

Comment 9

[Igor Bukanov]

(In reply to comment #8)
> This is a critical bug that allows to present to XML code any object as if it
> has Namespace class leading to memory corruption and other nasty features.

Note that on 1.9.0 branch the code calls JS_GetPrivate on the namespace object. That gives even more options as it allows to reinterpret as JSXMLNamespace any integer like in:

undefined = { a: 12345678 };

with (this) { 
  throw <x/>; 
}

Comment 10

[Igor Bukanov]

Attachment: v1

The fix skips with and block objects (the latter just to be sure) when doing the namespace scope search. I also changes the loop so obj is only assigned if it is something that can hold the namespaces.

I think we should really consider the option of stopping supporting E4X. With its negligible usability its complexity tax (leading to constant flow of critical bugs) is just to high.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 369882 [details] [diff] [review]
v1

I should have made the id some kind of reserved and unforgeable QName object id, not JSVAL_VOID.

Thanks for fixing.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> (From update of attachment 369882 [details] [diff] [review])
> I should have made the id some kind of reserved and unforgeable QName object
> id, not JSVAL_VOID.

I doubt it would make the bug less dangerous. A single exposed to script class with a resolve hook or a class getter that does, for example, toString converssion on the passed id, would defeat that.

Comment 13

[Brendan Eich [:brendan]]

That's the thing: the default xml namespace id should not convert to string -- it should throw on any use other than as a namespace.

/be

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> That's the thing: the default xml namespace id should not convert to string --
> it should throw on any use other than as a namespace.

Even if such id would throw the code would still have to ensure that it is never leaked into the script since any leak would allow for a script to alter its value. 

A safer solution IMO would be not to use the id at all but rather replace it with a weak varobject->ns hashtable. That would have prevented this bug.

Comment 15

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/6ee8f1b28425

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6ee8f1b28425

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9e65b45a4a91

Comment 18

[Samuel Sidler (old account; do not CC)]

Igor, does this patch apply to 1.9.0? Or can you work up a patch for us to take? Code freeze is April 22.

Comment 19

[Igor Bukanov]

Attachment: v1 for 1.9.0

For 1.9.0 the patch required a trivial backport - the plain diff between patches below shows that the only difference comes from the context above and the patch hunk.

1c1
< Index: tm/js/src/jsxml.cpp                                                                                                                                       
---
> Index: jsxml.c                                                                                                                                                   
3,9c3,12
< --- tm.orig/js/src/jsxml.cpp  2009-03-29 14:52:36.000000000 +0200                                                                                                
< +++ tm/js/src/jsxml.cpp       2009-03-29 16:40:46.000000000 +0200                                                                                                
< @@ -7713,25 +7713,28 @@ js_GetDefaultXMLNamespace(JSContext *cx,                                                                                                 
<      fp = js_GetTopStackFrame(cx);                                                                                                                               
<      ns = fp->xmlNamespace;                                                                                                                                      
<      if (ns) {                                                                                                                                                   
<          *vp = OBJECT_TO_JSVAL(ns);                                                                                                                              
---
> RCS file: /cvsroot/mozilla/js/src/jsxml.c,v                                                                                                                      
> retrieving revision 3.215                                                                                                                                        
> diff -U 8 -p -r3.215 jsxml.c                                                                                                                                     
> --- jsxml.c   20 Oct 2008 21:32:52 -0000      3.215                                                                                                              
> +++ jsxml.c   18 Apr 2009 09:44:42 -0000                                                                                                                         
> @@ -7836,25 +7836,28 @@ js_GetDefaultXMLNamespace(JSContext *cx,                                                                                                 
>      fp = cx->fp;                                                                                                                                                
>      nsobj = fp->xmlNamespace;                                                                                                                                   
>      if (nsobj) {                                                                                                                                                
>          *vp = OBJECT_TO_JSVAL(nsobj);                                                                                                                           
31,32c34,35
<      ns = js_ConstructObject(cx, &js_NamespaceClass.base, NULL, obj, 0, NULL);                                                                                   
<      if (!ns)                                                                                                                                                    
---
>      nsobj = js_ConstructObject(cx, &js_NamespaceClass.base, NULL, obj, 0, NULL);                                                                                
>      if (!nsobj)                                                                                                                                                 
34c37
<      v = OBJECT_TO_JSVAL(ns);                                                                                                                                    
---
>      v = OBJECT_TO_JSVAL(nsobj);

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 373457 [details] [diff] [review]
v1 for 1.9.0

Approved for 1.9.0.10, a=dveditz for release-drivers

Comment 21

[Igor Bukanov]

landed to 1.9.0:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.216; previous revision: 3.215
done

Comment 22

[Bob Clary [:bc] (inactive)]

Attachment: e4x/Regress/regress-426520.js

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with testcase given in comment 0 on trunk and 1.9.1 with the
following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090422 Minefield/3.6a1pre ID:20090422224452

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090422 Shiretoko/3.5b4pre ID:20090422122043

Comment 24

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

Verified fixed with the testcase using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11pre) Gecko/2009051111 GranParadiso/3.0.11pre. Verified the crash using 3.0.10 on same box.

Comment 25

[Daniel Veditz [:dveditz]]

Does not affect the 1.8 branch

Comment 26

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/3183adcd9699
/cvsroot/mozilla/js/tests/e4x/Regress/regress-426520.js,v  <--  regress-426520.js
initial revision: 1.1