1279814 - (CVE-2016-2838) Heap-buffer-overflow in nsBidi::BracketData::AddOpening

Status: VERIFIED FIXED

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

Description

[Atte Kettunen]

Attachment: firefox-heap-buffer-overflow-nsBidiBracketDataAddOpening.svg

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
.
.
.

Comment 1

[Jonathan Kew [:jfkthame]]

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.)

Comment 2

[Jonathan Kew [:jfkthame]]

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.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: Update mIsoRunLast index when handling PDI

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Crashtest

This is simply the original testcase here as a layout crashtest; we should land this once the bug is fixed on all branches.

Comment 5

[Jonathan Kew [:jfkthame]]

The bug here originated in mozilla-45, so AFAICT it will affect all current branches except esr38.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Update mIsoRunLast index when handling PDI

Update to add an underflow-check assertion.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 8

[Jonathan Kew [:jfkthame]]

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.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: 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 10

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

(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 :)

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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 13

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 14

[Jonathan Kew [:jfkthame]]

(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 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fcd5742e51897a469773c538098d9edb4dd4c57
Bug 1279814 - Update mIsoRunLast index when handling PDI. r=xidorn

Comment 16

[Jonathan Kew [:jfkthame]]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fcd5742e518

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8762904 [details] [diff] [review]
Update mIsoRunLast index when handling PDI

Sec-high, taking it

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cc0a07b00e38

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/182495fba019

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/5ffdebd7418e

Comment 22

[Cornel Ionce [:noni] [Hubs QA]]

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

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

We may want to land the testcase in comment 9. I'll have a look later.

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

(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

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch for crashtest

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2750ecaa0b5
followup - Add crashtest for this bug. r=jfkthame

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b2750ecaa0b5