1072847 - Crash [@ _moz_cairo_surface_destroy] with long DOM Notification, only on Windows

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Benjamin Smedberg]

Crash at poisoned memory, I'm going to call this critical-uaf based on my first glance.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this crashing the nightly, or does it need to be asan build or some such?  I don't crash on my Windows 7 with 2014-09-25 nightly with this about:support graphics section:

Adapter Description	NVIDIA Quadro 600
Adapter Drivers	nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Adapter RAM	1024
ClearType Parameters	Gamma: 2200 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 50
Device ID	0x0df8
Direct2D Enabled	true
DirectWrite Enabled	true (6.2.9200.16571)
Driver Date	5-19-2014
Driver Version	9.18.13.3788
GPU #2 Active	false
GPU Accelerated Windows	1/1 Direct3D 11 (OMTC)
Subsys ID	083510de
Vendor ID	0x10de
WebGL Renderer	Google Inc. -- ANGLE (NVIDIA Quadro 600 Direct3D9Ex vs_3_0 ps_3_0)
windowLayerManagerRemote	true
AzureCanvasBackend	direct2d
AzureContentBackend	direct2d
AzureFallbackCanvasBackend	cairo
AzureSkiaAccelerated	0

Comment 4

[(Away)]

Crashed for me with Win7SP1 and 9-25.

Adapter Description: NVIDIA Quadro 600
Adapter Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Adapter RAM: 1024
ClearType Parameters: D [ Gamma: 1800 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 50 ] D [ Gamma: 1800 Pixel Structure: R ClearType Level: 100 Enhanced Contrast: 50 ]
Device ID: 0x0df8
Direct2D Enabled: true
DirectWrite Enabled: true (6.2.9200.16571)
Driver Date: 12-29-2012
Driver Version: 9.18.13.1090
GPU #2 Active: false
GPU Accelerated Windows: 1/1 Direct3D 11 (OMTC)
Subsys ID: 083510de
Vendor ID: 0x10de
WebGL Renderer: Google Inc. -- ANGLE (NVIDIA Quadro 600 Direct3D9Ex vs_3_0 ps_3_0)
windowLayerManagerRemote: true
AzureCanvasBackend: direct2d
AzureContentBackend: direct2d
AzureFallbackCanvasBackend: cairo
AzureSkiaAccelerated: 0

Comment 5

[(Away)]

Looks like our machines differ only on driver version, but before you ask me to update, we have reports of this on later versions as well.

https://crash-stats.mozilla.com/search/?signature=%3D_moz_cairo_surface_destroy&address=~5a5a5&_facets=signature&_facets=platform&_facets=platform_version&_facets=release_channel&_facets=adapter_vendor_id&_facets=build_id&_facets=adapter_driver_version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform

There's a clear beginning to this with build 20140920030206.

Comment 6

[(Away)]

Aha, all the app notes say: Attempt to create DrawTarget for invalid surface

So these surfaces were already previously destroyed by InitAlreadyReferenced from bug 1069584.

Comment 7

[(Away)]

We probably shouldn't let this sit too long...

Comment 8

[(Away)]

There was a large spike in these crashes with nightly build 20141011030203. In builds since then, this is the #2 crash on Win32, #1 on Win64.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Unsure this is graphics, but since this is urgent, and dealing with the time difference, Jeff will take a look as well.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

It looks like this is caused by trying to create D3D11 compositor for a very wide window and things going bad from there.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: Initialize mSurface

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8504817 [details] [diff] [review]
Initialize mSurface

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think very easily. This is basically using an initialized member. There are no vtables on the member so getting execution seems hard.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes.

Which older supported branches are affected by this flaw?

Probably all of them.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The backports would be easy.

How likely is this patch to cause regressions; how much testing does it need?

It should not cause regressions and shouldn't need very much testing.

Comment 13

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

Comment on attachment 8504817 [details] [diff] [review]
Initialize mSurface

Please check in on Oct 28 for trunk. Sec-approval+.

We'll want this on Aurora, Beta, and ESR31 at that point as well. Can you prepare and nominate patches?

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8504817 [details] [diff] [review]
Initialize mSurface

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Use after free, security hole.
[Describe test coverage new/current, TBPL]:
[Risks and why]: None, we're initializing uninitialized variable.
[String/UUID change made/needed]: None.

Same patch applies cleanly to Aurora and Beta.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Can't land until the 28th per comment 13. Also, needs an esr31 nomination still.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

My bad - I read comment 13 as "make sure this lands before or on Oct 28th", but I guess it means "please do not land before Oct 28th".

Comment 18

[(Away)]

It's unfortunate that the checkin has had to wait so long. Meanwhile this continues to be a topcrash on nightly and aurora. It's not clear to me why waiting is better in this instance.

Comment 19

[Andrew McCreight [:mccr8]]

It is almost the 28th now, so it doesn't matter much, but in the future if there's some stability concern you should mention that in the approval request, or followup with Al Billings or Dan Veditz to see if something could be landed sooner.  There wasn't anything in this bug prior to your comment that indicated this was a top crash.

Comment 20

[(Away)]

The topcrash is being tracked as the dependent bug 1071079. You're right that it's not obvious to someone just looking at this bug. I'll do better about calling attention to this kind of thing in the future.

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 8504817 [details] [diff] [review]
Initialize mSurface

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: sec-critical
Fix Landed on Version: not landed yet
Risk to taking this patch (and alternatives if risky): low, simple patch
String or UUID changes made by this patch: nope

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c53d6dbd263

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1c53d6dbd263

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/286954ccc3ae
https://hg.mozilla.org/releases/mozilla-beta/rev/d5a855ee2081

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/c5ffa5e63d99

Comment 26

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original crash several times on my Win 7 SP1 x64 (VM) and Win 8.1 x64 using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/10/2014-10-11-03-02-03-mozilla-central/

OS's used:

- Windows 8.1 x64
- Windows 7 SP1 x64 (VM)

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-24-03-02-07-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-24-00-40-01-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/34.0b11/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-24-00-05-01-mozilla-esr31/

Went through the following test cases for each OS/Build listed above:

- opened the poc in several different windows/tabs
- opened the poc in several different private windows/tabs
- opened the poc in several e10s windows/tabs