Closed Bug 1368407 Opened 2 years ago Closed 2 years ago

Small PNG image hangs browser through memory consumption

Categories

(Core :: ImageLib, defect)

53 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 - wontfix
firefox55 + verified

People

(Reporter: glen.carmichael, Assigned: glennrp+bmo)

Details

(Keywords: csectype-dos)

Attachments

(2 files)

Attached image huge-malloc.png
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Open the attached PNG image.


Actual results:

Firefox tries to eat all the memory. I let it run to >50GB on macOS, which meant the kernel started to pause other processes/applications.

The image is only 41 bytes, but it defines its width as 2^32 - 1. This results in a huge malloc in libpng's png_read_start_row (pngrutil.c).


Expected results:

The image width should be verified or limited somehow. I believe it can be trivially fixed by defining PNG_MAX_MALLOC_64K for libpng.
Bobby, based on bug 505762 (blast from the past...), seems you might know about this code - any chance you could take a look, and/or could recommend someone else? :-)
Group: firefox-core-security → core-security
Component: Untriaged → ImageLib
Flags: needinfo?(bobbyholley)
Product: Firefox → Core
I stepped down from imagelib stuff years ago. You want whoever's maintaining imagelib now. I think that's tn now that seth is gone?
Flags: needinfo?(bobbyholley)
Status: UNCONFIRMED → NEW
Ever confirmed: true
This is a known feature and probably doesn't need to be hidden.
At one time there existed a width and height limit of 32k but
that was removed a long time ago.  Libpng by default uses
1,000,000 by 1,000,000 limits, but we override those (in
media/libpng/pnglibconf.h) with 0x7ffffffff (2^31 -1) by 0x7fffffff
limits (essentially unlimited).  I don't remember the rationale
for doing that.

One can build with different limits by defining MOZ_PNG_MAX_WIDTH
and/or MOZ_PNG_MAX_HEIGHT.
Assignee: nobody → glennrp+bmo
Status: NEW → ASSIGNED
Note that image/decoders/nsPNGDecoder.cpp also abides by
user-supplied MOZ_PNG_MAX_WIDTH and MOZ_PNG_MAX_HEIGHT
limits when using a system libpng instead of the in-tree
libpng.
Group: core-security
Keywords: csectype-dos
Tracking for 55, not really a new issue, but if we land a fix we could still potentially take a patch for 54.
What would you think of making MOZ_PNG_MAX_WIDTH and _HEIGHT
(and maybe a new MOZ_PNG_MAX_PIXELS) settable in build:config ?
Or, more generally, MOZ_IMAGE_MAX_WIDTH, etc.?
Other places in imagelib we have used SurfaceCache::CanHold before making a potentially large allocation (even if the allocated data won't be inserted into the surface cache) to decide if we want to make the allocation to prevent making huge allocations that will likely never produce anything visible to the user.
In this case the alloc happens inside libpng where SurfaceCache::CanHold is not available.  I'll see what I can do.
This patch checks for width exceeding MaximumCapacity()/8 (because libpng will allocate two row buffers of 4*width).  It also checks, in the case of ADAM7 interlaced images if there is enough room to allocate the complete 4*width*height buffer that we require for deinterlacing.

Please submit it to try.
Flags: needinfo?(ryanvm)
On my Ubuntu-16.04 platform, I get

Nightly: Attempts to allocate a huge amount of memory, then
[ImgDecoder #1]: E/PNGDecoder libpng error: Out of memory

Patched: Immediately abandons the PNG with this message
[ImgDecoder #1]: E/PNGDecoder libpng error: Image is too wide
Comment on attachment 8874162 [details] [diff] [review]
v00-1368407-check-png-width

Try results seem ok
Attachment #8874162 - Flags: review?(tnikkel)
Attachment #8874162 - Flags: review?(tnikkel) → review+
Attachment #8874162 - Flags: checkin?
Attachment #8874162 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/4142967e1f23
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I reproduced this issue using Fx55.0a1 (2017-05-28) on macOS X 10.12.5.
I can confirm this issue is fixed, I verified using Fx 55.0b2 on macOS X 10.12.5.

Cheers!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.