Closed Bug 866544 Opened 7 years ago Closed 7 years ago

[Mac] Buffer overflow of nsAutoTArray "breakState"

Categories

(Core :: Internationalization, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 --- affected
firefox21 + verified
firefox22 + verified
firefox23 + verified
firefox-esr17 21+ verified
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [adv-main21+][adv-esr1706+])

Crash Data

Attachments

(4 files)

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;
Attached file stack trace (ASan)
Nightly crashes [@ __stack_chk_fail], but Breakpad doesn't catch it (bug 717758).
Attachment #742850 - Attachment description: testcase (crashes Firefox when loaded) → testcase (EVEN "VIEW SOURCE" CRASHES)
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.
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
Attachment #742975 - Flags: review?(smontagu)
Assignee: nobody → jfkthame
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.
This is code that's been around "forever", so the bug affects all current branches AFAICT.
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 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?
Attachment #742975 - Flags: review?(smontagu) → review+
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 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
Attachment #742975 - Flags: sec-approval?
> 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.
Can you suggest a security rating for this? It is unlikely to be taken on branches without one.
Attachment #742975 - Flags: sec-approval? → sec-approval+
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.)
Keywords: sec-critical
Keywords: csec-bounds
(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 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
Attachment #742975 - Flags: approval-mozilla-esr17?
Attachment #742975 - Flags: approval-mozilla-beta?
Attachment #742975 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/dbb499490f45
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Summary: [Mac] Stack buffer overflow of breakState → [Mac] Buffer overflow of nsAutoTArray "breakState"
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 !
Attachment #742975 - Flags: approval-mozilla-esr17?
Attachment #742975 - Flags: approval-mozilla-esr17+
Attachment #742975 - Flags: approval-mozilla-beta?
Attachment #742975 - Flags: approval-mozilla-beta+
Attachment #742975 - Flags: approval-mozilla-aurora?
Attachment #742975 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main21+][adv-esr1706+]
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
Group: core-security
You need to log in before you can comment on or make changes to this bug.