Closed
Bug 1291016
(CVE-2016-5270)
Opened 8 years ago
Closed 8 years ago
Heap-buffer-overflow in nsCaseTransformTextRunFactory::TransformString
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
People
(Reporter: attekett, Assigned: jfkthame)
References
Details
(5 keywords, Whiteboard: [adv-main49+][adv-esr45.4+])
Attachments
(4 files)
8.29 KB,
text/html
|
Details | |
3.03 KB,
patch
|
heycam
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr45+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
988 bytes,
patch
|
Details | Diff | Splinter Review | |
2.88 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
Tested on:
OS: Ubuntu 14.04
Firefox: ASAN build from https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1470062453/
Repro-file as an attachment.
ASAN-trace:
==8332==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61d0002dd080 at pc 0x7f09051bc66b bp 0x7ffd0d8d03f0 sp 0x7ffd0d8d03e8
WRITE of size 1 at 0x61d0002dd080 thread T0 (Web Content)
#0 0x7f09051bc66a in nsCaseTransformTextRunFactory::TransformString(nsAString_internal const&, nsString&, bool, nsIAtom const*, nsTArray<bool>&, nsTArray<bool>&, nsTransformedTextRun*, nsTArray<unsigned char>*, nsTArray<RefPtr<nsTransformedCharStyle> >*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextRunTransformations.cpp:484:47
#1 0x7f09051bd328 in nsCaseTransformTextRunFactory::RebuildTextRun(nsTransformedTextRun*, mozilla::gfx::DrawTarget*, gfxMissingFontRecorder*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextRunTransformations.cpp:619:22
#2 0x7f0905165491 in FinishSettingProperties /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextRunTransformations.h:150:7
#3 0x7f0905165491 in BuildTextRunsScanner::BreakSink::Finish(gfxMissingFontRecorder*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:955
#4 0x7f0905164f05 in BuildTextRunsScanner::FlushLineBreaks(gfxTextRun*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:1483:5
#5 0x7f090515bf9f in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:1462:5
#6 0x7f090516e629 in BuildTextRuns /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:1386:3
#7 0x7f090516e629 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, mozilla::gfx::DrawTarget*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:2668
#8 0x7f09051ae576 in nsTextFrame::ReflowText(nsLineLayout&, int, mozilla::gfx::DrawTarget*, mozilla::ReflowOutput&, unsigned int&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:8879:5
#9 0x7f0904edd7cd in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, mozilla::ReflowOutput*, bool&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsLineLayout.cpp:945:5
#10 0x7f0904f799f5 in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsBlockFrame.cpp:4089:3
#11 0x7f0904f78711 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsBlockFrame.cpp:3891:5
.
.
.
0x61d0002dd080 is located 0 bytes to the right of 2048-byte region [0x61d0002dc880,0x61d0002dd080)
allocated by thread T0 (Web Content) here:
#0 0x4b27ce in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:71:3
#1 0x4e0d7d in moz_xrealloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/memory/mozalloc/mozalloc.cpp:105:20
#2 0x7f08fdaf2751 in Realloc /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray.h:182:12
#3 0x7f08fdaf2751 in nsTArrayInfallibleAllocator::ResultTypeProxy nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned long, unsigned long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray-inl.h:183
#4 0x7f09051babca in AppendElement<bool, nsTArrayInfallibleAllocator> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/nsTArray.h:2027:32
#5 0x7f09051babca in nsCaseTransformTextRunFactory::TransformString(nsAString_internal const&, nsString&, bool, nsIAtom const*, nsTArray<bool>&, nsTArray<bool>&, nsTransformedTextRun*, nsTArray<unsigned char>*, nsTArray<RefPtr<nsTransformedCharStyle> >*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextRunTransformations.cpp:570
#6 0x7f09051bd328 in nsCaseTransformTextRunFactory::RebuildTextRun(nsTransformedTextRun*, mozilla::gfx::DrawTarget*, gfxMissingFontRecorder*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextRunTransformations.cpp:619:22
#7 0x7f0905165491 in FinishSettingProperties /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextRunTransformations.h:150:7
#8 0x7f0905165491 in BuildTextRunsScanner::BreakSink::Finish(gfxMissingFontRecorder*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:955
#9 0x7f0905164f05 in BuildTextRunsScanner::FlushLineBreaks(gfxTextRun*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:1483:5
#10 0x7f090515bf9f in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:1462:5
#11 0x7f090516e629 in BuildTextRuns /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:1386:3
#12 0x7f090516e629 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, mozilla::gfx::DrawTarget*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsTextFrame.cpp:2668
.
.
.
Comment 1•8 years ago
|
||
Jonathan, we are rating this sec-critical, but we'd like you to evaluate. If it isn't as serious, we can consider downgrading, but CritSmash group feels it's worthy in any case. Thanks.
Flags: needinfo?(jfkthame)
Keywords: sec-critical
Updated•8 years ago
|
Group: core-security → gfx-core-security
Assignee | ||
Comment 2•8 years ago
|
||
The out-of-bounds write here is only writing a bool (and AFAICS the value written will always be 'true'), which seems like it will severely limit the flexibility an attacker has to manipulate things.
As such, I'm guessing it would be pretty hard to find a real exploit here, just based on the ability to overwrite something (on either the stack or heap -- the object with the overflow is an AutoTArray) with an 0x01 byte. But still, it _is_ an out-of-bounds write, which is a Bad Thing, and maybe in exactly the right circumstances it could be leveraged to achieve really bad outcomes; I'm not confident to claim it's impossible just because I don't personally know how to do it.
Fortunately, the fix is simple; patch coming.
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 3•8 years ago
|
||
The problem here arises when there's a character (like ß or ᾁ) that expands to multiple characters during uppercasing, and then we encounter a potential Irish grammatical prefix whose position (in the output string) we mark for possible further processing. This position will not be correct for the input string due to the earlier character expansion, leading to the out-of-bounds write if we then try to mark the input hyphen as a deleted character. The simple fix is to record both source and destination offsets, rather than using a single position as if it were valid for both.
Attachment #8779386 -
Flags: review?(cam)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Updated•8 years ago
|
Attachment #8779386 -
Flags: review?(cam) → review+
Assignee | ||
Updated•8 years ago
|
Blocks: 1014639
status-b2g-v2.6:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox-esr45:
--- → affected
status-thunderbird_esr38:
--- → affected
status-thunderbird_esr45:
--- → affected
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8779386 [details] [diff] [review]
Track source-string position of possible prefix
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
See comment 2; I think an actual exploit is probably difficult, because of the extremely limited scope of the flaw, but cannot rule it out entirely.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.
Which older supported branches are affected by this flaw?
All (everything since mozilla-32, I believe).
If not all supported branches, which bug introduced the flaw?
1014639
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch should transplant directly.
How likely is this patch to cause regressions; how much testing does it need?
Simple patch with minimal risk.
Attachment #8779386 -
Flags: sec-approval?
Assignee | ||
Comment 5•8 years ago
|
||
testcase |
This is a reduced version of the original testcase, to be used as a simple crashtest; it fails with 'Assertion failure: aIndex < Length() (invalid array index)' on current trunk, and loads fine after the patch. (To be landed once the bug is opened.)
Comment 6•8 years ago
|
||
sec-approval+ for the primary patch. We'll want a nominated patch for Aurora, Beta, and ESR45 once this lands on trunk as well.
tracking-firefox49:
--- → +
tracking-firefox50:
--- → +
tracking-firefox51:
--- → +
tracking-firefox-esr45:
--- → 49+
Updated•8 years ago
|
Attachment #8779386 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/32695cf856683158ecd0f70081837c3a789305c9
Bug 1291016 - Track source-string position of possible prefix. r=heycam
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8779386 [details] [diff] [review]
Track source-string position of possible prefix
Approval Request Comment
[Feature/regressing bug #]: Bug 1014639
[User impact if declined]: Content can trigger write-overflow of an AutoTArray buffer, leading to stack or heap corruption and potential crash or other exploit. (Fortunately, attacker only gets to write a single-byte bool 'true' value, which probably makes a full-blown exploit pretty hard to devise.)
[Describe test coverage new/current, TreeHerder]: Crashtest ready to land after we open the bug report.
[Risks and why]: Minimal, simple patch to use correct array index.
[String/UUID change made/needed]: None
Attachment #8779386 -
Flags: approval-mozilla-esr45?
Attachment #8779386 -
Flags: approval-mozilla-beta?
Attachment #8779386 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Flags: sec-bounty?
Comment 9•8 years ago
|
||
Comment on attachment 8779386 [details] [diff] [review]
Track source-string position of possible prefix
Crash fix, has sec approval. Let's uplift to aurora, beta, esr45.
Attachment #8779386 -
Flags: approval-mozilla-esr45?
Attachment #8779386 -
Flags: approval-mozilla-esr45+
Attachment #8779386 -
Flags: approval-mozilla-beta?
Attachment #8779386 -
Flags: approval-mozilla-beta+
Attachment #8779386 -
Flags: approval-mozilla-aurora?
Attachment #8779386 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr45/rev/7cd50d56bb61174296b20a9985ce71282a961ef3
Bug 1291016 - Track source-string position of possible prefix. r=heycam a=lizzard
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Updated•8 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•8 years ago
|
Group: gfx-core-security → core-security-release
Comment on attachment 8779386 [details] [diff] [review]
Track source-string position of possible prefix
Review of attachment 8779386 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/generic/nsTextRunTransformations.cpp
@@ +484,5 @@
> // (which must be the immediately-preceding char)
> NS_ASSERTION(str.Length() >= 2 && irishMark == str.Length() - 2,
> "bad irishMark!");
> str.Replace(irishMark, 2, ToLowerCase(str[irishMark]));
> + aDeletedCharsArray[irishMarkSrc + 1] = true;
irishMarkSrc is guaranteed to be set?
Comment 14•8 years ago
|
||
Hmm. It is guaranteed to be set to a valid value as much as irishMark is. Presumably the state machine in IrishMark.cpp never produces a "lowercase and delete" action without first producing a "mark" action, but it's not so easy to audit by eye.
How about we initialize irishMarkSrc to 0 (rather than -1, like irishMark) so that we at least write into a valid part of aDeletedCharsArray. And re-set it to 0 in the same places we re-set irishMark to -1.
Comment 15•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #14)
> How about we initialize irishMarkSrc to 0 (rather than -1, like irishMark)
> so that we at least write into a valid part of aDeletedCharsArray. And
> re-set it to 0 in the same places we re-set irishMark to -1.
Well, I'm not convinced of that. I'm not sure what the consequences are of flagging a deleted character in the input string different from the one that we actually remove from the output string.
Comment 16•8 years ago
|
||
So how about we just do this. I'm not sure whether it's worth adding explicit checks that we're not about to process a "lowercase and delete hyphen" action when irishMarkSrc == -1.
Attachment #8784221 -
Flags: review?(milan)
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•8 years ago
|
||
I don't know that this is a problem in practice, or that it is related to bug 1296630, but it's better than not initializing the variable.
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) from comment #14)
> Hmm. It is guaranteed to be set to a valid value as much as irishMark is.
> Presumably the state machine in IrishMark.cpp never produces a "lowercase
> and delete" action without first producing a "mark" action, but it's not so
> easy to audit by eye.
Yes, that is indeed the case. The only place the "lowercase and delete" action occurs is in kState_nt_ (9); the only way to get to kState_nt_ is from kState_n (8) or kState_t (10), and the only way to get to those states is from kState_Start via a transition that sets the mark.
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8784221 [details] [diff] [review]
initialize irishMarkSrc
Review of attachment 8784221 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/generic/nsTextRunTransformations.cpp
@@ +492,5 @@
> NS_ASSERTION(str.Length() >= 2 && irishMark == str.Length() - 2,
> "bad irishMark!");
> + MOZ_ASSERT((irishMark == uint32_t(-1)) ==
> + (irishMarkSrc == uint32_t(-1)),
> + "irishMark and irishMarkSrc initialization out of sync");
This seems unnecessarily verbose (and therefore takes a moment of extra thought to understand). How about just doing
MOZ_ASSERT(irishMark != uint32_t(-1) && irishMarkSrc != uint32_t(-1),
"failed to set irishMarks");
to verify that the marks have both been initialized?
Comment 20•8 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> This seems unnecessarily verbose (and therefore takes a moment of extra
> thought to understand). How about just doing
>
> MOZ_ASSERT(irishMark != uint32_t(-1) && irishMarkSrc != uint32_t(-1),
> "failed to set irishMarks");
>
> to verify that the marks have both been initialized?
Yes, I should've done that, thanks.
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8784221 [details] [diff] [review]
initialize irishMarkSrc
r=me for this, with a change along those lines.
(I don't believe this is related to bug 1296630, though.)
Attachment #8784221 -
Flags: review?(milan) → review+
Comment 22•8 years ago
|
||
Cam, don't forget to address the review comment and land the follow-up patch.
Flags: needinfo?(cam)
Updated•8 years ago
|
Whiteboard: [adv-main49+][adv-esr45.4+]
Updated•8 years ago
|
Flags: qe-verify+
Updated•8 years ago
|
Alias: CVE-2016-5270
Comment 23•8 years ago
|
||
I fixed the follow-up patch per comment 19 and pushed it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f86b1baa2d095862cab288ede231db4c37544c
Comment 24•8 years ago
|
||
Reproduced the issue using the ASAN build from comment 0.
Verified fixed ASAN builds 50b11, 51.0a2 (2016-10-27), ESR tinderbox 45.4.1 on Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Group: core-security-release
Updated•6 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•