If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Heap-buffer-overflow in nsBidi::ResolveImplicitLevels

RESOLVED FIXED in Firefox 40, Firefox OS v2.2

Status

()

Core
Layout
--
critical
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: Atte Kettunen, Assigned: tedders1)

Tracking

(4 keywords)

40 Branch
mozilla41
csectype-bounds, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, 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)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8604086 [details]
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)
Keywords: csectype-bounds, regression, sec-critical, testcase
Can we switch to using a nsTArray here please?
[Tracking Requested - why for this release]: sec-critical crash regression in v40
status-firefox40: --- → affected
tracking-firefox40: --- → ?
tracking-firefox41: --- → ?
Version: Trunk → 40 Branch
(Assignee)

Updated

2 years ago
Assignee: nobody → tclancy
Flags: needinfo?(tclancy)
Flags: sec-bounty?
(Assignee)

Comment 4

2 years ago
Created attachment 8604367 [details] [diff] [review]
bug-1163583-overflow-ResolveImplicitLevels.patch

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

Comment 5

2 years ago
Created attachment 8604379 [details] [diff] [review]
bug-1163583-overflow-ResolveImplicitLevels.patch
Attachment #8604367 - Attachment is obsolete: true
Attachment #8604367 - Flags: review?(smontagu)
Attachment #8604379 - Flags: review?(smontagu)
Attachment #8604379 - Flags: review?(smontagu) → review+
(Assignee)

Comment 6

2 years ago
Green treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaffb28ce2e7
(Assignee)

Updated

2 years ago
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
(Assignee)

Comment 9

2 years ago
Thanks for the info, Daniel.
Flags: needinfo?(tclancy)
(Assignee)

Comment 10

2 years ago
Daniel, does this mean I have to say "no bug" in the commit message? Does the mercurial hook require that?
Flags: needinfo?(dholbert)
(Assignee)

Comment 11

2 years ago
Oh, nevermind. The mercurial hook is only on the main repos, not on the try server.
Flags: needinfo?(dholbert)
(Assignee)

Comment 12

2 years ago
Created attachment 8605350 [details] [diff] [review]
bug-1163583-setpara-update.patch
Attachment #8604379 - Attachment is obsolete: true
status-firefox39: --- → unaffected
tracking-firefox40: ? → +
tracking-firefox41: ? → +
(Assignee)

Comment 13

2 years ago
Created attachment 8605353 [details] [diff] [review]
bug-1163583-setpara-update.patch
Attachment #8605350 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
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?
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → affected
status-firefox38: --- → unaffected
status-firefox38.0.5: --- → unaffected
status-firefox-esr31: --- → unaffected
status-firefox-esr38: --- → unaffected
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Group: layout-core-security
https://hg.mozilla.org/integration/mozilla-inbound/rev/307db24f332a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/307db24f332a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Please request Aurora approval on this patch when you get a chance.
status-b2g-master: affected → fixed
Flags: needinfo?(tclancy)
Flags: sec-bounty? → sec-bounty+
(Assignee)

Comment 19

2 years ago
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e29a733aee8c
status-firefox40: affected → fixed
(I filed bug 1167149 suggesting we use nsTArray members instead.)
Whiteboard: [systemsfe]
(Assignee)

Comment 22

2 years ago
This is needed for Bug 1166203, which is a confirmed blocker for 2.2.
blocking-b2g: --- → 2.2?
(Assignee)

Updated

2 years ago
Blocks: 1166203
status-b2g-v2.2: unaffected → affected
(Assignee)

Comment 23

2 years ago
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?
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3a54ec1f5ea8
status-b2g-v2.2: affected → fixed
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
status-firefox40: fixed → verified
status-firefox41: fixed → verified
Flags: needinfo?(kjozwiak)

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release

Comment 27

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/50cded09ac34

Comment 28

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/50cded09ac34

Updated

2 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.