Closed
Bug 1163583
Opened 9 years ago
Closed 9 years ago
Heap-buffer-overflow in nsBidi::ResolveImplicitLevels
Categories
(Core :: Layout, defect)
Tracking
()
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)
646 bytes,
text/html
|
Details | |
975 bytes,
patch
|
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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 . . .
Comment 1•9 years ago
|
||
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?
Comment 2•9 years ago
|
||
Can we switch to using a nsTArray here please?
Comment 3•9 years ago
|
||
[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•9 years ago
|
Assignee: nobody → tclancy
Flags: needinfo?(tclancy)
Updated•9 years ago
|
Flags: sec-bounty?
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
Attachment #8604367 -
Attachment is obsolete: true
Attachment #8604367 -
Flags: review?(smontagu)
Attachment #8604379 -
Flags: review?(smontagu)
Updated•9 years ago
|
Attachment #8604379 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Green treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaffb28ce2e7
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 7•9 years ago
|
||
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 8•9 years ago
|
||
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 10•9 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•9 years ago
|
||
Oh, nevermind. The mercurial hook is only on the main repos, not on the try server.
Flags: needinfo?(dholbert)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8604379 -
Attachment is obsolete: true
Updated•9 years ago
|
status-firefox39:
--- → unaffected
Updated•9 years ago
|
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8605350 -
Attachment is obsolete: true
Assignee | ||
Comment 14•9 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?
Updated•9 years ago
|
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 15•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Group: layout-core-security
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/307db24f332a
Keywords: checkin-needed
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/307db24f332a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 18•9 years ago
|
||
Please request Aurora approval on this patch when you get a chance.
Flags: needinfo?(tclancy)
Updated•9 years ago
|
Flags: sec-bounty? → sec-bounty+
Assignee | ||
Comment 19•9 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?
Updated•9 years ago
|
Attachment #8605353 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 21•9 years ago
|
||
(I filed bug 1167149 suggesting we use nsTArray members instead.)
Updated•9 years ago
|
Whiteboard: [systemsfe]
Assignee | ||
Comment 22•9 years ago
|
||
This is needed for Bug 1166203, which is a confirmed blocker for 2.2.
blocking-b2g: --- → 2.2?
Updated•9 years ago
|
Assignee | ||
Comment 23•9 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 24•9 years ago
|
||
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?
Updated•9 years ago
|
blocking-b2g: 2.2? → 2.2+
Updated•9 years ago
|
Group: layout-core-security
Updated•9 years ago
|
Flags: needinfo?(kjozwiak)
QA Contact: kjozwiak
Comment 26•9 years ago
|
||
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
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50cded09ac34
Updated•8 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•