Closed Bug 1279814 (CVE-2016-2838) Opened 4 years ago Closed 4 years ago

Heap-buffer-overflow in nsBidi::BracketData::AddOpening

Categories

(Core :: Layout: Text and Fonts, defect)

45 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 + verified
firefox49 + verified
firefox-esr38 --- unaffected
firefox-esr45 48+ verified
thunderbird_esr38 --- unaffected
thunderbird_esr45 --- affected
firefox50 + verified

People

(Reporter: attekett, Assigned: jfkthame)

References

Details

(Keywords: csectype-bounds, regression, sec-high, Whiteboard: [adv-main48+][adv-esr45.3+])

Attachments

(5 files, 1 obsolete file)

Tested on:

OS: Ubuntu 14.04

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

Repro-file as an attachment. Sorry for the large size of the file. My minimizer failed to to minimize it, because of some weird encoding issue. 

ASAN-trace:

=================================================================
==1043==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200070c92f at pc 0x7f56b71bc133 bp 0x7ffc090fca90 sp 0x7ffc090fca88
WRITE of size 128 at 0x60200070c92f thread T0 (Web Content)
    #0 0x7f56b71bc132 in nsBidi::BracketData::AddOpening(char16_t, int) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:669
    #1 0x7f56b71bcd99 in nsBidi::BracketData::ProcessChar(int, char16_t, unsigned char*, unsigned char*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:843
    #2 0x7f56b71b980e in nsBidi::ResolveExplicitLevels(nsBidiDirection*, char16_t const*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:1175
    #3 0x7f56b71b7c9b in nsBidi::SetPara(char16_t const*, int, unsigned char) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:310
    #4 0x7f56b71c2f46 in SetPara /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:210
    #5 0x7f56b71c2f46 in nsBidiPresUtils::ResolveParagraph(nsBlockFrame*, BidiParagraphData*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:702
    #6 0x7f56b71c0802 in ResolveParagraphWithinBlock /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:1247
    #7 0x7f56b71c0802 in nsBidiPresUtils::TraverseFrames(nsBlockFrame*, nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:1146
    #8 0x7f56b71bf16a in nsBidiPresUtils::Resolve(nsBlockFrame*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:673
    #9 0x7f56b745a2e7 in ResolveBidi /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:7516
    #10 0x7f56b745a2e7 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:1110
    #11 0x7f56b74788e2 in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockReflowContext.cpp:306
    #12 0x7f56b746eb97 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3392
    #13 0x7f56b746196a in ReflowLine /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:2751
    #14 0x7f56b746196a in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:2290
    #15 0x7f56b745a6b7 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:1171
    #16 0x7f56b74b8e1b in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsContainerFrame.cpp:1022
    #17 0x7f56b74b763e in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsCanvasFrame.cpp:644
    #18 0x7f56b75546a7 in ReflowChild /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsContainerFrame.cpp:1022
    #19 0x7f56b75546a7 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsGfxScrollFrame.cpp:541
.
.
.
Group: core-security → layout-core-security
This is a bug in the bidi bracket-pair support from bug 1157727; it's missing a decrement in nsBidi::BracketData::ProcessPDI(), so we end up running off the end of mIsoRuns[]. (Compare the ubidi.c version of this function at https://dxr.mozilla.org/mozilla-central/source/intl/icu/source/common/ubidi.c#733-739.)
FWIW, the testcase here hits the assertion at https://dxr.mozilla.org/mozilla-central/source/layout/base/nsBidi.cpp#641 in a debug build. In a non-debug build, though, it'll just proceed beyond the end of the array.
Attachment #8762895 - Flags: review?(bugzilla)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attached patch CrashtestSplinter Review
This is simply the original testcase here as a layout crashtest; we should land this once the bug is fixed on all branches.
Attachment #8762898 - Flags: review?(bugzilla)
The bug here originated in mozilla-45, so AFAICT it will affect all current branches except esr38.
Update to add an underflow-check assertion.
Attachment #8762904 - Flags: review?(bugzilla)
Attachment #8762895 - Attachment is obsolete: true
Attachment #8762895 - Flags: review?(bugzilla)
Comment on attachment 8762904 [details] [diff] [review]
Update mIsoRunLast index when handling PDI

Review of attachment 8762904 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense. Looks like the caller would ensure that the mIsoRunLast would be non-zero.
Attachment #8762904 - Flags: review?(bugzilla) → review+
Flags: sec-bounty?
Comment on attachment 8762904 [details] [diff] [review]
Update mIsoRunLast index when handling PDI

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch suggests a flaw in directional-isolate processing; it's not clear to me how easily this translates to an exploit. I tried a simple HTML testcase with deeply-nested isolates that didn't seem to hit this at all, but the original SVG example shows that it's possible for it to lead to a buffer overrun (writing), leading to heap memory corruption.

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?
Mozilla-45 and later, therefore Aurora/Beta/Release/Esr45 are all affected.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply unchanged across all affected branches, AFAICS.

How likely is this patch to cause regressions; how much testing does it need?
Minimal risk of regressions; passes all existing bidi reftests.
Attachment #8762904 - Flags: sec-approval?
Attached file simplified testcase
This is a simplified HTML crashtest for this bug.

This would only happen if there are enough valid isolate levels accumulated in total. Naively nesting isolate levels wouldn't trigger this bug because when the level stack has been filled (125 explicitly levels at most), additional isolate levels would just be counted as overflow without further handling. That's probably why jfkthame's testcase doesn't work.
Comment on attachment 8762898 [details] [diff] [review]
Crashtest

Review of attachment 8762898 [details] [diff] [review]:
-----------------------------------------------------------------

It is probably better to use a clearer crashtest for this bug. I don't think we can land the crashtest very soon anyway, given it is a sec-high, so I wouldn't consider making the test clearer would affect the security.
Attachment #8762898 - Flags: review?(bugzilla)
(In reply to Jonathan Kew (:jfkthame) from comment #8)
> Comment on attachment 8762904 [details] [diff] [review]
> Update mIsoRunLast index when handling PDI
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> The patch suggests a flaw in directional-isolate processing; it's not clear
> to me how easily this translates to an exploit. I tried a simple HTML
> testcase with deeply-nested isolates that didn't seem to hit this at all,
> but the original SVG example shows that it's possible for it to lead to a
> buffer overrun (writing), leading to heap memory corruption.

It might be easy for people who is familiar with Unicode Bidirectional Algorithm to translate it to an exploit, but supposedly most crackers wouldn't be that familiar with that algorithm since that's complicated  (I just read that 2 or 3 weeks ago, so the memory is still fresh). Given that it wasn't easy even for jfkthame to figure out the condition to trigger this bug, I guess it isn't really a big deal :)
It seems to me this is not a heap-buffer-overflow, but a stack-buffer-overflow. (BracketData is always allocated on stack). And an attacker would be able to override any length of memory in the stack with some limited pattern, which, I suppose, could affect the return path of the code. If attacker can put some native code, and make the code jump to that place, this would lead to a native code execution, which sounds like a sec-critical rather than sec-high.
Comment on attachment 8762904 [details] [diff] [review]
Update mIsoRunLast index when handling PDI

sec-approval+ for trunk. We'll want patches for affected branches too.
Attachment #8762904 - Flags: sec-approval? → sec-approval+
(In reply to Xidorn Quan [:xidorn] (UTC+1) from comment #12)
> It seems to me this is not a heap-buffer-overflow, but a
> stack-buffer-overflow.

The overflow of mIsoRuns[] is a stack-buffer-overflow, but what ASAN originally detected here was a heap-buffer-overflow of mOpeningsMemory. That is initially stack-allocated, but we switch to a heap-allocated block if we exceed the auto-allocated size. So I guess the bug here can lead to both stack- and heap-buffer overflows (if we don't crash quickly enough on the first overflow that happens).

Either way, it's bad, and could probably be used to arrive at native code execution by some path or other.... I'm not sure how to get there, given that I don't think the attacker has arbitrary control of what's written, but I feel pretty sure it could be done by those who focus on stuff like this.
Comment on attachment 8762904 [details] [diff] [review]
Update mIsoRunLast index when handling PDI

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec:high-rated memory-stomping bug

User impact if declined: potentially exploitable buffer overruns

Fix Landed on Version: 50

Risk to taking this patch (and alternatives if risky): minimal

String or UUID changes made by this patch: none


Approval Request Comment
[Feature/regressing bug #]: 1157727

[User impact if declined]: potentially exploitable buffer overruns

[Describe test coverage new/current, TreeHerder]: existing reftests for bidi functionality; we can land a new crashtest based on this bug after it is fixed

[Risks and why]: minimal

[String/UUID change made/needed]: none
Attachment #8762904 - Flags: approval-mozilla-esr45?
Attachment #8762904 - Flags: approval-mozilla-beta?
Attachment #8762904 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/5fcd5742e518
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8762904 [details] [diff] [review]
Update mIsoRunLast index when handling PDI

Sec-high, taking it
Attachment #8762904 - Flags: approval-mozilla-esr45?
Attachment #8762904 - Flags: approval-mozilla-esr45+
Attachment #8762904 - Flags: approval-mozilla-beta?
Attachment #8762904 - Flags: approval-mozilla-beta+
Attachment #8762904 - Flags: approval-mozilla-aurora?
Attachment #8762904 - Flags: approval-mozilla-aurora+
Group: layout-core-security → core-security-release
Blocks: 1157727
Flags: sec-bounty? → sec-bounty+
Keywords: regression
Version: unspecified → 45 Branch
Flags: qe-verify+
Alias: CVE-2016-2838
Whiteboard: [adv-main48+][adv-esr45.3+]
Confirming the fix on Ubuntu 14.04 x86 and x64 using the test cases from comment 4 and comment 8 for the following builds:
*Latest 50.0a1 Nightly, build ID 20160726030213
*Latest 49.0a2 Aurora, build ID 20160726004006
*Fx 48 RC, build ID 20160725093659
*ESR 45.3.0, build ID 20160725105554
Group: core-security-release
We may want to land the testcase in comment 9. I'll have a look later.
Flags: needinfo?(xidorn+moz)
Flags: in-testsuite?
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> FWIW, the testcase here hits the assertion at
> https://dxr.mozilla.org/mozilla-central/source/layout/base/nsBidi.cpp#641 in
> a debug build. In a non-debug build, though, it'll just proceed beyond the
> end of the array.

FWIW, the permalink of this assertion is https://dxr.mozilla.org/mozilla-central/rev/28b6f5924ce2/layout/base/nsBidi.cpp#641
Flags: needinfo?(xidorn+moz)
Attachment #8794058 - Flags: review?(jfkthame)
Attachment #8794058 - Flags: review?(jfkthame) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2750ecaa0b5
followup - Add crashtest for this bug. r=jfkthame
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.