|
From: Frank B. <s88...@ma...> - 2011-06-07 15:16:22
|
Hi, I have a small question about CVE-2004-0421: http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2004-0421 Between version 1.2.5 and 1.2.6 you changed the following lines in png_format_buffer: -- < png_memcpy(buffer+iout, error_message, 64); > png_strncpy(buffer+iout, error_message, 63); -- changelog: -- version 1.2.6beta3 [July 18, 2004] Fixed potential overrun in pngerror.c by using strncpy instead of memcpy. -- So I assume the "bug" was, that memcpy (defined as _fmemcpy) copies uninitialized/unallocated memory behind error_message into "buffer" if error_message is shorter than 63 bytes. In 1.2.23beta01: -- < png_strncpy(buffer+iout, error_message, 63); < buffer[iout+63] = '\0'; > png_strncpy(buffer+iout, error_message, PNG_MAX_ERROR_TEXT); > buffer[iout+PNG_MAX_ERROR_TEXT] = '\0'; -- 1.2.23beta02: -- < png_strncpy(buffer+iout, error_message, PNG_MAX_ERROR_TEXT); < buffer[iout+PNG_MAX_ERROR_TEXT] = '\0'; > png_memcpy(buffer+iout, error_message, PNG_MAX_ERROR_TEXT); -- resp. 1.2.23beta03: -- > buffer[iout+PNG_MAX_ERROR_TEXT-1] = '\0'; -- the change is reverted, but the Changelog is rather sparse: -- version 1.2.23beta02 [October 16, 2007] Eliminated png_strncpy() and png_strcpy() (Pierre Poissinger) -- version 1.2.23beta03 [October 16, 2007] Restore statement to set last character of buffer to \0 in pngerror.c -- Up to version 1.5.x png_format_buffer is nearly identical with 1.2.5 (PNG_MAX_ERROR_TEXT defined as 64). To put it in a nutshell: is CVE-2004-0421 not a bug (or accepted as a bug) or do I miss something? Kind regards, Frank. |
|
From: <jb...@ac...> - 2011-06-07 16:13:14
|
From: Frank Busse [mailto:s88...@ma...] >To put it in a nutshell: is CVE-2004-0421 not a bug (or accepted as a bug) or do I miss something? Thanks for the clear analysis. Unless I'm forgetting something it's a bug and you haven't missed anything. The code in 1.5 is certainly wrong, and as you say all previous versions currently have the same code. John Bowler <jb...@ac...> |
|
From: Glenn Randers-P. <gl...@gm...> - 2011-06-07 16:39:02
|
The 1.4 branch happened around 1.2.19 or 1.2.20 so it's possible that the bugfix did not get propagated to 1.4 and consequently to 1.5. Glenn |
|
From: <gl...@co...> - 2011-06-07 17:27:44
|
There was a merge of 1.4 fixes with 1.2.30, so all current versions 1.0.x, 1.2.x, 1.4.x, and 1.5.x have the bug that seems to have reappeared in 1.2.23. It allows an attacker to write a single character after the end of the (18+64)-byte buffer (the 65'th character of the error message or whatever follows in that memory location beyond the error message; none of the libpng error messages are that long), as in CVE-2004-0421. I don't know if anyone has figured out how to exploit that vulnerability. I guess we need to release new libpng-1.0 and 1.2 along with the planned 1.4 and 1.5, to patch this. Glenn ----- Original Message ----- From: Glenn Randers-Pehrson <gl...@gm...> To: PNG/MNG implementation discussion list <png...@li...> Sent: Tue, 07 Jun 2011 16:37:23 -0000 (UTC) Subject: Re: [png-mng-implement] CVE-2004-0421 The 1.4 branch happened around 1.2.19 or 1.2.20 so it's possible that the bugfix did not get propagated to 1.4 and consequently to 1.5. Glenn |
|
From: Frank B. <s88...@ma...> - 2011-06-07 18:29:12
|
Hello,
On Tue, 7 Jun 2011 17:27:37 +0000 (UTC)
glennrp wrote:
> It allows an attacker to write a single character
> after the end of the (18+64)-byte buffer (the 65'th
> character of the error message or whatever follows
> in that memory location beyond the error message; none
> of the libpng error messages are that long),
> as in CVE-2004-0421. I don't know if anyone has
> figured out how to exploit that vulnerability.
could you explain that? In my opinion iout is in the range {4,18} (the
longest string is "[__][__][__][__]: "). Therefore memcpy starts at
most with index 18 (buffer+iout) and copies 64 bytes, last byte is on
index 81 and within the index range {0,81} (msg[18+64]).
--
png_memcpy(buffer+iout, error_message, 64);
--
The problem I have with this code snippet is, that it copies memory
which isn't allocated. It just works correctly, if error_message is
longer than 63(+18-iout) characters (incl. '\0').
Kind regards,
Frank
|
|
From: <jb...@ac...> - 2011-06-07 18:54:28
|
From: Frank Busse [mailto:s88...@ma...] > png_memcpy(buffer+iout, error_message, 64); > >The problem I have with this code snippet is, that it copies memory which >isn't allocated. Yes, that's the bug. The overwrite problem was a temporary issue in the 1.2.23 betas and was never released. John Bowler <jb...@ac...> |
|
From: Pierre P. <pie...@gm...> - 2011-06-07 21:51:32
|
On Tue, Jun 7, 2011 at 8:54 PM, <jb...@ac...> wrote:
> From: Frank Busse [mailto:s88...@ma...]
>> png_memcpy(buffer+iout, error_message, 64);
>>
>>The problem I have with this code snippet is, that it copies memory which
>>isn't allocated.
>
> Yes, that's the bug. The overwrite problem was a temporary issue in the 1.2.23 betas and was never released.
Agreed
Btw, coz i love context: original trigger was a bug in 1.2.21:
incorrect strncpy crashing on quite a few PNG due to a typo:
diff -Nbur libpng-1.2.21.ori/pngset.c libpng-1.2.21/pngset.c
--- libpng-1.2.21.ori/pngset.c 2007-10-04 13:53:11.000000000 +0200
+++ libpng-1.2.21/pngset.c 2007-10-14 13:46:38.000000000 +0200
@@ -690,7 +690,7 @@
png_warning(png_ptr, "Insufficient memory to process iCCP chunk.");
return;
}
- png_strncpy(new_iccp_name, name, png_strlen(new_iccp_name)+1);
+ png_strncpy(new_iccp_name, name, png_strlen(name)+1);
new_iccp_profile = (png_charp)png_malloc_warn(png_ptr, proflen);
if (new_iccp_profile == NULL)
{
Quite a few betas in the 1.2.22 series to get a strncpy right pushed
me to go hunt down all the strncpy/strcpy to avoid a "off-by-X" with a
dep on the actual content of the data (quite a few of them were living
in png, mostly on fixed size strings)
But of course, I missed the fact that, in this particular func,
error_message comes from API and is not guaranteed to be at least 64
:-(
=> That one should really be a strncpy...
Damn !
Regs,
Pierre
--
>>> horsemen = ['war', 'pestilence', 'famine']
>>> horsemen.append('Powerbuilder')
|