Closed Bug 1418447 (CVE-2018-5095) Opened 6 years ago Closed 6 years ago

Heap overflow write in SkEdgeBuilder::buildPoly

Categories

(Core :: Graphics, defect)

57 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ verified
firefox57 --- wontfix
firefox58 + verified
firefox59 + verified

People

(Reporter: abbaolk, Assigned: lsalzman)

References

Details

(Keywords: csectype-intoverflow, sec-high, Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage])

Attachments

(3 files)

Attached file 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)
Skia -> gfx -> Milan, can you help triage this? :-)
Group: firefox-core-security → core-security
Component: Untriaged → Graphics
Flags: needinfo?(milan)
Product: Firefox → Core
Fixed upstream?  If not, maybe we can put something in to just crash safely when it's too much.
Flags: needinfo?(milan) → needinfo?(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.
Oops, meant bug 1224396 there, not bug 1244396 (typo).
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.
Flags: needinfo?(dveditz)
See Also: → CVE-2017-5113
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.
Assignee: nobody → lsalzman
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(lsalzman)
Attachment #8930683 - Flags: review?(milan)
Depends on: CVE-2017-5113
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).
Group: core-security → gfx-core-security
Attachment #8930683 - Flags: review?(milan) → review+
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.
Attachment #8930683 - Flags: sec-approval?
This affects 52 ESR as well. However, the allocator interface changed (SkChunkAlloc vs SkArenaAlloc), so the patch must be slightly modified here for backporting.
Attachment #8931005 - Flags: review?(milan)
Attachment #8931005 - Flags: review?(milan) → review+
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.
(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.
Flags: needinfo?(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 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.
Attachment #8930683 - Flags: sec-approval? → sec-approval+
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
Attachment #8930683 - Flags: approval-mozilla-beta?
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.
Attachment #8931005 - Flags: approval-mozilla-esr52?
Attachment #8930683 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/a81b848ebcc6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: gfx-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8931005 [details] [diff] [review]
limit Skia edge builder allocations (52 ESR)

Sec-high, ESR52+
Attachment #8931005 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [adv-main58+][adv-esr52.6+]
Alias: CVE-2018-5095
Flags: qe-verify+
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
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?).
Flags: needinfo?(lsalzman)
Also the Skia revision where their fix landed: https://skia.googlesource.com/skia/+/29c14a760682e2c449fa043b5e8b69937cb58f3a
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?
Flags: needinfo?(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.
Flags: needinfo?(lsalzman)
Marking issue as verified as fixed based on the above comments.Thanks
Status: RESOLVED → VERIFIED
See Also: → CVE-2018-12371
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: