Closed Bug 759249 (CVE-2012-1952) Opened 8 years ago Closed 8 years ago

Bad cast in nsTableFrame::InsertFrames

Categories

(Core :: Layout: Tables, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla15
Tracking Status
firefox13 --- wontfix
firefox14 + verified
firefox15 + verified
firefox-esr10 14+ verified

People

(Reporter: inferno, Assigned: mats)

Details

(Keywords: sec-critical, testcase, Whiteboard: [advisory-tracking+])

Attachments

(5 files)

Attached file Testcase
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
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?
Since the two objects have different layouts I'll assume the bad cast can be exploited.
I can take this.
Assignee: nobody → matspal
Severity: normal → critical
OS: Linux → All
Hardware: x86_64 → All
Attached patch part 2, fixSplinter Review
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)
(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.
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 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+
(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
Target Milestone: --- → mozilla15
(filed bug 760728 for using the new code in AppendFrames too)
Verified fixed in 5/5 nightly trunk build. Original testcase crashes before fix went in. No crash now.
Status: RESOLVED → VERIFIED
(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?
As long as we don't overwrite a vtable pointer, member fields having
bogus values seems a lot harder to exploit.
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 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+
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?
Whiteboard: [advisory-tracking+]
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+
Alias: CVE-2012-1952
Group: core-security
Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ac8aa731dd
Flags: in-testsuite? → in-testsuite+
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.