Last Comment Bug 326615 - CVE-2006-1530 compareDocumentPosition with E4X crashes [@ XPCWrappedNativeScope::SystemIsBeingShutDown]
: CVE-2006-1530 compareDocumentPosition with E4X crashes [@ XPCWrappedNativeSco...
Status: RESOLVED FIXED
[sg:critical] based on stack in comme...
: crash, testcase, verified1.8.0.2, verified1.8.1
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: ---
Assigned To: Brendan Eich [:brendan]
: Phil Schwartau
:
Mentors:
Depends on: 327697
Blocks: 326633
  Show dependency treegraph
 
Reported: 2006-02-09 20:20 PST by Jesse Ruderman
Modified: 2007-12-23 16:46 PST (History)
6 users (show)
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (77 bytes, text/html)
2006-02-09 20:21 PST, Jesse Ruderman
no flags Details
stack trace (5.87 KB, text/plain)
2006-02-09 20:22 PST, Jesse Ruderman
no flags Details
read and commence puking (2.93 KB, patch)
2006-02-09 21:46 PST, Brendan Eich [:brendan]
mrbkap: review-
Details | Diff | Splinter Review
just the rooting fix (481 bytes, patch)
2006-02-27 21:51 PST, Brendan Eich [:brendan]
brendan: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
just the rooting fix (481 bytes, patch)
2006-02-27 21:51 PST, Brendan Eich [:brendan]
brendan: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
just the rooting fix (481 bytes, patch)
2006-02-27 21:52 PST, Brendan Eich [:brendan]
brendan: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
just the rooting fix (481 bytes, patch)
2006-02-27 21:52 PST, Brendan Eich [:brendan]
brendan: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
just the rooting fix (481 bytes, patch)
2006-02-27 21:55 PST, Brendan Eich [:brendan]
brendan: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
just the rooting fix (481 bytes, patch)
2006-02-27 21:56 PST, Brendan Eich [:brendan]
brendan: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
just the rooting fix (481 bytes, patch)
2006-02-27 21:59 PST, Brendan Eich [:brendan]
mrbkap: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review

Description Jesse Ruderman 2006-02-09 20:20:15 PST
This crashes Firefox (tested with today's trunk on Mac)

<body><script>document.body.compareDocumentPosition(<foo/>);</script></body>

The crash appears to be exploitable, so I'm making this a security-sensitive bug.
Comment 1 Jesse Ruderman 2006-02-09 20:21:23 PST
Created attachment 211342 [details]
testcase
Comment 2 Jesse Ruderman 2006-02-09 20:22:10 PST
Created attachment 211343 [details]
stack trace

Note: it hangs for a while before crashing, so it's hard to get the crash in a debug build :(
Comment 3 Brendan Eich [:brendan] 2006-02-09 21:00:44 PST
nsXPCWrappedJSClass::CallMethod is failing to protect its result jsval local from the JS GC.

/be
Comment 4 Brendan Eich [:brendan] 2006-02-09 21:46:50 PST
Created attachment 211350 [details] [diff] [review]
read and commence puking

If you minus it, you own it.

/be
Comment 5 Brendan Eich [:brendan] 2006-02-09 21:52:48 PST
Much sickness here.  Besides the GC hazard (how many more must we endure?) in XPCWrappedJSClass:CallMethod, E4X sucks: it returns an empty XMLList for every property get of an unnamed child.  So the <foo/> point tag has no QueryInterface child, no parentNode, etc.  The empty XMLList is a JSObject, so XPConnect tries to wrap this result to make it look like an XPCOM object, etc.

The JS_GetMethod API was added for XPConnect, which oddly uses char *, not jschar * or PRUnichar * string name to call.  I didn't add a JS_GetUCMethod or (better) a JS_GetMethodById taking a jsid.  So the attached patch hacks mightily.  Not for checkin, but to point the way to a better fix.

Should XPConnect even try to JS_GetMethod a getter (parentNode) instead of calling JS_GetProperty?  That doesn't make sense.  But JS_GetProperty is doomed to get an empty XMLList (typically -- could be non-empty if tha <foo/> tag were non-empty and had a child named QueryInterface or whatever).  And wrapping an empty XMLList targeting the <foo/> object produces another list targeting the first list, and off we go to an infinite target-linked chain.

Better to fail an attempt to do a property get or set on an E4X object wrapped by XPConnect.  E4X really messes with JS's object model.

/be
Comment 6 Daniel Veditz [:dveditz] 2006-02-10 08:23:44 PST
Every time I try the testcase in a debug windows build I crash trying to access 0xd5d5d5d5 -- is that some kind of a magic value we use internally? I'm also getting a different stack than Jesse (handling the start token, not the end token). I suppose it's a moot point since there's a patch, but including anyway

>	GetProperty Line 3977 + 0x9
 	xml_getProperty
 	nsXPCWrappedJSClass::CallQueryInterfaceOnJSObject
 	nsXPCWrappedJSClass::GetRootJSObject
 	nsXPCWrappedJS::GetNewOrUsed
 	XPCConvert::JSObject2NativeInterface
 	XPCConvert::JSData2Native
 	nsXPCWrappedJSClass::CallMethod
 	nsXPCWrappedJS::CallMethod
 	PrepareAndDispatch
 	SharedStub
 	nsContentUtils::GetFirstDifferentAncestors
 	nsContentUtils::ComparePositionWithAncestors
 	nsNode3Tearoff::CompareDocumentPosition
 	XPTC_InvokeByIndex
 	XPCWrappedNative::CallMethod
 	XPC_WN_CallMethod
 	js_Invoke
 	js_Interpret
 	js_Execute
 	JS_EvaluateUCScriptForPrincipals
 	nsJSContext::EvaluateString
 	nsScriptLoader::EvaluateScript
 	nsScriptLoader::ProcessScriptElement
 	nsHTMLScriptElement::MaybeProcessScript
 	nsHTMLScriptElement::BindToTree
 	nsGenericElement::AppendChildTo
 	HTMLContentSink::ProcessSCRIPTTag
 	HTMLContentSink::AddLeaf
 	CNavDTD::AddLeaf
 	CNavDTD::HandleScriptToken
 	CNavDTD::OpenContainer
 	CNavDTD::HandleDefaultStartToken
 	CNavDTD::HandleStartToken
 	CNavDTD::HandleToken
 	CNavDTD::BuildModel
 	nsParser::BuildModel
 	nsParser::ResumeParse
 	nsDocumentOpenInfo::OnDataAvailable
 	nsDataChannel::OnDataAvailable
 	nsInputStreamPump::OnStateTransfer
 	nsInputStreamPump::OnInputStreamReady
 	nsInputStreamReadyEvent::EventHandler
 	PL_HandleEvent
 	PL_ProcessPendingEvents
 	nsEventQueueImpl::ProcessPendingEvents
 	nsWindow::DispatchPendingEvents
 	nsWindow::ProcessMessage
 	nsWindow::WindowProc
 	77d48744	
 	77d48826	
 	77d489dd	
 	77d48a20	
 	nsAppShell::Run
 	nsAppStartup::Run
 	XRE_main
 	main
 	mainCRTStartup
 	7c816d4f	
 	7c8399f3	
Comment 7 Daniel Veditz [:dveditz] 2006-02-10 08:32:01 PST
I was using a current 1.8.0 branch build if it makes a difference
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2006-02-10 14:54:42 PST
Er... AUTO_MARK_JSVAL takes a jsval, not a jsval*....  How'd that compile?
Comment 9 Brendan Eich [:brendan] 2006-02-10 15:11:13 PST
(In reply to comment #8)
> Er... AUTO_MARK_JSVAL takes a jsval, not a jsval*....  How'd that compile?

It takes either.  Read xpcprivate.h.

I want someone else to own this bug.  I'm happy to help, but a bit overloaded lately (not just with CTO biz, but that doesn't help).

/be
Comment 10 Brendan Eich [:brendan] 2006-02-10 15:15:47 PST
(In reply to comment #6)
> Every time I try the testcase in a debug windows build I crash trying to access
> 0xd5d5d5d5

See http://lxr.mozilla.org/mozilla/search?string=0xd5 -- yes, this is a DEBUG only tombstone over dead E4X data.  The patch fixes this bug, but it needs massaging.

/be
Comment 11 Blake Kaplan (:mrbkap) 2006-02-25 02:53:26 PST
Comment on attachment 211350 [details] [diff] [review]
read and commence puking

With bug 327697 fixed, most of this isn't necessary any more. Brendan, do you want to just submit the GC hazard part of this fix?
Comment 12 Brendan Eich [:brendan] 2006-02-27 21:51:31 PST
Created attachment 213415 [details] [diff] [review]
just the rooting fix
Comment 13 Brendan Eich [:brendan] 2006-02-27 21:51:47 PST
Created attachment 213416 [details] [diff] [review]
just the rooting fix
Comment 14 Brendan Eich [:brendan] 2006-02-27 21:52:24 PST
Created attachment 213417 [details] [diff] [review]
just the rooting fix

Is this needed for aviary1.0.x/mozilla1.7.x?

/be
Comment 15 Brendan Eich [:brendan] 2006-02-27 21:52:52 PST
Created attachment 213418 [details] [diff] [review]
just the rooting fix

Is this needed for aviary1.0.x/mozilla1.7.x?

/be
Comment 16 Brendan Eich [:brendan] 2006-02-27 21:55:54 PST
Created attachment 213419 [details] [diff] [review]
just the rooting fix

Is this needed for aviary1.0.x/mozilla1.7.x?

/be
Comment 17 Brendan Eich [:brendan] 2006-02-27 21:56:29 PST
Created attachment 213420 [details] [diff] [review]
just the rooting fix

Is this needed for aviary1.0.x/mozilla1.7.x?

/be
Comment 18 Brendan Eich [:brendan] 2006-02-27 21:59:25 PST
Created attachment 213421 [details] [diff] [review]
just the rooting fix

Is this needed for aviary1.0.x/mozilla1.7.x?

/be
Comment 19 Brendan Eich [:brendan] 2006-02-27 22:01:39 PST
Good grief!  Bugzilla kept giving me Internal Server Error, so I kept retrying.  I'll know better next time....

/be
Comment 20 Brendan Eich [:brendan] 2006-02-27 22:52:35 PST
<brendan> shouldn't give an error page if the mysql commit went through
<justdave> yeah, it died trying to send the email
<justdave> I'm trying to track down why...
<justdave> found it
<justdave> https://bugzilla.mozilla.org/show_bug.cgi?id=266147
<justdave> it's fixed in 2.20.1
<justdave> the patch fails miserably trying to apply it to our current install, and I get a ton of conflicts updating to the tip of the 2.20 branch :(
<justdave> (on the test copy of bmo of course)
<justdave> hmm, take that back, the patch doesn't fail quite so miserably as I thought...
<justdave> the failed chunk is a change in a comment. :)
<justdave> and the other was a one-line conflict with a local customization that was easy to merge
* justdave applies the patch to production
<justdave> done
<brendan> thanks

Thanks again for fixing b.m.o.

/be
Comment 21 Blake Kaplan (:mrbkap) 2006-02-27 23:21:09 PST
Comment on attachment 213421 [details] [diff] [review]
just the rooting fix

r=mrbkap, thanks.
Comment 22 Brendan Eich [:brendan] 2006-02-27 23:41:53 PST
Fixed on the trunk.

/be
Comment 23 Daniel Veditz [:dveditz] 2006-03-01 12:12:25 PST
Comment on attachment 213421 [details] [diff] [review]
just the rooting fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 24 Brendan Eich [:brendan] 2006-03-01 12:39:48 PST
Fixed on the 1.8 branches.

/be
Comment 25 Dave Liebreich [:davel] 2006-03-01 14:14:29 PST
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Comment 26 Tracy Walker [:tracy] 2006-03-06 13:24:28 PST
testcase crashes Mac Firefox 1.5.0.2 build from 20060306

reopening
Comment 27 Bob Clary [:bc:] 2006-03-06 13:42:02 PST
Tracy, does this crash on the trunk as well? If this only crashes on 1.5.0.2, then I don't think the bug should be reopened but that the fixed1.8.0.2 keyword should be removed.
Comment 28 Tracy Walker [:tracy] 2006-03-06 14:07:19 PST
okay, no crash on trunk. re-resolving fixed. leaving out fixed1.8.0.2 keyword
Comment 29 Tim Riley [:timr] 2006-03-06 15:29:18 PST
Brendan- can you look at this for 1.8.0.2.  It is a blocker for that release and we need final bits ASAP!  Thx!
Comment 30 Marcia Knous [:marcia - use ni] 2006-03-06 15:52:18 PST
FWIW, the test case also crashes on 1.5.0.1 Mac.
Comment 31 Brendan Eich [:brendan] 2006-03-07 09:35:10 PST
I'm suffering terrible wireless connectivity here at ETech.  Suggest mrbkap investigate.  Did I fail to land the patch on the 1.8.0 branch?

Re: 1.5.0.1 crashes, that's expected I think -- bclary can confirm.

/be
Comment 32 Daniel Veditz [:dveditz] 2006-03-07 11:18:34 PST
The hang is apparently bug 327697 -- applying that patch fixes my problems here. Dunno if the Mac crash is the same thing though :-(
Comment 33 Daniel Veditz [:dveditz] 2006-03-07 15:00:34 PST
The fix for bug 327697 should re-fix this and has been checked in.
Comment 34 Marcia Knous [:marcia - use ni] 2006-03-07 16:48:00 PST
Looks good on the 1.8 branch using the respun build Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.2) Gecko/20060307 Firefox/1.5.0.2 (2006-03-07-15). I see no crash using Jesse's testcase. Adding verified keyword.
Comment 35 Marcia Knous [:marcia - use ni] 2006-03-07 16:50:36 PST
Sorry for the bugspam, I mistyped and meant the 1.8.0 branch, not 1.8.

(In reply to comment #34)
> Looks good on the 1.8 branch using the respun build Mozilla/5.0 (Macintosh; U;
> PPC Mac OS X Mach-O; en-US; rv:1.8.0.2) Gecko/20060307 Firefox/1.5.0.2
> (2006-03-07-15). I see no crash using Jesse's testcase. Adding verified
> keyword.
> 

Comment 36 Marcia Knous [:marcia - use ni] 2006-08-10 12:36:55 PDT
verified on the 1.8.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1b1) Gecko/20060810 BonEcho/2.0b1. No crash with testcase. Adding keyword.
Comment 37 Jesse Ruderman 2007-12-23 16:46:22 PST
Crashtest checked in.

Note You need to log in before you can comment on or make changes to this bug.