Attachment 22251 Details for Bug 58008

libpng vendor-sec discussion

libpng.txt (text/plain), 32.68 KB, created by Thomas Biege on 2004-07-19 16:16:21 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Thomas Biege

Created: 2004-07-19 16:16:21 UTC

Size: 32.68 KB

patch

obsolete

>From chris@scary.beasts.org Mon Jul 19 09:55:38 2004
>Date: Tue, 13 Jul 2004 20:30:51 +0100 (BST)
>From: chris@scary.beasts.org
>To: vendor-sec@lst.de
>Subject: [vendor-sec] Nasty security hole in libpng
>
>Hi,
>
>My advisory is included below. This is potentially quite nasty; seems to
>affect both mozilla and konqueror (I'm not sure if mozilla links
>dynamically to libpng, statically, or has its own copy or what).
>
>I'm not sure what to do with the advisory; libpng doesn't seem
>particularly maintained. Who did people liase with in the case of the
>recent libpng problems? Ideas appreciated.
>
>Cheers
>Chris
>
>CESA-2004-001
>
>libPNG 1.2.5 stack-based buffer overflow and other code concerns
>================================================================
>
>This advisory lists code flaws discovered by inspection of the libpng-1.2.5
>code. Only the first one has been examined in practice to confirm
>exploitability. The other flaws certainly warrant fixing.
>
>1) Remotely exploitable stack-based buffer overrun in png_handle_tRNS
>(pngrutil.c)
>
>If a PNG file is of the correct format, a length check on PNG data is missed
>prior to filling a buffer on the stack from the PNG data. The exact flaw would
>seem to be a logic error; failure to bail out of a function after a warning
>condition is hit, here:
>
>      if (!(png_ptr->mode & PNG_HAVE_PLTE))
>      {
>         /* Should be an error, but we can cope with it */
>         png_warning(png_ptr, "Missing PLTE before tRNS");
>      }
>      else if (length > (png_uint_32)png_ptr->num_palette)
>      {
>         png_warning(png_ptr, "Incorrect tRNS chunk length");
>         png_crc_finish(png_ptr, length);
>         return;
>      }
>
>We can see, if the first warning condition is hit, the length check is missed
>due to the use of an "else if".
>
>A PNG crafted to trip this is available at
>http://scary.beasts.org/misc/pngtest_bad.png
>
>It crashes both mozilla and konqueror.
>
>
>2) Dangerous code in png_handle_sBIT (pngrutil.c) (Similar code in
>png_handle_hIST).
>
>Although seemingly not exploitable, there is dangerous code in this function.
>It relies on checks scattered elsewhere in the code in order to not overflow
>a 4-byte stack buffer. This line here should upper-bound the read onto the
>stack to 4 bytes:
>
>   png_crc_read(png_ptr, buf, truelen);
>
>
>3) Possible NULL-pointer crash in png_handle_iCCP (pngrutil.c) (this flaw is
>duplicated in multiple other locations).
>
>There are lots of lines such as these in the code:
>
>   chunkdata = (png_charp)png_malloc(png_ptr, length + 1);
>
>Where "length" comes from the PNG. If length is set to UINT_MAX then length + 1 will equate to zero, leading to the PNG malloc routines to return NULL and
>subsequent access to crash. These lengths are sometimes checked to ensure
>they are smaller that INT_MAX, but it is not clear that all code paths perform
>this check, i.e. png_push_read_chunk in pngpread.c does not do this check
>(this is progressive reading mode as used by browsers).
>
>
>4) Theoretical integer overflow in allocation in png_handle_sPLT (pngrutil.c)
>
>This isn't likely to cause problems in practice, but there's the possibility
>of an integer overflow during this allocation:
>
>   new_palette.entries = (png_sPLT_entryp)png_malloc(
>       png_ptr, new_palette.nentries * sizeof(png_sPLT_entry));
>
>
>5) Integer overflow in png_read_png (pngread.c)
>
>A PNG with excessive height may cause an integer overflow on a memory
>allocation and subsequent crash allocating row pointers. This line is possibly
>faulty; I can't see anywhere that enforces a maximum PNG height:
>
>      info_ptr->row_pointers = (png_bytepp)png_malloc(png_ptr,
>         info_ptr->height * sizeof(png_bytep));
>
>
>6) Integer overflows during progressive reading.
>
>There are many lines like the following, which are prone to integer overflow:
>
>      if (png_ptr->push_length + 4 > png_ptr->buffer_size)
>
>It is not clear how dangerous this is.
>
>
>7) Other flaws.
>
>There is broad potential for other integer overflows which I have not spotted -
>the amount of integer arithmetic surrounding buffer handling is large,
>unfortunately.
>
>
>CESA-2004-001
>Chris Evans
>chris@scary.beasts.org
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From alan@lxorguk.ukuu.org.uk Mon Jul 19 09:55:46 2004
>Date: Tue, 13 Jul 2004 19:45:04 +0100
>From: Alan Cox <alan@lxorguk.ukuu.org.uk>
>To: chris@scary.beasts.org
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>On Maw, 2004-07-13 at 20:30, chris@scary.beasts.org wrote:
>> A PNG crafted to trip this is available at
>> http://scary.beasts.org/misc/pngtest_bad.png
>> 
>> It crashes both mozilla and konqueror.
>
>Ouch.. this is going to affect mailers like evolution too so its very
>nasty, and nautilus even browsing such a file.
>
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mjc@redhat.com Mon Jul 19 09:55:50 2004
>Date: Tue, 13 Jul 2004 21:08:43 +0100 (BST)
>From: Mark J Cox <mjc@redhat.com>
>To: chris@scary.beasts.org
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>> I'm not sure what to do with the advisory; libpng doesn't seem
>> particularly maintained. Who did people liase with in the case of the
>> recent libpng problems? Ideas appreciated.
>
>Hi Chris; with the last libpng issue that was reported to us we just
>worked with the other vendor-sec folks and came up with a suitable embargo 
>date.  It looks like this issue is going to affect a whole load of 
>packages, do you have any particular timescale in mind for disclosure?
>
>Here are some CVE names
>
>> 1) Remotely exploitable stack-based buffer overrun in png_handle_tRNS
>> (pngrutil.c)
>> 2) Dangerous code in png_handle_sBIT (pngrutil.c) (Similar code in
>> png_handle_hIST).
>
>CAN-2004-0597 for these (we merge issues that have the same flaw type that 
>get fixed in the same versions).
>
>> 3) Possible NULL-pointer crash in png_handle_iCCP (pngrutil.c) (this 
>> flaw is duplicated in multiple other locations).
>
>CAN-2004-0598 for those
>
>> 4) Theoretical integer overflow in allocation in png_handle_sPLT 
>> (pngrutil.c)
>> 5) Integer overflow in png_read_png (pngread.c)
>> 6) Integer overflows during progressive reading.
>> 7) Other flaws.  [integer overflows]
>
>CAN-2004-0599 for those
>
>Cheers, Mark
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From chris@scary.beasts.org Mon Jul 19 09:55:53 2004
>Date: Wed, 14 Jul 2004 00:11:05 +0100 (BST)
>From: chris@scary.beasts.org
>To: Mark J Cox <mjc@redhat.com>
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>Hi Mark,
>
>On Tue, 13 Jul 2004, Mark J Cox wrote:
>
>> Hi Chris; with the last libpng issue that was reported to us we just
>> worked with the other vendor-sec folks and came up with a suitable embargo
>> date.  It looks like this issue is going to affect a whole load of
>> packages, do you have any particular timescale in mind for disclosure?
>
>No particular timescale - it can take as long as the vendors need to be
>happy with updates. I must admit I've lost track of how long the vendors
>like to have to properly prepare and test updates - so could you suggest a
>release date for the advisory?
>
>Regarding your comment about a whole load of packages being affected, we
>should probably start enumerating which ones exactly. i.e. does anyone
>know of any packages which statically link libpng, OR have their own
>libpng source tree copies?
>
>> Here are some CVE names
>
>[snip]
>
>Cool. A question on CAN policy here. I'm sure some of these issues are
>either DoS only (i.e. crash mozilla but not subvert it) and others are
>simply dodgy code (but no actual vulnerability). Does it matter if we
>allocate CANs for such issues?
>
>Regarding fixing this, I've appended a patch below. I believe it nails all
>of the issues in my advisory (thinkos excepted ;-) One part of the patch
>is a little heavy handed - PNG width and height are limited to (2^24)-1. Just
>FYI in case it's deemed too draconian.
>
>Cheers
>Chris
>
>diff -ru libpng-1.2.5/png.h libpng-1.2.5.fix/png.h
>--- libpng-1.2.5/png.h	2002-10-03 12:32:26.000000000 +0100
>+++ libpng-1.2.5.fix/png.h	2004-07-13 23:18:10.000000000 +0100
>@@ -835,6 +835,9 @@
> /* Maximum positive integer used in PNG is (2^31)-1 */
> #define PNG_MAX_UINT ((png_uint_32)0x7fffffffL)
>
>+/* Constraints on width, height, (2 ^ 24) - 1*/
>+#define PNG_MAX_DIMENSION 16777215
>+
> /* These describe the color_type field in png_info. */
> /* color type masks */
> #define PNG_COLOR_MASK_PALETTE    1
>diff -ru libpng-1.2.5/pngpread.c libpng-1.2.5.fix/pngpread.c
>--- libpng-1.2.5/pngpread.c	2002-10-03 12:32:28.000000000 +0100
>+++ libpng-1.2.5.fix/pngpread.c	2004-07-13 23:03:58.000000000 +0100
>@@ -209,6 +209,8 @@
>
>       png_push_fill_buffer(png_ptr, chunk_length, 4);
>       png_ptr->push_length = png_get_uint_32(chunk_length);
>+      if (png_ptr->push_length > PNG_MAX_UINT)
>+         png_error(png_ptr, "Invalid chunk length.");
>       png_reset_crc(png_ptr);
>       png_crc_read(png_ptr, png_ptr->chunk_name, 4);
>       png_ptr->mode |= PNG_HAVE_CHUNK_HEADER;
>@@ -638,6 +640,8 @@
>
>       png_push_fill_buffer(png_ptr, chunk_length, 4);
>       png_ptr->push_length = png_get_uint_32(chunk_length);
>+      if (png_ptr->push_length > PNG_MAX_UINT)
>+         png_error(png_ptr, "Invalid chunk length.");
>
>       png_reset_crc(png_ptr);
>       png_crc_read(png_ptr, png_ptr->chunk_name, 4);
>diff -ru libpng-1.2.5/pngrutil.c libpng-1.2.5.fix/pngrutil.c
>--- libpng-1.2.5/pngrutil.c	2004-07-13 13:36:37.000000000 +0100
>+++ libpng-1.2.5.fix/pngrutil.c	2004-07-13 23:43:02.000000000 +0100
>@@ -350,7 +350,11 @@
>    png_crc_finish(png_ptr, 0);
>
>    width = png_get_uint_32(buf);
>+   if (width > PNG_MAX_DIMENSION)
>+      png_error(png_ptr, "Width is too large");
>    height = png_get_uint_32(buf + 4);
>+   if (height > PNG_MAX_DIMENSION)
>+      png_error(png_ptr, "Height is too large");
>    bit_depth = buf[8];
>    color_type = buf[9];
>    compression_type = buf[10];
>@@ -675,7 +679,7 @@
>    else
>       truelen = (png_size_t)png_ptr->channels;
>
>-   if (length != truelen)
>+   if (length != truelen || length > 4)
>    {
>       png_warning(png_ptr, "Incorrect sBIT chunk length");
>       png_crc_finish(png_ptr, length);
>@@ -1244,7 +1248,8 @@
>          /* Should be an error, but we can cope with it */
>          png_warning(png_ptr, "Missing PLTE before tRNS");
>       }
>-      else if (length > (png_uint_32)png_ptr->num_palette)
>+      if (length > (png_uint_32)png_ptr->num_palette ||
>+          length > PNG_MAX_PALETTE_LENGTH)
>       {
>          png_warning(png_ptr, "Incorrect tRNS chunk length");
>          png_crc_finish(png_ptr, length);
>@@ -1400,7 +1405,7 @@
> void /* PRIVATE */
> png_handle_hIST(png_structp png_ptr, png_infop info_ptr, png_uint_32 length)
> {
>-   int num, i;
>+   unsigned int num, i;
>    png_uint_16 readbuf[PNG_MAX_PALETTE_LENGTH];
>
>    png_debug(1, "in png_handle_hIST\n");
>@@ -1426,8 +1431,8 @@
>       return;
>    }
>
>-   num = (int)length / 2 ;
>-   if (num != png_ptr->num_palette)
>+   num = length / 2 ;
>+   if (num != png_ptr->num_palette || num > PNG_MAX_PALETTE_LENGTH)
>    {
>       png_warning(png_ptr, "Incorrect hIST chunk length");
>       png_crc_finish(png_ptr, length);
>@@ -2868,6 +2873,9 @@
>                png_read_data(png_ptr, chunk_length, 4);
>                png_ptr->idat_size = png_get_uint_32(chunk_length);
>
>+               if (png_ptr->idat_size > PNG_MAX_UINT)
>+                  png_error(png_ptr, "Invalid chunk length.");
>+
>                png_reset_crc(png_ptr);
>                png_crc_read(png_ptr, png_ptr->chunk_name, 4);
>                if (png_memcmp(png_ptr->chunk_name, (png_bytep)png_IDAT, 4))
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mjc@redhat.com Mon Jul 19 09:55:56 2004
>Date: Wed, 14 Jul 2004 07:27:36 +0100 (BST)
>From: Mark J Cox <mjc@redhat.com>
>To: chris@scary.beasts.org
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>> I must admit I've lost track of how long the vendors
>> like to have to properly prepare and test updates - so could you suggest a
>> release date for the advisory?
>
>How about a provisional date three weeks from now, August 4th?  I say
>provisional because if you do deal with CERT I'm sure they'd need more 
>time than that.
> 
>> Cool. A question on CAN policy here. I'm sure some of these issues are
>> either DoS only (i.e. crash mozilla but not subvert it) and others are
>> simply dodgy code (but no actual vulnerability). Does it matter if we
>> allocate CANs for such issues?
>
>The crashes are definately worthy of CVE names.  Dodgy code with no actual 
>vulnerability are not. So:
>
>CAN-2004-0597 (stack overflows) is definately needed
>CAN-2004-0598 (NULL-pointer crashes) sounds definately needed
>CAN-2004-0599 (integer overflows) since there were many of these outlined 
>I figured at least one might have a security context.
>
>Cheers, Mark
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>
>From chris@scary.beasts.org Mon Jul 19 09:55:58 2004
>Date: Wed, 14 Jul 2004 15:17:57 +0100 (BST)
>From: chris@scary.beasts.org
>To: Mark J Cox <mjc@redhat.com>
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>On Wed, 14 Jul 2004, Mark J Cox wrote:
>
>> > I must admit I've lost track of how long the vendors
>> > like to have to properly prepare and test updates - so could you suggest a
>> > release date for the advisory?
>>
>> How about a provisional date three weeks from now, August 4th?  I say
>> provisional because if you do deal with CERT I'm sure they'd need more
>> time than that.
>
>Sounds good.
>
>> The crashes are definately worthy of CVE names.  Dodgy code with no actual
>> vulnerability are not. So:
>>
>> CAN-2004-0597 (stack overflows) is definately needed
>> CAN-2004-0598 (NULL-pointer crashes) sounds definately needed
>> CAN-2004-0599 (integer overflows) since there were many of these outlined
>> I figured at least one might have a security context.
>
>OK - just a note that the latter two are unverified. I settled for
>verifiying the most serious vulnerability and patching the rest.
>
>Cheers
>Chris
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mclasen@redhat.com Mon Jul 19 09:56:00 2004
>Date: Wed, 14 Jul 2004 13:38:14 -0400
>From: Matthias Clasen <mclasen@redhat.com>
>To: vendor-sec@lst.de
>Cc: chris@scary.beasts.org
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>Hey Chris,
>
>some comments on your patch:
>
>1) the pngpread.c changes look fine, I came up with exactly the same
>after reading your initial mail.
>
>2) the image dimension checks should probably be done in png_set_IHDR - 
>there are already checks against PNG_MAX_UINT there, which can be
>modified to be a bit stricter.
>
>3) Your additional checks against png_ptr->channels > 4 and
>png_ptr->num_palette > PNG_MAX_PALETTE_LENGTH are not really necessary,
>since libpng already enforces this. But you have to study
>png_handle_IHDR, png_set_IHDR and png_handle_PLTE carefully to determine
>this, so adding the extra checks may still be a good idea for clarity.
>
>4) Here is a fix for the theoretical overflow in png_handle_sPLT which
>you mention:
>
>--- libpng-1.2.5/pngrutil.c.chunklength	2002-10-03 07:32:30.000000000
>-0400
>+++ libpng-1.2.5/pngrutil.c	2004-07-14 12:27:03.000000000 -0400
>@@ -1154,6 +1154,8 @@
>    }
> 
>    new_palette.nentries = data_length / entry_size;
>+   if (new_palette.nentries > PNG_MAX_UINT / sizeof(png_sPLT_entry))
>+     png_error(png_ptr, "sPLT chunk too long");
>    new_palette.entries = (png_sPLT_entryp)png_malloc(
>        png_ptr, new_palette.nentries * sizeof(png_sPLT_entry));
> 
>5) I have constructed a crasher image for the "possible NULL pointer
>crash" which you mention as issue 3. My crasher image causes a segfault
>in png_push_handle_tEXT() when loaded incrementally. I'm attaching it
>wrapped in a tar file, to keep the email from crashing Evolution... 
>
>Regards,
>
>Matthias Clasen (RH libpng package maintainer)
>
>    [ Part 2, Application/X-TAR  14KB. ]
>    [ Unable to print this part. ]
>
>
>From chris@scary.beasts.org Mon Jul 19 09:56:03 2004
>Date: Wed, 14 Jul 2004 20:33:09 +0100 (BST)
>From: chris@scary.beasts.org
>To: Matthias Clasen <mclasen@redhat.com>
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>Hi!
>
>On Wed, 14 Jul 2004, Matthias Clasen wrote:
>
>> some comments on your patch:
>...
>> 3) Your additional checks against png_ptr->channels > 4 and
>> png_ptr->num_palette > PNG_MAX_PALETTE_LENGTH are not really necessary,
>> since libpng already enforces this. But you have to study
>> png_handle_IHDR, png_set_IHDR and png_handle_PLTE carefully to determine
>> this, so adding the extra checks may still be a good idea for clarity.
>
>Yep, I agree. In my advisory, I noted that it seems to not be exploitable
>and just a case of bad code. At one stage I thought it _was_ exploitable
>if a very specific set of circumstances was met - but it seems not. The
>lack of exploitability is IMHO "luck" so I'd be more comfortable keeping
>the redundant check in place. It will guard against future code changes
>accidentally opening up a hole.
>
>> 4) Here is a fix for the theoretical overflow in png_handle_sPLT which
>> you mention:
>
>I don't think this is needed because of the added checks against all PNG
>chunk lengths being <= PNG_UINT_MAX (which is actually defined as
>the C standard INT_MAX). Running through this function with length set
>at PNG_UINT_MAX, I don't think the overflow can trigger. Let me know if
>I'm wrong.
>
>> 5) I have constructed a crasher image for the "possible NULL pointer
>> crash" which you mention as issue 3. My crasher image causes a segfault
>> in png_push_handle_tEXT() when loaded incrementally. I'm attaching it
>> wrapped in a tar file, to keep the email from crashing Evolution...
>
>Hey, excellent. I'm glad I wasn't smoking regarding this one.
>
>The libpng maintainer has responded, and is working towards a libpng-1.2.6
>with all the known security issues fixed. He is also making tweaks to my
>patch. You might want to contact him
>Glenn Randers-Pehrson <glennrp@glennrp.com>
>and work together to get the best combined patch.
>
>Cheers
>Chris
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mjc@redhat.com Mon Jul 19 09:56:05 2004
>Date: Thu, 15 Jul 2004 11:40:53 +0100 (BST)
>From: Mark J Cox <mjc@redhat.com>
>To: chris@scary.beasts.org
>Cc: Matthias Clasen <mclasen@redhat.com>, vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>> Hey, excellent. I'm glad I wasn't smoking regarding this one.
>
>Just to clarify when writing this up using CVE names:
>
>1. A stack-based buffer overflow in libpng which can be triggered to run
>arbitrary code by a malicious png file: CAN-2004-0597
>
>2. A NULL-pointer crash in libpng which can be triggered by a malicious
>png file: CAN-2004-0598
>
>3. Various possible integer overflows in libpng which may have security
>consequences:  CAN-2004-0599
>
>Mark
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mdz@debian.org Mon Jul 19 09:56:07 2004
>Date: Fri, 16 Jul 2004 14:13:04 -0700
>From: Matt Zimmerman <mdz@debian.org>
>To: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>On Thu, Jul 15, 2004 at 11:40:53AM +0100, Mark J Cox wrote:
>
>> > Hey, excellent. I'm glad I wasn't smoking regarding this one.
>> 
>> Just to clarify when writing this up using CVE names:
>> 
>> 1. A stack-based buffer overflow in libpng which can be triggered to run
>> arbitrary code by a malicious png file: CAN-2004-0597
>> 
>> 2. A NULL-pointer crash in libpng which can be triggered by a malicious
>> png file: CAN-2004-0598
>> 
>> 3. Various possible integer overflows in libpng which may have security
>> consequences:  CAN-2004-0599
>
>CERT's advisory doesn't seem to be clear on the CVE names; you may want to
>send them a correction.
>
>-- 
> - mdz
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mclasen@redhat.com Mon Jul 19 09:56:09 2004
>Date: Thu, 15 Jul 2004 09:51:10 -0400
>From: Matthias Clasen <mclasen@redhat.com>
>To: chris@scary.beasts.org
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>On Wed, 2004-07-14 at 15:33, chris@scary.beasts.org wrote:
>
>> > 4) Here is a fix for the theoretical overflow in png_handle_sPLT which
>> > you mention:
>> 
>> I don't think this is needed because of the added checks against all PNG
>> chunk lengths being <= PNG_UINT_MAX (which is actually defined as
>> the C standard INT_MAX). Running through this function with length set
>> at PNG_UINT_MAX, I don't think the overflow can trigger. Let me know if
>> I'm wrong.
>> 
>
>Hmm, I guess that depends on how the compiler packs structs. length
>can be no larger than 2^31-1, thus slength and data_length are also
><= 2^31-1. Thus newpalette.nentries can be no larger than 
>2^31-1/6 = 0x15555555. Now if sizeof (png_sPLT_entry) is 10, we end up
>allocating not more than 0xD5555552 < 2^32 bytes, but if the size of the
>struct is 20 (is the compiler actually allowed to do that ?), then
>we may end up allocating 0x1AAAAAAA4 > 2^32 bytes and overflow.
>
>Matthias
>
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From chris@scary.beasts.org Mon Jul 19 09:56:11 2004
>Date: Thu, 15 Jul 2004 17:06:37 +0100 (BST)
>From: chris@scary.beasts.org
>To: Matthias Clasen <mclasen@redhat.com>
>Cc: vendor-sec@lst.de
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>Hi,
>
>On Thu, 15 Jul 2004, Matthias Clasen wrote:
>
>> Hmm, I guess that depends on how the compiler packs structs. length
>> can be no larger than 2^31-1, thus slength and data_length are also
>> <= 2^31-1. Thus newpalette.nentries can be no larger than
>> 2^31-1/6 = 0x15555555. Now if sizeof (png_sPLT_entry) is 10, we end up
>> allocating not more than 0xD5555552 < 2^32 bytes, but if the size of the
>> struct is 20 (is the compiler actually allowed to do that ?), then
>> we may end up allocating 0x1AAAAAAA4 > 2^32 bytes and overflow.
>
>Good point - better to be safe than sorry.
>
>Cheers
>Chris
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From trini@mvista.com Mon Jul 19 09:56:13 2004
>Date: Fri, 16 Jul 2004 09:00:49 -0700
>From: Tom Rini <trini@mvista.com>
>To: Matthias Clasen <mclasen@redhat.com>
>Cc: vendor-sec@lst.de, chris@scary.beasts.org
>Subject: Re: [vendor-sec] Nasty security hole in libpng
>
>On Wed, Jul 14, 2004 at 01:38:14PM -0400, Matthias Clasen wrote:
>
>[snip]
>> 4) Here is a fix for the theoretical overflow in png_handle_sPLT which
>> you mention:
>> 
>> --- libpng-1.2.5/pngrutil.c.chunklength	2002-10-03 07:32:30.000000000
>> -0400
>> +++ libpng-1.2.5/pngrutil.c	2004-07-14 12:27:03.000000000 -0400
>> @@ -1154,6 +1154,8 @@
>>     }
>>  
>>     new_palette.nentries = data_length / entry_size;
>> +   if (new_palette.nentries > PNG_MAX_UINT / sizeof(png_sPLT_entry))
>> +     png_error(png_ptr, "sPLT chunk too long");
>>     new_palette.entries = (png_sPLT_entryp)png_malloc(
>>         png_ptr, new_palette.nentries * sizeof(png_sPLT_entry));
>
>I've taken this, along with Chris' original patch, and backported to
>libpng-1.0.8 (which seg faults on both of the images otherwise).  The
>only thing I'm not totally sure about is defining PNG_MAX_PALETTE_LENGTH
>to 256.  In 1.0.x it looks like all of these areas are dynamically
>allocated, without an explicit max.
>
>diff -zruN libpng-1.0.8.orig/png.h libpng-1.0.8/png.h
>--- libpng-1.0.8.orig/png.h	2000-07-24 04:34:08.000000000 -0700
>+++ libpng-1.0.8/png.h	2004-07-15 10:03:30.410896800 -0700
>@@ -752,6 +752,12 @@
> /* Maximum positive integer used in PNG is (2^31)-1 */
> #define PNG_MAX_UINT ((png_uint_32)0x7fffffffL)
> 
>+/* Constraints on width, height, (2 ^ 24) - 1*/
>+#define PNG_MAX_DIMENSION 16777215
>+
>+/* Assumed constraint on pallete size, 256 (from libpng 1.2.x) */
>+#define PNG_MAX_PALETTE_LENGTH 256
>+
> /* These describe the color_type field in png_info. */
> /* color type masks */
> #define PNG_COLOR_MASK_PALETTE    1
>diff -zruN libpng-1.0.8.orig/pngpread.c libpng-1.0.8/pngpread.c
>--- libpng-1.0.8.orig/pngpread.c	2000-07-24 04:34:10.000000000 -0700
>+++ libpng-1.0.8/pngpread.c	2004-07-15 10:03:30.411896570 -0700
>@@ -209,6 +209,8 @@
> 
>       png_push_fill_buffer(png_ptr, chunk_length, 4);
>       png_ptr->push_length = png_get_uint_32(chunk_length);
>+      if (png_ptr->push_length > PNG_MAX_UINT)
>+         png_error(png_ptr, "Invalid chunk length.");
>       png_reset_crc(png_ptr);
>       png_crc_read(png_ptr, png_ptr->chunk_name, 4);
>       png_ptr->mode |= PNG_HAVE_CHUNK_HEADER;
>@@ -628,6 +630,8 @@
> 
>       png_push_fill_buffer(png_ptr, chunk_length, 4);
>       png_ptr->push_length = png_get_uint_32(chunk_length);
>+      if (png_ptr->push_length > PNG_MAX_UINT)
>+         png_error(png_ptr, "Invalid chunk length.");
> 
>       png_reset_crc(png_ptr);
>       png_crc_read(png_ptr, png_ptr->chunk_name, 4);
>diff -zruN libpng-1.0.8.orig/pngrutil.c libpng-1.0.8/pngrutil.c
>--- libpng-1.0.8.orig/pngrutil.c	2000-07-24 04:34:11.000000000 -0700
>+++ libpng-1.0.8/pngrutil.c	2004-07-15 10:07:02.029166062 -0700
>@@ -320,7 +320,11 @@
>    png_crc_finish(png_ptr, 0);
> 
>    width = png_get_uint_32(buf);
>+   if (width > PNG_MAX_DIMENSION)
>+      png_error(png_ptr, "Width is too large");
>    height = png_get_uint_32(buf + 4);
>+   if (height > PNG_MAX_DIMENSION)
>+      png_error(png_ptr, "Height is too large");
>    bit_depth = buf[8];
>    color_type = buf[9];
>    compression_type = buf[10];
>@@ -672,7 +676,7 @@
>    else
>       truelen = (png_size_t)png_ptr->channels;
> 
>-   if (length != truelen)
>+   if (length != truelen || length > 4)
>    {
>       png_warning(png_ptr, "Incorrect sBIT chunk length");
>       png_crc_finish(png_ptr, length);
>@@ -1118,6 +1122,8 @@
>    }
> 
>    new_palette.nentries = data_length / entry_size;
>+   if (new_palette.nentries > PNG_MAX_UINT / sizeof(png_sPLT_entry))
>+     png_error(png_ptr, "sPLT chunk too long");
>    new_palette.entries = (png_sPLT_entryp)png_malloc(
>        png_ptr, new_palette.nentries * sizeof(png_sPLT_entry));
> 
>@@ -1203,7 +1209,8 @@
>          /* Should be an error, but we can cope with it */
>          png_warning(png_ptr, "Missing PLTE before tRNS");
>       }
>-      else if (length > (png_uint_32)png_ptr->num_palette)
>+      if (length > (png_uint_32)png_ptr->num_palette ||
>+          length > PNG_MAX_PALETTE_LENGTH)
>       {
>          png_warning(png_ptr, "Incorrect tRNS chunk length");
>          png_crc_finish(png_ptr, length);
>@@ -1369,7 +1376,7 @@
> void /* PRIVATE */
> png_handle_hIST(png_structp png_ptr, png_infop info_ptr, png_uint_32 length)
> {
>-   int num, i;
>+   unsigned int num, i;
> 
>    png_debug(1, "in png_handle_hIST\n");
> 
>@@ -1394,14 +1401,14 @@
>       return;
>    }
> 
>-   if (length != (png_uint_32)(2 * png_ptr->num_palette))
>+   num = length / 2 ;
>+   if (num != png_ptr->num_palette || num > PNG_MAX_PALETTE_LENGTH)
>    {
>       png_warning(png_ptr, "Incorrect hIST chunk length");
>       png_crc_finish(png_ptr, length);
>       return;
>    }
> 
>-   num = (int)length / 2 ;
>    png_ptr->hist = (png_uint_16p)png_malloc(png_ptr,
>       (png_uint_32)(num * sizeof (png_uint_16)));
>    for (i = 0; i < num; i++)
>@@ -2774,6 +2781,9 @@
>                png_read_data(png_ptr, chunk_length, 4);
>                png_ptr->idat_size = png_get_uint_32(chunk_length);
> 
>+               if (png_ptr->idat_size > PNG_MAX_UINT)
>+                  png_error(png_ptr, "Invalid chunk length.");
>+
>                png_reset_crc(png_ptr);
>                png_crc_read(png_ptr, png_ptr->chunk_name, 4);
>                if (png_memcmp(png_ptr->chunk_name, (png_bytep)png_IDAT, 4))
>
>-- 
>Tom
>
>    [ Part 2, "Digital signature"  Application/PGP-SIGNATURE  196bytes. ]
>    [ Unable to print this part. ]
>
>
>From sarnold@immunix.com Mon Jul 19 09:56:15 2004
>Date: Tue, 13 Jul 2004 14:53:17 -0700
>From: Seth Arnold <sarnold@immunix.com>
>To: chris@scary.beasts.org
>Cc: vendor-sec@lst.de
>Subject: Re: [Security] [vendor-sec] Nasty security hole in libpng
>
>On Tue, Jul 13, 2004 at 08:30:51PM +0100, chris@scary.beasts.org wrote:
>> I'm not sure what to do with the advisory; libpng doesn't seem
>> particularly maintained. Who did people liase with in the case of the
>> recent libpng problems? Ideas appreciated.
>
>Back in 2002, this email thread announced some security fixes:
>ftp://swrinde.nde.swri.edu/pub/png-group/archives/png-list.200207
>The guy posting the new release is:
>Glenn Randers-Pehrson <glennrp@comcast.net>
>
>It appears he is still active on the list:
>ftp://swrinde.nde.swri.edu/pub/png-group/archives/png-list.200406
>
>Your guess is as good as mine if he's "the maintainer".
>
>    [ Part 2, Application/PGP-SIGNATURE  196bytes. ]
>    [ Unable to print this part. ]
>
>
>From chris@scary.beasts.org Mon Jul 19 09:56:18 2004
>Date: Tue, 13 Jul 2004 23:20:05 +0100 (BST)
>From: chris@scary.beasts.org
>To: Seth Arnold <sarnold@immunix.com>
>Cc: vendor-sec@lst.de
>Subject: Re: [Security] [vendor-sec] Nasty security hole in libpng
>
>Hi,
>
>On Tue, 13 Jul 2004, Seth Arnold wrote:
>
>> Back in 2002, this email thread announced some security fixes:
>> ftp://swrinde.nde.swri.edu/pub/png-group/archives/png-list.200207
>> The guy posting the new release is:
>> Glenn Randers-Pehrson <glennrp@comcast.net>
>
>Ah, this does appear to be "the maintainer", according to the libpng home
>page. It's unfortunate that the last full release (1.2.5) is years old,
>and contains known security issues.
>
>Once we've agreed a patch and a timeline, I'll contact Glenn and try and
>persuade him that a 1.2.6 or 1.2.5.1 release is warranted with both the
>old and new security fixes present.
>
>I aim to have an initial patch shortly.
>
>Cheers
>Chris
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From alan@lxorguk.ukuu.org.uk Mon Jul 19 09:56:21 2004
>Date: Wed, 14 Jul 2004 00:03:31 +0100
>From: Alan Cox <alan@lxorguk.ukuu.org.uk>
>To: chris@scary.beasts.org
>Cc: Seth Arnold <sarnold@immunix.com>, vendor-sec@lst.de
>Subject: Re: [Security] [vendor-sec] Nasty security hole in libpng
>
>On Maw, 2004-07-13 at 23:20, chris@scary.beasts.org wrote:
>> Once we've agreed a patch and a timeline, I'll contact Glenn and try and
>> persuade him that a 1.2.6 or 1.2.5.1 release is warranted with both the
>> old and new security fixes present.
>
>If you haven't already you want to also talk to CERT because libpng is
>used in a very large collection of places from cellphones to
>supercomputing. vendor-sec isn't going to reach many of them
>
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec
>
>From mjo@dojo.mi.org Mon Jul 19 09:56:24 2004
>Date: Wed, 14 Jul 2004 08:45:32 -0400 (EDT)
>From: Mike O'Connor <mjo@dojo.mi.org>
>To: vendor-sec List <vendor-sec@lst.de>
>Subject: Re: [Security] [vendor-sec] Nasty security hole in libpng
>
>:On Tue, Jul 13, 2004 at 08:30:51PM +0100, chris@scary.beasts.org wrote:
>:> I'm not sure what to do with the advisory; libpng doesn't seem
>:> particularly maintained. Who did people liase with in the case of the
>:> recent libpng problems? Ideas appreciated.
>:
>:Back in 2002, this email thread announced some security fixes:
>:ftp://swrinde.nde.swri.edu/pub/png-group/archives/png-list.200207
>:The guy posting the new release is:
>:Glenn Randers-Pehrson <glennrp@comcast.net>
>:
>:It appears he is still active on the list:
>:ftp://swrinde.nde.swri.edu/pub/png-group/archives/png-list.200406
>:
>:Your guess is as good as mine if he's "the maintainer".
>
>FWIW, the maintainer of the libpng.org WWW site (and a developer in his
>own right) is Greg Roelofs.  His wife was the manager of security at SGI
>for awhile.  If you're not getting any attention through the front door, 
>we may have ways of making them talk...  :)
>
>It'd be nice if we could get the sourceforge.net people to add some
>sort of "Security" option to the top of the sf menubar (along with
>Summary|Admin|Home Page|etc.), which would point to "security issues
>fixed" and "how to report security issues against this project".  If
>done in a consistent way, we could track all the security URLs on SF
>for changes.  No guarantee that any project would necessarily fill
>this stuff out, but it's a starting point...
>
>-- 
> Mail: mjo@dojo.mi.org  WWW: http://dojo.mi.org/~mjo/  Phone: +1 248 427 4481
> =--==--==--==--==--==--==--==--==--==--==--==--==--==--==--==--==--==--==--=
>"Take me out... to the black.  Tell 'em I ain't comin' back."        -Firefly
>_______________________________________________
>Vendor Security mailing list
>Vendor Security@lst.de
>https://www.lst.de/cgi-bin/mailman/listinfo/vendor-sec

Actions: View

Attachments on bug 58008: 22160 | 22164 | 22165 | 22251 | 22252