866544 - [Mac] Buffer overflow of nsAutoTArray "breakState"

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Jesse Ruderman]

Attachment: testcase (EVEN "VIEW SOURCE" CRASHES)

WRITE of size 1 at 0x7fff5fbceb70 thread T0
    #0 0x1088d76c2 in NS_GetComplexLineBreaks [nsCarbonBreaker.cpp:36]
    #1 0x1088bf7a0 in nsJISx4051LineBreaker::GetJISx4051Breaks [nsJISx4051LineBreaker.cpp:856]
    #2 0x10baeacbe in nsLineBreaker::FlushCurrentWord [nsLineBreaker.cpp:77]
    ...

nsLineBreaker::FlushCurrentWord:
   nsAutoTArray<uint8_t,4000> breakState;
   ...
   GetJISx4051Breaks(..., breakState.Elements());

nsCarbonBreaker.cpp:
   UCFindTextBreak(..., &offset);
   ...
   aBreakBefore[offset] = true;

Comment 1

[Jesse Ruderman]

Attachment: stack trace (ASan)

Comment 2

[Jesse Ruderman]

Nightly crashes [@ __stack_chk_fail], but Breakpad doesn't catch it (bug 717758).

Comment 3

[Jonathan Kew [:jfkthame]]

Interestingly, this does -not- crash (or even assert or anything...) in my Debug build, but it does indeed crash right away with a release build. Given the analysis below, it's probably the presence of frame pointers in the debug build that make it more robust, or something like that.

The root of the problem is that the call to UCFindTextBreak at http://mxr.mozilla.org/mozilla-central/source/intl/lwbrk/src/nsCarbonBreaker.cpp#25 may return an offset that is equal to aLength (i.e. implying a break -after- the last character of the text that was passed).

Our code isn't expecting that, as the aBreakBefore array is the same length as the text. (We only want the NS_GetComplexLineBreaks function to mark valid break positions -within- the text it's given.) So this writes to an element immediately beyond the end of the array.

This error is happening constantly in real-life Thai pages, for example, as my brief testing indicated that UCFindTextBreak routinely finds a break at the end of the text. In most cases we get away with it, as the passed-in array was allocated as an nsAutoTArray<uint8_t,4000> at http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsLineBreaker.cpp#60. So the incorrect write harmlessly touches the spare "dead space" in the autoarray's stack buffer.

But if the "word" we're processing was long enough that the stack buffer didn't suffice, we could corrupt the heap - or if it -exactly- fit, with no spare space in the preallocated stack buffer, we'll corrupt the stack itself. Which is exactly what happens here: we end up calling NS_GetComplexLineBreaks for a single-char Khmer run that is located at offset 3999 in a 4000-char "word". UCFindTextBreak says yes, we can break -after- it, which causes us to corrupt the byte immediately following the autoarray's built-in buffer on the stack. And....boom.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: ignore break position at end of text returned by UCFindTextBreak

This should prevent the crash. Pushed to tryserver to verify that it doesn't disturb unit tests - I believe we'd have ended up ignoring that "extra" flag that was being set at the end of the complex text segment, or overwriting with the start of the following segment anyway. https://tbpl.mozilla.org/?tree=Try&rev=b56fe5aab017

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: simplified testcase (crashes Nightly/OSX; view-source is safe)

Having understood the bug, it's trivial to construct a testcase that hits the crash - just dump a precisely 4000-Thai-character string into the document.

Comment 6

[Jonathan Kew [:jfkthame]]

This is code that's been around "forever", so the bug affects all current branches AFAICT.

Comment 7

[Jonathan Kew [:jfkthame]]

Confirmed that with the Opt build from https://tbpl.mozilla.org/?tree=Try&rev=b56fe5aab017, the testcases here (both Jesse's original fuzzer-mess and my simplified one) no longer crash.

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 742975 [details] [diff] [review]
ignore break position at end of text returned by UCFindTextBreak

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

I don't know this code well, but the commentary in comment 3 is very convincing.

What about other implementations of NS_GetComplexLineBreaks?

Comment 9

[Jonathan Kew [:jfkthame]]

The testcases here don't crash for me on either Windows or Linux; and AFAICS from inspecting the code, they don't have the same issue. The implementations of NS_GetComplexLineBreaks end up structured quite differently, as we're building on platform APIs that work in somewhat different ways.

(The nsPangoBreaker.cpp version on Linux is particularly confusing, thanks to the need to convert the text to utf-8. I'm suspicious that the handling of surrogate pairs there is actually wrong, but I need to stare at it some more to make sure I'm understanding it properly, and perhaps construct testcases to confirm/deny. But that'll be for a separate bug, if there is in fact an error there...)

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 742975 [details] [diff] [review]
ignore break position at end of text returned by UCFindTextBreak

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

An exploit that will crash the browser is easy to construct - the patch adds a length check, and it's not hard to work back from there to see how to create a page that will hit the case that's being checked.

It's unclear to me whether an attacker could actually leverage this to do something more sinister than crash the browser.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No more so than the code change itself. IMO, the proposed checkin comment doesn't hint at -why- we're adding this check.

We could land the simplified testcase (attachment 742981 [details]) as a crashtest, but as this demonstrates how to trivially DoS the browser on OS X (even quicker and simpler than the usual memory-exhaustion scripts), we shouldn't do that until the patch has been rolled out everywhere.

Which older supported branches are affected by this flaw?

All

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

n/a

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Same patch applies across all branches

How likely is this patch to cause regressions; how much testing does it need?

Minimal risk

Comment 11

[Jesse Ruderman]

> It's unclear to me whether an attacker could actually leverage this to do something 
> more sinister than crash the browser.

Sounds like a 1-byte overflow of (uint8_t)(true) in the attacker's choice of stack or heap.  I guess the stack buffer overflow will always overwrite the guard (NOT the return pointer), at least in the release-mode builds we tested.  But the heap buffer overflow could hit just about anything.

Comment 12

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

Can you suggest a security rating for this? It is unlikely to be taken on branches without one.

Comment 13

[Jonathan Kew [:jfkthame]]

Given the possibility of overwriting something in the heap, even though only a single byte can be reached, I'd guess we should call it sec-critical; if the attacker could somehow contrive to have a sufficiently important structure/object at just that place, who knows what they might be able to do...

(Jesse, WDYT? I'm sure you have a better sense of this stuff than I do.)

Comment 14

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb499490f45

Comment 15

[bhavana bajaj [:bajaj]]

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/dbb499490f45

Can you please request approval for branch specific patches here?
This will have to land on mozilla-beta before tomorrow noon to make it into our second last beta.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 742975 [details] [diff] [review]
ignore break position at end of text returned by UCFindTextBreak

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding bug, found by Jesse's fuzzing

User impact if declined: crash or potentially exploitable heap corruption

Testing completed (on m-c, etc.): landed safely on inbound, confirmed to fix the crash with testcases here

Risk to taking this patch (and alternatives if risky): minimal, just adds a bounds check before using a value returned by system API

String or IDL/UUID changes made by this patch: none

Comment 17

[Ed Morley [:emorley]]

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

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 742975 [details] [diff] [review]
ignore break position at end of text returned by UCFindTextBreak

Request to land on branches( with beta as priority) asap so this can get into our next beta which is going to build in a few hours. Thanks !

Comment 19

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-beta/rev/100e717b8bbd
https://hg.mozilla.org/releases/mozilla-aurora/rev/d3c964efa908
https://hg.mozilla.org/releases/mozilla-esr17/rev/6c0aee4bf55e

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

Reproduced on m-c 2013-04-28
Confirmed fixed on FF17.0.6esr candidate build 1
Confirmed fixed on FF21 candidate build 2
Confirmed fixed on FF22 2013-05-09
Confirmed fixed on FF23 2013-05-09