963962 - (CVE-2014-1528) crash in sse2_composite_src_x888_8888

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Jukka Jylänki]

1. Visit https://dl.dropboxusercontent.com/u/40949268/emcc/bugs/Tests_d_crashes/Tests_d.html

Observed: Web page crashes to desktop. See traces

https://crash-stats.mozilla.com/report/index/2f8f3cc4-f715-4961-835c-190282140125
https://crash-stats.mozilla.com/report/index/67e3f68d-8e07-4aa3-8f2d-171622140125

Reproduced on newest Macbook Pro line laptop running Windows 8.1 in bootcamp. Did not reproduce on a two year old Macbook Air line laptop running Windows 7 in bootcamp.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Jeff, how bad is this?

Comment 2

[Jukka Jylänki]

Hey, I think I might have been running this with the prefer-native-gl pref enabled, I can't remember for certain. That's worth checking out if the bug doesn't reproduce otherwise.

Comment 3

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

Jukka, can you confirm this with or without that pref enabled so we can know which it was?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Without being able to reproduce this it's hard to say much.

Comment 5

[Daniel Veditz [:dveditz]]

The crashes look exploitable as a best guess, without being able to repro.

Comment 6

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Tests_d.html

Comment 7

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Jukka - could you do us a favor and zip up the test file and all of its dependent files (JS, etc.) and attach them to this bug? The dropbox web site is very slow and sometimes inaccessible, and it would be useful for us to have the files available for us to stage ourselves. Thank you.

Comment 8

[Jukka Jylänki]

Hey thanks for poking with the needinfo - I had missed the earlier question, sorry Al!

I tested again on latest Nightly as of today and confirmed that webgl.prefer-native-gl is set to false. The crash still occurs - tested on both Nightly 64-bit and 32-bit on Windows. An offline .zip version is available for download at https://dl.dropboxusercontent.com/u/40949268/emcc/bugs/Tests_d_crashes/Tests_d_crashes.zip

Comment 9

[Jukka Jylänki]

On the same system:
  - an old Aurora 27.0a2 (2013-12-01) did not crash.
  - Firefox Stable 27.0.1 does not crash.
but
  - Aurora 29.0a2 (2013-03-04) does crash, and
  - Firefox Beta 28.0 does crash.

Comment 10

[Jukka Jylänki]

Crash reports:
 https://crash-stats.mozilla.com/report/index/4022df3a-0e40-4462-9170-e53422140305
 https://crash-stats.mozilla.com/report/index/b1fb8c34-ae62-4dfa-92b4-3b3c02140305
 https://crash-stats.mozilla.com/report/index/c998d5ce-5c8c-4328-8532-61feb2140305
 https://crash-stats.mozilla.com/report/index/01ab8af4-4b87-451e-b5a2-9b12f2140305

Comment 11

[Robert Kaiser]

Marking the affected versions with flags and requesting tracking given that dveditz says it's potentially exploitable in comment #5 and we know it's a 28 regression.

Comment 12

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Tests_d_crashes.zip

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

At this stage, we're about to go to final 28 beta and there's not even speculative work here.  As it's a high, not critical, wontfixing for 28.

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Went through following issue using build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/28.0b8/win32/en-US/

Using the proof of concept from comment #0, ran the following tests on actual Windows hardware:

- Lenovo X1 Carbon with Windows 8.1 -> no crashes (Passed)
- Surface Pro 2 with Windows 8.1 -> no crashes (Passed)

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

At this point, it appears that we probably need to configure a MacBook with dual boot Windows 8.1 in order to repro. This will take some time.

Comment 16

[Kamil Jozwiak [:kjozwiak]]

Matt, I'll do this sometime this weekend and will add the results.

Comment 17

[Kamil Jozwiak [:kjozwiak]]

I installed Windows 8.1 on OSX 10.9.2 using VMware Fusion. Windows 8.1 was fully updated and patched.

Used the following build for the verification process:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/28.0b9/win32/en-US/

- Loaded the website from comment #0 under Windows 8.1 in the OSX VMware Fusion VM without any issues (tried 10 times)
- Loaded the website from comment #0 under a dedicated Windows 8.1 Surface Pro 2 without any issues (tried 10 times)

I'm not sure if this is a problem, but under OSX, the "Blue" square doesn't load at the end of the test. The browser doesn't crash and completes downloading/loading but the blue square isn't appearing the same way it does under a dedicated Win 8.1 machine.

Comment 18

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Kamil. What we need is Windows 8.1 running via BootCamp. One of us will have to bite the bullet and install it. :)

Comment 19

[Robert Kaiser]

Note that this signature suddenly jumped up really much on Nightly over the weekend, see more reports at https://crash-stats.mozilla.com/report/list?signature=sse2_composite_src_x888_8888

The spike started 2014-03-08 so maybe some checkin on the 7th made us hit this more frequently - or is the spike a different bug?

Comment 20

[Tracy Walker [:tracy]]

This is startup crash that has jumped to #4 on Nightly.

Comment 21

[Benjamin Smedberg]

Tracy is going to check the per-build stats and build a regression range link for comment 19/20.

Comment 22

[Jukka Jylänki]

Retested this today with latest Nightly, some observations:
  - The tests_d.html crash rate is 100%, ran perhaps ~times.
  - Created a new profile with default settings to exclude any pref, but still crashes.
  - Checked that all add-ons are disabled when testing, does not relate to any of those (had geckoprofiler.xpi and IndexedDB Browser add-ons installed but both disabled while testing)

Also, the tests_d.html contains multiple unit tests, and I tried to track down the crash into a single test, but it looks like if I just run any single test in that suite, the crash does not occur. It is somehow caused by a sequence of multiple unit tests ran in order. I'll play with this more to find a minimal sequence of tests from tests_d.html that produces the crash.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

It seems to have happened in this range: 

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b0e7f63c2138&tochange=d01bf8596d3b

Jonathan it may be one of your changes.

Comment 24

[Jonathan Watt [:jwatt]]

It does seem likely that bug 979853 caused the increase in crashes, but bug 981020 should have fixed the issue there. I'm not seeing any crashes reported on crash-stats.mozilla.com with a build ID later than 2014030803, so hopefully that is the case.

Comment 25

[Robert Kaiser]

Comment #24 is probably right, I haven't look at by-buildid at this time, but I trust jwatt did get it correctly. And I see numbers go down in by-crash-date numbers as well.

Comment 26

[Jukka Jylänki]

I tested again with today's Nightly, but it still systematically crashes:
  -x64: https://crash-stats.mozilla.com/report/index/ed92c3b8-0b9a-4707-b34f-fef902140311
  -x86: https://crash-stats.mozilla.com/report/index/a2761743-42d8-42a5-b737-c9fe82140311

Comment 27

[Jonathan Watt [:jwatt]]

That's expected though, right? As in it consistently crashed for you before bug 979853 (presumably) caused the spike in crashes.

Comment 28

[Jukka Jylänki]

I've bisected this down to the following commit:

Jukka@GOZILLA /e/mozilla-central
$ hg bisect -b
The first bad revision is:
mchangeset:   152820:175163f053f7
user:        Jeff Gilbert <[e-mail]>
date:        Thu Oct 31 09:52:24 2013 -0400
summary:     Bug 924241 - Don't force-present after post-resize clear. r=bjacob

Does that help?

Comment 29

[Jukka Jylänki]

Stepping through the code, I see that cairo is painting out-of-bounds to the destination buffer in the compositing function. What happens is that the tests_d.html page resizes the canvas during its operation, and the new code added in the above commit ends up being called twice, first to resize from 640x480 to 621x480, and then a second time to resize from 621x480 to 621x449. After that the code goes to call gfxContext::Paint(gfxFloat alpha), which calls into cairo_paint_with_alpha(..), but in the cairo function, the paint rectangle size is still the old 640x480, and in the compositing loop, the a write to dst pointer gives the access violation. _Presumably_ the paint dst pointer is now allocated to smaller size (621x449 < 640x480) and the painting walks out of bounds past the end of the surface.

Perhaps there is some kind of cairo-related surface update/resize that's missing here? Jeff, what do you make of this?

Comment 30

[Kelsey Gilbert [:jgilbert]]

I can reproduce this, at least. Let me try to dig into this.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Ok, so this is what it looks like. We map into the CanvasLayer's D3D Texture, which has sizeA. But the frame we got from the producer is of sizeB. We're copying pixels as if sizeB is always equal to sizeA.

Indeed, I trip this assert that I added:

        DataSourceSurface* frameData = shareSurf->GetData();
        MOZ_ASSERT(frameData->GetSize().width == mBounds.width);
        MOZ_ASSERT(frameData->GetSize().height == mBounds.height);
        // Scope for DrawTarget, so it's destroyed before Unmap.

In `mapDt->DrawSurface`, we pass in `drawRect` (the rect we get from `frameData`) as both `aDest` and `aSrc`. We might want to fix this for correctness, but this shouldn't be the main bug.

Our bug should be above this, though, in the Factory::CreateDrawTargetForData line, where we wrap our mapped D3DTexture in a DrawTarget of the size of `frameData`. Oops.

Comment 32

[Kelsey Gilbert [:jgilbert]]

Attachment: patch ?: Try to setfault earlier and more predictably

We should consider taking something like this (sans printfs) so we catch this earlier. That said, it doesn't help me that might right now, since MSVC doesn't seem to pick up on the fault it generates. I'm guessing this is because we're in the middle of ASM.js stuff, and haven't reset our exception handlers.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Is my theory about ASM.js eating my segfault correct?

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Fix use of CreateDrawTargetForData in CanvasLayerD3D9/10

Comment 35

[Kelsey Gilbert [:jgilbert]]

With just patch 2, the testcase completes without crashing on my '13 retinabook pro running Win8.1, which can reproduce the testcase crashing on trunk.

Comment 36

[Luke Wagner [:luke]]

(In reply to Jeff Gilbert [:jgilbert] from comment #32)
I'm not sure exactly what you're asking.  The asm.js fault handler only "eats" (that is, safely resumes) faults that (1) come from within asm.js-generated code and (2) occur at known heap offsets, so likely it is not likely to be interacting here.

Comment 37

[Jukka Jylänki]

(In reply to Jeff Gilbert [:jgilbert] from comment #33)
> Is my theory about ASM.js eating my segfault correct?

While I was testing, I had the same issue that I could not at first get Visual Studio to break at the segfault. This was because the segfault is caught as a SEH exception in nsCrashOnException.cpp:35. To be able to break at the point where the segfault write occurred, one can 
   1. Attach Visual Studio to the process, and add a breakpoint to nsCrashOnException.cpp:35, inside the MOZ_SEH_EXCEPT block. Although this is a bit after-the-fact already.
   2. Possibly comment out the MOZ_SEH_TRY and MOZ_SEH_EXCEPT blocks in that file:line. I did not try this, but assuming there are no other exception handlers above in the call stack, that should work.
   3. Go to Debug->Exceptions..., and tick all the checkboxes in the "Thrown" column. (I think the actual exception is a Win32 Exception, but does not hurt to add them all)

I think that unconditional SEH try catch is a bit useless for debug builds and it trips up people trying to debug. I was scratching my head on this for a while, assuming at first that there was a C exit() being called somewhere as a response to the error, since Firefox quit so fast after the crash. Perhaps that try catch should be conditionally compiled out in "ac_add_options --enable-debug" builds? Would we ever send out crash reports from such builds?

Comment 38

[Luke Wagner [:luke]]

Whoa, we should definitely not be using SEH try/catch.  It's totally broken on Win64 when JITs are used so, with bug 844196, we'll probably switch breakpad to use "vectored exception handlers" (AddVectoredExceptionHandler) which are run before SEH even starts.  (Right now breakpad uses SetUnhandledExceptionFilter which runs *after* all SEH has been tried.)

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Jeff, sorry if I missed some information but is there a reason why the patch has not been checked in + uplifted? Thanks

Comment 40

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

No one has asked for sec-approval to check in, which would need to be done since this is a sec-high. I'm not sure why that hasn't happened yet.

Comment 41

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8390921 [details] [diff] [review]
patch 1: Fix use of CreateDrawTargetForData in CanvasLayerD3D9/10

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This should only be a vector, but I think it's pretty easy to use.

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?
29+

If not all supported branches, which bug introduced the flaw?
bug 979853

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

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

Comment 42

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

Comment on attachment 8390921 [details] [diff] [review]
patch 1: Fix use of CreateDrawTargetForData in CanvasLayerD3D9/10

Giving sec-approval+ for trunk. We should get branch patches.

You say this affects 29 and greater but the status flags say this is "won't fix" for 28, which means 28 is affected. Is 28 affected?

Comment 43

[Kelsey Gilbert [:jgilbert]]

(In reply to Al Billings [:abillings] from comment #42)
> Comment on attachment 8390921 [details] [diff] [review]
> patch 1: Fix use of CreateDrawTargetForData in CanvasLayerD3D9/10
> 
> Giving sec-approval+ for trunk. We should get branch patches.
> 
> You say this affects 29 and greater but the status flags say this is "won't
> fix" for 28, which means 28 is affected. Is 28 affected?

Yeah, my bad. It's affected from 28+.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8390921 [details] [diff] [review]
patch 1: Fix use of CreateDrawTargetForData in CanvasLayerD3D9/10

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac3e1179211

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Jeff, could you fill the uplift requests for aurora & beta? Thanks

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ac3e1179211

Comment 47

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8390921 [details] [diff] [review]
patch 1: Fix use of CreateDrawTargetForData in CanvasLayerD3D9/10

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 979853
User impact if declined: sec-high
Testing completed (on m-c, etc.): on m-c 
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/30c27cdcf400

This needs some branch-specific love on the beta patch.

Comment 49

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/635f912b3164