Skia heap use-after-freed in SkPath::addPath

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
10 months ago
3 months ago

People

(Reporter: dveditz, Assigned: RyanVM)

Tracking

({csectype-uaf, sec-high})

unspecified
mozilla64
Points:
---

Firefox Tracking Flags

(firefox-esr6063+ fixed, firefox62 wontfix, firefox63+ fixed, firefox64+ fixed)

Details

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

Attachments

(1 attachment)

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
AFAICT, ESR60 is also affected.
Attachment #9017303 - Flags: review?(lsalzman) → review+
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 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+
https://hg.mozilla.org/mozilla-central/rev/399335d39d4c
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Group: gfx-core-security → core-security-release
Whiteboard: [adv-main63+][adv-esr60.3+]

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]

(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.