1525815 - (CVE-2018-18335) Skia heap buffer overflow in SkiaGL

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Daniel Veditz [:dveditz]]

The Chromium stable release post for Chrome 71 references a Heap buffer overflow in Skia without much detail other than its bug number (895362) and CVE-2018-18335. It was apparently bad enough to be worth a $5000 bounty.

https://chromereleases.googleblog.com/2018/12/stable-channel-update-for-desktop.html
https://bugs.chromium.org/p/chromium/issues/detail?id=895362

I found a merge-to-71 cherry pick patch in skia/src/gpu/GrBufferAllocPool.cpp. We have that file in our tree but I don't know if we use that part of Skia. I also don't know if other patches were part of this -- we should check the commits for other references to 895362

https://skia.googlesource.com/skia/+/7469a9341afab19271b8ef07af5c16a0f2c4ccc1

For now I'll rate it as the Chrome team did, assuming we also use it.

Comment 1

[Daniel Veditz [:dveditz]]

Lee: does this affect us?

Comment 2

[Daniel Veditz [:dveditz]]

Via IRC Lee says:

that's a bug affecting SkiaGL which is prefed off on most releases
the only place it is actually prefed on by default still is Mac on 60 ESR

I'm trying to get more info from the Chrome team about this bug but we might still need to take it for ESR.

Comment 3

[Daniel Veditz [:dveditz]]

Another fix suggested by Lee would be to simply disable SkiaGL on ESR, which is what we've done for Release so we know it works fine.

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: deprecate SkiaGL for Canvas2D (60 ESR)

Security Approval Request

How easily could an exploit be constructed based on the patch?

This just disables the Canvas2D acceleration feature that exposes the buggy code in the first place, so unlikely.

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

No

Which older supported branches are affected by this flaw?

only 60 ESR

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

None

Do you have backports for the affected branches?

Yes

If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Already used in several release cycles where we disabled Skia acceleration for Canvas2D in bug 1468801. So this is really only just an uplift of the fix we made in that bug. Only will affect Mac and no other platforms.

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

User impact if declined

Fix Landed on Version

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

String or UUID changes made by this patch

Comment 5

[Daniel Veditz [:dveditz]]

Confirming the patch in comment 0 is what Chrome used, but turning the whole thing off (as this patch does) is even better.

Comment 6

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

Giving sec-approval+ even though this is really ESR only...

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9042310 [details] [diff] [review]
deprecate SkiaGL for Canvas2D (60 ESR)

Disables SkiaGL on ESR60 to match the behavior on other releases already. Approved for 60.5.1esr.

Comment 8

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/b17f7fff04e1a3882bc11bbe296eccc3d6d18200