Bug 814713 (CVE-2013-0744)

Heap-use-after-free in TableBackgroundPainter::TableBackgroundData::Destroy

RESOLVED FIXED in Firefox 18

Status

()

Core
Layout: Tables
--
critical
RESOLVED FIXED
5 years ago
9 months ago

People

(Reporter: Atte Kettunen, Assigned: mats)

Tracking

(4 keywords)

unspecified
mozilla20
crash, csectype-uaf, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox17 wontfix, firefox18+ fixed, firefox19+ fixed, firefox20+ fixed, firefox-esr1018+ fixed, firefox-esr1718+ fixed)

Details

(Whiteboard: [asan][adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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
.
.
.
(Reporter)

Updated

5 years ago
Whiteboard: [asan]

Updated

5 years ago
Component: General → Layout
Product: Firefox → Core

Comment 1

5 years ago
Näyttäisi olevan layout-bugi.
(Assignee)

Updated

5 years ago
Assignee: nobody → matspal
Severity: normal → critical
Keywords: crash, sec-critical
OS: Linux → All
Hardware: x86_64 → All
Version: Trunk → unspecified
(Assignee)

Updated

5 years ago
Component: Layout → Layout: Tables
(Assignee)

Comment 2

5 years ago
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.
Attachment #684853 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
status-firefox-esr10: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
(Assignee)

Comment 3

5 years ago
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=47cb93d6b6e0
Comment on attachment 684853 [details] [diff] [review]
fix

r=me
Attachment #684853 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 5

5 years ago
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
Attachment #684853 - Flags: sec-approval?
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?
Attachment #684853 - Flags: sec-approval? → sec-approval+
We'll want branch patches then too.

Comment 8

5 years ago
(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.
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
Flags: sec-bounty?
Attachment #684709 - Attachment mime type: text/plain → text/html
tracking-firefox-esr10: --- → 18+
tracking-firefox18: ? → +
tracking-firefox19: ? → +
tracking-firefox20: --- → +
tracking-firefox-esr17: --- → 18+

Comment 9

5 years ago
(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.
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b09993c15f7
Flags: in-testsuite?
https://hg.mozilla.org/mozilla-central/rev/6b09993c15f7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox20: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Comment 12

5 years ago
+cc: tn for the beta landing
https://hg.mozilla.org/releases/mozilla-beta/rev/40eef1dfd5de
status-firefox18: affected → fixed
(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 !
(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.)
Also, sec-approval isn't release driver approval for branch uplifts last I checked...
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.
https://hg.mozilla.org/releases/mozilla-esr17/rev/1d2053bdab61

It's also in my queue for aurora landing.
status-firefox17: affected → wontfix
status-firefox-esr17: affected → fixed
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f4072d98ec5
status-firefox19: affected → fixed
(Assignee)

Comment 20

5 years ago
Comment on attachment 684853 [details] [diff] [review]
fix

see comment 5
Attachment #684853 - Flags: approval-mozilla-esr10?
Flags: sec-bounty? → sec-bounty+
Keywords: testcase
Attachment #684853 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr10/rev/7bea75339532
status-firefox-esr10: affected → fixed
Whiteboard: [asan] → [asan][adv-main17+][adv-esr17+][adv-esr10+]
Whiteboard: [asan][adv-main17+][adv-esr17+][adv-esr10+] → [asan][adv-main18+][adv-esr17+][adv-esr10+]
Alias: CVE-2013-0744
Group: core-security
(Assignee)

Comment 23

4 years ago
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44871c9b39d0
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/44871c9b39d0
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.