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.
Created attachment 155535 [details] [diff] [review] Remove nonfunctioning code to skip unused chunks. Remove nonfunctioning code, recently added in bug #251381.
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
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?
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?
> 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.
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.
Attachment #155745 - Attachment is obsolete: true
Comment on attachment 156535 [details] [diff] [review] Rejects unpatched system libpng. Skips unused chunks. tor: r?
Ugh, did the values of argument two of png_set_keep_unkown_chunks really change between 1.2.5 and 1.2.6?
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.
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.
This bug was fixed by the check in of bug #334110.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Comment on attachment 156535 [details] [diff] [review] Rejects unpatched system libpng. Skips unused chunks. Rendered obsolete by check in of bug #334110
You need to log in before you can comment on or make changes to this bug.