Closed Bug 426520 Opened 13 years ago Closed 12 years ago

crash [@ ParseXMLSource / jsxml.c:1978]


(Core :: JavaScript Engine, defect, P2)






(Reporter: aha, Assigned: igor)


(5 keywords, Whiteboard: [sg:critical] fixed-in-tracemonkey)

Crash Data


(3 files, 1 obsolete file)

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:
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 Address	0x0

Some reports:
Version: 1.8 Branch → Trunk
Strangely, this testcase only crashes in the browser, not in the standalone js shell.
Confirmed on Mac.
Severity: normal → critical
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
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.
Ever confirmed: true
Flags: blocking1.9?
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
Keywords: regression
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.
Assignee: general → crowder
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
These all look like null-pointer derefs, so not sec-sensitive.  We should fix, but not block.
Flags: blocking1.9+
Dropping off my list for now, since it is not a blocker.  Will pick up again later, if no one else has gotten it.
Assignee: crowder → general
Summary: crash @ [ParseXMLSource / jsxml.c:1978] → crash [@ ParseXMLSource / jsxml.c:1978]
Assignee: general → igor
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.
Group: core-security
Whiteboard: [sg:critical?]
(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/>; 
Attached patch v1 (obsolete) — Splinter Review
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.
Attachment #369882 - Flags: review?(brendan)
Whiteboard: [sg:critical?] → [sg:critical]
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?
Flags: blocking1.9.1? → blocking1.9.1+
Comment on attachment 369882 [details] [diff] [review]

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

Thanks for fixing.

Attachment #369882 - Flags: review?(brendan) → review+
(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.
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.

(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.
landed to TM -
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey
Closed: 12 years ago
Resolution: --- → FIXED
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.10?
Flags: blocking1.9.0.10+
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.
Attached patch v1 for 1.9.0Splinter Review
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.

< Index: tm/js/src/jsxml.cpp                                                                                                                                       
> Index: jsxml.c                                                                                                                                                   
< --- 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);                                                                                                                           
<      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)                                                                                                                                                 
<      v = OBJECT_TO_JSVAL(ns);                                                                                                                                    
>      v = OBJECT_TO_JSVAL(nsobj);
Attachment #369882 - Attachment is obsolete: true
Attachment #373457 - Flags: approval1.9.0.10?
Attachment #373457 - Attachment is patch: true
Attachment #373457 - Attachment mime type: application/octet-stream → text/plain
Attachment #373457 - Flags: approval1.9.0.10? → approval1.9.0.10+
Comment on attachment 373457 [details] [diff] [review]
v1 for 1.9.0

Approved for, a=dveditz for release-drivers
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
Keywords: fixed1.9.0.10
Flags: in-testsuite+
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
Target Milestone: --- → mozilla1.9.2a1
Verified fixed with the testcase using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/2009051111 GranParadiso/3.0.11pre. Verified the crash using 3.0.10 on same box.
Does not affect the 1.8 branch
Flags: wanted1.8.1.x-
Group: core-security
/cvsroot/mozilla/js/tests/e4x/Regress/regress-426520.js,v  <--  regress-426520.js
initial revision: 1.1
Crash Signature: [@ ParseXMLSource / jsxml.c:1978]
You need to log in before you can comment on or make changes to this bug.