Closed
Bug 759249
(CVE-2012-1952)
Opened 12 years ago
Closed 12 years ago
Bad cast in nsTableFrame::InsertFrames
Categories
(Core :: Layout: Tables, defect)
Core
Layout: Tables
Tracking
()
VERIFIED
FIXED
mozilla15
People
(Reporter: inferno, Assigned: MatsPalmgren_bugz)
Details
(Keywords: sec-critical, testcase, Whiteboard: [advisory-tracking+])
Attachments
(5 files)
152 bytes,
text/html
|
Details | |
218 bytes,
text/html
|
Details | |
2.32 KB,
patch
|
bzbarsky
:
review+
akeybl
:
approval-mozilla-beta+
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
6.87 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
Details | Diff | Splinter Review |
Reproduces on Firefox trunk 20120528230131 http://hg.mozilla.org/mozilla-central/rev/79262a88881d First it hits the asserts here. NS_IMETHODIMP nsTableFrame::InsertFrames(ChildListID aListID, nsIFrame* aPrevFrame, nsFrameList& aFrameList) { ........ #ifdef DEBUG // verify that all sibling have the same type, if they do not, expect cellmap issues for (nsFrameList::Enumerator e(aFrameList); !e.AtEnd(); e.Next()) { const nsStyleDisplay* nextDisplay = e.get()->GetStyleDisplay(); NS_ASSERTION((display->mDisplay == NS_STYLE_DISPLAY_TABLE_COLUMN_GROUP) == (nextDisplay->mDisplay == NS_STYLE_DISPLAY_TABLE_COLUMN_GROUP), "heterogenous childlist"); } #endif Bad cast happens here. We get a nsTableRowGroupFrame, which gets incorrectly casted to nsTableColGroupFrame. void nsTableFrame::InsertColGroups(PRInt32 aStartColIndex, const nsFrameList::Slice& aColGroups) { PRInt32 colIndex = aStartColIndex; nsFrameList::Enumerator colGroups(aColGroups); for (; !colGroups.AtEnd(); colGroups.Next()) { nsTableColGroupFrame* cgFrame = static_cast<nsTableColGroupFrame*>(colGroups.get()); > xul.dll!nsTableFrame::InsertColGroups(int aStartColIndex=0, const nsFrameList::Slice & aColGroups={...}) Line 546 + 0x10 bytes C++ xul.dll!nsTableFrame::InsertFrames(mozilla::layout::FrameChildListID aListID=kPrincipalList, nsIFrame * aPrevFrame=0x00000000, nsFrameList & aFrameList={...}) Line 2240 + 0x10 bytes C++ xul.dll!nsFrameManager::InsertFrames(nsIFrame * aParentFrame=0x059edd90, mozilla::layout::FrameChildListID aListID=kPrincipalList, nsIFrame * aPrevFrame=0x00000000, nsFrameList & aFrameList={...}) Line 500 C++ xul.dll!nsCSSFrameConstructor::AppendFramesToParent(nsFrameConstructorState & aState={...}, nsIFrame * aParentFrame=0x059edd90, nsFrameItems & aFrameList={...}, nsIFrame * aPrevSibling=0x00000000, bool aIsRecursiveCall=false) Line 5728 C++ xul.dll!nsCSSFrameConstructor::ContentAppended(nsIContent * aContainer=0x059eff88, nsIContent * aFirstNewContent=0x050baec0, bool aAllowLazyConstruction=true) Line 6660 C++ xul.dll!PresShell::ContentAppended(nsIDocument * aDocument=0x08218b70, nsIContent * aContainer=0x059eff88, nsIContent * aFirstNewContent=0x050baec0, int aNewIndexInContainer=1) Line 4170 C++ xul.dll!nsNodeUtils::ContentAppended(nsIContent * aContainer=0x059eff88, nsIContent * aFirstNewContent=0x050baec0, int aNewIndexInContainer=1) Line 148 + 0xce bytes C++ xul.dll!nsHtml5PendingNotification::Fire() Line 61 + 0x2b bytes C++ xul.dll!nsHtml5TreeOpExecutor::FlushPendingAppendNotifications() Line 324 C++ xul.dll!nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor * aBuilder=0x08219ab8, nsIContent * * aScriptElement=0x0014ce48) Line 517 C++ xul.dll!nsHtml5TreeOpExecutor::RunFlushLoop() Line 530 C++ xul.dll!nsHtml5ExecutorReflusher::Run() Line 96 C++ xul.dll!nsThread::ProcessNextEvent(bool mayWait=false, bool * result=0x0014ceff) Line 657 + 0x19 bytes C++ xul.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x008a9e38, bool mayWait=false) Line 245 + 0x17 bytes C++ xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate=0x008a69a8) Line 110 + 0xe bytes C++ xul.dll!MessageLoop::RunInternal() Line 209 C++ xul.dll!MessageLoop::RunHandler() Line 202 C++ xul.dll!MessageLoop::Run() Line 176 C++ xul.dll!nsBaseAppShell::Run() Line 191 C++ xul.dll!nsAppShell::Run() Line 252 + 0x9 bytes C++ xul.dll!nsAppStartup::Run() Line 295 + 0x1c bytes C++ xul.dll!XRE_main(int argc=1, char * * argv=0x00188010, const nsXREAppData * aAppData=0x012fb844) Line 3703 + 0x25 bytes C++ firefox.exe!do_main(int argc=1, char * * argv=0x00188010) Line 190 + 0x13 bytes C++ firefox.exe!NS_internal_main(int argc=1, char * * argv=0x00188010) Line 277 + 0xd bytes C++ firefox.exe!wmain(int argc=1, wchar_t * * argv=0x00181b90) Line 107 + 0xd bytes C++ firefox.exe!__tmainCRTStartup() Line 583 + 0x19 bytes C firefox.exe!wmainCRTStartup() Line 403 C
Component: General → Layout: Tables
Product: Firefox → Core
QA Contact: general → layout.tables
Comment 1•12 years ago
|
||
So yeah. The incoming aFrameList has a table column group, then a table row group. The key part is that we have an :after frame which is a rowgroup, so when nsCSSFrameConstructor::AppendFramesToParent calls nsTableFrame::InsertFrames we can't just take the "no next sibling" escape to AppendFrames. Which suggests that we should do the childlist splitting bit that AppendFrames does in InsertFrames as well. Mats, do you have time to look at this, or should I grab it?
Comment 2•12 years ago
|
||
Comment 3•12 years ago
|
||
Since the two objects have different layouts I'll assume the bad cast can be exploited.
Keywords: sec-critical,
testcase
Assignee | ||
Comment 5•12 years ago
|
||
I can take this.
Assignee: nobody → matspal
Severity: normal → critical
OS: Linux → All
Hardware: x86_64 → All
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #628666 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 7•12 years ago
|
||
Collect all the ColGroupFrames into a separate frame list and insert those separately from the other frames. https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=bd916ffe5cba
Attachment #628671 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #3) > Since the two objects have different layouts I'll assume the bad cast can be > exploited. > class nsTableColGroupFrame : public nsContainerFrame > { > class nsTableRowGroupFrame > : public nsContainerFrame > , public nsILineIterator > { Is the (nsContainerFrame) vtable pointer is at the same offset for instances of these classes? If so, then it seems a lot harder to exploit this.
Updated•12 years ago
|
status-firefox-esr10:
--- → ?
status-firefox13:
--- → wontfix
status-firefox14:
--- → affected
status-firefox15:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox14:
--- → +
tracking-firefox15:
--- → +
Flags: in-testsuite?
Comment 10•12 years ago
|
||
Comment on attachment 628666 [details] [diff] [review] part 1, add a couple of asserts that will abort in debug builds r=me
Attachment #628666 -
Flags: review?(bzbarsky) → review+
Comment 11•12 years ago
|
||
Comment on attachment 628671 [details] [diff] [review] part 2, fix >+++ b/layout/tables/nsTableFrame.cpp > nsTableFrame::InsertFrames(ChildListID aListID, ... > // Asssume there's only one frame being inserted. The problem is that > // row group frames and col group frames go in separate child lists and > // so if there's more than one type of frames this gets messy... This comment no longer matches reality. Please update it? >+ // Collect ColGroupFrames into a separate list and insert those separately >+ // from the other frames (bug 759249). I'd been kinda hoping we could share code between here and AppendFrames. Too painful? >+nsTableFrame::HomogenousInsertFrames(ChildListID aListID, >+ // Verify that either all siblings have display:table-column-group, or they >+ // all not have display:table-column-group. "or they all have display values different from table-column-group". r=me with the comment nits picked, whether you share code with AppendFrames or not.
Attachment #628671 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #11) > This comment no longer matches reality. Please update it? The full comment now reads: // The frames in aFrameList can be a mix of row group frames and col group // frames. The problem is that they should go in separate child lists so // we need to deal with that here... // XXX The frame construction code should be separating out child frames // based on the type, bug 343048. > I'd been kinda hoping we could share code between here and AppendFrames. > Too painful? It should be possible to change AppendFrames to use the same code for separating the child frames based on type, then append each list to the respective child list (rather than appending each frame). (We could also try to move this separation to the frame constructor as we originally intended based on the XXX comment.) Anyway, I'll file a separate bug on doing the first step at least. I was aiming for the simplest possible fix here, to reduce risk for branches. https://hg.mozilla.org/integration/mozilla-inbound/rev/076b2d0381c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/05bd034be8af
Assignee | ||
Updated•12 years ago
|
Target Milestone: --- → mozilla15
Assignee | ||
Comment 13•12 years ago
|
||
(filed bug 760728 for using the new code in AppendFrames too)
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/076b2d0381c7 https://hg.mozilla.org/mozilla-central/rev/05bd034be8af
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 15•12 years ago
|
||
Verified fixed in 5/5 nightly trunk build. Original testcase crashes before fix went in. No crash now.
Status: RESOLVED → VERIFIED
Comment 16•12 years ago
|
||
(In reply to Mats Palmgren [:mats] from comment #9) > Is the (nsContainerFrame) vtable pointer is at the same offset for instances > of these classes? If so, then it seems a lot harder to exploit this. Does that matter? the functions called on the cast frame are all specific to nsTableColGroupFrame and don't exist in nsTableRowGroupFrame so who knows what those values will be?
Assignee | ||
Comment 17•12 years ago
|
||
As long as we don't overwrite a vtable pointer, member fields having bogus values seems a lot harder to exploit.
Assignee | ||
Comment 18•12 years ago
|
||
Comment on attachment 628666 [details] [diff] [review] part 1, add a couple of asserts that will abort in debug builds [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: crash Testing completed (on m-c, etc.): on m-c since 2012-06-02 (was uplifted to Fx15 aurora), no regressions so far Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none This request is for part 1 + 2.
Attachment #628666 -
Flags: approval-mozilla-beta?
Comment 19•12 years ago
|
||
Comment on attachment 628666 [details] [diff] [review] part 1, add a couple of asserts that will abort in debug builds [Triage Comment] Low risk, sg:crit fix. Approved for Beta 14.
Attachment #628666 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 20•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/8edb9d3cc28d https://hg.mozilla.org/releases/mozilla-beta/rev/922ad7eebb8e
Updated•12 years ago
|
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 628666 [details] [diff] [review] part 1, add a couple of asserts that will abort in debug builds [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: crash Testing completed (on m-c, etc.): on m-c since 2012-06-02 (was uplifted to Fx15 aurora), no regressions so far Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none This request is for part 1 + 2.
Attachment #628666 -
Flags: approval-mozilla-esr10?
Updated•12 years ago
|
Whiteboard: [advisory-tracking+]
Comment 22•12 years ago
|
||
Comment on attachment 628666 [details] [diff] [review] part 1, add a couple of asserts that will abort in debug builds Please go ahead and land, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process if you need any details about ESR specifics.
Attachment #628666 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 23•12 years ago
|
||
With the MOZ_ASSERTs changed to NS_ASSERTIONs: https://hg.mozilla.org/releases/mozilla-esr10/rev/b5e449c3fc40 https://hg.mozilla.org/releases/mozilla-esr10/rev/eec273cdea3b
Updated•12 years ago
|
Alias: CVE-2012-1952
Updated•12 years ago
|
Updated•12 years ago
|
Group: core-security
Assignee | ||
Comment 24•11 years ago
|
||
Crash tests: https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ac8aa731dd
Flags: in-testsuite? → in-testsuite+
Updated•11 years ago
|
Flags: sec-bounty+
You need to log in
before you can comment on or make changes to this bug.
Description
•