Daniel Veditz [:dveditz] Reporter
	Description • 6 years ago

This was reported by Ivan Fratric of Google Project Zero as
https://bugs.chromium.org/p/chromium/issues/detail?id=848716

At the time it was reported he thought Firefox was safe because we didn't use GPU acceleration in 2D by default. I don't know if that's still true or if we have plans to change it in the future. It may be safer to cherry pick these integer overflow fixes now rather than wait for our next Skia update. Last I heard that wasn't going to be any time soon.

Lee: have we enabled Skia gpu acceleration anywhere or have plans to do so?

The patch is https://skia.googlesource.com/skia/+/d5b4593024544c3405615066aa5b4f94352eb3cb

	Lee Salzman [:lsalzman]
	Comment 1 • 6 years ago

We currently don't enable GPU canvas2d by default on any platform. This change was made in bug 1468801 in version 62. Prior to that bug, we had it enabled by default on Mac and Android. Our goal is to eventually just try to remove even the ability to turn it on (remove the code).

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 2 • 6 years ago

(In reply to Lee Salzman [:lsalzman] from comment #1)
> Prior to that bug, we had it enabled by default on Mac and Android.

Does that mean ESR60 is affected by this issue?

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 3 • 6 years ago

The upstream patch almost applies cleanly to ESR60, but needs a bit of rebasing.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 4 • 6 years ago

Per the comments in this bug, I only worried about ESR60 for this one.

Green on Try:
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=cebc4f551706831a4dcd1a09b2b954dd83f815e7

	Daniel Veditz [:dveditz] Reporter
	Comment 5 • 6 years ago

Setting tracking flags for the current release pending the answer to comment 2. Better to over-track than to lose track.

	Ryan Hunt [:rhunt] (on leave until early May)
	Comment 6 • 6 years ago

The patch in bug 1468801 only flipped the prefs for SkiaGL in canvas off, it didn't remove support. So it's possible for users to have flipped on the prefs (before or after 62/63) and still be using SkiaGL.

I can confirm on nightly 64.0a1 on OSX that I can enable SkiaGL in canvas by setting gfx.canvas.azure.accelerated;true.

I don't know what the policy for uplifting for non-default configurations are, but I could imagine some users flipping the pref to make some canvas apps faster.

In the referenced chrome issue I see that chrome has a memory limit that prevents this from being triggered, I'm unsure if we have anything similar. I couldn't find anything in PathBuilderSkia or CanvasRenderingContext2D from a quick search.

With all that being said, the patch is sane and if rebasing it wasn't difficult I'm fine with uplifting it.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 7 • 6 years ago

Comment on attachment 9017304 [details] [diff] [review]
Backport the upstream patch (ESR60 only)

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: I honestly have no idea.

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?: All, those ESR60 primarily since that's where it's on by default still

If not all supported branches, which bug introduced the flaw?: N/A

Do you have backports for the affected branches?: Yes

If not, how different, hard to create, and risky will they be?: N/A

How likely is this patch to cause regressions; how much testing does it need?: It's green on Try and is a straight backport of upstream code with minor alteration, not sure what other testing I can reasonably hope to do here.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 8 • 6 years ago

Might as well take it everywhere, though like you said, ESR60 is the most affected since that's the only place SkiaGL for canvas is still on by default.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 9 • 6 years ago

Comment on attachment 9017304 [details] [diff] [review]
Backport the upstream patch (ESR60 only)

[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: 

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

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): 

String changes made/needed:

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 10 • 6 years ago

Comment on attachment 9017304 [details] [diff] [review]
Backport the upstream patch (ESR60 only)

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

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): 

String changes made/needed:

	Al Billings [:abillings - ex-MoCo]
	Comment 11 • 6 years ago

Comment on attachment 9017304 [details] [diff] [review]
Backport the upstream patch (ESR60 only)

Approvals given.

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 12 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/86f5694c6c50

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 13 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-release/rev/30bb4357d4e3
https://hg.mozilla.org/releases/mozilla-esr60/rev/23b23e12c548

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 14 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/86f5694c6c50

	Daniel Veditz [:dveditz] Reporter
	Comment 15 • 6 years ago

Google assigned CVE-2018-16070 to this one.

	Bogdan Maris, Desktop QA
	Comment 16 • 6 years ago

Is there something that manual QA help here? If so can you provide some guidance or steps?

	Ryan VanderMeulen [:RyanVM] Assignee
	Comment 17 • 6 years ago

I don't think there's anything to verify with respect to the sec fix itself. About the only useful thing we could test here would be making sure canvas on 60.3esr OSX (the one place this code is on by default) doesn't have any glaring issues.