Last Comment Bug 155222 - Heap corruption in PNG library
: Heap corruption in PNG library
Status: VERIFIED FIXED
[adt1 rtm]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.0.1
Assigned To: Stuart Parmenter
: bsharma
: David Keeler [:keeler] (use needinfo?)
Mentors:
: 161086 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-07-01 11:09 PDT by Mitchell Stoltz (not reading bugmail)
Modified: 2002-08-08 12:38 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch that changes that warning to an error. (589 bytes, patch)
2002-07-02 13:45 PDT, Gagan
pavlov: review+
Details | Diff | Splinter Review
updated patch with tor's review (1.01 KB, patch)
2002-07-03 01:19 PDT, Gagan
no flags Details | Diff | Splinter Review
as above with Glenn's suggested change (1.16 KB, patch)
2002-07-10 15:54 PDT, tor
pavlov: review+
tor: superreview+
asa: approval+
Details | Diff | Splinter Review

Description Mitchell Stoltz (not reading bugmail) 2002-07-01 11:09:45 PDT
Submitter name:                zen-parse
   Submitter email address:       zen-parse@gmx.net
   Acknowledgement checkbox:      on
   Product:                       Communicator 4.x
   Operating system:              Unix: x86 Linux
   OS version:                    Redhat 7.0
   Issue summary:                 PNG files can cause potentially exploitable
heap corruption

Issue details:
Heap Corruption Vulnerability.

It is possible to cause heap corruption in 
Netscape and Mozilla by supplying
a specially crafted PNG format file.

If the X size > (2^32 / 
number_bytes_needed_per_pixel) then the 
number of
bytes required for a row becomes greater than 
2^32 and overflows.


This condition is checked for in the png 
library, but by default isn't
treated as fatal (to the image) in 
Mozilla/Netscape. (warning handler)

pngtopnm (from netpbm-progs package) 
complains about the file with:

[zen@clarity netscaper]$ pngtopnm laa.png 
libpng warning: Width too large to process 
image data; rowbytes will overflow.
pnmtopng:  fatal libpng error: Extra 
compressed data
pngtopnm: setjmp returns error condition


(... 

Just found a variation on it. A much larger 
IDAT chunk than the data size is
also causes corruption. I'm not sure of the 
exact mechanism for that yet.

[zen@clarity netscaper]$ pngtopnm lbt.png 
pnmtopng:  fatal libpng error: Extra 
compressed data
pngtopnm: setjmp returns error condition
 
...)



[zen@clarity netscaper]$ file laa.png
laa.png: PNG image data, 1073741952 x 2, 
8-bit/color RGBA, non-interlaced
[zen@clarity netscaper]$ netscape laa.png 
(netscape opens, then closes when I move the 
mouse)
Bus error
[zen@clarity netscaper]$ gdb 
/usr/lib/netscape/netscape-communicator 
GNU gdb Red Hat Linux (5.1-0.7)
Copyright 2001 Free Software Foundation, Inc.
GDB is free software, covered by the GNU 
General Public License, and you are
welcome to change it and/or distribute copies 
of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  
Type "show warranty" for details.
This GDB was configured as 
"i386-redhat-linux"...
(no debugging symbols found)...
(gdb) q
[zen@clarity netscaper]$ gdb -q 
/usr/lib/netscape/netscape-communicator 
(no debugging symbols found)...(gdb) r 
laa.png 
Starting program: 
/usr/lib/netscape/netscape-communicator 
laa.png

Program received signal SIGSEGV, Segmentation 
fault.
0x4005f510 in _XtGetModifierIndex () from 
/usr/X11R6/lib/libXt.so.6
(gdb) x/i $eip
0x4005f510 <_XtGetModifierIndex+576>:   
movzwl 0x2(%edi),%edx
(gdb) info reg edi
edi            0xdeadbeef       -559038737
(gdb) c
Continuing.

Program received signal SIGBUS, Bus error.
0x40229bb1 in __kill () from /lib/libc.so.6
(gdb) c
Continuing.

Program terminated with signal SIGBUS, Bus 
error.
The program no longer exists.
(gdb) q
[zen@clarity netscaper]$ 

The problem is potentially exploitable to 
allow running arbitrary code, but
due to the asyncronous way events occur, it 
is not trivial to achieve this.

I have managed to get eip=0xdeadbeef on a 
number of occasions, but the
same file has also given other random errors 
on further runs depending on
the order of events processed. Also making 
one slight change to the file
can massively change the location of things 
in memory.

Exploitation is (at least theoretically) 
possible though.

A potential fix would be to enable a warning 
function handler in the png
libraries. 

The check in

http://lxr.mozilla.org/mozilla/source/modules/libimg/png/pngget.c#569

is the appropriate file and 

http://crash.ihug.co.nz/~Sneuro/netscape-example.png

is an example of a corrupt file. (Just the 
latest one I was playing with.)

This type of problem also affects Opera 6.0 
(so don't feel so bad).





Additional computer info:
The gdb dump is from 4.77 (not the latest 
version) but similar problems exist in later 
versions (according to reports of crashes from 
friends who tried to view the file.)


This form was submitted from
http://help.netscape.com/forms/bug-security.html?cp=bbpctr
with Mozilla/4.76 (Linux 2.2.19-7.0.16 i686; U) Opera 6.0  [en].
Comment 1 georgi - hopefully not receiving bugspam 2002-07-02 01:28:09 PDT
Some quick tests show that gimp and konqueror are also vulnerable on linux.
openoffice on windows also becomes unresponsive.
Comment 2 Gagan 2002-07-02 13:45:18 PDT
Created attachment 89975 [details] [diff] [review]
patch that changes that warning to an error.

Even though this is my whack at the patch, I strongly suspect we should also do
something about the return value (which seems to be ignored currently at
http://lxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/png/nsPNGDecoder.cpp#179
) Somebody with more png experience should look at this as well.
Comment 3 Gagan 2002-07-02 13:50:06 PDT
ccing tor
Comment 4 Stuart Parmenter 2002-07-02 14:00:50 PDT
Comment on attachment 89975 [details] [diff] [review]
patch that changes that warning to an error.

while we probably should add a check in the info_callback code so that we
listen to the return value of our call to png_get_IHDR, changing the warning to
an error will result in a longjmp out of the code and back to "safe" land.  it
will then abort loading of the image and all should be good.
Comment 5 tor 2002-07-02 14:07:54 PDT
Glenn, does it sound reasonable that this condition should call
png_error() instead of png_warning() inside libpng, since similar
tests in png_get_IHDR() also call png_error()?
Comment 6 tor 2002-07-02 14:13:21 PDT
Comment on attachment 89975 [details] [diff] [review]
patch that changes that warning to an error.

The libpng style in png_get_IHDR() is to just call png_error() without
a change in the return value.  Also add a line to MOZCHANGES so that
we can keep track of mozilla changes.
Comment 7 Glenn Randers-Pehrson 2002-07-02 14:22:49 PDT
At first glance it looks reasonable to change the warning to an error.  I'll
think about it more when I get back home tonight.

Glenn
Comment 8 Gagan 2002-07-03 01:19:25 PDT
Created attachment 90046 [details] [diff] [review]
updated patch with tor's review
Comment 9 Glenn Randers-Pehrson 2002-07-03 06:00:34 PDT
After further thought I still agree with changing the png_warning() to
png_error().

In libpng-1.0.13 and libpng-1.2.4 we have added a small safety margin
(see malloc of png_ptr->big_row_buf in pngrutil.c of libpng-1.2.x), and
the test should be

  if ((*width > PNG_MAX_UINT/rowbytes_per_pixel) - 64)

instead of

  if ((*width > PNG_MAX_UINT/rowbytes_per_pixel))

To avoid having a (pathological) class of images that can be processed
with mozilla+libpng-1.0.9+ and not with mozilla+libpng-1.2.x, I suggest
that the same test be used now in the patch to libpng-1.0.9.

I'll leave it to tor to update the attachment.

Glenn
Comment 10 Glenn Randers-Pehrson 2002-07-03 06:09:37 PDT
I wrote:

>the test should be
>
>  if ((*width > PNG_MAX_UINT/rowbytes_per_pixel) - 64)
> 
>
>instead of
>
>  if ((*width > PNG_MAX_UINT/rowbytes_per_pixel))

Phooey, misplaced ")".  Anyhow there seems to be an unnecessary set of ().

The test should be

   if (*width > PNG_MAX_UINT/rowbytes_per_pixel - 64)

Glenn
Comment 11 Glenn Randers-Pehrson 2002-07-03 08:19:16 PDT
>Just found a variation on it. A much larger 
>IDAT chunk than the data size is
>also causes corruption. I'm not sure of the 
>exact mechanism for that yet.

This part of the original bug report appears to duplicate bug
<a href="http://bugzilla.mozilla.org/show_bug.cgi?id=154996">154996</a>
which is being fixed.

Glenn
Comment 12 Stuart Parmenter 2002-07-10 15:22:12 PDT
tor?  is this patch ok?
Comment 13 tor 2002-07-10 15:54:39 PDT
Created attachment 90861 [details] [diff] [review]
as above with Glenn's suggested change
Comment 14 Stuart Parmenter 2002-07-12 22:32:41 PDT
Comment on attachment 90861 [details] [diff] [review]
as above with Glenn's suggested change

r=pavlov
Comment 15 scottputterman 2002-07-15 11:42:24 PDT
tor, could you do an sr= for this?
Comment 16 tor 2002-07-15 11:47:10 PDT
Comment on attachment 90861 [details] [diff] [review]
as above with Glenn's suggested change

sr=tor
Comment 17 Asa Dotzler [:asa] 2002-07-15 13:14:54 PDT
Comment on attachment 90861 [details] [diff] [review]
as above with Glenn's suggested change

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.
Comment 18 Stuart Parmenter 2002-07-15 13:25:30 PDT
fixed on trunk.  need adt approval for branch
Comment 19 Paul Wyskoczka 2002-07-15 16:54:31 PDT
Bindu, can you verify this fix on the trunk.  If you need help in testing,
please contact Gagan not Mitch.
Comment 20 Gagan 2002-07-15 17:07:30 PDT
nope. mitch would be right contact for help with this, not me. 
Comment 21 Mitchell Stoltz (not reading bugmail) 2002-07-15 18:04:18 PDT
Gagan, do we have a testcase that causes this crash, something Bindu can click
on and run?
Comment 22 Gagan 2002-07-15 18:16:34 PDT
none that i know of. ask the reporter? 
Comment 23 neuro 2002-07-16 05:55:47 PDT
http://crash.ihug.co.nz/~Sneuro/testing123.png
PNG image data, 536879104 x 1, 8-bit/color RGB, non-interlaced

http://crash.ihug.co.nz/~Sneuro/testing234.png
PNG image data, 1073741825 x 1, 8-bit/color RGB, non-interlaced

http://crash.ihug.co.nz/~Sneuro/testing345.png
PNG image data, 1431655770 x 1, 8-bit/color RGB, non-interlaced
Comment 24 bsharma 2002-07-16 11:42:11 PDT
Verified on 2002-07-16-trunk build on Win 2K.

Loaded following test png files:

http://crash.ihug.co.nz/~Sneuro/netscape-example.png
http://crash.ihug.co.nz/~Sneuro/testing123.png
http://crash.ihug.co.nz/~Sneuro/testing234.png
http://crash.ihug.co.nz/~Sneuro/testing345.png

All of above test cases gives the following error in the browser window:
The image “http://crash.ihug.co.nz/~Sneuro/<png file name>.png” cannot be
displayed, because it contains errors.
Comment 25 scottputterman 2002-07-16 16:47:42 PDT
adding adt1.0.1+. Please get drivers approval before checking into the branch.
Comment 26 scottputterman 2002-07-17 10:01:37 PDT
changing to verified on trunk based on Bindu's comments.
Comment 27 Judson Valeski 2002-07-17 11:42:25 PDT
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.
Comment 28 Stuart Parmenter 2002-07-17 16:21:36 PDT
checked in on branch
Comment 29 bsharma 2002-07-19 15:35:48 PDT
Verified on 2002-07-19-branch build on Win 2K.

Loaded following test png files:

http://crash.ihug.co.nz/~Sneuro/netscape-example.png
http://crash.ihug.co.nz/~Sneuro/testing123.png
http://crash.ihug.co.nz/~Sneuro/testing234.png
http://crash.ihug.co.nz/~Sneuro/testing345.png

All of above test cases gives the following error in the browser window:
The image “http://crash.ihug.co.nz/~Sneuro/<png file name>.png” cannot be
displayed, because it contains errors.

Comment 30 bsharma 2002-07-19 15:37:39 PDT
Added the verified1.0.1 keyword.
Comment 31 Simon Lucy 2002-08-01 10:32:26 PDT
Just for pedantry's sake shouldn't OS be set to All?
Comment 32 Glenn Randers-Pehrson 2002-08-05 06:16:38 PDT
Since this bug was discussed publicly in the libpng mailing lists
and is described and fixed publicly in libpng-1.2.4/1.0.14,
perhaps it can be made a "public" Mozilla bug.  Incidentally,
bug #154996 is public, and the security implications of that bug
are quite similar.

PS: I agree that changing OS and platform to "ALL" would make sense,
but I'm not sufficiently empowered to do that.

Glenn
Comment 33 tor 2002-08-05 10:55:16 PDT
*** Bug 161086 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.