1535518 - Possible out of bounds read in Skia

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

There is a skia bug mentioned in latest stable chrome release notes [1]:

$N/A][883596] Medium CVE-2019-5798: Out of bounds read in Skia. Reported by Tran Tien Hung (@hungtt28) of Viettel Cyber Security on 2018-09-13

That's https://bugs.chromium.org/p/chromium/issues/detail?id=883596 which is private but Dan has access to. From irc Dan thought the patch be:

https://skia.googlesource.com/skia/+/f026d896dce856dd3c757c4c341b2df6876e1d28

We already have this patch (bug 1502152)

Dan also thought this patch might be the test patch:
https://skia.googlesource.com/skia/+/fb5f43b09fc6194f097c5818158055ed9b74c498

Dveditz, need info for you to confirm since you have access. If its definately the first patch above, then we already have the fix. But need you to confirm.

[1] https://chromereleases.googleblog.com/2019/03/stable-channel-update-for-desktop_12.html

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

FWIW, Imean to link to the patched line in searchfox above: https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/core/SkPath.cpp#1720

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Assuming that's the right fix, ESR60 has the exact same code still.
https://searchfox.org/mozilla-esr60/source/gfx/skia/skia/src/core/SkPath.cpp#1679

Comment 3

[Lee Salzman [:lsalzman]]

Attachment: Bug 1535518 - Preserve fLastMoveToIndex in SkPath::transform r?rhunt

Comment 4

[Daniel Veditz [:dveditz]]

This chrome bug was fixed by a series of fixes. We picked up two out of three pieces in bug 1498701 (referencing crbug 882423). This third bit was originally fixed based on a fuzz tests the Skia team runs as https://skia-review.googlesource.com/c/skia/+/154625 (the patch you link in comment 0) and then when crbug 883596 was reported they found they had a fix for it already. That's apparently why this bug got a CVE (external reporter) but did not get a chrome bounty (the "[$N/A]" bit) because it was an already known bug.

As Ryan points out we don't have this fix on ESR-60 because that branch is not on m71. We did land the bug 1498701 fixes there though, so we still need this one.

Comment 5

[Lee Salzman [:lsalzman]]

Comment on attachment 9051792 [details]
Bug 1535518 - Preserve fLastMoveToIndex in SkPath::transform r?rhunt

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bounds vulnerability already disclosed by Skia/Chromium upstream.
• User impact if declined:
• Fix Landed on Version: 65
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Fix already in 65 release as of bug 1502152.
• String or UUID changes made by this patch:

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Am I understanding this correctly from comment 4, that it's only esr60 affected and there's nothing left to do for other branches?

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9051792 [details]
Bug 1535518 - Preserve fLastMoveToIndex in SkPath::transform r?rhunt

While this is sec-moderate, let's take the fix on ESR since the vulnerability is already public.

Comment 8

[Lee Salzman [:lsalzman]]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #6)

Am I understanding this correctly from comment 4, that it's only esr60 affected and there's nothing left to do for other branches?

Correct.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/9f059761b1df