[Fwd: iCCP profile name truncation / issue with long keywords breaking chunk content]

[Andreas Kleinert-2]

Hi all,

while dealing with png_write_iCCP() and png_read_iCCP()
I've found that passing an overlength name for the iCCP
chunk on writing will cause an invalid iCCP chunk being
written, which cannot be read anymore.

Peeking into the binary file shows that the originally
passed name was truncated at some point. I did not
analyze this further, but at the beginning of
png_write_iCCP() there is a keyword check done using
png_check_keyword() which throws a warning for
empty keywords *or* when the keyword exceeds
79 characters.

The former gives a silent error in png_write_iCCP(),
the latter does not.

If I'm not totally wrong, the following happens in
this case:

 - png_check_keyword() truncates the length to 79
   and sets name[79] := NUL
 - a length of 79 is returned to png_write_iCCP()
 - png_write_iCCP() again sets name[name_len+1] := NUL
 - in this case name_len+1 equals 80
 - thus the first byte of whatever data comes after
   the name is overwritten (probably the first byte
   of iCCP data)
 - this is likely to break the to-be-written iCCP
   profile, depending on implementation it may also
   break something else

Suggestion would be to truncate to 78 bytes in
png_check_keyword() instead.

--
Dipl.-Ing. Andreas Kleinert VDI (Andreas_Kleinert@...)
Embedded Systems Software Consultant

PGP Key: http://pgp.mit.edu/ -- Search: "Andreas Kleinert"

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[Glenn Randers-Pehrson]

On Sun, Sep 27, 2009 at 5:08 AM, Andreas Kleinert <info@...> wrote:

> Hi all,
>
> while dealing with png_write_iCCP() and png_read_iCCP()
> I've found that passing an overlength name for the iCCP
> chunk on writing will cause an invalid iCCP chunk being
> written, which cannot be read anymore.
>
> Peeking into the binary file shows that the originally
> passed name was truncated at some point. I did not
> analyze this further, but at the beginning of
> png_write_iCCP() there is a keyword check done using
> png_check_keyword() which throws a warning for
> empty keywords *or* when the keyword exceeds
> 79 characters.
>
> The former gives a silent error in png_write_iCCP(),
> the latter does not.
>
> If I'm not totally wrong, the following happens in
> this case:
>
>  - png_check_keyword() truncates the length to 79
>   and sets name[79] := NUL

      new_name[79] = NUL

>  - a length of 79 is returned to png_write_iCCP()
>  - png_write_iCCP() again sets name[name_len+1] := NUL

     It sets new_name, not name.  new_name is a string that
     was malloc'ed in png_check_keyword() to strlen(name)+2, which
     leaves room for this extra NUL.

>  - in this case name_len+1 equals 80
>  - thus the first byte of whatever data comes after
>   the name is overwritten (probably the first byte
>   of iCCP data)

     Nothing comes after the keyword in new_name.

>[...]

Glenn

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[Andreas Kleinert-2]

Glenn Randers-Pehrson schrieb:
>>  - png_check_keyword() truncates the length to 79
>>   and sets name[79] := NUL
>       new_name[79] = NUL

I wasn't aiming at the exact variable name.

>      It sets new_name, not name.  new_name is a string that
>      was malloc'ed in png_check_keyword() to strlen(name)+2, which
>      leaves room for this extra NUL.

You're right. I wasn't doing a real analysis, just a quick guess.

>      Nothing comes after the keyword in new_name.

Granted. Anyway, do you have any (other) explanation why libpng cannot
read ICC profiles anymore after attempting to write with beyond-limit
profile name?

I'm pretty sure the bug is prsent, even though its source may not
be exactly at that location. It just applied a workaround by shortening
the profile name without exactly tracking that down. However it should
be easy to reproduce for everyone...

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[Glenn Randers-Pehrson]

On Sun, Sep 27, 2009 at 3:43 PM, Andreas Kleinert <info@...> wrote:

> Granted. Anyway, do you have any (other) explanation why libpng cannot
> read ICC profiles anymore after attempting to write with beyond-limit
> profile name?

Not yet, but I will try to track it down.

> I'm pretty sure the bug is prsent, even though its source may not
> be exactly at that location. It just applied a workaround by shortening
> the profile name without exactly tracking that down. However it should
> be easy to reproduce for everyone...

Thanks.  I was not planning to go any further with 1.2.x but if this
turns out to be a security bug, which seems possible, then there
will indeed be a 1.2.41 release.

Glenn

------------------------------------------------------------------------------
Come build with us! The BlackBerry® Developer Conference in SF, CA
is the only developer event you need to attend this year. Jumpstart your
developing skills, take BlackBerry mobile applications to market and stay
ahead of the curve. Join us from November 9-12, 2009. Register now!
http://p.sf.net/sfu/devconf
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement