The default bug view has changed. See this FAQ.

Crash on websites with ico/favicon containing EXIF data in its bitmaps

RESOLVED FIXED in Firefox 15

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: s.lange, Assigned: bbondy)

Tracking

(4 keywords)

10 Branch
mozilla17
crash, icon, regression, reproducible
Points:
---

Firefox Tracking Flags

(firefox14 affected, firefox15 verified, firefox16 verified, firefox17 verified)

Details

(crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
Created attachment 638300 [details]
ratgeberebookseu.zip => Logo PNG image from which the favicon was generated + the favicon that leads to the firefox crash

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0.1
Build ID: 20120614114901

Steps to reproduce:

Open Website http://www.ratgeberebooks.eu or only the favicon
http://www.ratgeberebooks.eu/WebRoot/Store18/Shops/63630130/favicon.ico


Actual results:

Firefox crashes with bug report window.
(Reporter)

Comment 1

5 years ago
Crash report is: https://crash-stats.mozilla.com/report/index/35196c16-5fcd-472f-ae1a-bf8c02120702

Reproduceable under Windows 7 Prof. 64bit, Fedora Linux 17.

---

Notice that the favicon was generated from the Logo PNG image file with image magick. I think the issue might be some remaining EXIF data in the bitmap inside the ico. We had to call ImageMagick's $Image->Strip(); method to get rid of the metadata, then it works. Though firefox shoulnd't crash.
Keywords: icon
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 2

5 years ago
I can reproduce on Nightly: bp-34153b3b-af5c-45bd-b0eb-eaca22120702.
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::image::Decoder::HasError()]
Component: Untriaged → ImageLib
Ever confirmed: true
Keywords: crash, reproducible
Product: Firefox → Core
QA Contact: untriaged → imagelib
Version: 13 Branch → Trunk
(Assignee)

Updated

5 years ago
Assignee: nobody → netzen
(Assignee)

Updated

5 years ago
Blocks: 682677
(Assignee)

Comment 3

5 years ago
Created attachment 638573 [details] [diff] [review]
Patch v1

This bug was introduced in bug 682677, it is due to not saving the jump buffer before a call that can longjump.
Attachment #638573 - Flags: review?(joe)
(Assignee)

Comment 4

5 years ago
After review and landing I'll be requesting that this goes on Aurora and Beta.

Updated

5 years ago
status-firefox14: --- → affected
status-firefox15: --- → affected
Keywords: regression
Version: Trunk → 10 Branch
I think that the existing code and the v1 patch will leak the
mPNG struct; it probably needs png_destroy_read_struct() both inside
the setjmp block and in the "return false" block.
Also, IsValidICO should return false if png_width > 256
or png_height > 256.
The sample PNG is colortype 6 and has 10 text chunks but no EXIF data.
The converted favicon.ico has three PNG files (16x16, 32x32, and 64x64,
all colortype 3 (indexed)).  So the issue is apparently simply that
they are indexed PNGs, which are rejected by IsValidICO().
The ICO definition presented at wikipedia says nothing about
limiting the PNG to 8 bit, colortype 6.

The blog referenced in a comment in nsPNGDecoder.h says

"The format of a PNG-compressed image consists simply of a PNG image, starting with the PNG file signature. The image must be in 32bpp ARGB format (known to GDI+ as Pixel­Format­32bpp­ARGB). There is no BITMAP­INFO­HEADER prefix, and no monochrome mask is present."

I interpret that to mean that the PNG decoder must return a 32-bit ARGB image.  Which ours always does via png_set_expand and GFX_PACKED_PIXEL calls within nsPNGDecoder.cpp, no matter what the actual PNG colortype and bit depth.

IsValidICO() is looking at the PNG header data before these conversions have happened.
(Assignee)

Comment 8

5 years ago
> I think that the existing code and the v1 patch will leak the
> mPNG struct; it probably needs png_destroy_read_struct() both 
> inside the setjmp block and in the "return false" block.

No a call to png_destroy_read_struct will already happen when the mContainedDecoder is freed inside ~nsPNGDecoder.

> Also, IsValidICO should return false if png_width > 256
> or png_height > 256.

Although I agree with this assertion, it is not related to this bug.  Please post a followup bug and I can work on it there.  There are checks for the size inside the ICO decoder by the way already which is the only place this is used. 

> The sample PNG is colortype 6 and has 10 text chunks but no EXIF data.
> The converted favicon.ico has three PNG files (16x16, 32x32, and 64x64,
> all colortype 3 (indexed)).  So the issue is apparently simply that
> they are indexed PNGs, which are rejected by IsValidICO().

The issue is that there is an error in the call to png_check_IHDR (width == 0 && height==0).  Note that all other browsers also throw an error for this ICO.

> IsValidICO() is looking at the PNG header data before these conversions 
> have happened.

If you disagree with the logic in IsValidICO please post a follow up bug.  This bug is related to fixing a crash, and will be prioritized differently.  I.e. it will likely be moving up to different branches, but other changes would not.

Thanks for the feedback by the way.
Leaving the patch as is for Joe to review.
(In reply to Brian R. Bondy [:bbondy] from comment #8)

> Although I agree with this assertion, it is not related to this bug.  Please
> post a followup bug and I can work on it there.

OK

> The issue is that there is an error in the call to png_check_IHDR (width ==
> 0 && height==0).  Note that all other browsers also throw an error for this
> ICO.

ImageMagick sees valid dimensions:

identify favicon.ico
favicon.ico[0] ICO 16x16 16x16+0+0 8-bit DirectClass 8.83KB 0.000u 0:00.000
favicon.ico[1] ICO 32x32 32x32+0+0 8-bit PseudoClass 252c 8.83KB 0.010u 0:00.000
favicon.ico[2] ICO 64x64 64x64+0+0 8-bit PseudoClass 256c 8.83KB 0.010u 0:00.000

but colortype 3 (indexed):
identify -verbose favicon.ico
    [image 0]
    png:IHDR.bit_depth       : 8
    png:IHDR.color_type      : 3 (Indexed)
    png:IHDR.interlace_method: 0 (Not interlaced)
    png:IHDR.width,height    : 16, 16
    [image 1]
    png:IHDR.bit_depth       : 8
    png:IHDR.color_type      : 3 (Indexed)
    png:IHDR.interlace_method: 0 (Not interlaced)
    png:IHDR.width,height    : 32, 32
    [image 2]
    png:IHDR.bit_depth       : 8
    png:IHDR.color_type      : 3 (Indexed)
    png:IHDR.interlace_method: 0 (Not interlaced)
    png:IHDR.width,height    : 64, 64
(Assignee)

Comment 10

5 years ago
libpng sees width, height as 0 from that function call and throws an error. I'm not sure why exactly but in any case it now throws an error instead of crashing. We are now working consistently with every other browser.
(Assignee)

Comment 11

5 years ago
This is passing try tests on all platforms by the way:
https://tbpl.mozilla.org/?tree=Try&rev=2f6ae534c7a4
(Assignee)

Updated

5 years ago
status-firefox16: --- → affected
(Assignee)

Comment 12

5 years ago
This fixes a crash which can be triggered from page loads, so was wondering if there's someone else I can push the review to with more time? I think the fix is pretty straightforward.  

This won't make mozilla16, but I'd like to request it up to Aurora/Beta after the migration.
Attachment #638573 - Flags: review?(joe) → review+
It wasn't that I didn't have time, it was that I was on vacation :)
(Assignee)

Comment 14

5 years ago
Then by definition you had no time! :P Thanks for the review.
(Assignee)

Comment 15

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/91f65d802d58
Moving to mozilla17 because I *think* that's where it will land.
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/91f65d802d58
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 17

5 years ago
Comment on attachment 638573 [details] [diff] [review]
Patch v1

Request for Aurora 16 and Beta 15:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 682677
User impact if declined: A webpage can be loaded which contains a crafted ICO that causes a browser crash
Testing completed (on m-c, etc.): Tested locally, landed on m-c already as well
Risk to taking this patch (and alternatives if risky): Low
String or UUID changes made by this patch: none
Attachment #638573 - Flags: approval-mozilla-beta?
Attachment #638573 - Flags: approval-mozilla-aurora?
Comment on attachment 638573 [details] [diff] [review]
Patch v1

[Triage Comment]
This has had a day of bake time now without regression, and we're very early in the cycle. Approving for both Aurora 16 and Beta 15.
Attachment #638573 - Flags: approval-mozilla-beta?
Attachment #638573 - Flags: approval-mozilla-beta+
Attachment #638573 - Flags: approval-mozilla-aurora?
Attachment #638573 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/e8531445dd68
http://hg.mozilla.org/releases/mozilla-aurora/rev/1387ad6209b1
status-firefox15: affected → fixed
status-firefox16: affected → fixed
status-firefox17: --- → fixed

Comment 20

5 years ago
Verified as fixed on Firefox 15.0 beta 4 (20120808131812) with the ico file attached by the reporter. On the latest beta, the user receives an error message that the opened file cannot be displayed because it contains errors. Firefox does not crash anymore.
status-firefox15: fixed → verified
Keywords: verifyme

Updated

5 years ago
QA Contact: ioana.budnar

Comment 21

5 years ago
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0
Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/20100101 Firefox/16.0
BuildID: 20120925201946
status-firefox16: fixed → verified

Comment 22

5 years ago
Verified as fixed on:
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0
BuildID: 20121010150351
status-firefox17: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.