From 92a7c79db2c962d04006b35e2603ba9d5ce75541 Mon Sep 17 00:00:00 2001 From: John Bowler Date: Wed, 10 Aug 2016 15:35:09 -0700 Subject: [PATCH] [libpng16] Reject oversized iCCP profile length The code now validates the ICC profile length against the user chunk limit before the buffer is allocated, as opposed to doing it while the buffer is read. This removes the potential to consume virtual address space with a carefully crafted ICC profile; only an issue on 32-bit systems where a valid profile can be up to 2^32-4 bytes in length. libpng never writes beyond the application supplied limit, but previously it did allocate a buffer of the size specified in the profile header. The exploitability of this is almost zero; the address space is released as soon as the PNG read completes. Also clean up PNG_DEBUG compile of pngtest.c. Signed-off-by: John Bowler --- ANNOUNCE | 6 +++-- CHANGES | 4 +++- png.c | 40 ++++++++++++++++++++++++++++--- pngpriv.h | 2 ++ pngtest.c | 71 ++++++++++++++++++++++++++++++------------------------- 5 files changed, 85 insertions(+), 38 deletions(-) diff --git a/ANNOUNCE b/ANNOUNCE index c84bee908..e94265b30 100644 --- a/ANNOUNCE +++ b/ANNOUNCE @@ -1,4 +1,4 @@ -Libpng 1.6.25beta01 - August 4, 2016 +Libpng 1.6.25beta01 - August 10, 2016 This is not intended to be a public release. It will be replaced within a few weeks by a public version or by another test version. @@ -25,7 +25,9 @@ Other information: Changes since the last public release (1.6.24): -Version 1.6.25beta01 [August 4, 2016] +Version 1.6.25beta01 [August 10, 2016] + Reject oversized iCCP profile immediately. + Clean up PNG_DEBUG compile of pngtest.c. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/CHANGES b/CHANGES index 39a021c74..df4cef626 100644 --- a/CHANGES +++ b/CHANGES @@ -5675,7 +5675,9 @@ Version 1.6.24rc03 [August 2, 2016] Version 1.6.24[August 4, 2016] No changes. -Version 1.6.24[August 4, 2016] +Version 1.6.25beta01 [August 10, 2016] + Reject oversized iCCP profile immediately. + Clean up PNG_DEBUG compile of pngtest.c. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/png.c b/png.c index bc366a8ed..98a6abc1c 100644 --- a/png.c +++ b/png.c @@ -1931,8 +1931,8 @@ png_colorspace_set_sRGB(png_const_structrp png_ptr, png_colorspacerp colorspace, static const png_byte D50_nCIEXYZ[12] = { 0x00, 0x00, 0xf6, 0xd6, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0xd3, 0x2d }; -int /* PRIVATE */ -png_icc_check_length(png_const_structrp png_ptr, png_colorspacerp colorspace, +static int /* bool */ +icc_check_length(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length) { if (profile_length < 132) @@ -1942,6 +1942,40 @@ png_icc_check_length(png_const_structrp png_ptr, png_colorspacerp colorspace, return 1; } +#ifdef PNG_READ_iCCP_SUPPORTED +int /* PRIVATE */ +png_icc_check_length(png_const_structrp png_ptr, png_colorspacerp colorspace, + png_const_charp name, png_uint_32 profile_length) +{ + if (!icc_check_length(png_ptr, colorspace, name, profile_length)) + return 0; + + /* This needs to be here because the 'normal' check is in + * png_decompress_chunk, yet this happens after the attempt to + * png_malloc_base the required data. We only need this on read; on write + * the caller supplies the profile buffer so libpng doesn't allocate it. See + * the call to icc_check_length below (the write case). + */ +# ifdef PNG_SET_USER_LIMITS_SUPPORTED + else if (png_ptr->user_chunk_malloc_max > 0 && + png_ptr->user_chunk_malloc_max < profile_length) + return png_icc_profile_error(png_ptr, colorspace, name, profile_length, + "exceeds application limits"); +# elif PNG_USER_CHUNK_MALLOC_MAX > 0 + else if (PNG_USER_CHUNK_MALLOC_MAX < profile_length) + return png_icc_profile_error(png_ptr, colorspace, name, profile_length, + "exceeds libpng limits"); +# else /* !SET_USER_LIMITS */ + /* This will get compiled out on all 32-bit and better systems. */ + else if (PNG_SIZE_MAX < profile_length) + return png_icc_profile_error(png_ptr, colorspace, name, profile_length, + "exceeds system limits"); +# endif /* !SET_USER_LIMITS */ + + return 1; +} +#endif /* READ_iCCP */ + int /* PRIVATE */ png_icc_check_header(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length, @@ -2377,7 +2411,7 @@ png_colorspace_set_ICC(png_const_structrp png_ptr, png_colorspacerp colorspace, if ((colorspace->flags & PNG_COLORSPACE_INVALID) != 0) return 0; - if (png_icc_check_length(png_ptr, colorspace, name, profile_length) != 0 && + if (icc_check_length(png_ptr, colorspace, name, profile_length) != 0 && png_icc_check_header(png_ptr, colorspace, name, profile_length, profile, color_type) != 0 && png_icc_check_tag_table(png_ptr, colorspace, name, profile_length, diff --git a/pngpriv.h b/pngpriv.h index fe3355d83..c7bcee365 100644 --- a/pngpriv.h +++ b/pngpriv.h @@ -1494,9 +1494,11 @@ PNG_INTERNAL_FUNCTION(int,png_colorspace_set_ICC,(png_const_structrp png_ptr, /* The 'name' is used for information only */ /* Routines for checking parts of an ICC profile. */ +#ifdef PNG_READ_iCCP_SUPPORTED PNG_INTERNAL_FUNCTION(int,png_icc_check_length,(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length), PNG_EMPTY); +#endif /* READ_iCCP */ PNG_INTERNAL_FUNCTION(int,png_icc_check_header,(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length, diff --git a/pngtest.c b/pngtest.c index 2738bb889..f68408113 100644 --- a/pngtest.c +++ b/pngtest.c @@ -514,10 +514,10 @@ typedef struct memory_information typedef memory_information *memory_infop; static memory_infop pinformation = NULL; -static int current_allocation = 0; -static int maximum_allocation = 0; -static int total_allocation = 0; -static int num_allocations = 0; +static png_alloc_size_t current_allocation = 0; +static png_alloc_size_t maximum_allocation = 0; +static png_alloc_size_t total_allocation = 0; +static png_alloc_size_t num_allocations = 0; png_voidp PNGCBAPI png_debug_malloc PNGARG((png_structp png_ptr, png_alloc_size_t size)); @@ -604,9 +604,10 @@ png_debug_free(png_structp png_ptr, png_voidp ptr) if (pinfo->pointer == ptr) { *ppinfo = pinfo->next; - current_allocation -= pinfo->size; - if (current_allocation < 0) + if (current_allocation < pinfo->size) fprintf(STDERR, "Duplicate free of memory\n"); + else + current_allocation -= pinfo->size; /* We must free the list element too, but first kill the memory that is to be freed. */ memset(ptr, 0x55, pinfo->size); @@ -938,6 +939,12 @@ test_one_file(PNG_CONST char *inname, PNG_CONST char *outname) read_user_chunk_callback); #endif +#ifdef PNG_SET_USER_LIMITS_SUPPORTED +# ifdef CHUNK_LIMIT /* from the build, for testing */ + png_set_chunk_malloc_max(read_ptr, CHUNK_LIMIT); +# endif /* CHUNK_LIMIT */ +#endif + #ifdef PNG_SETJMP_SUPPORTED pngtest_debug("Setting jmpbuf for read struct"); if (setjmp(png_jmpbuf(read_ptr))) @@ -1876,7 +1883,7 @@ main(int argc, char *argv[]) { int i; #if defined(PNG_USER_MEM_SUPPORTED) && PNG_DEBUG - int allocation_now = current_allocation; + png_alloc_size_t allocation_now = current_allocation; #endif for (i=2; i