kojii@, do you mind to take a look?

Author: kojii
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/dcc13470a445d27e0d1722799f8ab780be8c90a1
Time: Tue Mar 15 00:40:59 2016
Lines 245-257 of file ShapeResult.cpp which potentially caused crash are changed in this cl (frame #1, "blink::ShapeResult::applySpacing").
Minimum distance from crash line to modified line: 0. (file: ShapeResult.cpp, crashed on: 256, modified: 256).

I'm having difficulties to reproduce as:
* Mac ASAN doesn't build locally, says libclang_rt.asan_osx_dynamic.dylib not found.
* Does not reproduce on Linux ASAN nor Mac with additional CHECK

but from the stack, it looks like:
  run->m_startIndex + glyphData.characterIndex
exceeds TextRun::length(), so the root cause is likely in HarfBuzzShaper to build ShapeResult.

I'm going to change ShapeResultSpacing to use TextRun::operator[] which has ASSERT_WITH_SECURITY_IMPLICATION rather than raw pointer to character buffer, so this should turn to a normal crash.

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

commit b18b753c893ad73fea5bd40046b7f55d1247fd10
Author: kojii <kojii@chromium.org>
Date: Wed Jun 22 11:06:26 2016

Change ShapeResultShaping::computeSpacing to use TextRun::operator[]

ShapeResultShaping::computeSpacing used to use raw pointer to character
buffer in TextRun. This patch changes it to use TextRun::operator[] so
that it can rely on the range-check built into TextRun.

BUG= 621843 

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

[modify] https://crrev.com/b18b753c893ad73fea5bd40046b7f55d1247fd10/third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.cpp
[modify] https://crrev.com/b18b753c893ad73fea5bd40046b7f55d1247fd10/third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.h

ClusterFuzz has detected this issue as fixed in range 401149:401251.

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

Fuzzer: inferno_twister_custom_bundle
Job Type: mac_asan_chrome
Platform Id: mac

Crash Type: Heap-buffer-overflow READ 2
Crash Address: 0x61000008faa6
Crash State:
  float blink::ShapeResultSpacing::computeSpacing<unsigned short>
  blink::ShapeResult::applySpacing
  blink::ShapeResult::applySpacingToCopy
  
Recommended Security Severity: High

Regressed: https://cluster-fuzz.appspot.com/revisions?job=mac_asan_chrome&range=381067:381276
Fixed: https://cluster-fuzz.appspot.com/revisions?job=mac_asan_chrome&range=401149:401251

Minimized Testcase (0.48 Kb): https://cluster-fuzz.appspot.com/download/AMIfv94LW4us-y46BAXJnMKWgZCcnWOLWZZ7iog69-UCoOG7Rk3CQJorGI4LJ871HC3TSGPacyg34gUvzbazGIKVUlKiiQBlbdEQm5RpFKJnIdLvGzN9-N9I1r8GCZKTg14LOl2EcYH3RbEPadJQH_4kaC7Js72YNQ?testcase_id=6324874025107456

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

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

Sorry I missed your question. There are a bit to note.

First of all, yes, this change is baked in Canary, and I think it's safe to merge. The diff is rather large due to a move from .h to .cpp but the actual change is not large.

Last Tested Stacktrace indicates that it crashed. This is expected. As the commit message indicates, the fix in #6 merely checks the array range before access, so that the security issue becomes a normal crash.

Are we ok to say a normal crash is "fixed"? If yes, I'm good to merge if approved.

For some reasons I could not build Mac ASAN, and this issue did not reproduce on Linux ASAN, so I wanted to see the crash in normal builds. But even with the range check in the release build, the crash is seen only on Mac ASAN. I'm suspecting there is something special font configuration for the Mac ASAN bot and that's causing this crash (as the code path around this can change depends on what fonts the machine has) but hard to know more.

Thank you kojii@. AFAIK we have ASAN build only for Windows (not for Mac & Linux). Am I missing anything here?

I'm OK to take this merge in for M52 as it is baked in canary/dev, safe to merge and fixed is verified by ClusterFuzz. But I will let awhalley@ to make a final call on this.

Yep, good to take this.

Approving merge to M52 branch 2743. Please merge ASAP (by 5:00 PM PST today, Friday). Thank you.

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

commit 2ed54182b9b0e31c3aab524f7f270f7cdcf2d8a5
Author: Koji Ishii <kojii@chromium.org>
Date: Fri Jul 15 16:10:20 2016

Change ShapeResultShaping::computeSpacing to use TextRun::operator[]

ShapeResultShaping::computeSpacing used to use raw pointer to character
buffer in TextRun. This patch changes it to use TextRun::operator[] so
that it can rely on the range-check built into TextRun.

BUG= 621843 

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

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

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

[modify] https://crrev.com/2ed54182b9b0e31c3aab524f7f270f7cdcf2d8a5/third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.cpp
[modify] https://crrev.com/2ed54182b9b0e31c3aab524f7f270f7cdcf2d8a5/third_party/WebKit/Source/platform/fonts/shaping/ShapeResultSpacing.h

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