CVE-2006-1530 compareDocumentPosition with E4X crashes [@ XPCWrappedNativeScope::SystemIsBeingShutDown]

RESOLVED FIXED

Status

()

Core
XPConnect
--
critical
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: brendan)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.2, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] based on stack in comment 2 [rft-dl], crash signature)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 211342 [details]
testcase
(Reporter)

Comment 2

12 years ago
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 :(
(Reporter)

Updated

12 years ago
Keywords: testcase
Whiteboard: [sg:critical?]
(Assignee)

Comment 3

12 years ago
nsXPCWrappedJSClass::CallMethod is failing to protect its result jsval local from the JS GC.

/be
(Assignee)

Comment 4

12 years ago
Created attachment 211350 [details] [diff] [review]
read and commence puking

If you minus it, you own it.

/be
Attachment #211350 - Flags: review?(mrbkap)
(Assignee)

Comment 5

12 years ago
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
(Reporter)

Updated

12 years ago
Blocks: 326633
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	
Assignee: dbradley → brendan
I was using a current 1.8.0 branch build if it makes a difference
Er... AUTO_MARK_JSVAL takes a jsval, not a jsval*....  How'd that compile?
(Assignee)

Comment 9

11 years ago
(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
(Assignee)

Comment 10

11 years ago
(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
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical?] → [sg:critical] based on stack in comment 2
(Reporter)

Updated

11 years ago
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?

Updated

11 years ago
Depends on: 327697
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?
Attachment #211350 - Flags: review?(mrbkap) → review-
(Assignee)

Comment 12

11 years ago
Created attachment 213415 [details] [diff] [review]
just the rooting fix
Attachment #211350 - Attachment is obsolete: true
Attachment #213415 - Flags: review?(mrbkap)
Attachment #213415 - Flags: approval1.8.0.2?
Attachment #213415 - Flags: approval1.7.13?
Attachment #213415 - Flags: approval-branch-1.8.1+
Attachment #213415 - Flags: approval-aviary1.0.8?
(Assignee)

Comment 13

11 years ago
Created attachment 213416 [details] [diff] [review]
just the rooting fix
Attachment #213415 - Attachment is obsolete: true
Attachment #213416 - Flags: review?(mrbkap)
Attachment #213416 - Flags: approval1.8.0.2?
Attachment #213416 - Flags: approval1.7.13?
Attachment #213416 - Flags: approval-branch-1.8.1+
Attachment #213416 - Flags: approval-aviary1.0.8?
Attachment #213415 - Flags: review?(mrbkap)
Attachment #213415 - Flags: approval1.8.0.2?
Attachment #213415 - Flags: approval1.7.13?
Attachment #213415 - Flags: approval-aviary1.0.8?
(Assignee)

Comment 14

11 years ago
Created attachment 213417 [details] [diff] [review]
just the rooting fix

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

/be
Attachment #213416 - Attachment is obsolete: true
Attachment #213417 - Flags: review?(mrbkap)
Attachment #213417 - Flags: approval1.8.0.2?
Attachment #213417 - Flags: approval-branch-1.8.1+
Attachment #213416 - Flags: review?(mrbkap)
Attachment #213416 - Flags: approval1.8.0.2?
Attachment #213416 - Flags: approval1.7.13?
Attachment #213416 - Flags: approval-aviary1.0.8?
(Assignee)

Comment 15

11 years ago
Created attachment 213418 [details] [diff] [review]
just the rooting fix

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

/be
Attachment #213417 - Attachment is obsolete: true
Attachment #213418 - Flags: review?(mrbkap)
Attachment #213418 - Flags: approval1.8.0.2?
Attachment #213418 - Flags: approval-branch-1.8.1+
Attachment #213417 - Flags: review?(mrbkap)
Attachment #213417 - Flags: approval1.8.0.2?
(Assignee)

Comment 16

11 years ago
Created attachment 213419 [details] [diff] [review]
just the rooting fix

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

/be
Attachment #213418 - Attachment is obsolete: true
Attachment #213419 - Flags: review?(mrbkap)
Attachment #213419 - Flags: approval1.8.0.2?
Attachment #213419 - Flags: approval-branch-1.8.1+
Attachment #213418 - Flags: review?(mrbkap)
Attachment #213418 - Flags: approval1.8.0.2?
(Assignee)

Comment 17

11 years ago
Created attachment 213420 [details] [diff] [review]
just the rooting fix

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

/be
Attachment #213419 - Attachment is obsolete: true
Attachment #213420 - Flags: review?(mrbkap)
Attachment #213420 - Flags: approval1.8.0.2?
Attachment #213420 - Flags: approval-branch-1.8.1+
Attachment #213419 - Flags: review?(mrbkap)
Attachment #213419 - Flags: approval1.8.0.2?
(Assignee)

Comment 18

11 years ago
Created attachment 213421 [details] [diff] [review]
just the rooting fix

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

/be
Attachment #213420 - Attachment is obsolete: true
Attachment #213421 - Flags: review?(mrbkap)
Attachment #213421 - Flags: approval1.8.0.2?
Attachment #213421 - Flags: approval-branch-1.8.1+
Attachment #213420 - Flags: review?(mrbkap)
Attachment #213420 - Flags: approval1.8.0.2?
(Assignee)

Comment 19

11 years ago
Good grief!  Bugzilla kept giving me Internal Server Error, so I kept retrying.  I'll know better next time....

/be
(Assignee)

Comment 20

11 years ago
<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 on attachment 213421 [details] [diff] [review]
just the rooting fix

r=mrbkap, thanks.
Attachment #213421 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 22

11 years ago
Fixed on the trunk.

/be
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.2+
Comment on attachment 213421 [details] [diff] [review]
just the rooting fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #213421 - Flags: approval1.8.0.2? → approval1.8.0.2+
(Assignee)

Comment 24

11 years ago
Fixed on the 1.8 branches.

/be
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.2, fixed1.8.1
Marking [rft-dl] (ready for testing in Firefox 1.5.0.2 release candidates)
Whiteboard: [sg:critical] based on stack in comment 2 → [sg:critical] based on stack in comment 2 [rft-dl]
testcase crashes Mac Firefox 1.5.0.2 build from 20060306

reopening
Status: RESOLVED → REOPENED
Keywords: fixed1.8.0.2
Resolution: FIXED → ---

Comment 27

11 years ago
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.
okay, no crash on trunk. re-resolving fixed. leaving out fixed1.8.0.2 keyword
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 29

11 years ago
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!
FWIW, the test case also crashes on 1.5.0.1 Mac.
(Assignee)

Comment 31

11 years ago
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
The hang is apparently bug 327697 -- applying that patch fixes my problems here. Dunno if the Mac crash is the same thing though :-(
The fix for bug 327697 should re-fix this and has been checked in.
Keywords: fixed1.8.0.2
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.
Keywords: fixed1.8.0.2 → verified1.8.0.2
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.
> 

Updated

11 years ago
Summary: compareDocumentPosition with E4X crashes [@ XPCWrappedNativeScope::SystemIsBeingShutDown] → CVE-2006-1530 compareDocumentPosition with E4X crashes [@ XPCWrappedNativeScope::SystemIsBeingShutDown]
Group: security
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.
Keywords: fixed1.8.1 → verified1.8.1
(Reporter)

Comment 37

10 years ago
Crashtest checked in.
Flags: in-testsuite+
Crash Signature: [@ XPCWrappedNativeScope::SystemIsBeingShutDown]
You need to log in before you can comment on or make changes to this bug.