crash [@ ParseXMLSource / jsxml.c:1978]

VERIFIED FIXED in mozilla1.9.2a1

Status

()

defect
P2
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: aha, Assigned: igor)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.11 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] fixed-in-tracemonkey, crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

11 years ago
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
Version: 1.8 Branch → Trunk

Comment 1

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

Comment 2

11 years ago
Confirmed on Mac.
Severity: normal → critical
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All

Comment 3

11 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9?
Reporter

Comment 4

11 years ago
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

Comment 6

11 years ago
These all look like null-pointer derefs, so not sec-sensitive.  We should fix, but not block.
Flags: blocking1.9+

Comment 7

11 years ago
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
Flags: wanted1.9.1?

Updated

10 years ago
Summary: crash @ [ParseXMLSource / jsxml.c:1978] → crash [@ ParseXMLSource / jsxml.c:1978]
Assignee

Updated

10 years ago
Assignee: general → igor
Assignee

Comment 8

10 years ago
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?]
Assignee

Comment 9

10 years ago
(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/>; 
}
Assignee

Comment 10

10 years ago
Posted 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)
Assignee

Updated

10 years ago
Whiteboard: [sg:critical?] → [sg:critical]
Assignee

Updated

10 years ago
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.0.10?

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+
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
Attachment #369882 - Flags: review?(brendan) → review+
Assignee

Comment 12

10 years ago
(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.

/be
Assignee

Comment 14

10 years ago
(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.
Assignee

Comment 15

10 years ago
landed to TM - http://hg.mozilla.org/tracemonkey/rev/6ee8f1b28425
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey

Comment 16

10 years ago
http://hg.mozilla.org/mozilla-central/rev/6ee8f1b28425
Status: NEW → RESOLVED
Last Resolved: 10 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.
Assignee

Comment 19

10 years ago
Posted 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.

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);
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 1.9.0.10, a=dveditz for release-drivers
Assignee

Comment 21

10 years ago
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
Keywords: fixed1.9.0.10

Updated

10 years ago
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
Status: RESOLVED → VERIFIED
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:1.9.0.11pre) 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

Comment 26

10 years ago
http://hg.mozilla.org/tracemonkey/rev/3183adcd9699
/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.