1496413 - (CVE-2018-18504) Investigate Skia crash on sueddeutsche.de with Firefox 59.0.1

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Julien Vehent]

+++ This bug was initially created as a clone of Bug #1496412 +++

X41 encountered a crash on sueddeutsche.de and reported it to us by email:

> We experienced a tab-crash of Firefox on one of our machines. The machine was
> running an unpatched version of Firefox (6 months old) in one-time Qubes VM
>(Qubes by default comes with an outdated Firefox on their Fedora containers...). 
> The crash is in libSkia / image rendering and looks very much like a failed 
> exploit attempt.
> What makes this so interesting for us is that it happened while browsing a 
> major German news site (sueddeutsche.de), and we are investigating if this was 
> a targeted attack against us, or an attack using ad-networks to exploit n-day 
> vulnerabilities in Firefox.
> Unfortunately the VM was already deleted before I was able to investigate the 
> crash further. The crash report is here:
> https://crash-stats.mozilla.com/report/index/c5f20570-4ca8-4b9d-a937-93f5d0180928

ni? :dveditz for triage

Comment 1

[Daniel Veditz [:dveditz]]

We've certainly fixed a ton of security bugs in Skia both before and after Firefox 59. This library is also used by Chrome.

Comment 2

[Daniel Veditz [:dveditz]]

Looks like SkSpriteBlitter_Src_SrcOver::blitRect was renamed to SkSpriteBlitter_Memcpy::blitRect in a Skia update we took in Firefox 60, but I still see crashes with essentially the same stack. When searching crash-stats look for the above in the "proto signature" because the actual signature is sometimes an unsymbolized library address like the libc-2.25.so@0x15ee8f in the original link here. I put a couple in the crash-signature field, but there are a couple per libc version and it's not worth listing them all.

Comment 3

[Lee Salzman [:lsalzman]]

Ryan, if you look at the stacks for SkSpriteBlitter_Memcpy, many of them look like the same strange OMTP issue with getting a corrupt Skia surface. Can you take a look?

Comment 4

[Maire Reavy [:mreavy]]

Adding Jessie (new graphics engineering manager) to all sec-crit and sec-high graphics bugs

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

The crashes with the PaintThread in the stack are almost always on a CopySurface command. Usually with a read violation, but sometimes with write violation.

Looking at all the callers of CopySurface, I only see two that can be used with a DrawTargetCapture and they're both in our tiling code. One of the calls appears to use a source texture client that could be deleted before the command is executed on the paint thread [1]. We're supposed to preserve the texture clients by adding them to the paint task, but it looks like this site was missed.

Here's a try run with a patch to fix this. [2]

[1] https://searchfox.org/mozilla-central/rev/8f0db72fb6e35414fb9a6fc88af19c69f332425f/gfx/layers/client/SingleTiledContentClient.cpp#204
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3504d0df618fce7e675ff7382f11bb7573e91306

Comment 6

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: pass-texture-clients.patch

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 9031159 [details] [diff] [review]
pass-texture-clients.patch

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Not easily. The issue is that the buffer of a texture client might be freed while it's used as the source of a copy. Which could lead to corruption in the destination buffer (which should be valid) or an access violation. If there's an exploit, it's not immediately apparent to me.

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?: Release, Beta, Nightly

If not all supported branches, which bug introduced the flaw?: Probably 1478815

Do you have backports for the affected branches?: Yes

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

How likely is this patch to cause regressions; how much testing does it need?: Not likely. It just keeps alive a texture longer.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. Please create and nominate a beta patch as well.

You say this doesn't affect ESR60 at all?

Comment 9

[Ryan Hunt [:rhunt] (on leave until early May)]

The crashes in `SkSpriteBlitter_Memcpy` that this patch is trying to fix were most likely introduced by bug 1478815, which landed in 63. So ESR60 should be unaffected.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8b861da55b65ab0641d0ab39f0a380686a1d7c
https://hg.mozilla.org/mozilla-central/rev/7a8b861da55b

Comment 11

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to Beta as-landed. Please nominate the patch for approval when you get a chance.

Comment 12

[Ryan Hunt [:rhunt] (on leave until early May)]

Comment on attachment 9031159 [details] [diff] [review]
pass-texture-clients.patch

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1478815

User impact if declined: Crashes, and potential reads from freed buffers

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

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): This change only adds some more textures to be kept alive longer during an async paint.

String changes made/needed:

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9031159 [details] [diff] [review]
pass-texture-clients.patch

[Triage Comment]
Fixes potentially exploitable crashes by holding onto objects a bit longer. Approved for 65.0b6.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/774f84eedb22