CESA-2004-001 - rev 3 libPNG 1.2.5 stack-based buffer overflow and other code concerns ================================================================ Programs : libpng users including mozilla, konqueror, various e-mail clients, generally lots. Also reports that some versions of IE are vulnerable to some of the problems. Severity : - A malicious website serving a malicious PNG file could compromise the browsers of visitors. - A malicious PNG could be sent via e-mail and compromise the e-mail viewer of the recipient. - For systems with user-providable images for "face browsers", a local system compromise could be possible via a malicious PNG. CAN identifier(s): CAN-2004-0597 (the serious one), CAN-2004-0598, CAN-2004-0599 CERT VU#s : VU#388984 (the serious one), VU#236656, VU#160448, VU#477512, VU#817368, VU#286464 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. A patch which should plug all these issues is appended beneath the advisory. NOTE! This patch serves as demo purposes for the flaws only. An official v1.2.6 libpng with an official, slightly different fix will be released by the libpng team in parallel with this advisory. 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. A scarier possibility is targetted exploitation by e-mailing a nasty PNG to someone who uses a graphical e-mail client to decode PNGs with a vulnerable libpng. 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 - rev 3 Chris Evans chris@scary.beasts.org [Advertisement: I am interested in moving into a security related field full-time. E-mail me to discuss.] 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))