Closed Bug 1237171 Opened 6 years ago Closed 6 years ago

crash in skia::ConvolveHorizontally_SSE2 when open certain icon images

Categories

(Core :: ImageLib, defect)

46 Branch
x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox43 --- unaffected
firefox44 + verified
firefox45 + fixed
firefox46 + verified
firefox-esr38 --- unaffected
firefox-esr45 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: alice0775, Assigned: n.nethercote)

References

()

Details

(5 keywords)

Crash Data

Attachments

(3 files)

This bug was filed from the Socorro interface and is 
report bp-59a9cd92-e368-416b-9ece-e41082160106.
=============================================================

Firefox crashed.
See http://forums.mozillazine.org/viewtopic.php?p=14465705


Reproducible: 100%

Steps to Reproduce:
1. Go to https://duckduckgo.com/?q=how+to+hide+%2Btrending+google+plus&t=ffab&ia=social
2. Now edit the edit box to make the search string like this : how to hide +trending +"google plus" (thus, simply adding the "" around the google plus string).
3. Click the search button again, and scroll down using the mousewheel.

OR
1. Open URL
2. Scroll down using the mousewheel

Actual results:
Crash
Regression pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=92407172ab7ce2cdcbbd8e8d54fcc22c90abcb17&tochange=609fa3c015842fa5bb9592e590164ea79088d247

Regressed by: Bug 1204394
Blocks: 1204394
Flags: needinfo?(seth)
Flags: needinfo?(n.nethercote)
Flags: needinfo?(gerv)
Component: Graphics → ImageLib
Attached file reduced html
STR
1. Clear cache
2. Open index.html
3. (optionally) Clear cache and then Reload(F5)
Crashed on Ubuntu
bp-7d153fe2-d005-499e-9659-2afe52160106
OS: Windows 7 → All
Summary: crash in skia::ConvolveHorizontally_SSE2 → crash in skia::ConvolveHorizontally_SSE2 when open certain icon images
Alice: how exactly do you think I can help with this bug?

Gerv
(In reply to Gervase Markham [:gerv] from comment #4)
> Alice: how exactly do you think I can help with this bug?
> 
> Gerv

At least, I think that the bunch of offending patch should be packed out from beta and aurora channel.
Gerv's involvement in bug 1204394 was minor. I will take a look at this crash tomorrow.
Flags: needinfo?(seth)
Flags: needinfo?(n.nethercote)
Flags: needinfo?(gerv)
I can reproduce the crash. The test case can be reduced down to just the www.papervisions.com.ico image.

In a debug build there's an assertion failure instead:

> 68	  MOZ_ASSERT(mTargetSize.width <= aOriginalSize.width,
> (gdb) bt
> #0  0x00007fffe4cbc28a in mozilla::image::Downscaler::BeginFrame (
>     this=0x7fffcbcb9c18, aOriginalSize=..., aFrameRect=..., 
>     aOutputBuffer=0x7fffcbcbac00 "", aHasAlpha=false, aFlipVertically=true)
>     at /home/njn/moz/mi7/image/Downscaler.cpp:68
> #1  0x00007fffe4cf72d0 in mozilla::image::nsBMPDecoder::ReadBitfields (
>     this=0x7fffcbcb9c00, aData=0x7fffcbcba228 "", aLength=0)
>     at /home/njn/moz/mi7/image/decoders/nsBMPDecoder.cpp:669
> #2  0x00007fffe4d06a6e in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0::operator()(mozilla::image::nsBMPDecoder::State, char const*, unsigned long) const (this=0x7fffd0193318, 
>     aState=mozilla::image::nsBMPDecoder::State::BITFIELDS, 
>     aData=0x7fffcbcba228 "", aLength=0)
>     at /home/njn/moz/mi7/image/decoders/nsBMPDecoder.cpp:439
> #3  0x00007fffe4cf5e39 in mozilla::image::StreamingLexer<mozilla::image::nsBMPDecoder::State, 16ul>::Lex<mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0>(char const*, unsigned long, mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0) (this=0x7fffcbcb9d70, 
>     aInput=0x7fffcbcba228 "", aLength=0, aFunc=...)
>     at ../../../image/StreamingLexer.h:343
> #4  0x00007fffe4cf55e7 in mozilla::image::nsBMPDecoder::WriteInternal (
>     this=0x7fffcbcb9c00, aBuffer=0x7fffcbcba200 "(", aCount=40)
>     at /home/njn/moz/mi7/image/decoders/nsBMPDecoder.cpp:433
> #5  0x00007fffe4cb93a4 in mozilla::image::Decoder::Write (
>     this=0x7fffcbcb9c00, aBuffer=0x7fffcbcba200 "(", aCount=40)
>     at /home/njn/moz/mi7/image/Decoder.cpp:183
Assignee: nobody → n.nethercote
Group: core-security
It looks like this can cause dangerous writes to memory. In the instance I just caught in my debugger it wrote to 0xe5e5e5e5e5e5e5e5.
There's no test because this still causes assertions in debug builds :/
Attachment #8705402 - Flags: review?(tnikkel)
Attachment #8705402 - Flags: review?(tnikkel) → review+
Here's my understanding of the regression.

The ICO dirEntry says the image is 0x0, which is ICO-speak for 256x256. (Yes
really! See nsICODecoder::GetReal{Height,Width}().) At this stage, we try the
ICO dirEntry, so RasterImage::mSize is set to 256x256, via the PostSize() call
in nsICODecoder.

Then we set up the Downscaler using RasterImage::mSize (256x256) size and a
target size of 16x16, which seems ok.

But then we start decoding the BMP, which is truly 0x0, and call
Downscaler::BeginFrame(), passing it 0x0 for the size. It sanity checks 0x0
against the previously specified target size of 16x16 and asserts because we
shouldn't try to downscale a 0x0 image to a target size of 16x16.

Ideally, we'd completely ignore the ICO dirEntry size and always work with the
BMP size. (Indeed, later on we override the former with the latter; see
nsICODecode::FixBitMap{Width,Height}().) But it's hard to get the BMP size
early because that would hurt the speed of metadata-only decodes. This came up
in a similar context in https://bugzilla.mozilla.org/show_bug.cgi?id=1223319#c7.

We really need a general solution to the ICO-size-doesn't-match-BMP-size
problem; it's come up in several bugs now. But I don't know what that solution
is, so in the meantime I'm doing a simple work-around that fixes the regression
introduced by bug 1204394.
https://hg.mozilla.org/integration/mozilla-inbound/rev/edf79b699755aaa8f0da7850ff0b2cccd9135c4e
Bug 1237171 - Improve a case where ICO and BMP files disagree on an image size. r=tn.
This bug needs sec-approval filled out because it is a sec-high or sec-critical bug that affects more than trunk.
Flags: needinfo?(n.nethercote)
Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

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

No idea. But writes to unaddressable memory are generally dangerous, and it's easy to craft a BMP that triggers this behaviour.

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

The log message is vague. The patch has a comment explaining the existence of an exceptional case, but says nothing about the consequence of failing to handle that exceptional case.

> Which older supported branches are affected by this flaw?

It's on Aurora and Beta. It hasn't made it to the Release channel yet.

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

Bug 1204394.

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

The same patch should apply without problem to Aurora and Beta.

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

Very unlikely to cause regressions. It's trivial.
Flags: needinfo?(n.nethercote)
Attachment #8705402 - Flags: sec-approval?
Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Approval Request Comment
[Feature/regressing bug #]: bug 1204394.

[User impact if declined]: crashes, potential security flaws.

[Describe test coverage new/current, TreeHerder]: no test coverage, unfortunately, because the triggering test case causes a (semi-related) assertion failure in debug builds.

[Risks and why]: low risk. The patch is trivial.

[String/UUID change made/needed]: none.
Attachment #8705402 - Flags: approval-mozilla-beta?
Let's fix this before we ship the problem. Approvals given.
Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Does this patch apply to Aurora? I don't see a nomination for that.
Attachment #8705402 - Flags: sec-approval?
Attachment #8705402 - Flags: sec-approval+
Attachment #8705402 - Flags: approval-mozilla-beta?
Attachment #8705402 - Flags: approval-mozilla-beta+
Attachment #8705402 - Flags: approval-mozilla-aurora?
> Does this patch apply to Aurora? I don't see a nomination for that.

It does. I thought I nominated but I must have misclicked.
Can I land this on Aurora/Beta immediately, or do I have to wait? (I can never remember the drill with security bugs, sorry.)
https://hg.mozilla.org/mozilla-central/rev/edf79b699755
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Just like for beta.
(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Comment on attachment 8705402 [details] [diff] [review]
> Improve a case where ICO and BMP files disagree on an image size
> 
> Just like for beta.

caused bustage and got backed out in https://treeherder.mozilla.org/logviewer.html#?job_id=725251&repo=mozilla-beta
Flags: needinfo?(n.nethercote)
Here's a trivial change to the patch to make it compile on Beta. Apologies for
the bustage!
Comment on attachment 8706669 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

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

Re-requesting approval for Beta; the other patch had a trivial compile error due to minor branch differences.
Attachment #8706669 - Flags: approval-mozilla-beta?
Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Clearing Beta approval for the patch that doesn't build on Beta.
Flags: needinfo?(n.nethercote)
Attachment #8705402 - Flags: sec-approval+
Attachment #8705402 - Flags: approval-mozilla-beta+
Tomcat: mccr8 suggested that I don't bother re-getting Beta approval, and that you should be able to land the new patch on Beta immediately. I'll let you decide if you're comfortable with that :)
Flags: needinfo?(cbook)
Group: core-security → core-security-release
I confirmed that the first patch (the one currently nominated for Aurora approval) applies and builds correctly on Aurora.
Flags: qe-verify+
(In reply to Nicholas Nethercote [:njn] from comment #25)
> Tomcat: mccr8 suggested that I don't bother re-getting Beta approval, and
> that you should be able to land the new patch on Beta immediately. I'll let
> you decide if you're comfortable with that :)

yeah sure mccr8 is right as always :) landed in https://hg.mozilla.org/releases/mozilla-beta/rev/72ae67129ad7
Flags: needinfo?(cbook)
Reproduced the crash on 44.0b4 → bp-8a9ad111-1263-4806-9c6f-7723a2160112, under Windows 7 64-bit.
Verified fixed with latest Nightly 46.0a1 (from 2016-01-11), across platforms [1] - no crash encountered with any of the provided STR.

Although this fix shows up in 44 beta 8 pushlog, I suspect it didn’t get into this one because it's still crashing, across platforms - E.g.: bp-3876af75-f841-41f0-bb08-5ca3f2160112

Could you please confirm?

[1] Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.5
Status: RESOLVED → VERIFIED
(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #29)
> Reproduced the crash on 44.0b4 → bp-8a9ad111-1263-4806-9c6f-7723a2160112,
> under Windows 7 64-bit.
> Verified fixed with latest Nightly 46.0a1 (from 2016-01-11), across
> platforms [1] - no crash encountered with any of the provided STR.
> 
> Although this fix shows up in 44 beta 8 pushlog, I suspect it didn’t get
> into this one because it's still crashing, across platforms - E.g.:
> bp-3876af75-f841-41f0-bb08-5ca3f2160112
> 
> Could you please confirm?
> 
> [1] Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.5

Hi Alexandra,

It might show up because it was shortly in and then got backedout (comment #21) for causing build bustage, so the fix landed in #comment 28 is not on beta 8
Hi Tomcat, I am confused. Does the patch need beta+? Based on some of the comments, it seems it already landed on m-b. Right?
Flags: needinfo?(cbook)
(In reply to Ritu Kothari (:ritu) from comment #31)
> Hi Tomcat, I am confused. Does the patch need beta+? Based on some of the
> comments, it seems it already landed on m-b. Right?

It did land; the revised version was simple enough that Tomcat landed without waiting for a second approval. But if you (or someone else) could re-give approval just for completeness, that would be good.

Also, the other patch still needs Aurora approval and landing. It should be a rubber stamp given that it's almost identical to the already-landed beta patch.

Thank you.
Comment on attachment 8706669 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Yes, this was landed on beta per comment 28.
Flags: needinfo?(cbook)
Attachment #8706669 - Flags: approval-mozilla-beta?
Comment on attachment 8706669 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

[Triage Comment]
This appears to be what landed on beta: restoring flags
Attachment #8706669 - Flags: approval-mozilla-beta+
Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

a=dveditz for aurora (and restoring sec-approval)
Attachment #8705402 - Flags: sec-approval+
Attachment #8705402 - Flags: approval-mozilla-aurora?
Attachment #8705402 - Flags: approval-mozilla-aurora+
Verified fixed with 44.0b9 (Build ID: 20160114165817), across platforms [1].

[1] Mac OS X 10.10.4, Windows 7 64-bit and Ubuntu 12.04 32-bit
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.