Closed Bug 770122 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

10 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox14 --- affected
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified

People

(Reporter: s.lange, Assigned: bbondy)

References

()

Details

(4 keywords)

Crash Data

Attachments

(2 files)

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.
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
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: nobody → netzen
Blocks: 682677
Attached patch Patch v1Splinter Review
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)
After review and landing I'll be requesting that this goes on Aurora and Beta.
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.
> 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
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.
This is passing try tests on all platforms by the way:
https://tbpl.mozilla.org/?tree=Try&rev=2f6ae534c7a4
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 :)
Then by definition you had no time! :P Thanks for the review.
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
Closed: 12 years ago
Resolution: --- → FIXED
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+
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.
Keywords: verifyme
QA Contact: ioana.budnar
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
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
You need to log in before you can comment on or make changes to this bug.