Closed
Bug 1498701
Opened 6 years ago
Closed 6 years ago
Skia heap use-after-freed in SkPath::addPath
Categories
(Core :: Graphics, defect)
Core
Graphics
Tracking
()
RESOLVED
FIXED
mozilla64
People
(Reporter: dveditz, Assigned: RyanVM)
References
Details
(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main63+][adv-esr60.3+][Google CVE-2018-18343])
Attachments
(1 file)
2.91 KB,
patch
|
rhunt
:
review+
abillings
:
approval-mozilla-release+
abillings
:
approval-mozilla-esr60+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
From Chrome bug https://bugs.chromium.org/p/chromium/issues/detail?id=882423 fixed in Chrome 69 (no regression range, but patch appears to apply to our version). I'm removing the sourcecode links because they weren't tied to a version and therefore don't point at the correct code anyway. The patches make it clear. VULNERABILITY DETAILS There is a heap use-after-free in Skia at SkPath::addPath() [...] Root cause: "RawIter iter(path);" variable at line [...] has pointer to the element "fConicWeights" of "path". If "path" is the same object with current SkPath, it could call "conicTo" function, trigger "SkPathRef::Editor::growForVerb" function and realloc "fConicWeights" buffer. But after realloc pointer of "fConicWeights" in "iter" have not been updated. That lead to heap use-after-free. This pattern may exist in some others place. REPRODUCTION CASE Build Skia program with ASAN ============================ #include "SkCanvas.h" #include "SkPath.h" int main (int argc, char * const argv[]) { SkRect rect = SkRect::MakeEmpty(); SkPath path; path.addOval(rect, SkPath::kCCW_Direction); path.addPath(path, 0.707106781f, 0.414213562f); return 0; } The Chrome folks rated this high, but weren't 100% sure it was reachable from web content or used by Chrome itself: > I believe Chrome could have run afoul of this bug. > > Well, NewTabButton::GetHitTestMask gets a path from > NewTabButton::GetBorderPath (which can add conics to the returned > path) and then calls addPath - adding the returned path to its mask > path. AFAICT that mask path is always empty though. So, although > Chrome comes close, I don't think it would actually trigger this bug. > > Still, there is nothing preventing Chrome from using SkPath in a manner > that would trigger this bug. > > Someone more familiar with Chrome might be able to provide more insight. In addition to patching addPath they found a similar issue in reverseAddPath https://skia.googlesource.com/skia/+/c3d8a48f1b27370049aa512019cd726c59354743 https://skia.googlesource.com/skia/+/8051d38358293df1e5b8a1a513f8114147ec9fa3
Assignee | ||
Comment 1•6 years ago
|
||
AFAICT, ESR60 is also affected.
status-firefox62:
--- → wontfix
status-firefox63:
--- → affected
status-firefox64:
--- → affected
status-firefox-esr60:
--- → affected
tracking-firefox63:
--- → +
tracking-firefox64:
--- → +
tracking-firefox-esr60:
--- → 63+
Assignee | ||
Comment 2•6 years ago
|
||
Green on Try. 64: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=c1705a90f47141dea9d3bee7ab2b544669d411ef 63: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=6771c38d533b19ac36d7833ac612508b876155ea ESR60: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=1a994860ea0c9fc0afd8b5ed0becb68647a1c819
Assignee: nobody → ryanvm
Attachment #9017303 -
Flags: review?(lsalzman)
Updated•6 years ago
|
Attachment #9017303 -
Flags: review?(lsalzman) → review+
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 9017303 [details] [diff] [review] Backport the upstream patches [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: [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: [Security Approval Request] How easily could an exploit be constructed based on the patch?: No clue, but the upstream commit is public knowledge. 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 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?: Green on Try, not sure what more we can realistically do.
Attachment #9017303 -
Flags: sec-approval?
Attachment #9017303 -
Flags: approval-mozilla-release?
Attachment #9017303 -
Flags: approval-mozilla-esr60?
Comment 4•6 years ago
|
||
Comment on attachment 9017303 [details] [diff] [review] Backport the upstream patches Approvals given.
Attachment #9017303 -
Flags: sec-approval?
Attachment #9017303 -
Flags: sec-approval+
Attachment #9017303 -
Flags: approval-mozilla-release?
Attachment #9017303 -
Flags: approval-mozilla-release+
Attachment #9017303 -
Flags: approval-mozilla-esr60?
Attachment #9017303 -
Flags: approval-mozilla-esr60+
Assignee | ||
Comment 5•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/399335d39d4c
Assignee | ||
Comment 6•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/ea3b45ed3e04 https://hg.mozilla.org/releases/mozilla-esr60/rev/9461988ff462
Assignee | ||
Comment 7•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/399335d39d4c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee | ||
Updated•6 years ago
|
Group: gfx-core-security → core-security-release
Updated•6 years ago
|
Whiteboard: [adv-main63+][adv-esr60.3+]
Reporter | ||
Comment 8•5 years ago
|
||
The Chrome sec team assigned CVE-2018-18343 to this Skia issue.
Whiteboard: [adv-main63+][adv-esr60.3+] → [adv-main63+][adv-esr60.3+][Google CVE-2018-18343]
Reporter | ||
Comment 9•5 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #0)
fixed in Chrome 69
This was not announced until Chrome 71. I think they decided not to uplift to 69 and let it ride their trains:
https://chromereleases.googleblog.com/2018/12/stable-channel-update-for-desktop.html
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•