Last Comment Bug 759249 - (CVE-2012-1952) Bad cast in nsTableFrame::InsertFrames
(CVE-2012-1952)
: Bad cast in nsTableFrame::InsertFrames
Status: VERIFIED FIXED
[advisory-tracking+]
: sec-critical, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla15
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-28 23:28 PDT by Abhishek Arya
Modified: 2014-07-02 12:48 PDT (History)
5 users (show)
rforbes: sec‑bounty+
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
14+
verified


Attachments
Testcase (152 bytes, text/html)
2012-05-28 23:28 PDT, Abhishek Arya
no flags Details
Simpler testcase (218 bytes, text/html)
2012-05-29 22:04 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
part 1, add a couple of asserts that will abort in debug builds (2.32 KB, patch)
2012-05-31 03:51 PDT, Mats Palmgren (:mats)
bzbarsky: review+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review
part 2, fix (6.87 KB, patch)
2012-05-31 03:56 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Review
part 3, crash tests (DO NOT LAND BEFORE BUG IS PUBLIC) (1.36 KB, patch)
2012-05-31 03:58 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review

Description Abhishek Arya 2012-05-28 23:28:57 PDT
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
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-29 22:03:40 PDT
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 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-29 22:04:19 PDT
Created attachment 628220 [details]
Simpler testcase
Comment 3 Daniel Veditz [:dveditz] 2012-05-30 10:33:32 PDT
Since the two objects have different layouts I'll assume the bad cast can be exploited.
Comment 5 Mats Palmgren (:mats) 2012-05-30 15:43:11 PDT
I can take this.
Comment 6 Mats Palmgren (:mats) 2012-05-31 03:51:41 PDT
Created attachment 628666 [details] [diff] [review]
part 1, add a couple of asserts that will abort in debug builds
Comment 7 Mats Palmgren (:mats) 2012-05-31 03:56:30 PDT
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
Comment 8 Mats Palmgren (:mats) 2012-05-31 03:58:15 PDT
Created attachment 628672 [details] [diff] [review]
part 3, crash tests (DO NOT LAND BEFORE BUG IS PUBLIC)
Comment 9 Mats Palmgren (:mats) 2012-05-31 04:11:48 PDT
(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 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-31 18:34:11 PDT
Comment on attachment 628666 [details] [diff] [review]
part 1, add a couple of asserts that will abort in debug builds

r=me
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-31 18:42:59 PDT
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.
Comment 12 Mats Palmgren (:mats) 2012-06-01 13:16:22 PDT
(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
Comment 13 Mats Palmgren (:mats) 2012-06-01 16:12:36 PDT
(filed bug 760728 for using the new code in AppendFrames too)
Comment 15 [On PTO until 6/29] 2012-06-06 14:06:54 PDT
Verified fixed in 5/5 nightly trunk build. Original testcase crashes before fix went in. No crash now.
Comment 16 Daniel Veditz [:dveditz] 2012-06-11 15:49:03 PDT
(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?
Comment 17 Mats Palmgren (:mats) 2012-06-11 19:01:39 PDT
As long as we don't overwrite a vtable pointer, member fields having
bogus values seems a lot harder to exploit.
Comment 18 Mats Palmgren (:mats) 2012-06-11 19:06:59 PDT
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.
Comment 19 Alex Keybl [:akeybl] 2012-06-14 09:53:47 PDT
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.
Comment 21 Mats Palmgren (:mats) 2012-07-09 08:45:21 PDT
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.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2012-07-11 12:52:35 PDT
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.
Comment 23 Mats Palmgren (:mats) 2012-07-11 14:55:17 PDT
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
Comment 24 Mats Palmgren (:mats) 2013-05-14 06:59:32 PDT
Crash tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ac8aa731dd
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-05-14 13:29:42 PDT
https://hg.mozilla.org/mozilla-central/rev/a8ac8aa731dd

Note You need to log in before you can comment on or make changes to this bug.