|
From: <bn...@pl...> - 2007-10-05 18:03:55
|
Hi, I think I found a memory corruption bug (and potential security problem) in the 1.2.21 release of libpng. Sorry if this was already reported, but I didn't see any mention of it on the archives. It seems likely this is caused by an incorrect fix for the "Suspicious `sizeof' line 694 of pngset.c" problem reported by Jeff Phillips about a month ago. http://sourceforge.net/mailarchive/forum.php?thread_name=5122753600C3E94F87FBDFFCC090D1FF0400EA68%40MERCMBX07.na.sas.com&forum_name=png-mng-implement http://tinyurl.com/2h2vwd pngset.c, png_set_iCCP(), lines 687-693: > new_iccp_name=(png_charp)png_malloc_warn(png_ptr, png_strlen(name)+1); > if (new_iccp_name == NULL) > { > png_warning(png_ptr, "Insufficient memory to process iCCP chunk."); > return; > } > png_strncpy(new_iccp_name, name, png_strlen(new_iccp_name)+1); That string copy is using the wrong size, it needs to be png_strlen(name) instead, right? That was the fix Jeff suggested originally, it looks like it was just a copy-paste error or typo. Thanks, Ben |
|
From: Jeff P. <Jef...@sa...> - 2007-10-05 18:31:12
|
I would concur with Ben and I didn't even catch that when I picked up the new 1.2.21, nor 1.2.21rc3. The allocation should include the +1, but the strcpy should not. Jeff Phillips -----Original Message----- From: png...@li... [mailto:png...@li...] On Behalf Of bn...@pl... Sent: Friday, October 05, 2007 2:04 PM To: png...@li... Subject: [png-mng-implement] libpng 1.2.21 iCCP chunk handling bug Hi, I think I found a memory corruption bug (and potential security problem) in the 1.2.21 release of libpng. Sorry if this was already reported, but I didn't see any mention of it on the archives. It seems likely this is caused by an incorrect fix for the "Suspicious `sizeof' line 694 of pngset.c" problem reported by Jeff Phillips about a month ago. http://sourceforge.net/mailarchive/forum.php?thread_name=3D5122753600C3E9= 4 F87FBDFFCC090D1FF0400EA68%40MERCMBX07.na.sas.com&forum_name=3Dpng-mng-imp= l ement http://tinyurl.com/2h2vwd pngset.c, png_set_iCCP(), lines 687-693: > new_iccp_name=3D(png_charp)png_malloc_warn(png_ptr, = png_strlen(name)+1); > if (new_iccp_name =3D=3D NULL) > { > png_warning(png_ptr, "Insufficient memory to process iCCP chunk."); > return; > } > png_strncpy(new_iccp_name, name, png_strlen(new_iccp_name)+1); That string copy is using the wrong size, it needs to be png_strlen(name) instead, right? That was the fix Jeff suggested originally, it looks like it was just a copy-paste error or typo. Thanks, Ben ------------------------------------------------------------------------ - This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/ _______________________________________________ png-mng-implement mailing list png...@li... https://lists.sourceforge.net/lists/listinfo/png-mng-implement |
|
From: Glenn Randers-P. <gl...@co...> - 2007-10-05 18:32:02
|
At 11:03 AM 10/5/2007 -0700, bn...@pl... wrote: >pngset.c, png_set_iCCP(), lines 687-693: >> png_strncpy(new_iccp_name, name, png_strlen(new_iccp_name)+1); > >That string copy is using the wrong size, it needs to be >png_strlen(name) instead, right Right. Thanks, will fix in 1.2.22 Glenn |
|
From: Glenn Randers-P. <gl...@co...> - 2007-10-05 18:39:56
|
At 02:30 PM 10/5/2007 -0400, Jeff Phillips wrote: >The allocation should include the +1, but the strcpy should not. I think it's OK to copy the terminating NULL. Glenn |
|
From: <bn...@pl...> - 2007-10-05 19:44:30
|
Hi, I think it's not only OK, I think it would be a bug to not copy the null terminating character. This is because png_malloc_warn() is not zero filling the allocated string as far as I can tell, and strncpy() is not going to add the null terminator for you in this case: "If there is no null byte in the first n bytes of the array pointed to by s2, the result will not be null-terminated." - http://www.opengroup.org/pubs/online/7908799/xsh/strncpy.html -Ben Glenn Randers-Pehrson wrote: > At 02:30 PM 10/5/2007 -0400, Jeff Phillips wrote: >> The allocation should include the +1, but the strcpy should not. > > I think it's OK to copy the terminating NULL. > > Glenn |
|
From: Jeff P. <Jef...@sa...> - 2007-10-05 19:47:33
|
You are correct, again. I was thinking that this memory was getting the memset done on it. I'm trying to balance too many things at work today and am not paying close attention to details :) -----Original Message----- From: png...@li... [mailto:png...@li...] On Behalf Of bn...@pl... Sent: Friday, October 05, 2007 3:44 PM To: PNG/MNG implementation discussion list Subject: Re: [png-mng-implement] libpng 1.2.21 iCCP chunk handling bug Hi, I think it's not only OK, I think it would be a bug to not copy the null terminating character. This is because png_malloc_warn() is not zero filling the allocated string as far as I can tell, and strncpy() is not going to add the null terminator for you in this case: "If there is no null byte in the first n bytes of the array pointed to by s2, the result will not be null-terminated." - http://www.opengroup.org/pubs/online/7908799/xsh/strncpy.html -Ben Glenn Randers-Pehrson wrote: > At 02:30 PM 10/5/2007 -0400, Jeff Phillips wrote: >> The allocation should include the +1, but the strcpy should not. >=20 > I think it's OK to copy the terminating NULL. >=20 > Glenn ------------------------------------------------------------------------ - This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/ _______________________________________________ png-mng-implement mailing list png...@li... https://lists.sourceforge.net/lists/listinfo/png-mng-implement |