From cf49919686fc9bb3353f1539cdc7593a0c0621fe Mon Sep 17 00:00:00 2001 From: John Bowler Date: Thu, 1 Mar 2012 21:54:07 -0600 Subject: [PATCH] [libpng16] Fixed some bugs in ICC profile writing. The code should now accept all potentially valid ICC profiles and reject obviously invalid ones. It now uses png_error() to do so rather than casually writing a PNG without the necessary color data. --- ANNOUNCE | 4 ++ CHANGES | 4 ++ contrib/libtests/makepng.c | 27 ++++++-- contrib/libtests/pngstest.c | 4 +- pngpriv.h | 2 +- pngwrite.c | 2 +- pngwutil.c | 128 ++++++++++++++++++------------------ 7 files changed, 98 insertions(+), 73 deletions(-) diff --git a/ANNOUNCE b/ANNOUNCE index a90be28b7..1ed89a8de 100644 --- a/ANNOUNCE +++ b/ANNOUNCE @@ -252,6 +252,10 @@ Version 1.6.0beta15 [March 2, 2012] still work-in-progress. Added tests for invalid palette index while reading and writing (work in progress, the latter isn't finished). + Fixed some bugs in ICC profile writing. The code should now accept + all potentially valid ICC profiles and reject obviously invalid ones. + It now uses png_error() to do so rather than casually writing a PNG + without the necessary color data. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/CHANGES b/CHANGES index 6efb168d0..aa6bf632a 100644 --- a/CHANGES +++ b/CHANGES @@ -4004,6 +4004,10 @@ Version 1.6.0beta15 [March 2, 2012] still work-in-progress. Added tests for invalid palette index while reading and writing (work in progress, the latter isn't finished). + Fixed some bugs in ICC profile writing. The code should now accept + all potentially valid ICC profiles and reject obviously invalid ones. + It now uses png_error() to do so rather than casually writing a PNG + without the necessary color data. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/contrib/libtests/makepng.c b/contrib/libtests/makepng.c index e6a0436dc..e4b01f090 100644 --- a/contrib/libtests/makepng.c +++ b/contrib/libtests/makepng.c @@ -749,7 +749,10 @@ insert_iCCP(png_structp png_ptr, png_infop info_ptr, int nparams, params[1], (unsigned long)fake_len); exit(1); } - proflen = (png_uint_32)fake_len; + proflen = (png_uint_32)(fake_len & ~3U); + /* Always fix up the profile length. */ + png_save_uint_32(profile, proflen); + break; } } @@ -777,7 +780,7 @@ insert_iCCP(png_structp png_ptr, png_infop info_ptr, int nparams, { fprintf(stderr, "--insert iCCP %s: profile length field wrong:\n", params[1]); - fprintf(stderr, " actual %lu, recorded value %lu (correted)\n", + fprintf(stderr, " actual %lu, recorded value %lu (corrected)\n", (unsigned long)proflen, (unsigned long)prof_header); png_save_uint_32(profile, proflen); } @@ -821,6 +824,20 @@ set_text(png_structp png_ptr, png_infop info_ptr, png_textp text, } break; + case '0': case '1': case '2': case '3': case '4': + case '5': case '6': case '7': case '8': case '9': + { + png_bytep data = NULL; + png_size_t fake_len = load_fake(param, &data); + + if (fake_len > 0) /* else a simple parameter */ + { + text->text_length = fake_len; + text->text = (png_charp)data; + break; + } + } + default: text->text = param; break; @@ -864,9 +881,9 @@ insert_iTXt(png_structp png_ptr, png_infop info_ptr, int nparams, check_param_count(nparams, 4); clear_text(&text, params[0]); text.compression = 2; /* iTXt + deflate */ - text.lang = params[2];/* language tag */ - text.lang_key = params[3]; /* translated keyword */ - set_text(png_ptr, info_ptr, &text, params[1]); + text.lang = params[1];/* language tag */ + text.lang_key = params[2]; /* translated keyword */ + set_text(png_ptr, info_ptr, &text, params[3]); } static void diff --git a/contrib/libtests/pngstest.c b/contrib/libtests/pngstest.c index fab845be1..5bb2701a7 100644 --- a/contrib/libtests/pngstest.c +++ b/contrib/libtests/pngstest.c @@ -1938,7 +1938,7 @@ static png_uint_16 gpc_error[16/*in*/][16/*out*/][4/*a*/] = { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 } }, { /* input: sRGB-rgb+alpha */ - { 0, 4, 7, 0 }, { 0, 14, 7, 0 }, { 0, 19, 0, 0 }, { 0, 0, 0, 0 }, + { 0, 4, 13, 0 }, { 0, 14, 13, 0 }, { 0, 19, 0, 0 }, { 0, 0, 0, 0 }, { 0, 832, 764, 0 }, { 0, 832, 764, 0 }, { 0, 897, 788, 0 }, { 0, 897, 788, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 } @@ -2013,7 +2013,7 @@ static png_uint_16 gpc_error_via_linear[16][4/*out*/][4] = }, { /* input: sRGB-rgb */ { 0, 0, 19, 0 }, { 0, 0, 19, 0 }, { 0, 0, 15, 0 }, { 0, 0, 15, 0 } }, { /* input: sRGB-rgb+alpha */ - { 0, 9, 10, 0 }, { 0, 180, 10, 0 }, { 0, 14, 15, 0 }, { 0, 186, 15, 0 } + { 0, 12, 14, 0 }, { 0, 180, 14, 0 }, { 0, 14, 15, 0 }, { 0, 186, 15, 0 } }, { /* input: linear-gray */ { 0, 0, 1, 0 }, { 0, 0, 1, 0 }, { 0, 0, 1, 0 }, { 0, 0, 1, 0 } }, { /* input: linear-gray+alpha */ diff --git a/pngpriv.h b/pngpriv.h index 9062367e6..56a545fb1 100644 --- a/pngpriv.h +++ b/pngpriv.h @@ -898,7 +898,7 @@ PNG_INTERNAL_FUNCTION(void,png_write_sRGB,(png_structrp png_ptr, #ifdef PNG_WRITE_iCCP_SUPPORTED PNG_INTERNAL_FUNCTION(void,png_write_iCCP,(png_structrp png_ptr, png_const_charp name, int compression_type, - png_const_charp profile, int proflen),PNG_EMPTY); + png_const_bytep profile, png_uint_32 proflen),PNG_EMPTY); /* Note to maintainer: profile should be png_bytep */ #endif diff --git a/pngwrite.c b/pngwrite.c index c6f772a74..1794d3ef2 100644 --- a/pngwrite.c +++ b/pngwrite.c @@ -73,7 +73,7 @@ png_write_info_before_PLTE(png_structrp png_ptr, png_inforp info_ptr) #ifdef PNG_WRITE_iCCP_SUPPORTED if (info_ptr->valid & PNG_INFO_iCCP) png_write_iCCP(png_ptr, info_ptr->iccp_name, PNG_COMPRESSION_TYPE_BASE, - (png_charp)info_ptr->iccp_profile, (int)info_ptr->iccp_proflen); + info_ptr->iccp_profile, info_ptr->iccp_proflen); #endif #ifdef PNG_WRITE_sBIT_SUPPORTED if (info_ptr->valid & PNG_INFO_sBIT) diff --git a/pngwutil.c b/pngwutil.c index 35cb31e02..e3a0b21af 100644 --- a/pngwutil.c +++ b/pngwutil.c @@ -373,7 +373,7 @@ typedef struct } compression_state; /* Compress given text into storage in the png_ptr structure */ -static int /* PRIVATE */ +static png_size_t /* PRIVATE */ png_text_compress(png_structrp png_ptr, png_const_charp text, png_size_t text_len, int compression, compression_state *comp) @@ -390,7 +390,7 @@ png_text_compress(png_structrp png_ptr, if (compression == PNG_TEXT_COMPRESSION_NONE) { comp->input = (png_const_bytep)text; - return((int)text_len); + return text_len; } if (compression >= PNG_TEXT_COMPRESSION_LAST) @@ -419,7 +419,7 @@ png_text_compress(png_structrp png_ptr, png_zlib_claim(png_ptr, PNG_ZLIB_FOR_TEXT); /* Set up the compression buffers */ - /* TODO: the following cast hides a potential overflow problem. */ + /* TODO: the following cast hides a ****CERTAIN**** overflow problem. */ png_ptr->zstream.avail_in = (uInt)text_len; /* NOTE: assume zlib doesn't overwrite the input */ @@ -516,9 +516,8 @@ png_text_compress(png_structrp png_ptr, old_ptr = comp->output_ptr; /* This could be optimized to realloc() */ - comp->output_ptr = (png_bytepp)png_malloc(png_ptr, - (png_alloc_size_t)(comp->max_output_ptr * - png_sizeof(png_charp))); + comp->output_ptr = png_voidcast(png_bytepp,png_malloc(png_ptr, + comp->max_output_ptr * png_sizeof(png_charp))); png_memcpy(comp->output_ptr, old_ptr, old_max * png_sizeof(png_charp)); @@ -527,15 +526,13 @@ png_text_compress(png_structrp png_ptr, } else - comp->output_ptr = (png_bytepp)png_malloc(png_ptr, - (png_alloc_size_t)(comp->max_output_ptr * - png_sizeof(png_charp))); + comp->output_ptr = png_voidcast(png_bytepp,png_malloc(png_ptr, + comp->max_output_ptr * png_sizeof(png_charp))); } /* Save the data */ - comp->output_ptr[comp->num_output_ptr] = - (png_bytep)png_malloc(png_ptr, - (png_alloc_size_t)png_ptr->zbuf_size); + comp->output_ptr[comp->num_output_ptr] = png_voidcast(png_bytep, + png_malloc(png_ptr, png_ptr->zbuf_size)); png_memcpy(comp->output_ptr[comp->num_output_ptr], png_ptr->zbuf, png_ptr->zbuf_size); @@ -559,12 +556,13 @@ png_text_compress(png_structrp png_ptr, } while (ret != Z_STREAM_END); /* Text length is number of buffers plus last buffer */ + /* TODO: this ****WILL**** overflow */ text_len = png_ptr->zbuf_size * comp->num_output_ptr; if (png_ptr->zstream.avail_out < png_ptr->zbuf_size) - text_len += png_ptr->zbuf_size - (png_size_t)png_ptr->zstream.avail_out; + text_len += png_ptr->zbuf_size - png_ptr->zstream.avail_out; - return((int)text_len); + return text_len; } /* Ship the compressed text out via chunk writes */ @@ -641,12 +639,13 @@ png_write_compressed_data_out(png_structrp png_ptr, compression_state *comp) } else - png_error(png_ptr, + png_warning(png_ptr, /* must be a warning or the data isn't freed */ "Invalid zlib compression method or flags in non-IDAT chunk"); } #endif /* PNG_WRITE_OPTIMIZE_CMF_SUPPORTED */ /* Write saved output buffers, if any */ + /* WARNING: SIDE EFFECT; the code below is what actually frees the data */ for (i = 0; i < comp->num_output_ptr; i++) { png_write_chunk_data(png_ptr, comp->output_ptr[i], @@ -1092,79 +1091,80 @@ png_write_sRGB(png_structrp png_ptr, int srgb_intent) /* Write an iCCP chunk */ void /* PRIVATE */ png_write_iCCP(png_structrp png_ptr, png_const_charp name, int compression_type, - png_const_charp profile, int profile_len) + png_const_bytep profile, png_uint_32 profile_len) { - png_size_t name_len; + png_size_t name_len, compressed_profile_len; png_charp new_name; compression_state comp; - int embedded_profile_len = 0; + png_uint_32 embedded_profile_len = 0; png_debug(1, "in png_write_iCCP"); + if (compression_type != PNG_COMPRESSION_TYPE_BASE) + png_error(png_ptr, "Unknown compression type for iCCP chunk"); + + if (profile == NULL) + png_error(png_ptr, "No profile for iCCP chunk"); + + if (profile_len < 132) + png_error(png_ptr, "ICC profile too short"); + + if (profile_len & 3) + png_error(png_ptr, "ICC profile length invalid (not a multiple of 4)"); + + embedded_profile_len = png_get_uint_32(profile); + + if (profile_len != embedded_profile_len) + png_error(png_ptr, "Profile length does not match profile"); + + if ((png_size_t)profile_len != profile_len) + { + /* This isn't an application error, technically, but all the same do + * not short-change the app by writing a PNG without the profile! + */ + png_error(png_ptr, "Profile length exceeds system limits"); + } + + if ((name_len = png_check_keyword(png_ptr, name, &new_name)) == 0) + return; + comp.num_output_ptr = 0; comp.max_output_ptr = 0; comp.output_ptr = NULL; comp.input = NULL; comp.input_len = 0; - if ((name_len = png_check_keyword(png_ptr, name, &new_name)) == 0) - return; + compressed_profile_len = png_text_compress(png_ptr, (png_const_charp)profile, + (png_size_t)/*ok*/profile_len, PNG_COMPRESSION_TYPE_BASE, &comp); - if (compression_type != PNG_COMPRESSION_TYPE_BASE) - png_warning(png_ptr, "Unknown compression type in iCCP chunk"); - - if (profile == NULL) - profile_len = 0; - - if (profile_len > 3) - embedded_profile_len = - ((*( (png_const_bytep)profile ))<<24) | - ((*( (png_const_bytep)profile + 1))<<16) | - ((*( (png_const_bytep)profile + 2))<< 8) | - ((*( (png_const_bytep)profile + 3)) ); - - if (embedded_profile_len < 0) + /* 'name_len' is 1..79, so the following is safe: */ + if (compressed_profile_len > PNG_UINT_31_MAX - name_len - 2) { - png_warning(png_ptr, - "Embedded profile length in iCCP chunk is negative"); - png_free(png_ptr, new_name); - return; + + /* TODO: there is no 'compression_free' function */ + if (comp.output_ptr) + { + int i; + for (i = 0; i < comp.num_output_ptr; i++) + png_free(png_ptr, comp.output_ptr[i]); + png_free(png_ptr, comp.output_ptr); + } + + png_error(png_ptr, "Compressed profile exceeds PNG size limits"); } - if (profile_len < embedded_profile_len) - { - png_warning(png_ptr, - "Embedded profile length too large in iCCP chunk"); - - png_free(png_ptr, new_name); - return; - } - - if (profile_len > embedded_profile_len) - { - png_warning(png_ptr, - "Truncating profile to actual length in iCCP chunk"); - - profile_len = embedded_profile_len; - } - - if (profile_len) - profile_len = png_text_compress(png_ptr, profile, - (png_size_t)profile_len, PNG_COMPRESSION_TYPE_BASE, &comp); - /* Make sure we include the NULL after the name and the compression type */ png_write_chunk_header(png_ptr, png_iCCP, - (png_uint_32)(name_len + profile_len + 2)); + (png_uint_32)/*ok*/(name_len + compressed_profile_len + 2)); new_name[name_len + 1] = 0x00; - png_write_chunk_data(png_ptr, (png_bytep)new_name, - (png_size_t)(name_len + 2)); + png_write_chunk_data(png_ptr, (png_bytep)new_name, name_len + 2); - if (profile_len) + if (compressed_profile_len) { - comp.input_len = profile_len; + comp.input_len = compressed_profile_len; png_write_compressed_data_out(png_ptr, &comp); }