|
From: Frank B. <s88...@ma...> - 2011-06-09 13:53:05
Attachments:
scal_zero2.png
|
Hi,
when the 1.2 series reads an empty sCAL chunk (12 bytes), length is 0
and one byte is allocated and set to 0 in chunkdata. Afterwards ep is
positioned behind chunkdata and then used in png_strtod resp.
png_strlen.
--
void /* PRIVATE */
png_handle_sCAL(png_structp png_ptr, png_infop info_ptr, png_uint_32
length) {
png_charp ep;
...
png_ptr->chunkdata = (png_charp)png_malloc_warn(png_ptr, length + 1);
...
slength = (png_size_t)length;
...
png_ptr->chunkdata[slength] = 0x00; /* Null terminate the last
string */
ep = png_ptr->chunkdata + 1; /* Skip unit byte */
...
width = png_strtod(png_ptr, ep, &vp);
...
swidth = (png_charp)png_malloc_warn(png_ptr, png_strlen(ep) + 1);
--
Kind regards,
Frank
Attachment: scal_zero2.png (based on brokensuite)
|
|
From: Glenn Randers-P. <gl...@gm...> - 2011-06-09 15:50:01
|
Thanks. I guess those infamous patches for libpng-1.0.5e and 1.0.5f (1999)
are going
to be haunting us with security problems forever.
The bug exists in all versions since then (libpng10, 12, 14, and 15).
Thankfully, it does not affect the embedded libpng in mozilla products, and
I believe
it also doesn't affect mozilla products that use a system libpng. It
doesn't
affect ImageMagick, GraphicsMagick, or pngcrush. All of these use the
png_set_keep_unknown_chunk mechanism to avoid decoding the
sCAL chunk and other chunks that they don't need.
On Thu, Jun 9, 2011 at 11:00 AM, Frank Busse <
s88...@ma...> wrote:
> Hi,
>
> when the 1.2 series reads an empty sCAL chunk (12 bytes), length is 0
> and one byte is allocated and set to 0 in chunkdata. Afterwards ep is
> positioned behind chunkdata and then used in png_strtod resp.
> png_strlen.
>
> --
> void /* PRIVATE */
> png_handle_sCAL(png_structp png_ptr, png_infop info_ptr, png_uint_32
> length) {
> png_charp ep;
> ...
> png_ptr->chunkdata = (png_charp)png_malloc_warn(png_ptr, length + 1);
> ...
> slength = (png_size_t)length;
> ...
> png_ptr->chunkdata[slength] = 0x00; /* Null terminate the last
> string */
>
> ep = png_ptr->chunkdata + 1; /* Skip unit byte */
> ...
> width = png_strtod(png_ptr, ep, &vp);
> ...
> swidth = (png_charp)png_malloc_warn(png_ptr, png_strlen(ep) + 1);
> --
>
>
>
> Kind regards,
>
> Frank
>
> Attachment: scal_zero2.png (based on brokensuite)
>
>
>
>
> ------------------------------------------------------------------------------
> EditLive Enterprise is the world's most technically advanced content
> authoring tool. Experience the power of Track Changes, Inline Image
> Editing and ensure content is compliant with Accessibility Checking.
> http://p.sf.net/sfu/ephox-dev2dev
> _______________________________________________
> png-mng-implement mailing list
> png...@li...
> https://lists.sourceforge.net/lists/listinfo/png-mng-implement
>
>
|
|
From: <jb...@ac...> - 2011-06-09 16:40:54
|
From: Glenn Randers-Pehrson [mailto:gl...@gm...] >The bug exists in all versions since then (libpng10, 12, 14, and 15). The code in question doesn’t exist in 15 (which doesn’t use strtod). 15 checks the unit *first*, and in the zero length sCAL the assignment of ‘\0’ causes it to be the invalid value 0. I think changing 10, 12 and 14 to match (check the unit immediately after the crc read and ‘\0’ termination) would work. John Bowler <jb...@ac...> |
|
From: Frank B. <s88...@ma...> - 2011-06-09 19:35:33
Attachments:
scal_inf_nan.png
|
Hi, small additions to my last mail: 0. there is no range test for the unit specifier 1. there is no check for errno==ERANGE after strtod 2. strtod is not equivalent to the specification (Ext. to PNG 1.2, v1.4.0), for instance NAN and INF are allowed and pass the <= 0. check 3. Why PNG_FLOATING_POINT_SUPPORTED disables PNG_FIXED_POINT_SUPPORTED in sCAL, but not in other chunks? -- #ifdef PNG_FLOATING_POINT_SUPPORTED ... #else #ifdef PNG_FIXED_POINT_SUPPORTED ... #endif #endif -- Kind regards, Frank attachment: scal_inf_nan.png |
|
From: <jb...@ac...> - 2011-06-10 02:17:10
|
Several difficult to answer questions:
From: Frank Busse [mailto:s88...@ma...]
>3. Why PNG_FLOATING_POINT_SUPPORTED disables PNG_FIXED_POINT_SUPPORTED
> in sCAL, but not in other chunks?
Well... really there is no fixed point support. The png_{set,get}_sCAL APIs have a double precision FP version and a 'string' version. The fact that the 'string' version depends on either floating or fixed point support is just wrong. This is fixed in 1.5 - the _sCAL_s APIs are always present, and there is a fixed API (though it has very limited use).
> There is no range test for the unit specifier 1, there is no check for errno==ERANGE after strtod 2.
>strtod is not equivalent to the specification (Ext. to PNG 1.2, v1.4.0), for instance NAN and INF are
>allowed and pass the <= 0 check
I think these are all fixed in 1.5, and, apart from the first, fixing them requires a major change like the 1.5 code (which follows the specified format of a number, excluding extensions like NAN and INF.) My reading of the C89 standard is that strtod should not accept 'INF' or 'NAN' as floating point values, however glibc certainly does. Of course you are correct about the check; rather than:
if (... width <= 0 ...)
it should be:
if (... !(width > 0) ...)
But that's moot in 1.5 because it won't accept either negative values or non-values. It does (with my fix) however accept zeros, contrary to the spec which requires "greater than zero". That's a lot more difficult to check, since zero can be an arbitrary string. Also strings like .1E-307 typically end up as zero, even though they aren't. Prior (floating point) implementations, of course, incorrectly fail on sCAL chunks that underflow the implementation floating point representation...
I'm pretty sure neither of these points raise security issues and, in practice, I can't see any real problems for applications with the non-conformance to a strict reading of the spec., either the 1.4 (accept non-positive values) or 1.5 (accept zero) versions. I can fix 1.5 to not accept a true zero, but I think I can pretty much guarantee it won't help anyone.
John Bowler <jb...@ac...>
|
|
From: Frank B. <s88...@ma...> - 2011-06-10 10:10:17
|
Hello,
On Thu, 9 Jun 2011 19:16:53 -0700
jbowler wrote:
> Several difficult to answer questions:
thank you for your reply.
> INF.) My reading of the C89 standard is that strtod should not
> accept 'INF' or 'NAN' as floating point values, however glibc
> certainly does.
Interesting, I used C99 (7.20.1.3) and POSIX:2004.
> I'm pretty sure neither of these points raise security issues and, in
> practice, I can't see any real problems for applications with the
> non-conformance to a strict reading of the spec., either the 1.4
> (accept non-positive values) or 1.5 (accept zero) versions.
For negative values it's simple:
--
height = -20000.1;
width = -30000.2;
/* allocate byte for every square meter */
if (height < max && width < max) {
size_t h = (size_t) height + 1;
size_t w = (size_t) width + 1;
malloc(h*w);
}
> I can fix 1.5 to not accept a true zero, but I think I can pretty much
> guarantee it won't help anyone.
According to C99/F3 "/" provides semantics of IEEE754 and therefore
(7.2) returns INF in case of division by zero. Shouldn't be a security
issue.
The point is, there shouldn't be any differences between specification
and reference implementation other than the natural ones (obvious API
limitations, for instance double for pixel width). Why should I care
about "unit specifier" > 2, if the specification says it's 1 or 2 and
the reference implementation signals "info_ptr->valid & PNG_INFO_sCAL"?
If the checks are too complex or differ from common APIs (strtod),
change the specification. ;)
Kind regards,
Frank
|
|
From: <jb...@ac...> - 2011-06-10 17:08:38
|
From: Frank Busse [mailto:s88...@ma...] >I said: >>I can't see any real problems for applications with the >> non-conformance to a strict reading of the spec., >For negative values it's simple: >-- >height = -20000.1; *Negative* (as opposed to non-positive) values are excluded by all versions (I think). Versions prior to 1.5.0 will also reject small positive values, but since they are either metres or radians this doesn't matter - the values rejected will be meaningless. >According to C99/F3 "/" provides semantics of IEEE754 and therefore >(7.2) returns INF in case of division by zero. Shouldn't be a security >issue. Some architectures fault on division by zero, but this is really an app issue. IRC we added the checking on cHRM because of systems where the FP crashed rather than generate a NaN. While libpng could guarantee that the string is non-zero it can't guarantee that the result of converting it to double or png_fixed_point is non-zero. In fact the _fixed API will call png_error if the result overflows, but it's more work for the app to catch this than it is to check for an out of range value... >If the checks are too complex or differ from common APIs (strtod), >change the specification. ;) Not all strtod implementations handle the strings inf and NaN. Libpng 1.5 has an internal function (png_check_fp_number) which is used to validate the format of numbers in the sCAL and pCAL chunks and which is well tested (unlike the handling of the sCAL chunk). The format was selected because it is portable, in the sense that it can be handled on a wide variety of systems. Extending the format would defeat that goal. 1.5 does much more checking, but there is still a raft of "TODO" statements in there (in the sCAL and pCAL support.) In fact I think that png_handle_pCAL isn't doing any parameter validation at all. > The point is, there shouldn't be any differences between specification >and reference implementation other than the natural ones (obvious API >limitations, for instance double for pixel width). Yes, but there are practical limitations to the checking done when a PNG is read: not all invalid chunks are excluded. That's particularly true of cHRM, iCCP and pCAL. Checking the FP number format adds quite a few bytes to the library, took quite a lot of time and introduced bugs (the current inability of 1.5 to read sCAL.) John Bowler <jb...@ac...> |
|
From: <jb...@ac...> - 2011-06-09 16:51:41
|
Does anyone have a PNG file with a *valid* sCAL chunk? Pngtest.png doesn’t have an sCAL. I think libpng 1.5 will fail to read sCAL at all. John Bowler jb...@ac... |