Closed Bug 913805 (CVE-2014-1557) Opened 11 years ago Closed 10 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

19 Branch
x86_64
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 + wontfix
firefox26 - wontfix
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- verified
firefox32 --- verified
firefox33 + verified
firefox-esr17 --- unaffected
firefox-esr24 --- verified
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: over68, Assigned: gw280)

References

Details

(4 keywords, Whiteboard: [adv-main31+][adv-esr24.7+])

Crash Data

Attachments

(3 files)

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
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
Severity: normal → critical
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 .
Blocks: 801358
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Keywords: crash, reproducible
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
Blocks: 795940
No longer blocks: 801358
Component: General → ImageLib
This isn't a null-deref....
Group: core-security
Flags: needinfo?(joe)
Flags: needinfo?(joe) → needinfo?(jmuizelaar)
George, can you chase this one instead, perhaps with the help of the Skia team?
Assignee: nobody → gwright
Flags: needinfo?(jmuizelaar) → needinfo?(gwright)
We'll need a security rating prior to tracking this issue. ni? dveditz
Flags: needinfo?(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
Crash Signature: [@ skia::`anonymous namespace''::ConvolveHorizontally<int>(unsigned char const*, skia::ConvolutionFilter1D const&, unsigned char*) ]
Flags: needinfo?(dveditz)
Keywords: sec-high
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.
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?
Ioana, can you please see if you can reproduce this based on comment 0?
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.
QA Contact: ioana.budnar
Keywords: qawanted
Current crash-stats:
> 24.0   #115 0.09%
> 25.0b  #139 0.08%
> 26.0a2 #185 0.06%
> 27.0a1 #134 0.11%
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.
Current Ranking:
 * Release: #94  (0.10%)
 * Beta:    #162 (0.07%)
 * Aurora:  #130 (0.10%)
 * Nightly: N/A

This appears to be dropping off slightly.
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
with such low crash rates, this isn't worth tracking but we can accept a low-risk fix for uplift if available.
George, any updates here?
Group: gfx-core-security
This appears to be fixed since landing bug 910754. I will look into it further.
Flags: needinfo?(gwright)
George - fixed? Yes? :)
Flags: needinfo?(gwright)
Well, I can't reproduce it. Can QA verify please?
Flags: needinfo?(gwright)
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.
George, pleaase see the exciting news in comment 21, can you try to recreate again?
Flags: needinfo?(gwright)
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.
Flags: needinfo?(over68)
Flags: needinfo?(ioana_damy)
Flags: needinfo?(gwright)
(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.
Flags: needinfo?(over68)
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.
Flags: needinfo?(ioana_damy)
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.
Attachment #8428045 - Flags: review?(matt.woodrow)
Trivial build fix for SkConvolver.cpp on OS X.
Attachment #8428046 - Flags: review?(matt.woodrow)
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?
Flags: needinfo?(jmuizelaar)
Related note: I'm speaking with upstream about being provided an acceptable public API for scaling images, as SkBitmapScaler is currently private.
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.
Attachment #8428046 - Flags: review?(matt.woodrow) → review+
(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?
(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.
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.
Flags: needinfo?(jmuizelaar)
Attachment #8435976 - Flags: review?(seth)
I will file a separate bug to switch over to using the Skia downscaler
Updating status flags since this is definitely wontfix for 28 and likely wontfix for 29.
Attachment #8428045 - Flags: review?(matt.woodrow)
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.
Attachment #8435976 - Flags: review?(seth) → review+
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.
Attachment #8435976 - Flags: sec-approval?
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.
Attachment #8435976 - Flags: sec-approval? → sec-approval+
Had to make some small changes to the patch as bug 994081 changed some of the code in RasterImage
https://hg.mozilla.org/mozilla-central/rev/0f2821706cdb
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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
Attachment #8435976 - Flags: approval-mozilla-esr24?
Attachment #8435976 - Flags: approval-mozilla-beta?
Attachment #8435976 - Flags: approval-mozilla-aurora?
Attachment #8435976 - Flags: approval-mozilla-esr24?
Attachment #8435976 - Flags: approval-mozilla-esr24+
Attachment #8435976 - Flags: approval-mozilla-beta?
Attachment #8435976 - Flags: approval-mozilla-beta+
Attachment #8435976 - Flags: approval-mozilla-aurora?
Attachment #8435976 - Flags: approval-mozilla-aurora+
Keywords: verifyme
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.
Flags: needinfo?(florin.mezei)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
Keywords: verifyme
Group: gfx-core-security
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
QA Whiteboard: [qa!]
Whiteboard: [adv-main31+][adv-esr24.7+]
Alias: CVE-2014-1557
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Depends on: 1050815
Group: core-security → core-security-release
Group: core-security-release
Flags: sec-bounty-hof+
You need to log in before you can comment on or make changes to this bug.