Closed Bug 432068 Opened 16 years ago Closed 15 years ago

Crash [@ nsListBoxBodyFrame::GetNextItemBox] with <xul:listbox>, XBL

Categories

(Core :: XUL, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: bzbarsky)

References

Details

(5 keywords, Whiteboard: [sg:critical?] needs r=bzbarsky)

Crash Data

Attachments

(6 files)

Loading the testcase makes Firefox crash [@ nsListBoxBodyFrame::GetNextItemBox].
I can reproduc it with Linux /Firefox 30b5 :
Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
(I tested this on mozilla-central latest-trunk nightly build on WinXP SP3)

!exploitable shows this is probably exploitable, turning security-sensitive and nominating blocking1.9.1?


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:1062c568 mov eax,dword ptr [esi]

Basic Block:
    1062c568 mov eax,dword ptr [esi]
       Tainted Input Operands: esi
    1062c56a mov ecx,esi
       Tainted Input Operands: esi
    1062c56c call dword ptr [eax+4ch]
       Tainted Input Operands: eax, ecx

Exception Hash (Major/Minor): 0x6224767a.0x6444e23

Stack Trace:
xul!nsListBoxBodyFrame::GetNextItemBox+0x46
xul!nsListBoxBodyFrame::CreateRows+0x9f
xul!nsListBoxBodyFrame::ReflowFinished+0x19
xul!PresShell::HandlePostedReflowCallbacks+0x3d
xul!PresShell::ProcessReflowCommands+0x179
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: 0x1062c568

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!nsListBoxBodyFrame::GetNextItemBox+0x46 (Hash=0x6224767a.0x6444e23)

The data from the faulting address is later used as the target for a branch.
Group: core-security
Flags: blocking1.9.1?
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:critical?]
Assignee: nobody → Olli.Pettay
Assignee: Olli.Pettay → nobody
I'm seeing a null-dereference; I think that's because we've created a content tree where a particular node has no parent, although I don't completely follow what's going on.
If I breakpoint in the nsListBoxBodyFrame::OnContentRemoved call from that removeChild call, here's what the relevant part of the frametree looks like:

                Box(listboxbody)(0)@0x14ef838 [view=0x1d718510] {0,0,71100,11880} [state=80802000] [content=0x1d710f50] [sc=0x14f94e0] pst=:-moz-scrolled-content<
                  nsGridRowLeaf(listitem)(0)@0x14f9828 next=0x14e9c14 {0,0,71100,840} [state=80c00010] [content=0x1c7bb7e0] [sc=0x14f97cc]<
                    Box(listcell)(-1)@0x14e986c {60,60,70980,720} [state=80600000] [content=0x1d71a410] [sc=0x14f96b8]<
                      XULLabel(label)(-1)@0x14e9b98 {0,330,70980,60} [state=00d00000] sc=0x14e9aa8(i=0,b=0)<
                      >
                    >
                  >
                  nsGridRowLeaf(listitem)(-1)@0x14e9c14 next=0x14e9d3c {0,840,71100,840} [state=80c00000] [content=0x1c7bb870] [sc=0x14f97cc]<
                    Box(listcell)(-1)@0x14e9c7c {60,60,70980,720} [state=80600000] [content=0x1d71dee0] [sc=0x14f96b8]<
                      XULLabel(label)(-1)@0x14e9ce4 {0,330,70980,60} [state=00d00000] sc=0x14e9aa8(i=0,b=0)<
                      >
                    >
                  >
                  nsGridRowLeaf(listitem)(-1)@0x14e9d3c {0,1680,71100,840} [state=80c00000] [content=0x1c7bb870] [sc=0x14f97cc]<
                    Box(listcell)(-1)@0x14e9da4 {60,60,70980,720} [state=80600000] [content=0x1d71dee0] [sc=0x14f96b8]<
                      XULLabel(label)(-1)@0x14e9e0c {0,330,70980,60} [state=00d00000] sc=0x14e9aa8(i=0,b=0)<
                      >
                    >
                  >

Note that the second and third nsGridRowLeaf frames have the same content node; in fact the node we're removing.  So we remove one of them, and then have a frame in the tree which points to a content node whose parent is null.  When we dereference that null later, we crash.
The only reason XBL was needed was to make sure that:

1)  A frame reconstruct (and in particular a ContentInserted) happened
2)  No frame was constructed for the content passed to
    nsListBoxBodyFrame::OnContentInserted

This testcase just uses display:none on the listitem for similar effect.  We call nsListBoxBodyFrame::OnContentInserted, which calls nsListBoxBodyFrame::CreateRows, which does the whole GetNextItemBox thing.  Then GetNextItemBox calls CreatListBoxContent, which returns null.  This triggers this lovely bit of code:

      if (result) {
        if (aCreated)
           *aCreated = PR_TRUE;
      } else
        return GetNextItemBox(aBox, ++aOffset, aCreated);

which then tries to construct a frame for the next content... but we have a frame for it already.
Attached patch Proposed fixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attached patch diff -wSplinter Review
I think this is the right thing to do; just skipping the create if GetPrimaryFrameFor() returns something different from mLinkupFrame might not work well if mRowsToPrepend > 0.  But then again, I'm not quite sure exactly what this code should be doing when mRowsToPrepend > 0....
Attachment #370256 - Flags: superreview?(roc)
Attachment #370256 - Flags: review?(roc)
Attachment #370256 - Flags: review?(neil)
Attachment #370256 - Flags: superreview?(roc)
Attachment #370256 - Flags: superreview+
Attachment #370256 - Flags: review?(roc)
Attachment #370256 - Flags: review+
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment on attachment 370256 [details] [diff] [review]
diff -w

>@@ -1203,30 +1203,34 @@ nsListBoxBodyFrame::GetNextItemBox(nsIBo
> 
>     PRInt32 i = parentContent->IndexOf(prevContent);
I didn't try the first test case, but the second test case crashes here; is that supposed to happen? Note that this is a debug build, so I'm assuming it's not doing any optimisation tricks that would result in a confusing stack.
Yes, the second testcase is expected to crash on that line (as does the first).  parentContent is null, since prevContent is no longer in the document.
Attachment #370256 - Flags: review?(neil) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/41b09fdb40d8

No test pushed yet, as usual.  :(
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment on attachment 370254 [details] [diff] [review]
Proposed fix

Presumably need this on 1.9.0 as well, right?  Not sure which is the right milestone to request approval for there.
Attachment #370254 - Flags: approval1.9.0.9?
Attachment #370254 - Flags: approval1.9.0.10?
Attachment #370254 - Flags: approval1.9.0.9?
Component: XUL → XP Toolkit/Widgets: XUL
QA Contact: xptoolkit.widgets → xptoolkit.xul
Attachment #371189 - Flags: approval1.9.0.10?
Attachment #370254 - Flags: approval1.9.0.10?
Keywords: fixed1.9.1
Comment on attachment 371189 [details] [diff] [review]
roll-up patch suitable for 1.9.0 branch

Approved for 1.9.0.10. a=ss
Attachment #371189 - Flags: approval1.9.0.10? → approval1.9.0.10+
Fixed in CVS.
Keywords: fixed1.9.0.10
v 1.9.0, 1.9.1, 1.9.2 mac
Status: RESOLVED → VERIFIED
Affects 1.8.1 too.
Flags: wanted1.8.1.x+
Attached patch 1.8.0 patchSplinter Review
There's a patch for 1.8.0 there, works fine for the second testcase but cycles in an endless loop for the first one.
Flags: blocking1.8.1.next?
Comment on attachment 378005 [details] [diff] [review]
1.8.0 patch

Boris, can you spot check this for 1.8.0/1.8.1?
Attachment #378005 - Flags: review?(bzbarsky)
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Whiteboard: [sg:critical?] → [sg:critical?] needs r=bzbarsky
Comment on attachment 378005 [details] [diff] [review]
1.8.0 patch

Looks ok
Attachment #378005 - Flags: review?(bzbarsky) → review+
Attachment #378005 - Flags: approval1.8.1.next?
Attachment #378005 - Flags: approval1.8.1.next? → approval1.8.1.next+
Comment on attachment 378005 [details] [diff] [review]
1.8.0 patch

Approved for 1.8.1.22, a=dveditz for release-drivers
Fix checked into the 1.8(.1) branch
Checking in layout/xul/base/src/nsListBoxBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp,v  <--  nsListBoxBodyFrame.cpp
new revision: 1.50.12.5; previous revision: 1.50.12.4
done
Keywords: fixed1.8.1.22
Verified using Seamonkey nightly (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.22pre) Gecko/20090604 SeaMonkey/1.1.17pre). Last shipped version crashes on testcase but the new nightly does not.
Group: core-security
Checked in the two tests on m-c, 1.9.1, and 1.9.0.
Flags: in-testsuite? → in-testsuite+
Blocks: 488210
No longer blocks: 488210
Depends on: 488210
Crashing still on the first test case (not the second one), using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.22) Gecko/20090606 SeaMonkey/1.1.17
Crash Signature: [@ nsListBoxBodyFrame::GetNextItemBox]
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: