Closed Bug 1525815 (CVE-2018-18335) Opened 3 years ago Closed 3 years ago

Skia heap buffer overflow in SkiaGL

Categories

(Core :: Graphics, enhancement)

Unspecified
macOS
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox-esr60 65+ disabled
firefox65 --- disabled
firefox66 --- disabled
firefox67 --- disabled

People

(Reporter: dveditz, Assigned: lsalzman)

References

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: (Google CVE-2018-18335))

Attachments

(1 file)

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.

Lee: does this affect us?

Flags: needinfo?(lsalzman)

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.

OS: Unspecified → macOS
Summary: Skia heap buffer overflow → Skia heap buffer overflow in SkiaGL

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.

Flags: needinfo?(lsalzman)

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

Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #9042310 - Flags: sec-approval?
Attachment #9042310 - Flags: review?(rhunt)
Attachment #9042310 - Flags: approval-mozilla-esr60?
Assignee: lsalzman → nobody
Status: ASSIGNED → NEW
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #9042310 - Flags: review?(rhunt) → review+

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

Alias: CVE-2018-18335
Attachment #9042310 - Flags: sec-approval? → sec-approval+

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

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.
Attachment #9042310 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Group: gfx-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.