1306628 - (CVE-2016-9894) Heap-buffer-overflow in AAHairlineBatch::onPrepareDraws

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Atte Kettunen]

Attachment: firefox-heap-buffer-overflow-add_line.html

Tested on:

OS: Ubuntu 16.04.1 LTS

Firefox: moz_source_stamp: f713114b8c8d352b668b3e8052bc51ece4df34e0


ASAN-trace:

==6370==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fb8bec14003 at pc 0x7fb8cdf9c78a bp 0x7ffc4226db70 sp 0x7ffc4226db68
WRITE of size 8 at 0x7fb8bec14003 thread T0
    #0 0x7fb8cdf9c789 in add_line /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/batches/GrAAHairLinePathRenderer.cpp:592:25
    #1 0x7fb8cdf9c789 in AAHairlineBatch::onPrepareDraws(GrVertexBatch::Target*) const /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/batches/GrAAHairLinePathRenderer.cpp:861
    #2 0x7fb8ce2a9799 in GrVertexBatch::onPrepare(GrBatchFlushState*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/batches/GrVertexBatch.cpp:19:5
    #3 0x7fb8ce1ca790 in prepare /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/batches/GrBatch.h:109:46
    #4 0x7fb8ce1ca790 in GrDrawTarget::prepareBatches(GrBatchFlushState*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/GrDrawTarget.cpp:202
    #5 0x7fb8ce1db713 in GrDrawingManager::flush() /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/GrDrawingManager.cpp:77:9
    #6 0x7fb8ce1c73a1 in flush /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/GrContext.cpp:237:9
    #7 0x7fb8ce1c73a1 in GrContext::prepareSurfaceForExternalIO(GrSurface*) /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/gpu/GrContext.cpp:537
    #8 0x7fb8c6aaf1d4 in mozilla::layers::PersistentBufferProviderBasic::ReturnDrawTarget(already_AddRefed<mozilla::gfx::DrawTarget>) /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/layers/PersistentBufferProvider.cpp:46:5
    #9 0x7fb8c948f28b in mozilla::dom::CanvasRenderingContext2D::ReturnTarget() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/canvas/CanvasRenderingContext2D.cpp:1767:5
    #10 0x7fb8c9494016 in mozilla::dom::CanvasRenderingContext2D::OnStableState() /builds/slave/m-cen-l64-asan-000000000000000/build/src/dom/canvas/CanvasRenderingContext2D.cpp:1499:3
    #11 0x7fb8c9550a22 in applyImpl<mozilla::dom::CanvasRenderingContext2D, void (mozilla::dom::CanvasRenderingContext2D::*)()> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:729:12
    #12 0x7fb8c9550a22 in apply<mozilla::dom::CanvasRenderingContext2D, void (mozilla::dom::CanvasRenderingContext2D::*)()> /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:735
    #13 0x7fb8c9550a22 in mozilla::detail::RunnableMethodImpl<void (mozilla::dom::CanvasRenderingContext2D::*)(), true, false>::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:764
.
.
.
0x7fb8bec14003 is located 6141 bytes to the left of 140400-byte region [0x7fb8bec15800,0x7fb8bec37c70)
allocated by thread T0 here:
    #0 0x4b27ce in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:71:3
    #1 0x4e0d7d in moz_xrealloc /builds/slave/m-cen-l64-asan-000000000000000/build/src/memory/mozalloc/mozalloc.cpp:105:20
    #2 0x7fb8ce56eefe in makeSpace /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/include/core/SkPathRef.h:424:46
    #3 0x7fb8ce56eefe in SkPathRef::growForVerb(int, float) /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/core/SkPathRef.cpp:439
    #4 0x7fb8ce552bce in growForVerb /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/include/core/SkPathRef.h:73:20
    #5 0x7fb8ce552bce in SkPath::moveTo(float, float) /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/skia/skia/src/core/SkPath.cpp:731
    #6 0x7fb8c85f18b1 in MoveTo /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/dom/CanvasRenderingContext2D.h:306:7
    #7 0x7fb8c85f18b1 in mozilla::dom::CanvasRenderingContext2DBinding::moveTo(JSContext*, JS::Handle<JSObject*>, mozilla::dom::CanvasRenderingContext2D*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:6060
.
.
.

Comment 1

[Daniel Veditz [:dveditz]]

Jeff: who investigates Skia bugs these days?

Comment 2

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

Lee, can you reproduce this?  If yes, what about with Skia 55?

Comment 3

[Lee Salzman [:lsalzman]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Lee, can you reproduce this?  If yes, what about with Skia 55?

I can't manage to reproduce this one.

Comment 4

[Lee Salzman [:lsalzman]]

Reproduced. Found the cause.

Comment 5

[Lee Salzman [:lsalzman]]

The bug was introduced upstream by this patch: https://skia.googlesource.com/skia/+/e935f1a0e2351373c33600b8388492ce1218014a

We incorporated this into our tree in 46 via bug 1082598.

Comment 6

[Lee Salzman [:lsalzman]]

Attachment: handle large sizes in GrResourceProvider::createBuffer

This test-case is causing SkiaGL to create a buffer whose size is will not fit in 32 bits. When creating a GrGLBuffer, the size gets truncated to 32 bits.

So, essentially, Skia calls glBufferData(uint32_t(size_that_wont_fit)). Then Skia calls glMapBufferRange() on this buffer, to map it into client memory, thinking it is that larger size, even though the size got truncated down to 32 bits. That's why we're overflowing.

This only affects platform on which we use SkiaGL and where sizeof(size_t) == 8. So, basically, this is Mac only in terms of our default platform/prefs setups.

Android we use SkiaGL, but size_t is 4 bytes there.

Comment 7

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

This is probably hidden in nightly because of bug 1309913, as we don't use SkiaGL on OS X at all.

Comment 8

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

Comment on attachment 8800921 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer

Review of attachment 8800921 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/skia/skia/src/gpu/GrResourceProvider.cpp
@@ +110,5 @@
>  
>      // bin by pow2 with a reasonable min
> +    static const size_t MIN_SIZE = 1 << 12;
> +    size_t allocSize = sizeof(size_t) > sizeof(uint32_t) && size > (1u << 31)
> +                       ? size_t(uint64_t(GrNextPow2(uint32_t(uint64_t(size) >> 32))) << 32)

Is there a constant somewhere, defined to be 32, that means "this is how much can fit in the GrGLBuffer" that we could use instead of "32" and "31"?

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Comment on attachment 8800921 [details] [diff] [review]
> handle large sizes in GrResourceProvider::createBuffer
> 
> Review of attachment 8800921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/skia/skia/src/gpu/GrResourceProvider.cpp
> @@ +110,5 @@
> >  
> >      // bin by pow2 with a reasonable min
> > +    static const size_t MIN_SIZE = 1 << 12;
> > +    size_t allocSize = sizeof(size_t) > sizeof(uint32_t) && size > (1u << 31)
> > +                       ? size_t(uint64_t(GrNextPow2(uint32_t(uint64_t(size) >> 32))) << 32)
> 
> Is there a constant somewhere, defined to be 32, that means "this is how
> much can fit in the GrGLBuffer" that we could use instead of "32" and "31"?

The 31/32 here only refer to the size of a dword, and not particular to anything in a GrGLBuffer, and is just to turn a 32 bit next-power-of-2 routine into a 64 bit one, independent of any other limit. The actual validation of whether the allocation succeeds is done elsewhere, so the issue is just making sure we are actually giving the allocation the size we thought we were giving it. In this case, we were giving it an inaccurate size because of that 64->32 bit issue.

Comment 10

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

Lee, are we going to ask for sec-approval for this soon?

Comment 11

[Lee Salzman [:lsalzman]]

Comment on attachment 8800921 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The patch itself doesn't implicate mapping the buffer with the truncated size as being the problem, so you'd have to have intimate knowledge of Skia to understand why this becomes a security vulnerability. You'd also need to be able to have knowledge of where whatever GPU driver is running is going to map buffer memory into the address space to do much with this.

> Which older supported branches are affected by this flaw?
46+

> If not all supported branches, which bug introduced the flaw?
https://bugzilla.mozilla.org/show_bug.cgi?id=1082598

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should work for all branches.

> How likely is this patch to cause regressions; how much testing does it need?
This shouldn't really cause any problems, since it only deals with the allocation of extremely large buffers, larger than just about anyone will ever use in practice, and is only correcting the size of the allocations of said buffers.

Comment 12

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

Release management, is it too late to take this for 50?

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Al Billings [:abillings] from comment #12)
> Release management, is it too late to take this for 50?

This is sec-crit (risk assessment by dev seems like it's not a common scenario anyways) and we still have another week before we hit RC so I'd be ok to take this one before I gtb 50.0b10 (Monday Oct 24th) noon PST.

Comment 14

[Lee Salzman [:lsalzman]]

Upstream restricted/security bug: https://bugs.chromium.org/p/skia/issues/detail?id=5882

Comment 15

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

This missed the window of noon today because I didn't see your reply. I assume we need to punt this to 51 now?

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Al Billings [:abillings] from comment #15)
> This missed the window of noon today because I didn't see your reply. I
> assume we need to punt this to 51 now?

Hi Al, I will gtb 50.0b11 on Thursday, we can take it then if the patch is low risk.

Comment 17

[Daniel Veditz [:dveditz]]

(In reply to Lee Salzman [:lsalzman] from comment #14)
> Upstream restricted/security bug:
> https://bugs.chromium.org/p/skia/issues/detail?id=5882

How does your patch compare to the upstream?  If it's the same that might reassure release drivers since presumably it's gotten testing

Comment 18

[Lee Salzman [:lsalzman]]

(In reply to Daniel Veditz [:dveditz] from comment #17)
> (In reply to Lee Salzman [:lsalzman] from comment #14)
> > Upstream restricted/security bug:
> > https://bugs.chromium.org/p/skia/issues/detail?id=5882
> 
> How does your patch compare to the upstream?  If it's the same that might
> reassure release drivers since presumably it's gotten testing

Upstream has not fixed this yet. I merely reported the issue so they can fix it in a way of their choosing, though I relayed our particular fix to them.

Comment 19

[Lee Salzman [:lsalzman]]

Attachment: handle large sizes in GrResourceProvider::createBuffer

I realized there was a potential vulnerability still in the 32-bit size_t case, where if NextPow2 gets called on a value like 0x80000001, that will end up returning 0. So this patch instead handles that by always treating these large values as 64-bit, then clamping them by SIZE_MAX. So for 32-bit size_t 0x80000001 will get converted to 0xFFffFFff (32-bit SIZE_MAX), which while not a power-of-2 is still safe for binning and will not overflow. This will allow the downstream buffer allocation to gracefully fail.

Since we still do have 32 bit builds, I didn't want to leave this lurking gremlin in there, even though this bug was pointing at 64 bit builds.

Comment 20

[Lee Salzman [:lsalzman]]

Comment on attachment 8804147 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer

Sec-approval request remains the same, more or less.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The patch itself doesn't implicate mapping the buffer with the truncated size as being the problem, so you'd have to have intimate knowledge of Skia to understand why this becomes a security vulnerability. You'd also need to be able to have knowledge of where whatever GPU driver is running is going to map buffer memory into the address space to do much with this.

> Which older supported branches are affected by this flaw?
46+

> If not all supported branches, which bug introduced the flaw?
Bug 1082598

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch can be easily backported to all branches.

> How likely is this patch to cause regressions; how much testing does it need?
This shouldn't really cause any problems, since it only deals with the allocation of extremely large buffers, larger than just about anyone will ever use in practice, and is only correcting the size of the allocations of said buffers.

Comment 21

[Lee Salzman [:lsalzman]]

Attachment: handle large sizes in GrResourceProvider::createBuffer (aurora/beta)

This is just a simple bitrot fix of the patch for aurora/beta. Actual changed code still remains exactly the same as in other patch.

Comment 22

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

Comment on attachment 8804147 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer

Sec-approval for checkin into trunk on 11/22 (a week after we ship 50). We'll want an Aurora and Beta patch to go in shortly after it goes into trunk.

Comment 23

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-critical

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55405164269f027b2e028d56b10c8aa5261b997f

Setting 50 back to affected to get it on the radar for 50.1 consideration.

Comment 25

[Gerry Chang [:gchang]]

Track 51+ as sec-critical.

Comment 26

[Marcia Knous [:marcia]]

Tracking 53+ due to it being sec-critical.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8804992 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer (aurora/beta)

Please nominate this for Aurora/Beta/Release approval ASAP so we can hopefully get it uplifted in time for 51b6.

Comment 28

[Lee Salzman [:lsalzman]]

Comment on attachment 8804992 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer (aurora/beta)

Approval Request Comment
[Feature/Bug causing the regression]: bug 1082598
[User impact if declined]: Buffer overrun exploit
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: beta/release
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just fixes an allocation size overflow issue and there is already a similar upstream fix.
[String changes made/needed]: None

Comment 29

[Lee Salzman [:lsalzman]]

Comment on attachment 8804147 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer

Approval Request Comment
[Feature/Bug causing the regression]: bug 1082598
[User impact if declined]: Buffer overrun exploit
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: aurora
[Is the change risky?]: No
[Why is the change risky/not risky?]: Just fixes an allocation size overflow issue and there is already a similar upstream fix.
[String changes made/needed]: None

Comment 30

[Ryan VanderMeulen [:RyanVM]]

This landed on m-c awhile ago, not sure how the bug didn't get marked.

https://hg.mozilla.org/mozilla-central/rev/55405164269f027b2e028d56b10c8aa5261b997f

Comment 31

[Gerry Chang [:gchang]]

Comment on attachment 8804992 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer (aurora/beta)

Fix a sec-critical. Beta51+ and Aurora52+. Should be in 51 beta 6.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1258309afc85

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/057879e7de42

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8804992 [details] [diff] [review]
handle large sizes in GrResourceProvider::createBuffer (aurora/beta)

Sec-crit, meets the triage bar for inclusion in 50.1.0

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/7ca25ca26db3