1076918 - Heap-buffer-overflow in nsTransformedTextRun::SetCapitalization

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Atte Kettunen]

Attachment: repro-file.html

Tested on:

OS: Ubuntu 12.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1412244122/


The ASAN-trace of this bug looks a lot like the trace in bug 1041512.

ASAN-trace:

==14684==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c0002a4388 at pc 0x7f6be330e505 bp 0x7fffbe533980 sp 0x7fffbe533978
READ of size 8 at 0x60c0002a4388 thread T0
    #0 0x7f6be330e504 in Length /builds/slave/m-cen-l64-asan-ntly-0000000000/build/obj-firefox/layout/generic/../../dist/include/nsTArray.h:329:0    #1 0x7f6be330e504 in IsEmpty /builds/slave/m-cen-l64-asan-ntly-0000000000/build/obj-firefox/layout/generic/../../dist/include/nsTArray.h:332:0    #2 0x7f6be330e504 in nsTransformedTextRun::SetCapitalization(unsigned int, unsigned int, bool*, gfxContext*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextRunTransformations.cpp:60:0
    #3 0x7f6be210d31a in nsLineBreaker::AppendText(nsIAtom*, char16_t const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsLineBreaker.cpp:291:0
    #4 0x7f6be210dd4b in nsLineBreaker::AppendText(nsIAtom*, unsigned char const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsLineBreaker.cpp:327:0
    #5 0x7f6be32c7505 in BuildTextRunsScanner::SetupBreakSinksForTextRun(gfxTextRun*, void const*, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:2416:0
    #6 0x7f6be32bd933 in BuildTextRunsScanner::SetupLineBreakerContext(gfxTextRun*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:2321:0
    #7 0x7f6be32bbb33 in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:1459:0
    #8 0x7f6be32ca2b8 in BuildTextRuns /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:1407:0    #9 0x7f6be32ca2b8 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:2576:0
    #10 0x7f6be32fb8c5 in nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, nsHTMLReflowMetrics&, unsigned int&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:8014:0
.
.
.
0x60c0002a4388 is located 12 bytes to the right of 124-byte region [0x60c0002a4300,0x60c0002a437c)
allocated by thread T0 here:
    #0 0x7f6beebac4f1 in malloc _asan_rtl_:0
    #1 0x7f6bdfb63c39 in AllocateStorageForTextRun /builds/slave/m-cen-l64-asan-ntly-0000000000/build/gfx/thebes/gfxTextRun.cpp:105:0    #2 0x7f6bdfb63c39 in Create /builds/slave/m-cen-l64-asan-ntly-0000000000/build/gfx/thebes/gfxTextRun.cpp:122:0    #3 0x7f6bdfb63c39 in gfxFontGroup::MakeSpaceTextRun(gfxTextRunFactory::Parameters const*, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/gfx/thebes/gfxTextRun.cpp:1911:0
    #4 0x7f6be32c14fd in MakeTextRun<unsigned char> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:563:0    #5 0x7f6be32c14fd in BuildTextRunsScanner::BuildTextRunForFrames(void*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:2162:0
    #6 0x7f6be32bc1c0 in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:1478:0
    #7 0x7f6be32ca2b8 in BuildTextRuns /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:1407:0    #8 0x7f6be32ca2b8 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:2576:0
    #9 0x7f6be32fb8c5 in nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, nsHTMLReflowMetrics&, unsigned int&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsTextFrame.cpp:8014:0
    #10 0x7f6be30cdadf in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/layout/generic/nsLineLayout.cpp:857:0
.
.
.

Comment 1

[Mats Palmgren (inactive)]

The crash symptoms are the same as in bug 1041512.
In this case though the resulting nsStyleChangeList in ComputeAndProcessStyleChange
ends up being empty, so we bail out early in ProcessRestyledFrames.

Comment 2

[Mats Palmgren (inactive)]

[Tracking Requested - why for this release]: sec-high

Comment 3

[Jonathan Kew [:jfkthame]]

If this is a regression from bug 950526, why doesn't it affect versions prior to firefox-35?

Comment 4

[Mats Palmgren (inactive)]

Sorry, I meant to remove that dependency since I have confirmed that my Aurora/Beta/Release
builds don't crash.  So this is a later regression.  I'll see if I can find a regression
range.

Comment 5

[Mats Palmgren (inactive)]

The regression is from bug 931668.  The revision before it (f83374a6c136) does NOT crash,
the revision after it (635055d16df6) does crash.
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dddbe46f3ceb&tochange=0f2020c52ad7

Comment 6

[Mats Palmgren (inactive)]

Fwiw, the crash doesn't occur with an unconditional ClearTextRuns() in
nsTextFrame::DidSetStyleContext (that bug 950526 removed):
https://bugzilla.mozilla.org/attachment.cgi?id=8393354&action=diff#a/layout/generic/nsTextFrame.cpp_sec1
Fwiw, my wallpaper patch in bug 1041512 also prevents the crash.

Comment 7

[Cameron McCormack (:heycam)]

So maybe we do indeed need a style change hint that means "rebuild text runs" (David's second suggestion in bug 1041512 comment 19), which nsStyleText::CalcDifference can return on a text-transform change?

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Cameron McCormack (:heycam) (away October 6 - November 5) from comment #7)
> So maybe we do indeed need a style change hint that means "rebuild text
> runs" (David's second suggestion in bug 1041512 comment 19), which
> nsStyleText::CalcDifference can return on a text-transform change?

That sounds pretty sensible to me. :mats, are you up for trying that, given that it looks like :heycam is about to disappear for a month?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd still like to understand why the existing mechanism isn't working here; it seems like something else is wrong.

I think it's probably also worth taking attachment 8475392 [details] [diff] [review] as a belt-and-braces fix, but with an assertion in the condition as well.

Comment 10

[Mats Palmgren (inactive)]

Attachment: wallpaper

The "wip" patch from bug 1041512 with an added NS_ERROR that dbaron
requested in comment 9.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8503667 [details] [diff] [review]
wallpaper

r=dbaron, although it would probably be good to add a comment that this is a belt-and-braces check

Comment 12

[Mats Palmgren (inactive)]

Attachment: wallpaper

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?
>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The patch reveals that it has something to do with text-transform:capitalize.
It appears it's not that hard to find a small testcase that crashes using a fuzzer,
as this bug and bug 1081550, and bug 1041512 shows.  I haven't analyzed the crash
condition in detail so I don't know how exploitable it is.

>Which older supported branches are affected by this flaw?

none

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

bug 931668

>Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

n/a

>How likely is this patch to cause regressions; how much testing does it need?

minimal risk, no special testing needed

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8503679 [details] [diff] [review]
wallpaper

You don't need sec-approval for bugs that have not yet reached mozilla-aurora:

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And landed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3e41e40b7e
so that it won't end up on aurora.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4e3e41e40b7e

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on Fx35, 2014-09-15.
Verified fixed on Fx35 release candidate, 2015-01-06.