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

VERIFIED FIXED

Status

()

defect
P2
critical
VERIFIED FIXED
11 years ago
a year ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

(Blocks 1 bug, 5 keywords)

Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.8.1.next +
wanted1.8.1.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] needs r=bzbarsky, crash signature)

Attachments

(6 attachments)

(Reporter)

Description

11 years ago
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

Updated

11 years ago
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?]

Updated

10 years ago
Assignee: nobody → Olli.Pettay

Updated

10 years ago
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.
Posted patch Proposed fixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Posted 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.

Updated

10 years ago
Attachment #370256 - Flags: review?(neil) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/41b09fdb40d8

No test pushed yet, as usual.  :(
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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?

Updated

10 years ago
Depends on: 486842

Updated

10 years ago
Depends on: 486848
(Assignee)

Updated

10 years ago
Component: XUL → XP Toolkit/Widgets: XUL
QA Contact: xptoolkit.widgets → xptoolkit.xul
(Assignee)

Updated

10 years ago
Attachment #370254 - Flags: approval1.9.0.10?
(Assignee)

Updated

10 years ago
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

Comment 16

10 years ago
v 1.9.0, 1.9.1, 1.9.2 mac
Status: RESOLVED → VERIFIED
Affects 1.8.1 too.
Flags: wanted1.8.1.x+
Posted 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
(Reporter)

Updated

10 years ago
No longer blocks: 488210
Depends on: 488210

Comment 25

10 years ago
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.