Bug 1306628 (CVE-2016-9894)

Heap-buffer-overflow in AAHairlineBatch::onPrepareDraws

RESOLVED FIXED in Firefox 50

Status

()

Core
Graphics
RESOLVED FIXED
11 months ago
5 months ago

People

(Reporter: Atte Kettunen, Assigned: lsalzman)

Tracking

({csectype-bounds, sec-critical})

46 Branch
mozilla53
csectype-bounds, sec-critical
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr45 unaffected, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed, firefox53+ fixed)

Details

(Whiteboard: [gfx-noted][adv-main50.1+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 months ago
Created attachment 8796563 [details]
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
.
.
.
Jeff: who investigates Skia bugs these days?
Group: core-security → gfx-core-security
Component: General → Graphics
Flags: needinfo?(jmuizelaar)
Flags: in-testsuite?

Updated

11 months ago
Assignee: nobody → lsalzman
Flags: needinfo?(jmuizelaar)

Updated

11 months ago
Keywords: sec-critical

Updated

11 months ago
Keywords: csectype-bounds
Lee, can you reproduce this?  If yes, what about with Skia 55?
Flags: needinfo?(lsalzman)
(Assignee)

Comment 3

10 months ago
(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.
Flags: needinfo?(lsalzman)
(Assignee)

Comment 4

10 months ago
Reproduced. Found the cause.
(Assignee)

Comment 5

10 months ago
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.
Has Regression Range: --- → yes
Has STR: --- → yes
Whiteboard: [gfx-noted]
Version: unspecified → 46 Branch
(Assignee)

Comment 6

10 months ago
Created attachment 8800921 [details] [diff] [review]
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.
Attachment #8800921 - Flags: review?(mchang)
(Assignee)

Updated

10 months ago
Status: NEW → ASSIGNED

Updated

10 months ago
Attachment #8800921 - Flags: review?(mchang) → review+
This is probably hidden in nightly because of bug 1309913, as we don't use SkiaGL on OS X at all.
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"?
(Assignee)

Comment 9

10 months ago
(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.

Updated

10 months ago
Flags: sec-bounty?
Lee, are we going to ask for sec-approval for this soon?
status-firefox52: --- → affected
tracking-firefox52: --- → +
Flags: needinfo?(lsalzman)
(Assignee)

Comment 11

10 months ago
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.
Flags: needinfo?(lsalzman)
Attachment #8800921 - Flags: sec-approval?
Release management, is it too late to take this for 50?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)

Comment 13

10 months ago
(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.
Flags: needinfo?(rkothari)
status-firefox49: --- → wontfix
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox-esr45: --- → unaffected
Flags: needinfo?(lhenry)
(Assignee)

Comment 14

10 months ago
Upstream restricted/security bug: https://bugs.chromium.org/p/skia/issues/detail?id=5882
This missed the window of noon today because I didn't see your reply. I assume we need to punt this to 51 now?
Flags: needinfo?(rkothari)

Comment 16

10 months ago
(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.
Flags: needinfo?(rkothari)
(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
Flags: needinfo?(lsalzman)
(Assignee)

Comment 18

10 months ago
(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.
Flags: needinfo?(lsalzman)
(Assignee)

Comment 19

10 months ago
Created attachment 8804147 [details] [diff] [review]
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.
Attachment #8800921 - Attachment is obsolete: true
Attachment #8800921 - Flags: sec-approval?
Attachment #8804147 - Flags: review?(mchang)

Updated

10 months ago
Attachment #8804147 - Flags: review?(mchang) → review+
(Assignee)

Comment 20

10 months ago
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.
Attachment #8804147 - Flags: sec-approval?
(Assignee)

Comment 21

10 months ago
Created attachment 8804992 [details] [diff] [review]
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.
Attachment #8804992 - Flags: review+
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.
Attachment #8804147 - Flags: sec-approval? → sec-approval+

Updated

10 months ago
status-firefox50: affected → wontfix
Whiteboard: [gfx-noted] → [gfx-noted][checkin on 11/22]
[Tracking Requested - why for this release]: sec-critical
status-firefox53: --- → affected
tracking-firefox53: --- → ?
https://hg.mozilla.org/integration/mozilla-inbound/rev/55405164269f027b2e028d56b10c8aa5261b997f

Setting 50 back to affected to get it on the radar for 50.1 consideration.
status-firefox50: wontfix → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
Whiteboard: [gfx-noted][checkin on 11/22] → [gfx-noted]
Track 51+ as sec-critical.
tracking-firefox51: ? → +
Tracking 53+ due to it being sec-critical.
tracking-firefox53: ? → +
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.
Flags: needinfo?(lsalzman)
(Assignee)

Comment 28

9 months ago
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
Flags: needinfo?(lsalzman)
Attachment #8804992 - Flags: approval-mozilla-release?
Attachment #8804992 - Flags: approval-mozilla-beta?
(Assignee)

Comment 29

9 months ago
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
Attachment #8804147 - Flags: approval-mozilla-aurora?
This landed on m-c awhile ago, not sure how the bug didn't get marked.

https://hg.mozilla.org/mozilla-central/rev/55405164269f027b2e028d56b10c8aa5261b997f
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Attachment #8804992 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

9 months ago
Attachment #8804147 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/1258309afc85
status-firefox52: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/057879e7de42
status-firefox51: affected → fixed

Updated

9 months ago
tracking-firefox50: ? → +

Comment 34

9 months ago
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
Attachment #8804992 - Flags: approval-mozilla-release? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-release/rev/7ca25ca26db3
status-firefox50: affected → fixed
Whiteboard: [gfx-noted] → [gfx-noted][adv-main50.1+]
Alias: CVE-2016-9894
Group: gfx-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.