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 2 users
link

Issue 890576: heap buffer overflow in skia::SkTDPQueue::insert

Reported by cdsrc2...@gmail.com, Sep 29

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3396.99 Safari/537.36

Steps to reproduce the problem:
1. download and unzip the release asan chromium :asan-linux-release-593799
2. Run ./chrome crash.html

What is the expected behavior?

What went wrong?
heap buffer overflow in skia::SkTDPQueue::insert

1. download and unzip the release asan chromium :asan-linux-release-593799
2. Run ./chrome poc/crash.html

Could get 3 kinds of crash:oom ,UAF and double free.

The skia's SkTDPQueue seems not to be thread safe.

in src/third_party/skia/src/core/SkTDPQueue.h:
    void insert(T entry) {                       <----if two threads call into here together
        this->validate();
        int index = fArray.count();
        *fArray.append() = entry;                <----A
        this->setIndex(fArray.count() - 1);
        this->percolateUpIfNecessary(index);
        this->validate();
    }
The append() is not protected.So when two threads came into here ,could cause series of problems.One of the problems could be like this(code line marked A and B) :

in third_party/skia/include/private/SkTDArray.h:
    void resizeStorageToAtLeast(int count) {
        SkASSERT(count > fReserve);
        ...
        fReserve = SkTo<int>(reserve);
        fArray = (T*)sk_realloc_throw(fArray, fReserve * sizeof(T));  <----B                                                                                                                              
    }

1: If one thread comes to code line B while another thread is writing some data using old fArray pointer(see code line A),out of bounds write(or heap buffer overflow) happened.Stacktrace of this situation sees in log/oom.log
2: If two threads both come code line B,double free happened.Stacktrace of this situation sees log/double_free.log

To trigger this bug,i tried to let a worker connect with main thread using api about WebGraphicsContext3D.Sees in poc/crash.html

I also got situation 3 in this way.Perhaps they all cause by the unsafe operation through different threads?Or they should belong to two different issue?

3: in src/third_party/skia/src/gpu/GrResourceCache.cpp:
    while(fNonpurgeableResources.count()) {
        GrGpuResource* back = *(fNonpurgeableResources.end() - 1);
        SkASSERT(!back->wasDestroyed());
        back->cacheAccess().release();     <----C
    }
If other unsafe operation makes the SkTDArray holds two same pointer,the "back" pointer could be used after deleted at code line C.Stacktrace of this situation sees in log/UAF.log.

I tried a mitigation patch.Situation 1 and 2 sees in patch/patch1.diff situation 3 sees in patch/patch2.diff.Hope my work helps.

Did this work before? N/A 

Chrome version: 71.0.3561.0  Channel: n/a
OS Version: 16.04
Flash Version:
 
poc+log+patch.zip
45.2 KB Download

Comment 1 by cdsrc2...@gmail.com, Sep 29

If can not repro,please change the timeout at crash.html:52 to make it more stable to repro and get different kind of crash.

Comment 2 by dominickn@chromium.org, Sep 30

Cc: reed@chromium.org mtklein@chromium.org
Components: Internals>Skia
Owner: hcm@chromium.org
Status: Assigned (was: Unconfirmed)
Thanks for the report. +skia folks for triage.

Comment 3 by mtklein@chromium.org, Oct 1

Owner: bsalo...@google.com
I don't think any of these data structures are meant to be thread safe.  This seems like Chromium's somehow breaking Skia's expectations about using Ganesh in multiple threads.

Comment 4 by bsalo...@google.com, Oct 1

Cc: bsalo...@google.com
Owner: fs...@chromium.org
From looking over mtklein@'s shoulder it seems offscreen canvas causes a texture-backed SkImage created by GrContext on one thread to be unreffed on another thread. This is not currently allowed. The destructor accesses manipulates non-atomic ref cnts and accesses GrResourceCache which is designed to be thread safe.

Comment 5 by dominickn@chromium.org, Oct 1

Cc: zakerinasab@chromium.org
Components: Blink>Canvas
Thanks for investigating - canvas folks, can you follow up on this non-thread-safe use of SkImage?

Comment 6 by fs...@chromium.org, Oct 2

Cc: fs...@chromium.org
Owner: davidqu@chromium.org

Comment 7 by mea...@chromium.org, Oct 3

Labels: Security_Severity-High Security_Impact-Stable
Adjusting labels.

Comment 8 by sheriffbot@chromium.org, Oct 4

Project Member
Labels: M-69 Target-69

Comment 9 by sheriffbot@chromium.org, Oct 4

Project Member
Labels: -Pri-2 Pri-1

Comment 10 by sheriffbot@chromium.org, Oct 14

Project Member
davidqu: 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

Comment 11 by fs...@chromium.org, Oct 15

Cc: davidqu@chromium.org
Owner: fs...@chromium.org
Status: Started (was: Assigned)

Comment 12 by sheriffbot@chromium.org, Oct 17

Project Member
Labels: -M-69 Target-70 M-70

Comment 13 by fs...@chromium.org, Oct 30

Ahn! Found the fix! :)
Patch on its way.

Comment 15 by palmer@chromium.org, Oct 30

Labels: OS-Android OS-Chrome OS-Fuchsia OS-Mac OS-Windows
Security team thanks you, fserb! :)

Comment 16 by fs...@chromium.org, Oct 31

Cc: gab@chromium.org bsalomon@chromium.org jbroman@chromium.org
cc'ing folks on my CL so they can have context.

Comment 17 by bugdroid1@chromium.org, Oct 31

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78d89fe556cb5dabbc47b4967cdf55e607e29580

commit 78d89fe556cb5dabbc47b4967cdf55e607e29580
Author: Fernando Serboncini <fserb@chromium.org>
Date: Wed Oct 31 22:25:41 2018

Fix *StaticBitmapImage ThreadChecker and unaccelerated SkImage destroy

- AcceleratedStaticBitmapImage was misusing ThreadChecker by having its
own detach logic. Using proper DetachThread is simpler, cleaner and
correct.
- UnacceleratedStaticBitmapImage didn't destroy the SkImage in the
proper thread, leading to GrContext/SkSp problems.

Bug:  890576 
Change-Id: Ic71e7f7322b0b851774628247aa5256664bc0723
Reviewed-on: https://chromium-review.googlesource.com/c/1307775
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604427}
[modify] https://crrev.com/78d89fe556cb5dabbc47b4967cdf55e607e29580/third_party/blink/renderer/platform/graphics/accelerated_static_bitmap_image.cc
[modify] https://crrev.com/78d89fe556cb5dabbc47b4967cdf55e607e29580/third_party/blink/renderer/platform/graphics/accelerated_static_bitmap_image.h
[modify] https://crrev.com/78d89fe556cb5dabbc47b4967cdf55e607e29580/third_party/blink/renderer/platform/graphics/unaccelerated_static_bitmap_image.cc
[modify] https://crrev.com/78d89fe556cb5dabbc47b4967cdf55e607e29580/third_party/blink/renderer/platform/graphics/unaccelerated_static_bitmap_image.h

Comment 18 by fs...@chromium.org, Nov 1

Status: Fixed (was: Started)
We did it, folks! :)
(does restrict-view get automatically dropped after a while?)

Comment 19 by jbroman@chromium.org, Nov 1

Yes, sheriffbot will come by and make it public in 14 weeks.

Comment 20 by sheriffbot@chromium.org, Nov 2

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 21 by sheriffbot@chromium.org, Nov 4

Project Member
Labels: Merge-Request-71

Comment 22 by sheriffbot@chromium.org, Nov 4

Project Member
Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: M71 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

Comment 23 by gov...@chromium.org, Nov 5

Cc: awhalley@google.com
+ awhalley@ (Security TPM) for M71 merge review. Thank you.

Comment 24 by awhalley@chromium.org, Nov 5

Labels: reward-topanel

Comment 25 by awhalley@google.com, Nov 5

govind@ - good for M71

Comment 26 by gov...@chromium.org, Nov 5

Labels: -Merge-Review-71 Merge-Approved-71
Approving merge to M71 branch 3578 based on comment #25. Pls merge ASAP. Thank you.

Comment 27 by fs...@chromium.org, Nov 5

Done.

Comment 28 by bugdroid1@chromium.org, Nov 5

Project Member
Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1d98e2c78a40847353dffb23b765e83620e9c51a

commit 1d98e2c78a40847353dffb23b765e83620e9c51a
Author: Fernando Serboncini <fserb@chromium.org>
Date: Mon Nov 05 17:40:12 2018

Fix *StaticBitmapImage ThreadChecker and unaccelerated SkImage destroy

- AcceleratedStaticBitmapImage was misusing ThreadChecker by having its
own detach logic. Using proper DetachThread is simpler, cleaner and
correct.
- UnacceleratedStaticBitmapImage didn't destroy the SkImage in the
proper thread, leading to GrContext/SkSp problems.

Bug:  890576 
Change-Id: Ic71e7f7322b0b851774628247aa5256664bc0723
Reviewed-on: https://chromium-review.googlesource.com/c/1307775
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604427}(cherry picked from commit 78d89fe556cb5dabbc47b4967cdf55e607e29580)
Reviewed-on: https://chromium-review.googlesource.com/c/1318175
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#500}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/1d98e2c78a40847353dffb23b765e83620e9c51a/third_party/blink/renderer/platform/graphics/accelerated_static_bitmap_image.cc
[modify] https://crrev.com/1d98e2c78a40847353dffb23b765e83620e9c51a/third_party/blink/renderer/platform/graphics/accelerated_static_bitmap_image.h
[modify] https://crrev.com/1d98e2c78a40847353dffb23b765e83620e9c51a/third_party/blink/renderer/platform/graphics/unaccelerated_static_bitmap_image.cc
[modify] https://crrev.com/1d98e2c78a40847353dffb23b765e83620e9c51a/third_party/blink/renderer/platform/graphics/unaccelerated_static_bitmap_image.h

Comment 29 by cr-audit...@appspot.gserviceaccount.com, Nov 5

Project Member
Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/1d98e2c78a40847353dffb23b765e83620e9c51a

Commit: 1d98e2c78a40847353dffb23b765e83620e9c51a
Author: fserb@chromium.org
Commiter: fserb@chromium.org
Date: 2018-11-05 17:40:12 +0000 UTC

Fix *StaticBitmapImage ThreadChecker and unaccelerated SkImage destroy

- AcceleratedStaticBitmapImage was misusing ThreadChecker by having its
own detach logic. Using proper DetachThread is simpler, cleaner and
correct.
- UnacceleratedStaticBitmapImage didn't destroy the SkImage in the
proper thread, leading to GrContext/SkSp problems.

Bug:  890576 
Change-Id: Ic71e7f7322b0b851774628247aa5256664bc0723
Reviewed-on: https://chromium-review.googlesource.com/c/1307775
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Commit-Queue: Fernando Serboncini <fserb@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#604427}(cherry picked from commit 78d89fe556cb5dabbc47b4967cdf55e607e29580)
Reviewed-on: https://chromium-review.googlesource.com/c/1318175
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#500}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}

Comment 30 by awhalley@chromium.org, Nov 12

Labels: -reward-topanel reward-unpaid reward-3000
*** 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.
*********************************

Comment 31 by awhalley@google.com, Nov 12

Hi cdsrc2016@, $3,000 for this report from the Chrome VRP panel, thanks!

Comment 32 by awhalley@google.com, Nov 12

Labels: -reward-unpaid reward-inprocess

Comment 33 by cdsrc2...@gmail.com, Nov 12

Thanks very much for the reward :)

Comment 34 by awhalley@google.com, Dec 3

Labels: Release-0-M71

Comment 35 by awhalley@chromium.org, Dec 11

Labels: CVE-2018-18338 CVE_description-missing

Comment 36 by awhalley@chromium.org, Dec 11

Labels: -CVE_description-missing CVE_description-submitted

Comment 37 by sheriffbot@chromium.org, Feb 8

Project Member
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