1418447 - (CVE-2018-5095) Heap overflow write in SkEdgeBuilder::buildPoly

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[abbaolk]

Attachment: poc.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171115095126

Steps to reproduce:


System: Ubuntu 16.04.3 LTS (64bit)
Firefox version: 57.0 (64-bit)
RAM size:16376108 kB(16G) 


Root cause:

Int overflow could happen there:
https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkEdgeBuilder.cpp#251
    int maxEdgeCount = path.countPoints();
    if (iclip) {
        // clipping can turn 1 line into (up to) kMaxClippedLineSegments, since
        // we turn portions that are clipped out on the left/right into vertical
        // segments.
        maxEdgeCount *= SkLineClipper::kMaxClippedLineSegments; // Int overflow
    } 
     size_t edgeSize = fAnalyticAA ? sizeof(SkAnalyticEdge) : sizeof(SkEdge);
    char* edge = fAnalyticAA ? (char*)fAlloc.makeArrayDefault<SkAnalyticEdge>(maxEdgeCount)
                             : (char*)fAlloc.makeArrayDefault<SkEdge>(maxEdgeCount);


When "maxEdgeCount" become negative,  "fAlloc.makeArrayDefault" will alloc much less memory than needed:
 makeArrayDefault(size_t count) {
        uint32_t safeCount = SkTo<uint32_t>(count); // incorrect conversion


Reproduce:
To trigger the vulnerability, we need to make the path have  about 0x80000000/3 points, which would consume about 6.1G RAM.
Hence the vulnerability can only be fast triggered on a computer with at least 8GB RAM.

Open attached poc.html, wait for about 30 seconds or less, the tab will crash.

With gdb attached and  firefox-dbg installed, we can see the stack backtrace:
(gdb) bt 8
#0  SkEdgeBuilder::CombineVertical (this=0x7ffd64103020, edge=0x7f1087747218, last=0x25700000000)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkEdgeBuilder.cpp:22
#1  0x00007f10a80b24b5 in SkEdgeBuilder::buildPoly (this=this@entry=0x7ffd64103020, path=..., 
    iclip=iclip@entry=0x7ffd64102f80, shiftUp=shiftUp@entry=2, canCullToTheRight=canCullToTheRight@entry=true)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkEdgeBuilder.cpp:284
#2  0x00007f10a80b642e in SkEdgeBuilder::build (this=this@entry=0x7ffd64103020, path=..., 
    iclip=iclip@entry=0x7ffd64102f80, shiftUp=shiftUp@entry=2, canCullToTheRight=<optimized out>, 
    analyticAA=analyticAA@entry=false)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkEdgeBuilder.cpp:352
#3  0x00007f10a811d3b4 in sk_fill_path (path=..., clipRect=..., blitter=blitter@entry=0x7ffd641031b0, start_y=-150, 
    stop_y=150, shiftEdgesUp=shiftEdgesUp@entry=2, pathContainedInClip=false)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkScan_Path.cpp:405
#4  0x00007f10a7e850a1 in SkScan::AntiFillPath (path=..., origClip=..., blitter=<optimized out>, 
    blitter@entry=0x7ffd64103cf8, forceRLE=forceRLE@entry=false)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkScan_AntiPath.cpp:726
#5  0x00007f10a7e855c8 in SkScan::AntiFillPath (path=..., clip=..., blitter=0x7ffd64103cf8)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkScan_AntiPath.cpp:783
#6  0x00007f10a801df10 in SkDraw::drawDevPath (this=this@entry=0x7ffd64104ac0, devPath=..., paint=..., 
    drawCoverage=drawCoverage@entry=false, customBlitter=customBlitter@entry=0x0, doFill=doFill@entry=true)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkDraw.cpp:1070
#7  0x00007f10a80203c0 in SkDraw::drawPath (this=this@entry=0x7ffd64104ac0, origSrcPath=..., origPaint=..., 
    prePathMatrix=prePathMatrix@entry=0x0, pathIsMutable=<optimized out>, pathIsMutable@entry=false, 
    drawCoverage=drawCoverage@entry=false, customBlitter=0x0)
    at /build/firefox-HLWaua/firefox-57.0+build4/gfx/skia/skia/src/core/SkDraw.cpp:1163

In function "SkEdgeBuilder::Combine SkEdgeBuilder::CombineVertical(const SkEdge* edge, SkEdge* last) ", "last" points to invalid memory.(First be read and then be written)

Comment 2

[:Gijs (he/him)]

Skia -> gfx -> Milan, can you help triage this? :-)

Comment 3

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

Fixed upstream?  If not, maybe we can put something in to just crash safely when it's too much.

Comment 4

[Lee Salzman [:lsalzman]]

Not fixed upstream, also kind of a mess because this is somewhat related to bug 1224396. This one triggers at a lower tolerance, though, to the point where it is probably somewhat feasible, but as noted in the summary, you do need to have a machine with so much memory you don't OOM first before you hit this one, though this will trigger at a lower threshold than bug 1244396.

Comment 5

[Lee Salzman [:lsalzman]]

Oops, meant bug 1224396 there, not bug 1244396 (typo).

Comment 6

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

We have just over 11% of users with 8GB or more, so not insignificant.  Dan, is there an upstream Skia/Google security bug for this? The change would be in that code, and the uplifting is easier if they're driving the bug, even if we have a local patch.

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: limit Skia edge builder allocations

This fix isn't ideal, but we are hedged in by the fact that SkEdgeBuilder::buildPoly() must return an int here. We need to verify that maxEdgeCount won't overflow that int, so we just have a release assert that verifies it won't. This uses a release assert to follow the precedent set by Skia upstream in bug 1224396.

This is also dependent on the fix in bug 1224396, where Skia upstream already fixed the SkArenaAlloc::makeArrayDefault() calls to do release asserts if further multiplying the count by element size would cause an overflow.

Comment 8

[abbaolk]

In fact, 4GB memory + enough swap(usually the same size of RAM) is enough for trigger of the vulnerability, except not that fast(several minutes maybe).

Comment 9

[Lee Salzman [:lsalzman]]

Comment on attachment 8930683 [details] [diff] [review]
limit Skia edge builder allocations

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

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

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
All currently supported release branches.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
52 ESR would require some slight revising of the patch, but there should be no risk to doing it.

> How likely is this patch to cause regressions; how much testing does it need?
Just sets limits on allocations - using release asserts - that would otherwise cause overflows in sizes specified to memory allocations (and thus cause crashes or worse anyway), but otherwise doesn't change semantics.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: limit Skia edge builder allocations (52 ESR)

This affects 52 ESR as well. However, the allocator interface changed (SkChunkAlloc vs SkArenaAlloc), so the patch must be slightly modified here for backporting.

Comment 11

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

I'm trying to figure out a security rating for this issue, since it needs one before it can have security approval. 

Based on comments here, it is hard to tell how exploitable the crash is.

Comment 12

[Lee Salzman [:lsalzman]]

(In reply to Al Billings [:abillings] from comment #11)
> I'm trying to figure out a security rating for this issue, since it needs
> one before it can have security approval. 
> 
> Based on comments here, it is hard to tell how exploitable the crash is.

Related bug 1224396 was originally sec-moderate, but upgraded to sec-high, if that helps.

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Milan Sreckovic [:milan] from comment #6)
> Dan, is there an upstream Skia/Google security bug for this? The change would be
> in that code, and the uplifting is easier if they're driving the bug, even
> if we have a local patch.

I couldn't find one, but I can't see Skia security bugs and can only see Chrome sec bugs just before they're released. The Chrome bugs can include Skia bugs if they were found by or reported to the Chrome team (e.g. through their bug bounty program).

Comment 14

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

Comment on attachment 8930683 [details] [diff] [review]
limit Skia edge builder allocations

sec-approval+ for trunk. Let's get this nominated for beta as well and get the tweaked version of the patch nominated for ESR52.

Comment 15

[Lee Salzman [:lsalzman]]

Comment on attachment 8930683 [details] [diff] [review]
limit Skia edge builder allocations

Approval Request Comment
[Feature/Bug causing the regression]: Pre-existing condition affecting all release channels.
[User impact if declined]: Memory overrun exploitable via normal web pages or canvas.
[Is this code covered by automated tests?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: Just prevents overflow from occuring with an assert, where a security vulnerability + crash would normally happen anyway.
[String changes made/needed]: none

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 8931005 [details] [diff] [review]
limit Skia edge builder allocations (52 ESR)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high memory overrun
User impact if declined: Memory overrun exploitable via web pages or canvas. 
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/49281ca9bc9c

Comment 20

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

Comment on attachment 8931005 [details] [diff] [review]
limit Skia edge builder allocations (52 ESR)

Sec-high, ESR52+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ce6f3fb2bf58

Comment 22

[Daniel Veditz [:dveditz]]

Lee: did you report this to the upstream Skia folks? If so please add a link to their bug in the "See Also" field (if not how do we preserve this fix so future Skia updates don't stomp it?).

Comment 23

[Lee Salzman [:lsalzman]]

Fixed in upstream Skia: https://bugs.chromium.org/p/skia/issues/detail?id=7391

Comment 24

[Lee Salzman [:lsalzman]]

Also the Skia revision where their fix landed: https://skia.googlesource.com/skia/+/29c14a760682e2c449fa043b5e8b69937cb58f3a

Comment 25

[Gabi Cheta [:Gabi] Desktop QA]

The tab crashes for oom when opening the above attachment on Ubuntu 16.4 16GB memory using Fx 58.0b16 and the latest nightly build. Is there anything that I've might missed that will help in verifying this issue?

Comment 26

[Lee Salzman [:lsalzman]]

Nope, it's supposed to do that, as the patch adds a release assert that will be hit either hit, or run out of memory, whichever comes first. This is the alternative to leaving a sec exploit in place.

Comment 27

[Gabi Cheta [:Gabi] Desktop QA]

Marking issue as verified as fixed based on the above comments.Thanks