Closed Bug 1163583 Opened 9 years ago Closed 9 years ago

Heap-buffer-overflow in nsBidi::ResolveImplicitLevels

Categories

(Core :: Layout, defect)

40 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
blocking-b2g 2.2+
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- unaffected
firefox39 --- unaffected
firefox40 + verified
firefox41 + verified
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: attekett, Assigned: tedders1)

References

Details

(4 keywords, Whiteboard: [systemsfe])

Attachments

(2 files, 3 obsolete files)

Attached file repro-file.html
Tested on:

OS: Ubuntu 14.04

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

ASAN-trace:

=================================================================
==13442==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x604000081184 at pc 0x7f242df1b3f7 bp 0x7ffeceda9350 sp 0x7ffeceda9348
WRITE of size 2 at 0x604000081184 thread T0 (Web Content)
    #0 0x7f242df1b3f6 in nsBidi::ResolveImplicitLevels(int, int, unsigned char, unsigned char) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:1271:0
    #1 0x7f242df18cee in nsBidi::SetPara(char16_t const*, int, unsigned char, unsigned char*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:419:0
    #2 0x7f242df21035 in SetPara /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:210:0
    #3 0x7f242df21035 in nsBidiPresUtils::ResolveParagraph(nsBlockFrame*, BidiParagraphData*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:702:0
    #4 0x7f242df1d63a in nsBidiPresUtils::Resolve(nsBlockFrame*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:686:0
    #5 0x7f242e187874 in ResolveBidi /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:7343:0
    #6 0x7f242e187874 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsBlockFrame.cpp:1099:0
.
.
.
0x604000081184 is located 4 bytes to the right of 48-byte region [0x604000081150,0x604000081180)
allocated by thread T0 (Web Content) here:
    #0 0x474c01 in __interceptor_malloc _asan_rtl_:0
    #1 0x7f242df188cb in GetMemory /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:203:0
    #2 0x7f242df188cb in nsBidi::SetPara(char16_t const*, int, unsigned char, unsigned char*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidi.cpp:330:0
    #3 0x7f242df21035 in SetPara /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:210:0
    #4 0x7f242df21035 in nsBidiPresUtils::ResolveParagraph(nsBlockFrame*, BidiParagraphData*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:702:0
    #5 0x7f242df2003f in ResolveParagraphWithinBlock /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:1249:0
    #6 0x7f242df2003f in nsBidiPresUtils::TraverseFrames(nsBlockFrame*, nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsBidiPresUtils.cpp:1173:0
.
.
.
We're writing past the end of the allocated memory for 'mIsolates' here:
http://hg.mozilla.org/mozilla-central/annotate/7d4feaf04bca/layout/base/nsBidi.cpp#l1271
It was previously allocated here:
http://hg.mozilla.org/mozilla-central/annotate/7d4feaf04bca/layout/base/nsBidi.cpp#l330

It looks like a regression from bug 1157726?
Blocks: 1157726
Severity: normal → critical
Component: General → Layout
Flags: needinfo?(tclancy)
Can we switch to using a nsTArray here please?
[Tracking Requested - why for this release]: sec-critical crash regression in v40
Version: Trunk → 40 Branch
Assignee: nobody → tclancy
Flags: needinfo?(tclancy)
Flags: sec-bounty?
I found the likely source of this problem.

This is a bug that existed in the original ICU code that I copied. (And my apologies for not catching it then.)

I had a look at the ICU repo (http://bugs.icu-project.org/trac/browser), and I see that this bug was fixed 16 months ago in revision 34934. But the version of ICU we have in our gecko repo, and upon which I based Bug 1157726, is older than that.
Attachment #8604367 - Flags: review?(smontagu)
Attachment #8604367 - Attachment is obsolete: true
Attachment #8604367 - Flags: review?(smontagu)
Attachment #8604379 - Flags: review?(smontagu)
Attachment #8604379 - Flags: review?(smontagu) → review+
Keywords: checkin-needed
For security bugs, you need to request (& receive) sec-approval before this can land. Process described here:  https://wiki.mozilla.org/Security/Bug_Approval_Process

Also, future tips about pushing patches for security sensitive bugs to TryServer:
 1) Don't push to Try it until you're really ready to land [so, after you've got sec-approval], to minimize time that the patch is exposed. (potentially giving away the fact that there's a flaw in that code)
 2) Remove any comments or commit message that would give away the fact that the patch is sec-sensitive. This includes bug numbers (for hidden bugs) & mentions of things like "buffer overrun".
 3) Don't include testcases that trigger / give away the bug (& you didn't in comment 6, which is great).
Flags: needinfo?(tclancy)
Keywords: checkin-needed
Comment on attachment 8604379 [details] [diff] [review]
bug-1163583-overflow-ResolveImplicitLevels.patch

>Bug 1163583 - Fix buffer overrun in ResolveImplicitLevels. r=smontagu

And while I'm here: ideally this commit message should be improved to describe what's actually changing (and to make this commit a less obvious/tasty target for attackers to monkey with).

Per [1], the commit message should describe the change (so, what about the computation is being fixed, in this case), rather than describing the problem (buffer overrun).

[1] https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Thanks for the info, Daniel.
Flags: needinfo?(tclancy)
Daniel, does this mean I have to say "no bug" in the commit message? Does the mercurial hook require that?
Flags: needinfo?(dholbert)
Oh, nevermind. The mercurial hook is only on the main repos, not on the try server.
Flags: needinfo?(dholbert)
Attached patch bug-1163583-setpara-update.patch (obsolete) — Splinter Review
Attachment #8604379 - Attachment is obsolete: true
Attachment #8605350 - Attachment is obsolete: true
Comment on attachment 8605353 [details] [diff] [review]
bug-1163583-setpara-update.patch

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

A clever person could probably figure out how to possibly crash the browser, but the affected buffer isn't on the stack, so they couldn't use a "stack buffer overrun" exploit to change the return address and execute arbitrary code.

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?

None. The flaw is only on Aurora right now.

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

Bug 1157726

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

N/A. The problem is only on Aurora.

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

It shouldn't cause any regressions.
Attachment #8605353 - Flags: sec-approval?
Comment on attachment 8605353 [details] [diff] [review]
bug-1163583-setpara-update.patch

sec-approval+. Let's make sure to get it into Aurora too.
Attachment #8605353 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
Group: layout-core-security
https://hg.mozilla.org/mozilla-central/rev/307db24f332a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Please request Aurora approval on this patch when you get a chance.
Flags: needinfo?(tclancy)
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8605353 [details] [diff] [review]
bug-1163583-setpara-update.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1157726

[User impact if declined]:
Browser can crash when showing some pages.

[Describe test coverage new/current, TreeHerder]:
Green treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaffb28ce2e7

[Risks and why]: 
There shouldn't be any problems from taking this patch.

[String/UUID change made/needed]:
None.
Flags: needinfo?(tclancy)
Attachment #8605353 - Flags: approval-mozilla-aurora?
Attachment #8605353 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(I filed bug 1167149 suggesting we use nsTArray members instead.)
Whiteboard: [systemsfe]
This is needed for Bug 1166203, which is a confirmed blocker for 2.2.
blocking-b2g: --- → 2.2?
Blocks: 1166203
Comment on attachment 8605353 [details] [diff] [review]
bug-1163583-setpara-update.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
RTL support for B2G (Bug 906270)

This patch is required for Bug 1166203, which is confirmed as blocking 2.2. (And probably a bunch of similar issues which haven't been spotted yet.)

User impact if declined: 
Punctuation marks will appear in the wrong place when LTR phrases appear within RTL text, or vice versa.

Testing completed: 
Green treeherder run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaffb28ce2e7

Risk to taking this patch (and alternatives if risky): 
None foreseen.

String or UUID changes made by this patch:
None.
Attachment #8605353 - Flags: approval-mozilla-b2g37?
Comment on attachment 8605353 [details] [diff] [review]
bug-1163583-setpara-update.patch

B2G sec-crits have auto-approval. It's on the radar to land once bug 1157726 is approved.
https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_5
Attachment #8605353 - Flags: approval-mozilla-b2g37?
blocking-b2g: 2.2? → 2.2+
Group: layout-core-security
Flags: needinfo?(kjozwiak)
QA Contact: kjozwiak
Reproduced the issue using the following m-c build: (same one original poster used in comment #0)
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1431340891/

Using the test case provided in comment #0, I reproduced the crash instantly with both non-e10s and e10s.

Went through verification using the following builds:

- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1437991336/
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1437982809/
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1437749378/

Test Cases Used:

- opened the poc several times via new windows using both non-e10s and e10s
- opened the poc several times via new tabs using both non-e10s and e10s
- opened the poc several times via private browsing using both non-10s and e10s
Flags: needinfo?(kjozwiak)
Group: core-security → core-security-release
Group: core-security-release
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.