New issue
Advanced search Search tips

Issue 835048 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

Use-of-uninitialized-value in SkPictureShader::onMakeContext

Project Member Reported by ClusterFuzz, Apr 20 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=4981521468096512

Fuzzer: attekett_surku_fuzzer
Job Type: linux_msan_chrome
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  SkPictureShader::onMakeContext
  SkShaderBase::makeContext
  SkBlitter::Choose
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_chrome&range=550057:550061

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4981521468096512

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, Apr 20 2018

Components: Internals>Skia
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, Apr 20 2018

Labels: Test-Predator-Auto-Owner
Owner: fmalita@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://skia.googlesource.com/skia/+/c6c5eade823a399efc1671c2c7f1bc278273d2d5 (Reland "Fix handling of MaskFilter matrices").

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
I think this is a duplicate of https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=7738


I don't have access to that one, can you add me to it?  Or better yet, assign to me, since I'm pretty sure it is my change.
It's assigned to your google account, you need to switch accounts to view it. I can't reassign it to your chromium account right now, otherwise I would (I also find having to switch accounts annoying). 

Let me know if you want oss-fuzz to use your chromium account from now on.
D'oh, somehow I didn't notice it.

Yes, if you don't mind please switch oss-fuzz to my @chromium.org address.

Thanks!
Status: Started (was: Assigned)
Project Member

Comment 8 by sheriffbot@chromium.org, Apr 20 2018

Labels: M-67
Project Member

Comment 9 by sheriffbot@chromium.org, Apr 20 2018

Labels: ReleaseBlock-Stable
This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

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

Comment 10 by sheriffbot@chromium.org, Apr 20 2018

Labels: Pri-1
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/8f88d89796d0ab7fefb302b21e03cc186dfc8bc3

commit 8f88d89796d0ab7fefb302b21e03cc186dfc8bc3
Author: Florin Malita <fmalita@chromium.org>
Date: Fri Apr 20 14:14:45 2018

Fix use-of-uninitialized-value in SkPictureShader::onMakeContext

SkPictureShader::refBitmapShader is expected to always initialize the
scale adjust vector when returning a non-null shader.  But the code path
returning EmptyShader does not do that.

Instead of hauling around a separate scale adjustment, we can refactor
to avoid this problem by adjusting the local matrix directly, if needed,
in refBitmapShader.  The local matrix is conveniently already stored in
a SkTCopyOnFirstWrite.

Bug:  chromium:835048 ,  oss-fuzz:7738 
Change-Id: I2df3bde7d6237f01bc71857c2fe254e86b186dc0
Reviewed-on: https://skia-review.googlesource.com/122544
Auto-Submit: Florin Malita <fmalita@chromium.org>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>

[modify] https://crrev.com/8f88d89796d0ab7fefb302b21e03cc186dfc8bc3/src/shaders/SkPictureShader.h
[modify] https://crrev.com/8f88d89796d0ab7fefb302b21e03cc186dfc8bc3/src/shaders/SkPictureShader.cpp

Cc: kjlubick@chromium.org
Labels: Merge-Request-67
Status: Fixed (was: Started)
Labels: OS-Android OS-Chrome OS-Mac OS-Windows
Project Member

Comment 15 by bugdroid1@chromium.org, Apr 20 2018

Labels: merge-merged-m67
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/ad3f3c7162bc5f96fcd2203b1ea6273432ff46ef

commit ad3f3c7162bc5f96fcd2203b1ea6273432ff46ef
Author: Florin Malita <fmalita@chromium.org>
Date: Fri Apr 20 17:46:27 2018

Fix use-of-uninitialized-value in SkPictureShader::onMakeContext

SkPictureShader::refBitmapShader is expected to always initialize the
scale adjust vector when returning a non-null shader.  But the code path
returning EmptyShader does not do that.

Instead of hauling around a separate scale adjustment, we can refactor
to avoid this problem by adjusting the local matrix directly, if needed,
in refBitmapShader.  The local matrix is conveniently already stored in
a SkTCopyOnFirstWrite.

Bug:  chromium:835048 ,  oss-fuzz:7738 
Change-Id: I2df3bde7d6237f01bc71857c2fe254e86b186dc0
Reviewed-on: https://skia-review.googlesource.com/122544
Auto-Submit: Florin Malita <fmalita@chromium.org>
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>
(cherry picked from commit 8f88d89796d0ab7fefb302b21e03cc186dfc8bc3)
Reviewed-on: https://skia-review.googlesource.com/122820

[modify] https://crrev.com/ad3f3c7162bc5f96fcd2203b1ea6273432ff46ef/src/shaders/SkPictureShader.h
[modify] https://crrev.com/ad3f3c7162bc5f96fcd2203b1ea6273432ff46ef/src/shaders/SkPictureShader.cpp

Cc: gov...@chromium.org
For the record, the regression was introduced shortly before M67 and the merge should be completely safe.
Labels: Merge-Approved-67
Thank you  fmalita@ for the ping. As discussed, this should be a safe merge so no need to revert from M67. Next time, pls request a merge and wait for approval before merging.

Comment 19 Deleted

Labels: -Merge-Request-67 Merge-Approved-67
Labels: -Merge-Approved-67
Removing "Merge-Approved-67" as it is already merged to M67 at #15.

Project Member

Comment 22 by bugdroid1@chromium.org, Apr 21 2018

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

commit 2564c15a034a4460cc4b647c5755ff9bf9f0d637
Author: skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Sat Apr 21 01:27:19 2018

Roll src/third_party/skia/ 0d24c5021..5209d7fce (18 commits)

https://skia.googlesource.com/skia.git/+log/0d24c502113b..5209d7fce35f

$ git log 0d24c5021..5209d7fce --date=short --no-merges --format='%ad %ae %s'
2018-04-20 bsalomon Fix starting location in GrRTC::drawOval when converting to GrShape
2018-04-20 bsalomon Analytic dashing of circles with single on/off intervals and butt caps.
2018-04-20 brianosman Fix argument names and documentation for SkMatrix44::set3x3()
2018-04-18 enne Reland "Add stub gpu workaround generators"
2018-04-20 robertphillips Add ability to dump opList info at flush-time
2018-04-20 benjaminwagner Fix Housekeeper-PerCommit-InfraTests
2018-04-20 scroggo Reduce threads used for DNG decodes on Android
2018-04-19 benjaminwagner Add SwiftShader Build jobs.
2018-04-19 benjaminwagner Add cmake_linux asset.
2018-04-20 egdaniel Add discard check when deciding if we should execute op list or not.
2018-04-20 robertphillips Remove another writePixels instance
2018-04-18 csmartdalton ccpr: Don't solve for cubic roots that are out of range
2018-04-19 benjaminwagner Add SwiftShader to DEPS.
2018-04-19 csmartdalton ccpr: Fix flatness and triangle-ness detection for conics
2018-04-20 robertphillips Prevent matrix stack from being corrupted if a rotated image filter is clipped out
2018-04-20 caryclark guard against NaN in path is rect
2018-04-19 fmalita Fix use-of-uninitialized-value in SkPictureShader::onMakeContext
2018-04-19 benjaminwagner Disable retries for Build, CT, and Calmbench tasks.

Created with:
  roll-dep src/third_party/skia
BUG= chromium:829614 ,chromium:b/78120086,chromium:835048


The AutoRoll server is located here: https://autoroll.skia.org

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff, who should
be CC'd on the roll, and stop the roller if necessary.


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
TBR=scroggo@chromium.org

Change-Id: I1547f2fccadcd1c6ca11f12d08bbf615a0fa1305
Reviewed-on: https://chromium-review.googlesource.com/1022719
Reviewed-by: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Commit-Queue: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Cr-Commit-Position: refs/heads/master@{#552541}
[modify] https://crrev.com/2564c15a034a4460cc4b647c5755ff9bf9f0d637/DEPS

Project Member

Comment 23 by ClusterFuzz, Apr 21 2018

ClusterFuzz has detected this issue as fixed in range 552538:552542.

Detailed report: https://clusterfuzz.com/testcase?key=4981521468096512

Fuzzer: attekett_surku_fuzzer
Job Type: linux_msan_chrome
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  SkPictureShader::onMakeContext
  SkShaderBase::makeContext
  SkBlitter::Choose
  
Sanitizer: memory (MSAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_msan_chrome&range=550057:550061
Fixed: https://clusterfuzz.com/revisions?job=linux_msan_chrome&range=552538:552542

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4981521468096512

See https://github.com/google/clusterfuzz-tools for more information.

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

Comment 24 by ClusterFuzz, Apr 21 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Fixed)
ClusterFuzz testcase 4981521468096512 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 25 by sheriffbot@chromium.org, Apr 21 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Is cl listed at #22 need a merge to M67?
No, that's just the regular Skia -> Chromium auto roller, pushing the fix to Chromium trunk.  We merged so early that we even beat the roll (which was delayed for other reasons).

The fix is already in Skia's m67 branch (c#15), nothing else is needed here.
Ok, got it. Thank you fmalita@.
Labels: -reward-topanel reward-unpaid reward-1000
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************
Labels: -Reward-1000 reward-1500
Nice one attekett! $1,000 for this, and $500 clusterfuzz bonus.

Comment 31 by aarya@google.com, Apr 27 2018

Just fyi, we did an incorrect payment here. This bug was already found by OSS-Fuzz in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=7738. And also C#27 validates that.
Labels: OSS-Fuzz-Found-First
Labels: -reward-unpaid reward-inprocess
Labels: -ReleaseBlock-Stable
Project Member

Comment 35 by sheriffbot@chromium.org, Jul 28

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