814713 - (CVE-2013-0744) Heap-use-after-free in TableBackgroundPainter::TableBackgroundData::Destroy

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Atte Kettunen]

Attachment: 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][[e-mail]]]

Näyttäisi olevan layout-bugi.

Comment 2

[Mats Palmgren (inactive)]

Attachment: 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 3

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=47cb93d6b6e0

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 684853 [details] [diff] [review]
fix

r=me

Comment 5

[Mats Palmgren (inactive)]

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 - ex-MoCo]]

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 - ex-MoCo]]

We'll want branch patches then too.

Comment 8

[Robert Kaiser]

(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]]

(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 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b09993c15f7

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6b09993c15f7

Comment 12

[Jet Villegas (inactive)]

+cc: tn for the beta landing

Comment 13

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-beta/rev/40eef1dfd5de

Comment 14

[bhavana bajaj [:bajaj]]

(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]]

(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]]

Also, sec-approval isn't release driver approval for branch uplifts last I checked...

Comment 17

[Timothy Nikkel (:tnikkel)]

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

https://hg.mozilla.org/releases/mozilla-esr17/rev/1d2053bdab61

It's also in my queue for aurora landing.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f4072d98ec5

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 684853 [details] [diff] [review]
fix

see comment 5

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/7bea75339532

Comment 23

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44871c9b39d0

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44871c9b39d0