New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user
Status: Fixed
Owner:
Closed: Aug 16
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android, Chrome
Pri: 1
Type: Bug-Security



Sign in to add a comment
Security: out of bound write in filter_fuzz_stub
Reported by look.wan...@gmail.com, Jul 16 Back to list
VERSION
Chrome Version: asan-v8-arm-linux-release-487006
Operating System: Ubuntu 16.04.2 LTS


REPRODUCTION CASE
Run "./filter_fuzz_stub poc":

[0716/225254.319810:INFO:filter_fuzz_stub.cc(37)] Valid stream detected.
ASAN:DEADLYSIGNAL
=================================================================
==5233==ERROR: AddressSanitizer: SEGV on unknown address 0x782315b0 (pc 0x08be1f76 bp 0xfffa8dd8 sp 0xfffa8d00 T0)
==5233==The signal is caused by a WRITE memory access.
    #0 0x8be1f75 in SkEdgeBuilder::buildPoly(SkPath const&, SkIRect const*, int, bool) third_party/skia/src/core/SkEdgeBuilder.cpp
    #1 0x8be2aea in SkEdgeBuilder::build(SkPath const&, SkIRect const*, int, bool, bool) third_party/skia/src/core/SkEdgeBuilder.cpp:352:22
    #2 0x8be40f1 in SkEdgeBuilder::build_edges(SkPath const&, SkIRect const*, int, bool, bool) third_party/skia/src/core/SkEdgeBuilder.cpp:451:23


Root Cause(My guess, may not correct):

https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkEdgeBuilder.cpp?l=255

    char* edge = fAnalyticAA ? (char*)fAlloc.makeArrayDefault<SkAnalyticEdge>(maxEdgeCount)
                             : (char*)fAlloc.makeArrayDefault<SkEdge>(maxEdgeCount);

    SkDEBUGCODE(char* edgeStart = edge);
    char** edgePtr = fAlloc.makeArrayDefault<char*>(maxEdgeCount);
    fEdgeList = (void**)edgePtr;


"fAlloc.makeArrayDefault" didn't alloc new memory due to error in "allocObject":
(https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkArenaAlloc.h?gsn=commonArrayAlloc&l=157)
    char* allocObject(uint32_t size, uint32_t alignment) {
        uintptr_t mask = alignment - 1;
        char* objStart = (char*)((uintptr_t)(fCursor + mask) & ~mask);
        if ((ptrdiff_t)size > fEnd - objStart) {
            this->ensureSpace(size, alignment);
            objStart = (char*)((uintptr_t)(fCursor + mask) & ~mask);
        }
        return objStart;
    }

"ptrdiff_t" is  signed here, and size may be larger than 0x7fffffff.
"fCursor" would then point to a illegal address . 
 
poc
352 bytes View Download
Components: Internals>Skia
Labels: Security_Severity-Medium Security_Impact-Stable OS-Android OS-Chrome
Owner: hcm@chromium.org
Status: Assigned
hcm: could you please help triage this? Thanks!
Project Member Comment 2 by sheriffbot@chromium.org, Jul 17
Labels: M-60
Project Member Comment 3 by sheriffbot@chromium.org, Jul 17
Labels: Pri-1
Cc: hcm@chromium.org
Owner: liyuqian@chromium.org
Cc: look.wan...@gmail.com raymes@chromium.org
Thank you for reporting the bug! Can you please give me the exact gn args that you used to compile the filter_fuzz_stub which reproduces this error? My local asan build on my Ubuntu desktop doesn't reproduce it with "is_asan = true" and "is_debug = false".
Project Member Comment 6 by clusterf...@chromium.org, Jul 18
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6135705498812416.
Project Member Comment 7 by clusterf...@chromium.org, Jul 18
ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5291035654881280.
is_debug = false
target_cpu = "x86"

BTW:
With gn.args as
"
is_debug = false
target_cpu = "x64"
"

filter_fuzz_stub would crash at a different place:
#0  0x00007ffff6e8a536 in SkArenaAlloc::installPtrFooter (this=0x7fffffff5df0, 
    action=0x7ffff6e8a5a0 <SkArenaAlloc::NextBlock(char*)>, ptr=0x7fffffff5e30 "\200\336\f\201\033\026", padding=0)
    at ../../third_party/skia/src/core/SkArenaAlloc.cpp:67
#1  0x00007ffff6e8a890 in SkArenaAlloc::ensureSpace (this=0x7fffffff5df0, size=3190948320, alignment=8)
    at ../../third_party/skia/src/core/SkArenaAlloc.cpp:139

But only a NULL pointer crash
Cc: liyuqian@chromium.org herb@google.com
Owner: herb@chromium.org
Confirmed reproducible in 'target_cpu = "x64"' no matter whether is_debug is true or false. The root cause is that the path has too many edges and "char* newBlock = new char[allocationSize];" is returning a 0x0 in SkArenaAlloc::ensureSpace, which then sets fCursor to 0x0. It seems that we need a way for SkArenaAlloc to check whether we have the space available. For example, alloc may return false (instead of void) when the space is unavailable. Change owner to herb@ who is the author of SkArenaAlloc.
if fCursor is NULL, would crash here(just null address):
void SkArenaAlloc::installPtrFooter(FooterAction* action, char* ptr, uint32_t padding) {
    memmove(fCursor, &ptr, sizeof(char*)); // crash
    fCursor += sizeof(char*);
    this->installFooter(action, padding);
}

If not NULL, calculate of fCursor would be wrong, eventually point to some illegal address:

https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkArenaAlloc.h?l=178
        if (skstd::is_trivially_destructible<T>::value) {
            objStart = this->allocObject(arraySize, alignment);
            fCursor = objStart + arraySize; // wrong here
        }



----------------------------------------

Keypoint: "this->ensureSpace(size, alignment);" will not be executed due th compare "((ptrdiff_t)size > fEnd - objStart)"  (ptrdiff_t is signed)


    char* allocObject(uint32_t size, uint32_t alignment) {
        uintptr_t mask = alignment - 1;
        char* objStart = (char*)((uintptr_t)(fCursor + mask) & ~mask);
        if ((ptrdiff_t)size > fEnd - objStart) { 
            // never go here
            this->ensureSpace(size, alignment);
            objStart = (char*)((uintptr_t)(fCursor + mask) & ~mask);
        }
        return objStart;
    }

It seems to me more exploitable on x86 platform
Project Member Comment 13 by sheriffbot@chromium.org, Jul 31
herb: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
herb@ is OOO the next 2 weeks. Here are the comments of herb@ during a brief hangout chat:

1. "The basic point is that the C++ spec says that new can *never* return a nullptr."
2. "There is no such thing a signed subtraction. Subtraction is the same for signed and unsigned number."
Wrong.
In skia, a failed "new" never throw.(Due to the memory library)

ptrdiff_t 
.......
Project Member Comment 16 by sheriffbot@chromium.org, Aug 15
herb: Uh oh! This issue still open and hasn't been updated in the last 29 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 17 by bugdroid1@chromium.org, Aug 15
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/0bc4d60fe0a1ceae6dde0103c3fdf78ee1673ffa

commit 0bc4d60fe0a1ceae6dde0103c3fdf78ee1673ffa
Author: Herb Derby <herb@google.com>
Date: Tue Aug 15 20:40:27 2017

Fix bogus math in object allocation.

When a size_t is convert from a very large number to ptrdiff_t, it
becomes negative causing the existing block to be used instead of
allocating a new block.

BUG= chromium:744109 

Change-Id: I0bf98e3fb924851c162f6eca43d29a3f40dc9eaa
Reviewed-on: https://skia-review.googlesource.com/34541
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Herb Derby <herb@google.com>

[modify] https://crrev.com/0bc4d60fe0a1ceae6dde0103c3fdf78ee1673ffa/src/core/SkArenaAlloc.h
[modify] https://crrev.com/0bc4d60fe0a1ceae6dde0103c3fdf78ee1673ffa/tests/ArenaAllocTest.cpp

Project Member Comment 18 by bugdroid1@chromium.org, Aug 15
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/32491566dad004800d189447cc31eda1432730be

commit 32491566dad004800d189447cc31eda1432730be
Author: Florin Malita <fmalita@chromium.org>
Date: Tue Aug 15 21:07:19 2017

Revert "Fix bogus math in object allocation."

This reverts commit 0bc4d60fe0a1ceae6dde0103c3fdf78ee1673ffa.

Reason for revert: https://chromium-swarm.appspot.com/task?id=38007b6df51fef10&refresh=10

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
Caught signal 6 [Aborted], was running:
	8888 image scanline_kNonNative_premul interlaced3.png
	unit test  ArenaAllocReallyBigAlloc
	unit test  ClampRange
	565 image scaled_codec_premul_0.200 interlaced3.png_0.200
	unit test  ClipStack
Likely culprit:
	unit test  ArenaAllocReallyBigAlloc
Stack trace:
    /mnt/pd0/s/w/ir/out/Debug/dm(+0x2381c1) [0x567aa1c1]
    linux-gate.so.1(__kernel_sigreturn+0) [0xf770cca0]
    linux-gate.so.1(__kernel_vsyscall+0x9) [0xf770cc89]
    /lib/i386-linux-gnu/libc.so.6(gsignal+0xb0) [0xf7027dc0]
    /lib/i386-linux-gnu/libc.so.6(abort+0x157) [0xf7029287]
    /usr/lib/i386-linux-gnu/libstdc++.so.6(_ZN9__gnu_cxx27__verbose_terminate_handlerEv+0x16f) [0xf729d2ff]
    /usr/lib/i386-linux-gnu/libstdc++.so.6(+0x71ea4) [0xf729aea4]
    /usr/lib/i386-linux-gnu/libstdc++.so.6(+0x71f1d) [0xf729af1d]
    /usr/lib/i386-linux-gnu/libstdc++.so.6(__cxa_rethrow+0) [0xf729b1d0]
    /usr/lib/i386-linux-gnu/libstdc++.so.6(+0x727ff) [0xf729b7ff]
    /usr/lib/i386-linux-gnu/libstdc++.so.6(_Znaj+0x18) [0xf729b898]
    /mnt/pd0/s/w/ir/out/Debug/dm(_ZN12SkArenaAlloc11ensureSpaceEjj+0xa0) [0x56f113a6]
    /mnt/pd0/s/w/ir/out/Debug/dm(+0x3c2462) [0x56934462]
    /mnt/pd0/s/w/ir/out/Debug/dm(+0x239acb) [0x567abacb]
    /mnt/pd0/s/w/ir/out/Debug/dm(+0x239bdc) [0x567abbdc]
    /mnt/pd0/s/w/ir/out/Debug/dm(+0xa6417b) [0x56fd617b]
    /mnt/pd0/s/w/ir/out/Debug/dm(_ZNKSt8functionIFvvEEclEv+0x20) [0x56f10504]
    /mnt/pd0/s/w/ir/out/Debug/dm(_ZN12SkThreadPool4LoopEPv+0x298) [0x56f10bdb]
    /mnt/pd0/s/w/ir/out/Debug/dm(+0xb39700) [0x570ab700]
    /lib/i386-linux-gnu/libpthread.so.0(+0x627a) [0xf76df27a]
    /lib/i386-linux-gnu/libc.so.6(clone+0x66) [0xf70e3b56]
Aborted
Command exited with code 134

Original change's description:
> Fix bogus math in object allocation.
> 
> When a size_t is convert from a very large number to ptrdiff_t, it
> becomes negative causing the existing block to be used instead of
> allocating a new block.
> 
> BUG= chromium:744109 
> 
> Change-Id: I0bf98e3fb924851c162f6eca43d29a3f40dc9eaa
> Reviewed-on: https://skia-review.googlesource.com/34541
> Reviewed-by: Ben Wagner <bungeman@google.com>
> Commit-Queue: Herb Derby <herb@google.com>

TBR=bungeman@google.com,herb@google.com

Change-Id: I8ce2b45d13178395247dabd7af6853354399721c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:744109 
Reviewed-on: https://skia-review.googlesource.com/35000
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@chromium.org>

[modify] https://crrev.com/32491566dad004800d189447cc31eda1432730be/src/core/SkArenaAlloc.h
[modify] https://crrev.com/32491566dad004800d189447cc31eda1432730be/tests/ArenaAllocTest.cpp

Project Member Comment 19 by bugdroid1@chromium.org, Aug 15
The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/8618338c4afc7f8dd27e415ac44b40f2938cb363

commit 8618338c4afc7f8dd27e415ac44b40f2938cb363
Author: Herb Derby <herb@google.com>
Date: Tue Aug 15 21:54:27 2017

Fix bogus math in object allocation.

When a size_t is convert from a very large number to ptrdiff_t, it
becomes negative causing the existing block to be used instead of
allocating a new block.

Remove broken test from last try.

TBR=bungeman@google.com
BUG= chromium:744109 

Change-Id: Ifc81bb8f416d64f640730c66bce56a8e4c677173
Reviewed-on: https://skia-review.googlesource.com/35080
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Herb Derby <herb@google.com>

[modify] https://crrev.com/8618338c4afc7f8dd27e415ac44b40f2938cb363/src/core/SkArenaAlloc.h

Project Member Comment 20 by bugdroid1@chromium.org, Aug 16
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e2b929d9e09c8701f0c07d740f41356a56d75d17

commit e2b929d9e09c8701f0c07d740f41356a56d75d17
Author: skia-deps-roller@chromium.org <skia-deps-roller@chromium.org>
Date: Wed Aug 16 00:13:46 2017

Roll src/third_party/skia/ 15bb26ec7..25954b64c (8 commits)

https://skia.googlesource.com/skia.git/+log/15bb26ec70c9..25954b64c066

$ git log 15bb26ec7..25954b64c --date=short --no-merges --format='%ad %ae %s'
2017-08-15 mtklein explicitly vectorize sk_memset{16,32,64}
2017-08-15 mtklein upstream cr/165303354
2017-08-15 herb Fix bogus math in object allocation.
2017-08-15 fmalita Revert "Fix bogus math in object allocation."
2017-08-15 bungeman Remove SkTypeface::style().
2017-08-15 herb Fix bogus math in object allocation.
2017-08-15 fmalita Skip bilerp for integral-translate-only matrices (!clamp-clamp case)
2017-08-15 brianosman Speed up convexpaths GM in debug builds

Created with:
  roll-dep src/third_party/skia
BUG= 755391 , 744109 , 744109 , 744674 


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

If the roll is causing failures, see:
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium#TOC-Failures-due-to-DEPS-rolls


CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
TBR=bsalomon@chromium.org

Change-Id: I14e7599bb7b37734099f0984baf5e6488e5b3e10
Reviewed-on: https://chromium-review.googlesource.com/616164
Reviewed-by: Skia Deps Roller <skia-deps-roller@chromium.org>
Commit-Queue: Skia Deps Roller <skia-deps-roller@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494622}
[modify] https://crrev.com/e2b929d9e09c8701f0c07d740f41356a56d75d17/DEPS

Status: Fixed
I was able to reproduce this with gn args:
  is_debug=false
  target_cpu="x86"
  is_component_build=true
  sanitizer_keep_symbols=true
  is_asan=true

Before the fix asan generated the following error:

[0816/132713.608975:INFO:filter_fuzz_stub.cc(60)] Test case: /usr/local/google/home/herb/Downloads/poc
[0816/132713.610160:INFO:filter_fuzz_stub.cc(37)] Valid stream detected.
ASAN:DEADLYSIGNAL
=================================================================
==41824==ERROR: AddressSanitizer: SEGV on unknown address 0x763315b0 (pc 0xf635b0ee bp 0xffacc338 sp 0xffacc2e0 T0)
==41824==The signal is caused by a WRITE memory access.


After the error, asan returned out of memory, which is the correct behavior.

[0816/132905.109759:INFO:filter_fuzz_stub.cc(60)] Test case: /usr/local/google/home/herb/Downloads/poc
[0816/132905.110942:INFO:filter_fuzz_stub.cc(37)] Valid stream detected.
==42633==AddressSanitizer's allocator is terminating the process instead of returning 0
==42633==If you don't like this behavior set allocator_may_return_null=1
==42633==AddressSanitizer CHECK failed: /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/sanitizer_common/sanitizer_allocator.cc:218 "((0)) != (0)" (0x0, 0x0)

Project Member Comment 22 by sheriffbot@chromium.org, Aug 17
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Labels: -reward-topanel reward-unpaid reward-5000
*** 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.
*********************************
Nice one!  The VRP panel decided to award $5,000 for this report.  Cheers!
Labels: -reward-unpaid reward-inprocess
Labels: -M-60 M-62
Project Member Comment 28 by sheriffbot@chromium.org, Sep 15
Labels: Merge-Request-62
Project Member Comment 29 by sheriffbot@chromium.org, Sep 15
Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This merge request was au-generated. herb@ do you mean to merge this into M62?
My understand is that M62 was cut on 8/31. My change went in on 8/15, so I think it is already in. If this is not the case, just ping me.
Labels: -Hotlist-Merge-Review -Merge-Review-62
Labels: Release-0-M62
Labels: CVE-2017-5131
Owner: awhalley@chromium.org
Project Member Comment 36 by sheriffbot@chromium.org, Yesterday (39 hours ago)
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