Closed Bug 1076918 Opened 5 years ago Closed 5 years ago

Heap-buffer-overflow in nsTransformedTextRun::SetCapitalization

Categories

(Core :: CSS Parsing and Computation, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 --- unaffected
firefox34 --- unaffected
firefox35 + verified
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.2 --- fixed

People

(Reporter: attekett, Assigned: mats, NeedInfo)

References

Details

(5 keywords)

Attachments

(2 files, 1 obsolete file)

Attached file 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
.
.
.
Component: General → Layout: Text
Flags: needinfo?(dbaron)
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.
Blocks: 950526
Severity: normal → critical
Component: Layout: Text → CSS Parsing and Computation
[Tracking Requested - why for this release]: sec-high
If this is a regression from bug 950526, why doesn't it affect versions prior to firefox-35?
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.
No longer blocks: 950526
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
Blocks: 931668
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.
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?
(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?
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.
Attached patch wallpaper (obsolete) — Splinter Review
The "wip" patch from bug 1041512 with an added NS_ERROR that dbaron
requested in comment 9.
Attachment #8503667 - Flags: review?(jfkthame)
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
Attachment #8503667 - Flags: review?(jfkthame) → review+
Attached patch wallpaperSplinter Review
[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
Attachment #8503667 - Attachment is obsolete: true
Attachment #8503679 - Flags: sec-approval?
Attachment #8503679 - Flags: review+
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
Attachment #8503679 - Flags: sec-approval?
Flags: needinfo?(dbaron)
Flags: needinfo?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/4e3e41e40b7e
Assignee: nobody → mats
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Duplicate of this bug: 1081550
Confirmed crash on Fx35, 2014-09-15.
Verified fixed on Fx35 release candidate, 2015-01-06.
Status: RESOLVED → VERIFIED
Group: core-security
You need to log in before you can comment on or make changes to this bug.