Closed Bug 1498460 Opened 10 months ago Closed 10 months ago

Multiple integer overflows in Skia GPU path rendering when computing vertex/idex count

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- unaffected
firefox63 + fixed
firefox64 + fixed

People

(Reporter: dveditz, Assigned: RyanVM)

References

Details

(Whiteboard: [adv-esr60.3+][Google CVE-2018-16070])

Attachments

(1 file)

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
Flags: needinfo?(lsalzman)
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).
Flags: needinfo?(lsalzman)
(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?
Flags: needinfo?(lsalzman)
The upstream patch almost applies cleanly to ESR60, but needs a bit of rebasing.
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
Assignee: nobody → ryanvm
Attachment #9017304 - Flags: review?(lsalzman)
Setting tracking flags for the current release pending the answer to comment 2. Better to over-track than to lose track.
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.
Flags: needinfo?(lsalzman)
Attachment #9017304 - Flags: review?(lsalzman) → review+
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.
Attachment #9017304 - Flags: sec-approval?
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.
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:
Attachment #9017304 - Flags: approval-mozilla-esr60?
Attachment #9017304 - Flags: approval-mozilla-beta?
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:
Attachment #9017304 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Comment on attachment 9017304 [details] [diff] [review]
Backport the upstream patch (ESR60 only)

Approvals given.
Attachment #9017304 - Flags: sec-approval?
Attachment #9017304 - Flags: sec-approval+
Attachment #9017304 - Flags: approval-mozilla-release?
Attachment #9017304 - Flags: approval-mozilla-release+
Attachment #9017304 - Flags: approval-mozilla-esr60?
Attachment #9017304 - Flags: approval-mozilla-esr60+
https://hg.mozilla.org/mozilla-central/rev/86f5694c6c50
Group: gfx-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Whiteboard: [adv-esr60.3+]
Google assigned CVE-2018-16070 to this one.
Whiteboard: [adv-esr60.3+] → [adv-esr60.3+][Google CVE-2018-16070]
Is there something that manual QA help here? If so can you provide some guidance or steps?
Flags: needinfo?(ryanvm)
Flags: needinfo?(dveditz)
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.
Flags: qe-verify-
Flags: needinfo?(ryanvm)
Flags: needinfo?(dveditz)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.