New issue
Advanced search Search tips

Issue 620952 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

i < m_len

Project Member Reported by ClusterFuzz, Jun 17 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6606065282318336

Fuzzer: inferno_twister_custom_bundle
Job Type: mac_asan_chrome
Platform Id: mac

Crash Type: ASSERT
Crash Address: 
Crash State:
  i < m_len
  blink::LayoutSVGInlineText::addMetricsFromRun
  blink::LayoutSVGInlineText::updateMetricsList
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=mac_asan_chrome&range=384406:384437

Minimized Testcase (0.49 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94sx9miECiMyMWzHjkGmjljS1o4EeUT7pH1xjOgvH6LHKD-n9eeAcNOjG5-Y-TRPSYRKrrKdrcbKZhEbhzn9C9ayvXSPvAxEx9yp-LgqmCiF5B2trKsUyY6BLV8362LiVsX6DZPRmLuczLuazOs8Tus76wNbA

Additional requirements: Requires HTTP

Filer: inferno

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Jun 17 2016

Labels: M-51
Project Member

Comment 2 by sheriffbot@chromium.org, Jun 17 2016

Labels: Pri-1

Comment 3 by est...@chromium.org, Jun 17 2016

Components: Blink>SVG
Owner: f...@opera.com
fs, could you please take a look at this security bug? Thanks.
Project Member

Comment 4 by ClusterFuzz, Jun 17 2016

Status: Assigned (was: Available)

Comment 5 by f...@opera.com, Jun 18 2016

Cc: pdr@chromium.org
This looks a lot like  issue 613915  (stacktrace and TC), but the fix for that should be merged to 2743 and 2704 (since a few days back.)

Comment 6 by pdr@chromium.org, Jun 21 2016

Cc: -pdr@chromium.org
Owner: pdr@chromium.org
I agree that this looks like 613915. I've kicked off a re-do of the clusterfuzz results to see if anything comes up.

Comment 7 by pdr@chromium.org, Jun 28 2016

I've minimized this on OSX 10.9.5. These are the lowest values that still crash, and all 5 values are required
<!doctype html>
<svg xmlns="http://www.w3.org/2000/svg" height="100px" width="100px">
  <text>&#x1fffe;&#x1fffe;&#xfe0e;&#xd7fc;&#x30000;</text>
</svg>

&#x1fffe; - noncharacter - http://www.fileformat.info/info/unicode/char/1fffe/index.htm
&#x1fffe; - noncharacter - http://www.fileformat.info/info/unicode/char/1fffe/index.htm
&#xfe0e; - variation selector 15 - http://www.fileformat.info/info/unicode/char/fe0e/index.htm
&#xd7fc; - invalid - http://www.fileformat.info/info/unicode/char/d7fc/index.htm
&#x30000; - invalid - http://www.fileformat.info/info/unicode/char/30000/index.htm

I have no idea what's going on.

Comment 8 by pdr@chromium.org, Jun 28 2016

Status: Started (was: Assigned)
On OSX 10.9.5 with a debug build at tip-of-tree the above repro asserts in Font::individualCharacterRanges where ranges.size = 10 and run.length = 8. This is sort of the opposite issue from  issue 613915 .

I'm going to try to repro on OSX 10.10.5. If I can't figure this out there, I can switch the DCHECK in Font::individualCharacterRanges to a CHECK which will protect our users and possibly let us get better testcases if this affects platforms other than 10.9.5.

Comment 9 by pdr@chromium.org, Jun 29 2016

Cc: f...@opera.com drott@chromium.org e...@chromium.org behdad@chromium.org
+cc eae, drott, and behdad just as an FYI.

The testcase in comment #7 crashes only on OSX 10.9.5 and I think it's due to a bug in the OS that gets passed up through Harfbuzz into the shaper. While minimizing the testcase I found I could get a stack overflow in coretext which sounds similar to  https://crbug.com/458869 .

Because this test involves &#xfe0e;, it's possible https://crbug.com/615661 or  https://crbug.com/491556  are related.

I'm planning on turning a DCHECK into a CHECK for this which will:
1) Protect users from this security bug.
2) Use crash reporting to find out if this affects other platforms.
3) Use crash reporting to find out if users in the wild are actually hitting this.

Comment 10 by e...@chromium.org, Jun 29 2016

Sounds good, thanks for the heads up pdr!
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4ff7ba1cfab002517238841505ca9bb7aff6595

commit e4ff7ba1cfab002517238841505ca9bb7aff6595
Author: pdr <pdr@chromium.org>
Date: Wed Jun 29 19:55:32 2016

Harden a security CHECK in Font::individualCharacterRanges

This patch switches to a check (crash) for an assertion failure in
Font::individualCharacterRanges which is only known to occur on OSX
10.9.5. A comment has been added to explain this further. This has the
side benefit of exposing other cases of this bug on other platforms that
may be a more serious issue.

BUG= 620952 

Review-Url: https://codereview.chromium.org/2110703002
Cr-Commit-Position: refs/heads/master@{#402912}

[modify] https://crrev.com/e4ff7ba1cfab002517238841505ca9bb7aff6595/third_party/WebKit/Source/platform/fonts/Font.cpp

Comment 12 by pdr@chromium.org, Jun 29 2016

Labels: Merge-Request-51 Merge-Request-52
Very simple merge, just converting a debug assert into a release assert.
Project Member

Comment 13 by sheriffbot@chromium.org, Jun 30 2016

Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 14 by sheriffbot@chromium.org, Jul 1 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

Comment 16 by pdr@chromium.org, Jul 14 2016

This has been on Canary for some time and should be safe to merge. I checked the crash servers and this doesn't appear to have hit anyone.
Labels: -Merge-Request-52 Merge-Approved-52
Approving merge to M52 branch 2743 based on comment #16  and also the change is baked in M53 dev. Please merge ASAP. Thank you.
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 14 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/da8f89d0a31c471924f02479b126a7e0bb709205

commit da8f89d0a31c471924f02479b126a7e0bb709205
Author: Philip Rogers <pdr@chromium.org>
Date: Thu Jul 14 22:42:01 2016

Harden a security CHECK in Font::individualCharacterRanges

This patch switches to a check (crash) for an assertion failure in
Font::individualCharacterRanges which is only known to occur on OSX
10.9.5. A comment has been added to explain this further. This has the
side benefit of exposing other cases of this bug on other platforms that
may be a more serious issue.

BUG= 620952 

Review-Url: https://codereview.chromium.org/2110703002
Cr-Commit-Position: refs/heads/master@{#402912}
(cherry picked from commit e4ff7ba1cfab002517238841505ca9bb7aff6595)

Review URL: https://codereview.chromium.org/2152063002 .

Cr-Commit-Position: refs/branch-heads/2743@{#634}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/da8f89d0a31c471924f02479b126a7e0bb709205/third_party/WebKit/Source/platform/fonts/Font.cpp

Labels: -M-51 -Merge-Request-51 Release-0-M52 M-52

Comment 20 by pdr@chromium.org, Jul 21 2016

 Issue 628528  has been merged into this issue.
Project Member

Comment 21 by sheriffbot@chromium.org, Oct 6 2016

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment