Bug 759249 (CVE-2012-1952)

Bad cast in nsTableFrame::InsertFrames

VERIFIED FIXED in Firefox 14

Status

()

Core
Layout: Tables
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Abhishek Arya, Assigned: mats)

Tracking

({sec-critical, testcase})

Trunk
mozilla15
sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox13 wontfix, firefox14+ verified, firefox15+ verified, firefox-esr1014+ verified)

Details

(Whiteboard: [advisory-tracking+])

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Created attachment 627858 [details]
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?
Created attachment 628220 [details]
Simpler testcase
Since the two objects have different layouts I'll assume the bad cast can be exploited.
Keywords: sec-critical, testcase
(Assignee)

Comment 5

5 years ago
I can take this.
Assignee: nobody → matspal
Severity: normal → critical
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 6

5 years ago
Created attachment 628666 [details] [diff] [review]
part 1, add a couple of asserts that will abort in debug builds
Attachment #628666 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

5 years ago
Created attachment 628671 [details] [diff] [review]
part 2, fix

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

5 years ago
Created attachment 628672 [details] [diff] [review]
part 3, crash tests (DO NOT LAND BEFORE BUG IS PUBLIC)
(Assignee)

Comment 9

5 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.
status-firefox-esr10: --- → ?
status-firefox13: --- → wontfix
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox14: --- → +
tracking-firefox15: --- → +
Flags: in-testsuite?
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+
(Assignee)

Comment 12

5 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

5 years ago
Target Milestone: --- → mozilla15
(Assignee)

Comment 13

5 years ago
(filed bug 760728 for using the new code in AppendFrames too)
https://hg.mozilla.org/mozilla-central/rev/076b2d0381c7
https://hg.mozilla.org/mozilla-central/rev/05bd034be8af
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
(Assignee)

Comment 17

5 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

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

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/8edb9d3cc28d
https://hg.mozilla.org/releases/mozilla-beta/rev/922ad7eebb8e
status-firefox14: affected → fixed
status-firefox15: affected → fixed
status-firefox-esr10: ? → affected
tracking-firefox-esr10: ? → 14+
(Assignee)

Comment 21

5 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?
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+
(Assignee)

Comment 23

5 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
status-firefox-esr10: affected → fixed
Alias: CVE-2012-1952

Updated

5 years ago
status-firefox-esr10: fixed → verified
status-firefox14: fixed → verified
status-firefox15: fixed → verified
Group: core-security
(Assignee)

Comment 24

4 years ago
Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ac8aa731dd
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/a8ac8aa731dd
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.