1525433 - (CVE-2019-5785) Stack buffer overflow in Skia reported by Ivan Fratric of Google Project Zero

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:

Google Project Zero has a post today about a stack buffer (and other memory) overwrite bug in Skia convexity calculation that was reachable from Chrome:
https://googleprojectzero.blogspot.com/2019/02/the-curious-case-of-convexity-confusion.html

The blog post references two patches:
https://skia.googlesource.com/skia/+/26d8d77aae56c20b7174ac06056c1e5ec7903e6b
https://skia.googlesource.com/skia/+/db80cbe34ad7ea44aea86e47d76034cfa716afd7

The first has a test but was insufficient. I haven't checked to see if it was backed out or simply had the second bit added.

They reference this currently hidden chrome bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=899689

Which was fixed as part of Chrome 72 announced last week:
https://chromereleases.googleblog.com/2019/01/stable-channel-update-for-desktop.html (no CVE listed)

Marked blocking for releases since this is a public vulnerability. Might even be worth a 65 point release or ride-along to make sure it's not a gimme for Pwn2Own.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

I was planning a 65.0.1 build early next week, can definitely take this as a ride-along if needed. Is ESR60 also affected?

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: check path convexity after transform

This incorporates the following upstream fixes:

https://skia.googlesource.com/skia/+/07105bbcbe9840f4e797bd914bef8a90c2cd21f9%5E%21/
https://skia.googlesource.com/skia/+/497b3680a9084881bbe88e36b3a9d6164a2b6c78%5E%21/#F0
https://skia.googlesource.com/skia/+/db80cbe34ad7ea44aea86e47d76034cfa716afd7%5E%21/

I stripped out some of the incriminating comments from the patches for now.

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: check path convexity after transforms (60ESR)

Same patch, just backported to ESR60, which this also seems to affect.

Comment 4

[Lee Salzman [:lsalzman]]

Comment on attachment 9041621 [details] [diff] [review]
check path convexity after transform

Security Approval Request

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

The vulnerability is already disclosed upstream for Skia and Chrome. Tried to scrub any comments that would help identify the vulnerability.

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?

60+

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?

Upstream reported that in some cases this might cause a small performance hit, but otherwise if this is not addressed there is an overflow vulnerability that can be exploited via canvas 2d, etc. This is already incorporated into upstream Skia/Chrome, so theoretically they have already tested it.

Comment 5

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

Comment on attachment 9041621 [details] [diff] [review]
check path convexity after transform

sec-approval+ for trunk. We'll want this on affected branches too.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/f8e19c32d3dd480c166cef39b24ed352b986195e

Please nominate this for Beta/Release/ESR60 approval as soon as you can.

Comment 7

[Lee Salzman [:lsalzman]]

Comment on attachment 9041621 [details] [diff] [review]
check path convexity after transform

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

Disclosed vulnerability

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

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

Already tested upstream

String changes made/needed

Comment 8

[Lee Salzman [:lsalzman]]

Comment on attachment 9041625 [details] [diff] [review]
check path convexity after transforms (60ESR)

ESR Uplift Approval Request

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

User impact if declined

Disclosed vulnerability

Fix Landed on Version

Risk to taking this patch

Low

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

Upstream fix

String or UUID changes made by this patch

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9041621 [details] [diff] [review]
check path convexity after transform

[Triage Comment]
Fixes a publicly-disclosed Skia vulnerability. Approved for 66.0b6.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bf7ac7d809cf

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9041621 [details] [diff] [review]
check path convexity after transform

[Triage Comment]
Fixes a publicly-disclosed Skia sec bug. Approved for 65.0.1 and 60.5.1esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/50a9cd9af1e7

Comment 14

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/1f0a03852bb67a6a48c60b99beced88f699431c1

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Lee Salzman [:lsalzman] from comment #7)

Is this code covered by automated tests?

Yes

Needs manual test from QE?

No

Setting this as qe-verify- based on comment 7.

Comment 16

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.