Still vulnerable to libpng bug with iCCP if using system library

RESOLVED FIXED

Status

()

Core
ImageLib
RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: Glenn Randers-Pehrson, Assigned: Glenn Randers-Pehrson)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 obsolete attachments)

(Assignee)

Description

14 years ago
I find that part of the fix for bug #251381 does not actually work.  This is
the use of "png_set_keep_unknown_chunks()" to skip processing of the
vulnerable chunks, when the system libpng is used.  Based on testing and
debug printouts I find that this method only works with the PNG sequential
reader (pngread.c) and not with the progressive reader (pngpread.c) that
we are using.

This leaves us open to crashes due to bad iCCP chunks (sorry, I don't have
a demo PNG with bad iCCP chunk handy) and possibly some of the other chunks
such as sPLT.

It is fairly easy to fix pngpread.c to behave in the same manner as
pngread.c, but that doesn't help with old unpatched system libraries,
nor is there any reason to fix pngpread.c in the embedded libpng.

The only "fix" for this problem that I've found so far is to require that
the system library be at version 1.2.6 or greater.
(Assignee)

Comment 1

14 years ago
Created attachment 155535 [details] [diff] [review]
Remove nonfunctioning code to skip unused chunks.

Remove nonfunctioning code, recently added in bug #251381.
(Assignee)

Comment 2

14 years ago
Created attachment 155745 [details] [diff] [review]
Skip unused chunks if system library is > libpng1.2.5

The chunk-skipping stuff is useful but only works if the system libpng
is libpng-1.2.6rc3 or newer.  This patch checks for the libpng version
and corrects the second argument to png_set_keep_unknown_chunks (was 0).
Attachment #155535 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

14 years ago
Some test files for this bug are available at
http://www.graphicsmagick.org/libpng/FFFF

Each PNG at this site has a particular chunk type having
length=0xffffffff.  First the good news: on my platform and on
Windows XP, Firefox 0.9.3 and Mozilla 1.7.2 have no problem with
 any of them.  But if I link Mozilla-1.7.2 on the FreeBSD platform with
an unpatched system libpng-1.2.5, I get crashes with the tEXt, zTXt,
iCCP, and sPLT chunks, and there is a malloc failure (out of memory)
with the pCAL chunk.  When I click the zTXt.html (PNG with oversized
zTXt chunk in an <img src > tag), not only did the browser crash but
my console window and login session disappeared as well.
Just want to clarify: you aren't affected if you are building --with-system-png
and your system png is patched (not necessarily 1.2.6), right?
(Assignee)

Comment 5

14 years ago
caillon: That is correct.  The critical patch for this vulnerability is
the one that limits chunk length to 0x7fffffff.  The bug is triggered by
a chunk with length 0xffffffff because the malloc for the chunk data
receives a request for 0xffffffff+1 which wraps around to zero.  The
patched library, and libpng-1.2.6, simply skip any chunk with the large
length.  In the new test files, which don't really have that much data in
them, the datastream is exhausted and mozilla says "the file cannot be
displayed because it has errors".

BTW nsPNGDecoder could test for whether the library has been patched.
Patched libraries have PNG_UINT_31_MAX defined, unpatched don't.

It would be possible to intercept the input stream in nsPNGDecoder and
check for oversize "length" fields.  This would involve setting up a
"read_data" callback with a small buffer.  I've done this in the past
for ImageMagick.  Do you think it would be worth while here?
(Assignee)

Comment 6

14 years ago
> nsPNGDecoder could test for whether the library has been patched.
> Patched libraries have PNG_UINT_31_MAX defined, unpatched don't.

D'oh.  We'd need access to png.h for the system library, which I guess
we don't have.  Back to Plan B, intercepting the input data.
(Assignee)

Comment 7

14 years ago
Created attachment 156535 [details] [diff] [review]
Rejects unpatched system libpng.  Skips unused chunks.

This patch will cause configure to reject old system libpng libraries that have
not had the security patch applied.  Unused chunks are skipped, if the system
library is libpng-1.2.5 or later, avoiding any future security problems with
those chunks.
(Assignee)

Updated

14 years ago
Attachment #155745 - Attachment is obsolete: true
(Assignee)

Comment 8

14 years ago
Comment on attachment 156535 [details] [diff] [review]
Rejects unpatched system libpng.  Skips unused chunks.

tor: r?
Attachment #156535 - Flags: review?(tor)

Comment 9

14 years ago
Ugh, did the values of argument two of png_set_keep_unkown_chunks really
change between 1.2.5 and 1.2.6?
(Assignee)

Comment 10

14 years ago
The values didn't change but the documentation was in error.  Ugh, indeed.
Also, as I said in the initial comment, the png_set_keep_chunks() mechanism
didn't work with progressive reading.  That is fixed in libpng-1.2.6.
Clearing confidential flag because I don't see it doing any good here. The
libpng vulnerabilities are known so it wouldn't take a genius to think of trying
to exploit an unpatched system library. Especially since that attack could come
through any number of other programs that use the system libpng.
Group: security
(Assignee)

Comment 12

13 years ago
A year has passed and the 1.7 branch is still vulnerable, if it uses an old
system libpng.  Later branches are not because they require system libpng to be
1.2.7 or later.  However, PNG decoding would be more efficient even with the
later branches if the patch were applied because they wouldn't be wasting cycles
decoding chunks that they don't intend to use.
(Assignee)

Comment 13

12 years ago
This bug was fixed by the check in of bug #334110.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 14

12 years ago
Comment on attachment 156535 [details] [diff] [review]
Rejects unpatched system libpng.  Skips unused chunks.

Rendered obsolete by check in of bug #334110
(Assignee)

Updated

12 years ago
Attachment #156535 - Attachment is obsolete: true
Attachment #156535 - Flags: review?(tor)
You need to log in before you can comment on or make changes to this bug.