913805 - (CVE-2014-1557) Crash in [@ skia::`anonymous namespace''::ConvolveHorizontally<int>(unsigned char const*, skia::ConvolutionFilter1D const&, unsigned char*) ]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[blinky]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130906030202

Steps to reproduce:

1. Disable hardware acceleration.
2. Restart Firefox.
3. Go to http://dl.dropboxusercontent.com/u/95157096/85f61cf7/tgdvp4vc.jpg
4. Open a new tab.


Actual results:

Browser crash.
See http://s449.photobucket.com/user/movh/media/3pq52uw_zps09d07c82.mp4.html
Crash Report: bp-da1fdfd7-0c61-4dfb-9aa8-ab2f32130907

Comment 1

[blinky]

This happen since the version 19.0a1 (2012-10-18).

http://hg.mozilla.org/mozilla-central/rev/cb573b9307e5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0
ID: 20121018030618

Comment 2

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/a76c1f4c4112
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121017110913
Bad:
http://hg.mozilla.org/mozilla-central/rev/5142bbd4da12
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121017191029
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a76c1f4c4112&tochange=5142bbd4da12

Regression window(m-c)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/396742c90b0a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121017083113
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4eb7de485ee8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121017084912
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=396742c90b0a&tochange=4eb7de485ee8

Suspected: Bug 801358

FYI, It does not crash if image.high_quality_downscaling.enabled = false OR image.mem.discardable = false .

Comment 3

[Alice0775 White]

In local build with force image.high_quality_downscaling.enabled = true
Last Good: 0e3e42751195
First Bad: b0a701fd2322
Regressed by: 
	b0a701fd2322	Joe Drew — Bug 795940 - Part 0.1 - Allocate ScaleRequests dynamically and only hold on to a ScaleResult in RasterImage. Let ScaleRequests only hold a weak pointer to RasterImage so their lifetimes aren't bound. Finally, make ScaleRequests hold on to references to their surfaces instead of imgFrames. r=jlebar,jrmuizel

Comment 4

[Boris Zbarsky [:bzbarsky]]

This isn't a null-deref....

Comment 5

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

George, can you chase this one instead, perhaps with the help of the Skia team?

Comment 6

[Alex Keybl [:akeybl]]

We'll need a security rating prior to tracking this issue. ni? dveditz

Comment 7

[Daniel Veditz [:dveditz]]

I can't reproduce this on a Win8 machine. Is this specific to Win7 or am I just doing it wrong? I have
layers.acceleration.disabled  = true and both prefs mentioned in comment 2 set to true.

Hard to know how to rate without knowing what's going on. At 20,000 pixels wide we shouldn't be getting into integer overflow territory (unless there are some 16-bit ints in there that I didn't see). It's troubling that (according to the crash in comment 0) we seem to have the wrong size buffer. In that code we're only reading from it, but have we made the same wrong size calculation on the output buffer we're writing to?

At the very least we appear to be trying to incorporate random memory into the image and that's usually up in the sec-high territory by itself. If we're also/later writing into the buffer then it'd be sec-critical.

There are a lot of crashes at this signature in crash-stats, mostly read violations. The couple of writes I looked at gave strange data, seemed to be pointing at a write violation into the local stack-allocated accum variable. That doesn't make sense, is crash-stats lying or is there some hidden optimization/inlining making that code look nothing like what the source code says at that spot?

https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&signature=skia%3A%3A%60anonymous+namespace%27%27%3A%3AConvolveHorizontally%3Cint%3E%28unsigned+char+const*%2C+skia%3A%3AConvolutionFilter1D+const%26%2C+unsigned+char*%29

Comment 8

[Timothy Nikkel (:tnikkel)]

I don't know if this is related but bug 827946 was a crash on the same (or close) line but on Linux with gcc. I determined that it was a compiler problem by looking at the code and disassembly. The generated assembly was SSE2 and it read four 32 bit values from memory, only used the first 3, and crashed when the unused fourth was past the allocated memory boundary.

It's strange that there is a Windows crash that looks almost the same but with a totally different compiler. So perhaps I missed something in my analysis of that bug.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Can QA reproduct so we can get verification here if/when a fix is ready and perhaps even to give Dan a chance to see what's going on?

Comment 10

[u279076]

Ioana, can you please see if you can reproduce this based on comment 0?

Comment 11

[Ioana (away)]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 (20130923194050)

I reproduced this crash, with the same first 3 steps from in comment 0. It never reproduced for me by just opening a new tab, but it did reproduce while browsing through tabs. Sometimes just by moving to another tab, other times I had to browse around for a while before this happened. I always had to keep returning to the tab with the image for the crash to happen.

Comment 12

[u279076]

Current crash-stats:
> 24.0   #115 0.09%
> 25.0b  #139 0.08%
> 26.0a2 #185 0.06%
> 27.0a1 #134 0.11%

Comment 13

[u279076]

Current Ranking:
 * Release: #94  (0.11%)
 * Beta:    #149 (0.08%)
 * Aurora:  N/A
 * Nightly: N/A

It would appear as though this crash has all but disappeared on Nightly and Aurora and is stable on Beta and Release.

Comment 14

[u279076]

Current Ranking:
 * Release: #94  (0.10%)
 * Beta:    #162 (0.07%)
 * Aurora:  #130 (0.10%)
 * Nightly: N/A

This appears to be dropping off slightly.

Comment 15

[blinky]

Other steps to reproduce:

1. Disable hardware acceleration.
2. Restart Firefox.
3. Go to http://dl.dropboxusercontent.com/u/95157096/85f61cf7/cFMrlbcZ.html
4. Scroll to the bottom.

See http://s449.photobucket.com/user/movh/media/f5oA6tKp_zpsb8380bd4.mp4.html

Crash Report: bp-01528c6e-ee1b-4969-8461-506612131022

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

with such low crash rates, this isn't worth tracking but we can accept a low-risk fix for uplift if available.

Comment 17

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

George, any updates here?

Comment 18

[George Wright (:gw280) (needinfo me!)]

This appears to be fixed since landing bug 910754. I will look into it further.

Comment 19

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

George - fixed? Yes? :)

Comment 20

[George Wright (:gw280) (needinfo me!)]

Well, I can't reproduce it. Can QA verify please?

Comment 21

[Ioana (away)]

https://crash-stats.mozilla.com/report/index/ccc8db3c-0079-4bd6-807b-2d6d42140321

I'm reproducing this on Firefox 31.0a1 (20140321030203) with the same steps as in comment 11.

Comment 22

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

George, pleaase see the exciting news in comment 21, can you try to recreate again?

Comment 23

[George Wright (:gw280) (needinfo me!)]

So I've got what may be a related "issue" but it doesn't crash. Basically, following the steps in either comment 1 or comment 11, I sometimes find that the image disappears and Firefox shows a broken file icon instead, claiming the image data is corrupted. It may be related, so I'm going to go down that avenue to see if I can get a stacktrace and compare them.

That being said, if anyone can give me more details on reproducing it'd help a lot. Exactly which prefs have been changed from the default, the graphics card in the machine that can reproduce, the amount of RAM in the machine etc.

Comment 24

[blinky]

(In reply to George Wright (:gw280) from comment #23)
> That being said, if anyone can give me more details on reproducing it'd help
> a lot. Exactly which prefs have been changed from the default, the graphics
> card in the machine that can reproduce, the amount of RAM in the machine etc.

The crash happens with HWA disabled, Try the steps in comment 15.

Comment 25

[George Wright (:gw280) (needinfo me!)]

I've managed to reproduce it locally by running Windows inside a VM with 2GB RAM.

Quick status update:

- This is related to the high quality image scaling code that Joe integrated from Chromium which lives in image_operations.cpp/h and convolver.cpp/h in gfx/2d
- convolver and image_operations should be removed because they are part of Skia itself as SkConvolver and SkBitmapScaler respectively.
- I have a patch to use Skia's upstream implementations but this does not fix the issue, so I'm going to need to dig deeper. We should use Skia's ones anyway as we shouldn't be maintaining a fork ourselves.

Comment 26

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Remove-our-fork-of-SkConvolver-and-use-Skia-s-built-.patch

So switching to using Skia's built in stuff (which we probably ought to do anyway) causes this crash to not be reproducible anymore. We also get the added benefit of having access to the arch-optimised "convolution procs" for NEON/SSE/etc.

This currently results in an additional copy as SkBitmapScaler won't scale the bitmap directly into a memory buffer it doesn't own. I will fix that, but I'd like feedback first to see if this is an acceptable solution.

Comment 27

[George Wright (:gw280) (needinfo me!)]

Attachment: 0002-Fix-SkConvolver-so-that-it-builds-on-OS-X.patch

Trivial build fix for SkConvolver.cpp on OS X.

Comment 28

[George Wright (:gw280) (needinfo me!)]

Jeff, what's your opinion on how this will impact things, based on the fact that Chrome/Blink doesn't appear to use SkBitmapScaler, and it's potentially untested status? My personal feeling is that it's fine because the Skia code is based off the original Chrome code.

How would we test this to determine if it's acceptable?

Comment 29

[George Wright (:gw280) (needinfo me!)]

Related note: I'm speaking with upstream about being provided an acceptable public API for scaling images, as SkBitmapScaler is currently private.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8428045 [details] [diff] [review]
0001-Remove-our-fork-of-SkConvolver-and-use-Skia-s-built-.patch

Review of attachment 8428045 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/Scale.cpp
@@ +47,5 @@
> +                              dstWidth, dstHeight, convolveProcs)) {
> +    return false;
> +  }
> +
> +  return temp.copyPixelsTo(dstData, dstStride*dstHeight, dstStride, true);

It might be worth writing an implementation of SkBitmap::Allocator to pass into Resize which 'allocates' dstData rather than needing this extra copy.

High quality scaling is fairly performance critical as I understand it, and adding an extra allocation/memcpy into it isn't great.

Comment 31

[George Wright (:gw280) (needinfo me!)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #30)
> It might be worth writing an implementation of SkBitmap::Allocator to pass
> into Resize which 'allocates' dstData rather than needing this extra copy.

Yep, that's what I was considering doing to resolve the extra copy which I stated in comment 26 will be fixed before I deem this landable :)

Are you ok with the rest of the patch, conceptually?

Comment 32

[Matt Woodrow (:mattwoodrow)]

(In reply to George Wright (:gw280) from comment #31)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #30)
> > It might be worth writing an implementation of SkBitmap::Allocator to pass
> > into Resize which 'allocates' dstData rather than needing this extra copy.
> 
> Yep, that's what I was considering doing to resolve the extra copy which I
> stated in comment 26 will be fixed before I deem this landable :)
> 
> Are you ok with the rest of the patch, conceptually?

Ok, didn't read that far up the bug. Yes, the rest of the patch seems fine.

Comment 33

[George Wright (:gw280) (needinfo me!)]

Attachment: 0001-Hold-a-lock-on-the-RasterImage-in-ScaleRequest-so-th.patch

This bug is caused by RasterImage being seen as discardable because its lock count is zero. The scaling operation can take a very long time and if an event comes in that causes us to free up memory, the RasterImage is deleted and all its associated frames are too, causing Skia to blow up because it's in the middle of accessing that frame data on another thread.

The solution here is to make ScaleRequest hold a lock on RasterImage whilst its surfaces are "active" so that they don't go away underneath us in the middle of a scaling operation.

Comment 34

[George Wright (:gw280) (needinfo me!)]

I will file a separate bug to switch over to using the Skia downscaler

Comment 35

[u279076]

Updating status flags since this is definitely wontfix for 28 and likely wontfix for 29.

Comment 36

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8435976 [details] [diff] [review]
0001-Hold-a-lock-on-the-RasterImage-in-ScaleRequest-so-th.patch

Review of attachment 8435976 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 37

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8435976 [details] [diff] [review]
0001-Hold-a-lock-on-the-RasterImage-in-ScaleRequest-so-th.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not too easily. The code surrounding image scaling isn't straightforward to understand and whilst the patch does show where the lifetime of the image data is incorrectly managed, it doesn't indicate where the use-after-free occurs (which is only read rather than written).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I don't think so.

Which older supported branches are affected by this flaw?
Fx 30 onwards.

If not all supported branches, which bug introduced the flaw?
Adding the high quality image scaler which I believe was done in bug 486918

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Shouldn't cause any regressions, the testsuite on m-c should be enough.

Comment 38

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

Comment on attachment 8435976 [details] [diff] [review]
0001-Hold-a-lock-on-the-RasterImage-in-ScaleRequest-so-th.patch

Sec-approval+. I'd like to see this on Aurora, Beta, and ESR24 as well after it lands on Trunk.

Comment 39

[George Wright (:gw280) (needinfo me!)]

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

Comment 40

[George Wright (:gw280) (needinfo me!)]

Had to make some small changes to the patch as bug 994081 changed some of the code in RasterImage

Comment 41

[Carsten Book [:Tomcat]]

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

Comment 42

[George Wright (:gw280) (needinfo me!)]

Comment on attachment 8435976 [details] [diff] [review]
0001-Hold-a-lock-on-the-RasterImage-in-ScaleRequest-so-th.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 33
Risk to taking this patch (and alternatives if risky): sec-high bug goes unfixed. user crashes on low-memory computers possible.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 486918
User impact if declined: potential crashes and security issues with large images and low memory devices
Testing completed (on m-c, etc.): m-c testsuite
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e908afe16c31
https://hg.mozilla.org/releases/mozilla-beta/rev/e64edb137dca
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/728fb350f32e
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/4bd02a8597c9
https://hg.mozilla.org/releases/mozilla-esr24/rev/6c9957bb1f7a

Comment 44

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

Florin, if you have someone who can verify this tonight that would be awesome, but if not, we can try it for the next beta.

Comment 45

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I easily reproduced the issue on Windows 7 64bit, using Firefox 26.0, with both scenarios: from comment 0 and from comment 15 (could not reproduce on Win 8, Mac OS 10.9 and Linux 13.04).

I could not reproduce any of the scenarios (comment 0, comment 11, comment 15) on Firefox 31 Beta 3 build 1, latest Aurora or latest Nightly:
- User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
BuildID: 20140619131915
- User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
BuildID: 20140622004004
- User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
BuildID: 20140622030203

Note that trying to load image from comment 0 without previously having disabled hardware acceleration (and with a clean profile), will cause Firefox to crash during loading with signature: [@ OOM | large | mozalloc_abort(char const* const) | mozalloc_handle_oom(unsigned int) | moz_xmalloc | mozilla::gfx::ImageHalfScaler::ScaleForSize(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&) ]. This is bug 981323.

Comment 46

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-06-16-00-05-01-mozilla-esr24/

Used the following build during verification:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-07-14-00-05-00-mozilla-esr24/

Win 7 x64 - couldn't reproduce the orignal issue
OSX 10.9.4 x64 - couldn't reproduce the original issue

- Went through the test cases outlined in comment #0, comment #11 and comment #15 without any issues

Comment 47

[Daniel Veditz [:dveditz]]

Attachment: [e-mail], 3000, 2013-09-07, 2014-06-17, 2014-7-28, true