Last Comment Bug 814713 - (CVE-2013-0744) Heap-use-after-free in TableBackgroundPainter::TableBackgroundData::Destroy
(CVE-2013-0744)
: Heap-use-after-free in TableBackgroundPainter::TableBackgroundData::Destroy
Status: RESOLVED FIXED
[asan][adv-main18+][adv-esr17+][adv-e...
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla20
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-23 08:11 PST by Atte Kettunen
Modified: 2014-07-24 14:37 PDT (History)
10 users (show)
dveditz: sec‑bounty+
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
18+
fixed
18+
fixed


Attachments
repro-file (2.68 KB, text/html)
2012-11-23 08:11 PST, Atte Kettunen
no flags Details
fix (1.16 KB, patch)
2012-11-24 07:36 PST, Mats Palmgren (:mats)
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑esr10+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description Atte Kettunen 2012-11-23 08:11:25 PST
Created attachment 684709 [details]
repro-file

Tested on:

OS: Ubuntu 12.04 x86_64

Firefox build: https://people.mozilla.com/~choller/firefox/asan/20121123-mozilla-central-linux64-debug-d8e4f06198dc+asan.html


ASAN-report snippet:

###!!! ASSERTION: colgroup data should not be null - bug 237421: 'mCols[i].mColGroup', file /builds/slave/try-lnx64-dbg/build/layout/tables/nsTablePainter.cpp, line 314
###!!! ASSERTION: colgroup data should not be null - bug 237421: 'mCols[i].mColGroup', file /builds/slave/try-lnx64-dbg/build/layout/tables/nsTablePainter.cpp, line 314
=================================================================
==5093== ERROR: AddressSanitizer heap-use-after-free on address 0x7fb9757737a8 at pc 0x7fb99e71f5a3 bp 0x7fffd048eae0 sp 0x7fffd048ead8
READ of size 8 at 0x7fb9757737a8 thread T0
    #0 0x7fb99e71f5a2 in TableBackgroundPainter::TableBackgroundData::Destroy(nsPresContext*) /builds/slave/try-lnx64-dbg/build/layout/tables/nsTablePainter.cpp:123
    #1 0x7fb99e72016b in ~TableBackgroundPainter /builds/slave/try-lnx64-dbg/build/layout/tables/nsTablePainter.cpp:239
    #2 0x7fb99e6df4b6 in nsTableFrame::PaintTableBorderBackground(nsRenderingContext&, nsRect const&, nsPoint, unsigned int) /builds/slave/try-lnx64-dbg/build/layout/tables/nsTableFrame.cpp:1310
    #3 0x7fb99e6df1a8 in nsDisplayTableBorderBackground::Paint(nsDisplayListBuilder*, nsRenderingContext*) /builds/slave/try-lnx64-dbg/build/layout/tables/nsTableFrame.cpp:1086
    #4 0x7fb99e20e74c in mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*) /builds/slave/try-lnx64-dbg/build/layout/base/FrameLayerBuilder.cpp:3274
    #5 0x7fb9a094c008 in mozilla::layers::BasicThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*) /builds/slave/try-lnx64-dbg/build/gfx/layers/basic/BasicThebesLayer.h:94
.
.
.
freed by thread T0 here:
    #0 0x43f180 in operator delete(void*) ??:0
    #1 0x7fb99e72017b in ~TableBackgroundPainter /builds/slave/try-lnx64-dbg/build/layout/tables/nsTablePainter.cpp:240
    #2 0x7fb99e6df4b6 in nsTableFrame::PaintTableBorderBackground(nsRenderingContext&, nsRect const&, nsPoint, unsigned int) /builds/slave/try-lnx64-dbg/build/layout/tables/nsTableFrame.cpp:1310
    #3 0x7fb99e6df1a8 in nsDisplayTableBorderBackground::Paint(nsDisplayListBuilder*, nsRenderingContext*) /builds/slave/try-lnx64-dbg/build/layout/tables/nsTableFrame.cpp:1086
    #4 0x7fb99e20e74c in mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*) /builds/slave/try-lnx64-dbg/build/layout/base/FrameLayerBuilder.cpp:3274
    #5 0x7fb9a094c008 in mozilla::layers::BasicThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*) /builds/slave/try-lnx64-dbg/build/gfx/layers/basic/BasicThebesLayer.h:94
.
.
.
Comment 1 Olli Pettay [:smaug] (reviewing overload) 2012-11-23 10:24:45 PST
Näyttäisi olevan layout-bugi.
Comment 2 Mats Palmgren (:mats) 2012-11-24 07:36:07 PST
Created attachment 684853 [details] [diff] [review]
fix

The testcase has 76 colgroups, each with 1000 cols (the SPAN attr value
is capped at 1000).  In TableBackgroundPainter::PaintTable we have
mNumCols == 76000, and it allocates an array (mCols) of that size to
create a TableBackgroundData struct for each col.  The problem is we
write into that array with "mCols[colIndex]" where colIndex comes from
the col frame's mColIndex so we start overwriting the start of the array
after col 65535, leaving later mCols entries uninitialized.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-11-26 19:41:35 PST
Comment on attachment 684853 [details] [diff] [review]
fix

r=me
Comment 5 Mats Palmgren (:mats) 2012-11-27 13:36:42 PST
Comment on attachment 684853 [details] [diff] [review]
fix

[Security approval request comment]
How easily can the security issue be deduced from the patch?
The code comment and code change makes it is easy to figure out that it's an
integer overflow type of problem involving <table>, <col> or <colgroup>.
Armed with that knowledge I suspect it would only take a few hours to
come up with a testcase that crashes.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
see above

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
same patch should apply on all branches (with the usual uint32_t/PRUint32)

How likely is this patch to cause regressions; how much testing does it need?
very unlikely to cause regressions
Comment 6 Al Billings [:abillings] 2012-11-27 16:36:10 PST
Comment on attachment 684853 [details] [diff] [review]
fix

This is sec-approval+ but not until December 15 or later. Please check it in then.

Does this need unit tests?
Comment 7 Al Billings [:abillings] 2012-11-27 16:36:45 PST
We'll want branch patches then too.
Comment 8 Robert Kaiser 2012-11-27 18:21:57 PST
(In reply to Al Billings [:abillings] from comment #7)
> We'll want branch patches then too.

Given that, nominating for tracking the branches so we don't forget it.
Comment 9 Alex Keybl [:akeybl] 2012-12-10 06:41:51 PST
(In reply to Al Billings [:abillings] from comment #6)
> Comment on attachment 684853 [details] [diff] [review]
> fix
> 
> This is sec-approval+ but not until December 15 or later. Please check it in
> then.
> 
> Does this need unit tests?

Correction - please land today on mozilla-central and prepare branch patches for landing tomorrow. We'd like this to make it into Firefox 18 Beta 4.
Comment 11 Ed Morley [:emorley] 2012-12-11 08:31:55 PST
https://hg.mozilla.org/mozilla-central/rev/6b09993c15f7
Comment 12 Jet Villegas (:jet) 2012-12-11 14:37:39 PST
+cc: tn for the beta landing
Comment 13 Timothy Nikkel (:tnikkel) 2012-12-11 14:45:51 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/40eef1dfd5de
Comment 14 bhavana bajaj [:bajaj] 2012-12-12 15:29:06 PST
(In reply to Mats Palmgren [:mats] from comment #10)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6b09993c15f7

Can you please help with landing patches needed for aurora/esr branches ? Thanks !
Comment 15 Daniel Holbert [:dholbert] 2012-12-12 15:32:48 PST
(In reply to Alex Keybl [:akeybl] from comment #9)
> Correction - please land today on mozilla-central and prepare branch patches
> for landing tomorrow. We'd like this to make it into Firefox 18 Beta 4.

(In reply to Timothy Nikkel (:tn) from comment #13)
> https://hg.mozilla.org/releases/mozilla-beta/rev/40eef1dfd5de

(For future reference: it's best to land on branches in reverse order, to be sure we never backport a fix to Firefox N while accidentally leaving Firefox N+1 vulnerable when it's released.  So, in comment 13, we technically should've landed this on aurora before landing it on beta.)
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-12-12 15:46:38 PST
Also, sec-approval isn't release driver approval for branch uplifts last I checked...
Comment 17 Timothy Nikkel (:tnikkel) 2012-12-12 15:49:09 PST
Indeed. I was asked to land this on beta and I assumed all the ducks were in order to do so but I failed to look closely enough at this bug to notice they weren't. Sorry.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-12-12 15:52:03 PST
https://hg.mozilla.org/releases/mozilla-esr17/rev/1d2053bdab61

It's also in my queue for aurora landing.
Comment 19 Ryan VanderMeulen [:RyanVM] 2012-12-12 16:41:00 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f4072d98ec5
Comment 20 Mats Palmgren (:mats) 2012-12-14 20:49:01 PST
Comment on attachment 684853 [details] [diff] [review]
fix

see comment 5
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-12-19 13:45:22 PST
https://hg.mozilla.org/releases/mozilla-esr10/rev/7bea75339532
Comment 23 Mats Palmgren (:mats) 2013-05-13 07:34:47 PDT
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44871c9b39d0
Comment 24 Ryan VanderMeulen [:RyanVM] 2013-05-13 13:45:10 PDT
https://hg.mozilla.org/mozilla-central/rev/44871c9b39d0

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