Closed Bug 1174811 Opened 5 years ago Closed 5 years ago

Firefox downloads huge favicon files

Categories

(Toolkit :: Places, defect)

38 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: inglor, Assigned: poiru)

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.124 Safari/537.36

Steps to reproduce:

1. Go to https://github.com/benjamingr/favicon-bug
2. Download code and run it
3. Go to localhost:3000


Actual results:

Firefox attempts to download the infinite size favicon.ico without showing any visual indication, after a while the browser crashes.

Here is the crash agent report:

AdapterDeviceID: 0x 116
AdapterVendorID: 0x8086
Add-ons: %7B972ce4c6-7e08-4474-a285-3208198ce6fd%7D:38.0.5
BuildID: 20150525141253
CrashTime: 1434389443
EMCheckCompatibility: true
EventLoopNestingLevel: 1
FramePoisonBase: 7ffffffff0dea000
FramePoisonSize: 4096
InstallTime: 1434097119
Notes: AdapterVendorID: 0x8086, AdapterDeviceID: 0x 116GL Layers? GL Context? GL Context+ GL Layers+ 
OOMAllocationSize: 2147024897
ProductID: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
ProductName: Firefox
ReleaseChannel: release
SecondsSinceLastCrash: 84
StartupTime: 1434389390
Theme: classic/1.0
Throttleable: 1
URL: http://localhost:3000/
Vendor: Mozilla
Version: 38.0.5
useragent_locale: en-US

This report also contains technical information about the state of the application when it crashed.


Expected results:

The browser should not have crashed. I tested on Firefox version38.0.5 on Mac OSX 10.10
Crash details: https://crash-stats.mozilla.org/report/index/45f4d9c3-bd3f-49b9-b84f-a453e2150615
Assignee: nobody → birunthan
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → Places
Ever confirmed: true
Product: Firefox → Toolkit
Attached patch Handle OOM with huge favicons (obsolete) — Splinter Review
This fixes the OOM, but I think we should also limit the favicon size to maybe a megabyte or two. Any thoughts?
Attachment #8622696 - Flags: review?(mak77)
Attachment #8622708 - Flags: review?(mak77)
Attachment #8622696 - Attachment is obsolete: true
Attachment #8622696 - Flags: review?(mak77)
Sigh. We really need to lean more heavily on ImageLib for favicons instead of reinventing everything, with new bugs.
Comment on attachment 8622708 [details] [diff] [review]
Handle OOM with huge favicons

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

Yes I think it's a good idea to limit the size too, there's no point to just handle oom when we don't really support storing huge blobs.

Note I'm traveling the next days and in Whistler the next week, and I'm quite busy packing up today, but I'm sure you can find an alternative reviewer in fx-team if this is urgent.
(In reply to Seth Fowler [:seth] from comment #4)
> Sigh. We really need to lean more heavily on ImageLib for favicons instead
> of reinventing everything, with new bugs.

Provided we retain the possibility to store blobs in a single database, I'd be very happy if we could use external code, but I don't know which parts we could replace since I don't know much about imageLib. If you have ideas I'd be happy to brainstorm on that.
Comment on attachment 8622708 [details] [diff] [review]
Handle OOM with huge favicons

Changing reviewer based on comment 5.
Attachment #8622708 - Flags: review?(mak77) → review?(nfroyd)
Comment on attachment 8622708 [details] [diff] [review]
Handle OOM with huge favicons

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

Exercising my toolkit peer powers for the year and judging this to be sufficiently straightforward that I can review it.
Attachment #8622708 - Flags: review?(nfroyd) → review+
Attachment #8623026 - Flags: review?(nfroyd) → review+
Hm... You guys are restricting download size to 1MiB download... That can still be pretty huge unpacked as a zipbomb no?
In fact the originally reported incident was an accidental compression.


http://m8y.org/tmp/x.png
$ identify x.png
x.png PNG 10240x10240 10240x10240+0+0 8-bit DirectClass 12.9KB 0.000u 0:00.000

Presumably if I made this file a favicon, it would use 10240*10240*4 to represent as an image right?

400MiB of RAM for a 13KiB file.

Seems the abort should be on creation of the image file based on its dimensions...

But maybe I'm totally misreading the patch.
FWIW, here's another one...

http://m8y.org/tmp/big.png
$ identify big.png 
big.png PNG 32768x32768 32768x32768+0+0 8-bit DirectClass 131KB 0.000u 0:00.000

128KiB for 4GiB of RAM?
Eh. Nevermind. You guys must have additional checks beyond file size somewhere that prevent zipbombs.  And they must have additional sanity checking on reasonable favicon sizes since even x.png which would load in browser doesn't spike my memory.

Sooo I guess you guys were just missing some extra on-the-wire large file sanity check as well.

Should have tested before bugspamming. My bad.
On the other hand...
http://m8y.org/tmp/zipbomb/foo.html  this page hits slightly-under-1MiB svg favicon w/ basically a looong <text> of 3s
It was adapted from http://m8y.org/tmp/zipbomb/test.html

Firefox happily loads and tries to parse this SVG, and sucked up gigabytes of RAM and a fair amount of CPU doing so.

On the other hand, I'm not too sure what you guys could do to prevent this, apart from imposing a lower limit on compressed SVGs or something :-/
https://hg.mozilla.org/mozilla-central/rev/47caf1b25775
https://hg.mozilla.org/mozilla-central/rev/8861436c0ddb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Awesome, thanks for the quick fix!
You need to log in before you can comment on or make changes to this bug.