Closed
Bug 254847
Opened 20 years ago
Closed 18 years ago
Still vulnerable to libpng bug with iCCP if using system library
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
People
(Reporter: glennrp+bmo, Assigned: glennrp+bmo)
References
()
Details
Attachments
(3 obsolete files)
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•20 years ago
|
||
Remove nonfunctioning code, recently added in bug #251381.
Assignee | ||
Comment 2•20 years ago
|
||
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•20 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•20 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.
Comment 4•20 years ago
|
||
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•20 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•20 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•20 years ago
|
||
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•20 years ago
|
Attachment #155745 -
Attachment is obsolete: true
Assignee | ||
Comment 8•20 years ago
|
||
Comment on attachment 156535 [details] [diff] [review] Rejects unpatched system libpng. Skips unused chunks. tor: r?
Attachment #156535 -
Flags: review?(tor)
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•20 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.
Comment 11•20 years ago
|
||
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•19 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•18 years ago
|
||
This bug was fixed by the check in of bug #334110.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•18 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•18 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.
Description
•