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)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 + fixed
firefox-esr45 49+ verified
b2g-v2.6 --- affected
thunderbird_esr38 --- affected
thunderbird_esr45 --- affected
firefox50 + verified
firefox51 + verified

People

(Reporter: attekett, Assigned: jfkthame)

References

Details

(4 keywords, Whiteboard: [adv-main49+][adv-esr45.4+])

Attachments

(4 files)

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
.
.
.
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
Group: core-security → gfx-core-security
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)
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: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8779386 - Flags: review?(cam) → review+
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?
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.)
sec-approval+ for the primary patch. We'll want a nominated patch for Aurora, Beta, and ESR45 once this lands on trunk as well.
Attachment #8779386 - Flags: sec-approval? → sec-approval+
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?
Flags: sec-bounty?
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+
https://hg.mozilla.org/releases/mozilla-esr45/rev/7cd50d56bb61174296b20a9985ce71282a961ef3
Bug 1291016 - Track source-string position of possible prefix. r=heycam a=lizzard
https://hg.mozilla.org/mozilla-central/rev/32695cf85668
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: sec-bounty? → sec-bounty+
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?
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.
(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.
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(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.
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?
(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.
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+
Cam, don't forget to address the review comment and land the follow-up patch.
Flags: needinfo?(cam)
Whiteboard: [adv-main49+][adv-esr45.4+]
Flags: qe-verify+
Alias: CVE-2016-5270
I fixed the follow-up patch per comment 19 and pushed it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8f86b1baa2d095862cab288ede231db4c37544c
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Flags: needinfo?(cam) → in-testsuite?
Keywords: crash, testcase
Resolution: --- → FIXED
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.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: