Closed Bug 1496413 (CVE-2018-18504) Opened 2 years ago Closed 2 years ago

Investigate Skia crash on sueddeutsche.de with Firefox 59.0.1

Categories

(Core :: Graphics, defect)

59 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- wontfix
firefox65 + fixed
firefox66 + fixed

People

(Reporter: jvehent, Assigned: rhunt)

References

Details

(Keywords: csectype-bounds, regression, sec-high, Whiteboard: [post-critsmash-triage][adv-main65+])

Crash Data

Attachments

(1 file)

+++ 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
Flags: needinfo?(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.
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.
Crash Signature: [@ libc-2.25.so@0x15ee8f ] [@ memcpy | SkSpriteBlitter_Src_SrcOver::blitRect ] [@ memcpy | SkSpriteBlitter_Memcpy::blitRect ] [@ libc-2.25.so@0x1620ad ]
Flags: needinfo?(dveditz)
QA Contact: wleung
Group: core-security → core-security-release
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?
Flags: needinfo?(rhunt)
Component: Security → Graphics
Summary: Investigate crash on sueddeutsche.de with Firefox 59.0.1 → Investigate Skia crash on sueddeutsche.de with Firefox 59.0.1
Assignee: nobody → rhunt
Adding Jessie (new graphics engineering manager) to all sec-crit and sec-high graphics bugs
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
Flags: needinfo?(rhunt)
Attachment #9031159 - Flags: review?(nical.bugzilla)
Attachment #9031159 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #9031159 - Flags: sec-approval?
sec-approval+ for trunk. Please create and nominate a beta patch as well.

You say this doesn't affect ESR60 at all?
Flags: needinfo?(rhunt)
Attachment #9031159 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(rhunt)
This grafts cleanly to Beta as-landed. Please nominate the patch for approval when you get a chance.
Flags: needinfo?(rhunt)
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:
Flags: needinfo?(rhunt)
Attachment #9031159 - Flags: approval-mozilla-beta?
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.
Attachment #9031159 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Blocks: 1478815
Keywords: regression
Alias: CVE-2018-18504
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.