CVE-2006-1531 [FIX]Crash with evil xul testcase, using listbox/listitem and display: table-cell

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
Layout
P1
critical
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

(5 keywords)

Trunk
mozilla1.9alpha1
crash, regression, testcase, verified1.8.0.2, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?][rft-dl] not in moz1.7/ff1.0)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
See upcoming testcase, which crashes current trunk Mozilla's build when visiting the page.
Doesn't crash in 2006-01-30 build, crashes in 2006-01-31 build, so a regression from bug 282105, I think.
(Reporter)

Comment 1

12 years ago
Created attachment 211511 [details]
testcase
(Reporter)

Updated

12 years ago
Blocks: 282105
(Assignee)

Comment 2

12 years ago
Yeah.  I bet prior to bug 282105 we just totally failed to call into the listbox here.  Now we do, and it screws up the OnContentRemoved notification, sorta.  I bet we could produce similar effects even before the patch for bug 282105.

The problem is that the table cell is not actually a child of the listboxbody -- the anon table frame is.  So when we call into nsListBoxBodyFrame::RemoveChildFrame we don't actually remove any frames from anywhere... but we DO call Destroy() on aFrame. So after that we have a deleted frame in the frametree, and we're dead.

Now I could easily fix things to not call Destroy() if we fail to destroy aFrame.  But then we'd end up with the frame not getting removed, and ContentInserted creating a new frame and bad stuff would happen.

I guess what we should do is only notify the listbox body if the parent of our frame is a listbox body frame.  But that means that the pseudo work will need to deal with this later (that is, when we get pseudo removal working).
Flags: blocking1.8.0.2?
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
(Assignee)

Comment 3

12 years ago
Created attachment 211519 [details] [diff] [review]
Proposed patch

I'll merge this to branches once I know it's ok in general.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #211519 - Flags: superreview?(roc)
Attachment #211519 - Flags: review?(neil)
(Assignee)

Updated

12 years ago
Group: security
Flags: blocking1.8.1?
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Crash with evil xul testcase, using listbox/listitem and display: table-cell → [FIX]Crash with evil xul testcase, using listbox/listitem and display: table-cell
Target Milestone: --- → mozilla1.9alpha

Comment 4

12 years ago
Comment on attachment 211519 [details] [diff] [review]
Proposed patch

I don't have time to give this a proper review as I need to get up early tomorrow morning to go on holiday (US: vacation) for the rest of the week. But it occurs to me that you could tack your extra condition onto the one just missed by your -u8 diff since aChildFrame is always null when notifying for insertions.
(Assignee)

Comment 5

12 years ago
Comment on attachment 211519 [details] [diff] [review]
Proposed patch

I can't, since my extra condition needs to compare to |listBoxBodyFrame|, which I don't have up there.

I think we want this in sooner than a week.  Roc, are you ok with doing r+sr on this?
Attachment #211519 - Flags: review?(neil) → review?(roc)

Comment 6

12 years ago
(In reply to comment #5)
>(From update of attachment 211519 [details] [diff] [review])
>I can't, since my extra condition needs to compare to |listBoxBodyFrame|, which
>I don't have up there.
I do need the sleep, don't I ;-) How about putting the condition on the call to RemoveChildFrame in nsListBoxBodyFrame::OnContentRemoved i.e.
if (aChildFrame && aChildFrame->GetParent() == this) {
  RemoveChildFrame(state, aChildFrame);
}
(Assignee)

Comment 7

12 years ago
> How about putting the condition on the call to RemoveChildFrame in
> nsListBoxBodyFrame::OnContentRemoved 

I need to return different booleans in the frame constructor depending on this condition; if we're not removing via OnContentRemoved we need to remove through the normal codepath.  See comment 2 paragraph 3.  And nsListBoxBodyFrame::OnContentRemoved returns void.  I could change this, I guess, and just return what I'm checking up front right now...
Attachment #211519 - Flags: superreview?(roc)
Attachment #211519 - Flags: superreview+
Attachment #211519 - Flags: review?(roc)
Attachment #211519 - Flags: review+
I think we should take this patch and add an assertion to OnContentRemoved that aChildFrame's parent == this if it's non-null.
(Assignee)

Comment 9

12 years ago
Created attachment 211635 [details] [diff] [review]
Updated to comments
Attachment #211519 - Attachment is obsolete: true
(Assignee)

Comment 10

12 years ago
Created attachment 211637 [details] [diff] [review]
1.8.0.x branch patch
Attachment #211637 - Flags: approval1.8.0.2?
(Assignee)

Comment 11

12 years ago
Created attachment 211639 [details] [diff] [review]
1.8 version of patch
Attachment #211639 - Flags: approval-branch-1.8.1?(roc)
(Assignee)

Comment 12

12 years ago
This is not an issue on the 1.7 branch since we didn't do table pseudos for XUL there.
Flags: blocking1.7.13?
Flags: blocking-aviary1.0.8?
(Assignee)

Comment 13

12 years ago
Fixed on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Attachment #211639 - Flags: approval-branch-1.8.1?(roc) → approval-branch-1.8.1+
(Assignee)

Comment 14

12 years ago
Fixed on 1.8.1 branch.
Keywords: fixed1.8.1
Verified FIXED using build 2006-02-13-09 with testcase: https://bugzilla.mozilla.org/attachment.cgi?id=211511&action=view on Windows XP SeaMonkey trunk.
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1?
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
Flags: blocking1.8.0.2+
Comment on attachment 211637 [details] [diff] [review]
1.8.0.x branch patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #211637 - Flags: approval1.8.0.2? → approval1.8.0.2+
(Assignee)

Comment 17

12 years ago
Fixed on 1.8.0.2 branch.
Keywords: fixed1.8.0.2

Updated

12 years ago
Whiteboard: [rft-dl]

Comment 18

12 years ago
v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, no crash with testcase.
Keywords: fixed1.8.0.2 → verified1.8.0.2
Whiteboard: [rft-dl] → [rft-dl] not in moz1.7
Whiteboard: [rft-dl] not in moz1.7 → [sg:critical?][rft-dl] not in moz1.7/ff1.0

Updated

11 years ago
Summary: [FIX]Crash with evil xul testcase, using listbox/listitem and display: table-cell → CVE-2006-1531 [FIX]Crash with evil xul testcase, using listbox/listitem and display: table-cell
Group: security
Verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060815 BonEcho/2.0b1, as well as MacOS X version.

Firefox did not crash with the testcase.
Keywords: fixed1.8.1 → verified1.8.1

Comment 20

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/60280a93b9cd
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.