Closed Bug 326615 Opened 19 years ago Closed 19 years ago

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

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jruderman, Assigned: brendan)

References

Details

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

Crash Data

Attachments

(3 files, 7 obsolete files)

testcase
77 bytes, text/html
Details
stack trace
5.87 KB, text/plain
Details
just the rooting fix
481 bytes, patch
mrbkap
: review+
brendan
: approval-branch-1.8.1+
dveditz
: approval1.8.0.2+
Details | Diff | Splinter Review
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.
Attached file testcase —
Attached file stack trace —
Note: it hangs for a while before crashing, so it's hard to get the crash in a debug build :(
Keywords: testcase
Whiteboard: [sg:critical?]
nsXPCWrappedJSClass::CallMethod is failing to protect its result jsval local from the JS GC. /be
Attached patch read and commence puking (obsolete) — — Splinter Review
If you minus it, you own it. /be
Attachment #211350 - Flags: review?(mrbkap)
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
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?
(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
(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
Whiteboard: [sg:critical?] → [sg:critical] based on stack in comment 2
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?
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-
Attached patch just the rooting fix (obsolete) — — Splinter Review
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?
Attached patch just the rooting fix (obsolete) — — Splinter Review
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?
Attached patch just the rooting fix (obsolete) — — Splinter Review
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?
Attached patch just the rooting fix (obsolete) — — Splinter Review
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?
Attached patch just the rooting fix (obsolete) — — Splinter Review
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?
Attached patch just the rooting fix (obsolete) — — Splinter Review
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?
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?
Good grief! Bugzilla kept giving me Internal Server Error, so I kept retrying. I'll know better next time.... /be
<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+
Fixed on the trunk. /be
Status: NEW → RESOLVED
Closed: 19 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+
Fixed on the 1.8 branches. /be
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 → ---
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
Closed: 19 years ago → 19 years ago
Resolution: --- → FIXED
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.
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. >
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: