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

(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.

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.

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....

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.

Pushed http://hg.mozilla.org/mozilla-central/rev/41b09fdb40d8 No test pushed yet, as usual. :(

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dbb9e2e53ea6

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.

Comment on attachment 371189 [details] [diff] [review] roll-up patch suitable for 1.9.0 branch Approved for 1.9.0.10. a=ss

Fixed in CVS.

v 1.9.0, 1.9.1, 1.9.2 mac

Affects 1.8.1 too.

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.

Comment on attachment 378005 [details] [diff] [review] 1.8.0 patch Boris, can you spot check this for 1.8.0/1.8.1?

Comment on attachment 378005 [details] [diff] [review] 1.8.0 patch Looks ok

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

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.

Checked in the two tests on m-c, 1.9.1, and 1.9.0.

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

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336