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?
Can we switch to using a nsTArray here please?
[Tracking Requested - why for this release]: sec-critical crash regression in v40
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.
Created attachment 8604379 [details] [diff] [review] bug-1163583-overflow-ResolveImplicitLevels.patch
Green treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaffb28ce2e7
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).
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 , 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).  https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment
Thanks for the info, Daniel.
Daniel, does this mean I have to say "no bug" in the commit message? Does the mercurial hook require that?
Oh, nevermind. The mercurial hook is only on the main repos, not on the try server.
Created attachment 8605350 [details] [diff] [review] bug-1163583-setpara-update.patch
Created attachment 8605353 [details] [diff] [review] bug-1163583-setpara-update.patch
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.
Comment on attachment 8605353 [details] [diff] [review] bug-1163583-setpara-update.patch sec-approval+. Let's make sure to get it into Aurora too.
Please request Aurora approval on this patch when you get a chance.
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.
(I filed bug 1167149 suggesting we use nsTArray members instead.)
This is needed for Bug 1166203, which is a confirmed blocker for 2.2.
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.
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
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