Closed Bug 1076918 Opened 5 years ago Closed 5 years ago
Heap-buffer-overflow in ns
Transformed Text Run::Set Capitalization
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 . . .
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.
[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
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.
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+
[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 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?
And landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3e41e40b7e so that it won't end up on aurora.
Assignee: nobody → mats
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Confirmed crash on Fx35, 2014-09-15. Verified fixed on Fx35 release candidate, 2015-01-06.
You need to log in before you can comment on or make changes to this bug.