384037 - Crash [@ nsIFrame::IsBoxFrame] with <xul:splitter>

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Andrew Miller]

The cause of this problem is that nsFrameNavigator::GetChildBeforeAfter assumes that the parent is a box frame, but this assumption is not true in this case (it isn't even a XUL frame).

Perhaps if mParent is null we can just return nsnull from nsFrameNavigator::GetChildBeforeAfter?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Seems to me that nsFrameNavigator should just use nsIFrame* everywhere. There is nothing box-specific about it.

Comment 3

[Eli Friedman]

I think we should actually just add an early return in nsSplitterFrame::UpdateState (and maybe a couple other places) if mParentBox is null.  A splitter can't really work anyway without a box parent and box siblings b because of the dependencies on box layout and box attributes.  (It seems kind of weird to me that the splitter code is messing around with attributes on random content nodes, though; it could have very odd effects in some cases.  Probably something to consider changing for XUL2.)

We should also probably get rid of nsFrameNavigator; it's only used in nsSplitterFrame, and all the functions can easily be written in one line using  nsFrameList (a couple more if you care about whether the frames are boxes).

Comment 4

[Jesse Ruderman]

Still crashes [@ nsIFrame::IsBoxFrame] on trunk.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(I tested this on latest-trunk mozilla-central nightly build on WinXP SP3)

Turning security-sensitive and nominating blocking1.9.1? just-in-case, due to !exploitable PROBABLY_EXPLOITABLE result.

0:000> !exploitable -v
HostMachine\HostUser
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
Exception Faulting Address: 0x0
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Read Access Violation

Faulting Instruction:1013b3d0 mov eax,dword ptr [esi]

Basic Block:
    1013b3d0 mov eax,dword ptr [esi]
       Tainted Input Operands: esi
    1013b3d2 mov edx,dword ptr [eax+128h]
       Tainted Input Operands: eax
    1013b3d8 push 100h
    1013b3dd mov ecx,esi
       Tainted Input Operands: esi
    1013b3df call edx
       Tainted Input Operands: ecx, edx

Exception Hash (Major/Minor): 0x48565475.0x5e0f3b47

Stack Trace:
xul!nsIFrame::GetChildBox+0x0
xul!nsFrameNavigator::IndexOf+0xd
xul!nsFrameNavigator::GetChildBeforeAfter+0x14
xul!nsSplitterFrameInner::UpdateState+0x11cf64
xul!nsSplitterFrame::DoLayout+0x1f
xul!nsBoxFrame::Reflow+0x113
xul!nsLineLayout::ReflowFrame+0x8fa
xul!nsBlockFrame::ReflowInlineFrame+0x31
xul!nsBlockFrame::DoReflowInlineFrames+0x1a6
xul!nsBlockFrame::ReflowInlineFrames+0x188
xul!nsBlockFrame::ReflowLine+0x74
xul!nsBlockFrame::ReflowDirtyLines+0x171
xul!nsBlockFrame::Reflow+0x1e2
xul!nsBlockReflowContext::ReflowBlock+0xd4
xul!nsBlockFrame::ReflowBlockFrame+0x43d
xul!nsBlockFrame::ReflowLine+0x15d
xul!nsBlockFrame::ReflowDirtyLines+0x171
xul!nsBlockFrame::Reflow+0x1e2
xul!nsContainerFrame::ReflowChild+0x6c
xul!CanvasFrame::Reflow+0x11c
xul!nsContainerFrame::ReflowChild+0x6c
xul!nsHTMLScrollFrame::ReflowScrolledFrame+0x19a
xul!nsHTMLScrollFrame::ReflowContents+0x7b
xul!nsHTMLScrollFrame::Reflow+0x1d9
xul!nsContainerFrame::ReflowChild+0x6c
xul!ViewportFrame::Reflow+0x11c
xul!PresShell::DoReflow+0x249
xul!PresShell::ProcessReflowCommands+0x11e
xul!PresShell::DoFlushPendingNotifications+0x12f
xul!PresShell::ReflowEvent::Run+0x37
xul!nsThread::ProcessNextEvent+0x213
xul!nsBaseAppShell::Run+0x4a
xul!nsAppStartup::Run+0x1e
xul!XRE_main+0xcb9
Unknown
Instruction Address: 0x1013b3d0

Description: Data from Faulting Address controls Code Flow
Short Description: TaintedDataControlsCodeFlow
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at xul!nsIFrame::GetChildBox+0x0 (Hash=0x48565475.0x5e0f3b47)

The data from the faulting address is later used as the target for a branch.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This is more or less what Eli suggested. Gets rid of nsFrameNavigator. Replaces its usage with nsFrameList. Sprinkles a few IsBoxFrame checks so that we bail out if our sibling or parent is not a XUL box.

FWIW this bug is a pure null deref, as far as I can tell, so the "!exploitable" tool is not giving us a good assessment here.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 370174 [details] [diff] [review]
fix

r+sr=dbaron; sorry for the delay.  This was simpler than I'd expected it to be.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/d56efd447561

Comment 9

[Bob Clary [:bc] (inactive)]

1.9.0 and 1.9.1 !exploitable report:
PROBABLY_EXPLOITABLE: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!nsIFrame::IsBoxFrame

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dec49167c08c

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

This caused a regression: see bug 491323

Comment 12

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using:

->Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre

->Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre

->Win Vista equivalent nightly