Bug 497056 (CVE-2010-0205)

very slow decoding of png

RESOLVED FIXED

Status

()

--
major
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: timeless, Assigned: glennrp+bmo)

Tracking

(4 keywords)

Trunk
perf, verified1.9.0.19, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.19 -
wanted1.9.0.x +

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 needed, status1.9.2 .2-fixed, blocking1.9.1 needed, status1.9.1 .9-fixed)

Details

(Whiteboard: [sg:dos], URL)

Attachments

(6 attachments, 35 obsolete attachments)

22.93 KB, text/plain
Details
22.51 KB, patch
joe
: review+
Details | Diff | Splinter Review
21.33 KB, patch
joe
: review+
Details | Diff | Splinter Review
20.42 KB, patch
joe
: review+
Details | Diff | Splinter Review
14.38 KB, patch
Details | Diff | Splinter Review
3.73 KB, patch
joe
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
Created attachment 382257 [details]
zip containing testcase

Test case from Codenomicon Defensics test suites (http://www.codenomicon.com/). Distributed with permission.

This image causes the Firefox browsers we tested (OS X, Linux, Windows) to hang for a very long time.

I'm attaching it as a zip file because it's easier for me to leave it that way (including the readme).
(Reporter)

Comment 1

10 years ago
Created attachment 382258 [details]
log from windbg
We're hanging in:
memcpy()
png_decompress_chunk ()
png_handle_iCCP ()
It looks like looping in png_decompress_chunk()
(Assignee)

Comment 4

10 years ago
Verified that other libpng-based applications pngcrush and GraphicsMagick also hang while reading this file.  Here is the pngcheck report:

$ pngcheck -v hang.png
File: hang.png (58775 bytes)
  chunk IHDR at offset 0x0000c, length 13
    72 x 14 image, 4-bit colormap, non-interlaced
  chunk iCCP at offset 0x00025, length 58359
    profile name = Default ICC profile, compression method = 0 (deflate)
    compressed profile = 58338 bytes
  chunk PLTE at offset 0x0e428, length 48: 16 palette entries
  chunk IDAT at offset 0x0e464, length 129
    zlib:  deflated, 32K window, maximum compression
  chunk tEXt at offset 0x0e4f1, length 146, keyword: Description
  chunk IEND at offset 0x0e58f, length 0
No errors detected in hang.png (-11561.7% compression).
$
(Assignee)

Updated

10 years ago
Assignee: nobody → glennrp
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
Firefox on Linux is hanging for me even when color_management is disabled.
(Assignee)

Comment 6

10 years ago
Seems to be in a memory-alloc/free thrashing mode.
pngcrush -n -v -v hang.png says:
...
io has been initialized.
Reading info struct
Pointer 673263616x allocated 58360 bytes
Pointer 673325056x allocated 32768 bytes
Pointer 673357824x allocated 8214 bytes
Pointer 673370112x allocated 16406 bytes
Pointer 673357824x freed 8214 bytes
Pointer 673390592x allocated 24598 bytes
Pointer 673370112x freed 16406 bytes
Pointer 673419264x allocated 32790 bytes
Pointer 673390592x freed 24598 bytes
Pointer 673357824x allocated 40982 bytes
...
Pointer 676331520x allocated 12640278 bytes
Pointer 689963008x freed 12632086 bytes
Pointer 689963008x allocated 12648470 bytes
Pointer 676331520x freed 12640278 bytes
Pointer 676331520x allocated 12656662 bytes
Pointer 689963008x freed 12648470 bytes
^C
(Assignee)

Comment 7

10 years ago
From comment #1
int comp_type = 81788928, unsigned int chunklength = 0xdf0000, unsigned int prefix_size = 0x1021bc86

Those look a bit alarming, but libpng is interpreting them as
reasonable numbers.

libpng debug: png_decompress_chunk:
libpng debug: compression type = 0
libpng debug: chunklength = 58359
libpng debug: prefix_size = 21

The deflate stream looks peculiar though:
0000000: 8950 4e47 0d0a 1a0a 0000 000d 4948 4452  .PNG........IHDR
0000010: 0000 0048 0000 000e 0403 0000 007e c247  ...H.........~.G
0000020: 0500 00e3 f769 4343 5044 6566 6175 6c74  .....iCCPDefault
0000030: 2049 4343 2070 726f 6669 6c65 0000 789c   ICC profile..x.
0000040: ecc1 8100 0000 0080 20d6 fd25 16a9 0a00  ........ ..%....
0000050: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000060: 0000 0000 0000 0000 0000 0000 0000 0000  ................
0000070: 0000 0000 0000 0000 0000 0000 0000 0000  ................
...
0001040: 0000 0000 0000 0000 0000 0000 0080 d983  ................
0001050: 0301 0000 0000 20ff d746 5055 5555 5555  ...... ..FPUUUUU
0001060: 5555 5555 5555 5555 5555 5555 5555 5555  UUUUUUUUUUUUUUUU
...
and then mostly 5555 for another 58 kbytes.
(Assignee)

Comment 8

10 years ago
I don't know if this needs to be kept quiet.  It is no secret that any compressed PNG chunk (IDAT, fdAT, iCCP, iTXt, zTXt, maybe others) can expand immensely when decoded, especially when a malevolent PNG is designed to do so.  That is what is happening here.  The only real defense is to impose some arbitrary limit on what an application is willing to accept, and bail out if the iCCP, zTXt, or whatever chunk expands too much.
(Assignee)

Comment 9

10 years ago
(In reply to comment #5)
> Firefox on Linux is hanging for me even when color_management is disabled.

That's because of a design shortcoming in libpng.  In order for png_set_keep_unknown_chunks() to work, PNG_READ_UNKNOWN_CHUNKS_SUPPORTED must be defined.  We are undefining it in the bundled libpng (by defining PNG_NO_READ_UNKNOWN_CHUNKS) in mozpngconf.h.  Something to keep in mind for libpng-1.2.39 or 1.4.0.
(Assignee)

Comment 10

10 years ago
(In reply to comment #6)
> Seems to be in a memory-alloc/free thrashing mode.

This is due to the design of png_decompress_chunk().  It asks for 8192 bytes
at a time, by default.  This can be increased by defining
PNG_ZBUF_SIZE to a larger number. 

    Here are some times for decoding the hang.png with pngcrush-1.6.16:

     PNG_ZBUF_SIZE       CPU Sec

         8192 (default)
        30000               355
       100000               106
       300000                36
      1000000                11
      3000000                 4
     10000000                 1

So there is a tradeoff of memory versus performance in decoding
this deliberatly malformed chunk.  For real PNG files with reasonalbly
sized iCCP chunks or no iCCP chunk, it's only a sacrifice of memory for
no gain in performance.

Real users are going to hit the STOP icon, or ^C in other libpng
applications, so the longer times won't be as large as shown in
the table.

Nothing bad happens when the 60-Megabyte decompressed iCCP chunk
is finally decoded.  libpng recognizes that it is malformed and
rejects it with a png_warning() and continues to process the rest
of the file.

BTW pngcrush-1.6.19, the current version, ignores the iCCP
chunk and only takes .008 second to decode the PNG file.
(Assignee)

Comment 11

10 years ago
(In reply to comment #10)
> (In reply to comment #6)

>     Here are some times for decoding the hang.png with pngcrush-1.6.16:
> 
>      PNG_ZBUF_SIZE       CPU Sec
> 
>          8192 (default)

Oops, left off this: 8192 (default)  1296 sec
(Reporter)

Comment 12

10 years ago
fwiw, i'm not wed to keeping it private.

however the stop button generally doesn't work until we reach the event loop, which at least for short intervals is not the case. Image decoding off the UI thread will save us there :).
(Assignee)

Comment 13

10 years ago
Shortly I'll provide a simple patch that at least prevents the slowdown when color_management is off.  There will be about a 2-kbyte increase in the size of the compiled libpng.  Most of that size penalty will be eliminated in the next libpng release (I don't know when that will occur, though).

What would you think of increasing the PNG_ZBUF_SIZE to say 100k?  Unfortunately that penalizes everyone with an increased memory footprint for the sake of speeding up the handling of these (probably very rare) malformed iCCP chunks when color_management is enabled. In my own opinion that's probably not an excellent idea.
(Assignee)

Comment 14

10 years ago
Created attachment 382868 [details] [diff] [review]
Allow the bundled libpng to skip iCCP chunk when color_management is off (v00).

This patch lets the png_set_keep_unknown_chunks() mechanism to be used in the bundled libpng to ignore the iCCP chunk (and cHRM chunk) when color_management is not enabled.  Addressing the problem when color_management is enabled will be a bit more complex, probably requiring libpng to implement a user-specified limit on how large a compressed chunk can grow when decompressed.
Attachment #382868 - Flags: review?(joe)
Attachment #382868 - Flags: superreview?(vladimir)
Attachment #382868 - Flags: review?(joe)
Attachment #382868 - Flags: review+
Attachment #382868 - Flags: superreview?(vladimir) → superreview+
(Assignee)

Comment 15

10 years ago
I think the APNG fdAT chunk may also be susceptible.  I'll try to put together a test PNG file to be sure.  The patch only deals with the iCCP chunk and won't fix any problem with the fdAT chunk.  Increasing PNG_ZBUF_SIZE might help but needs testing.
Flags: wanted1.9.1?
Keywords: checkin-needed
(Assignee)

Comment 16

10 years ago
The patch is incorporated in the patch to update the bundled libpng on the trunk to libpng-1.2.38 (see bug #504805).  It is still worth checking this small patch into the branch, however.  As I mentioned in comment #14, this is only a partial solution to the problem, though.
(Assignee)

Updated

10 years ago
Depends on: 504805

Updated

10 years ago
Whiteboard: [sg:dos]
status1.9.1: --- → ?
Flags: wanted1.9.1?
Attachment #382868 - Flags: approval1.9.1.7?
(Assignee)

Comment 17

9 years ago
Libpng-1.4.0 will be out in a month or so and will provide a more complete solution by allowing us to specify a limit to the size of decompressed data.  An open question is what size this limit should be.  Libpng limits it naturally to 2GB.  Perhaps 10 MB would be a reasonable limit.  No actual ICC profile is really likely to exceed 1 MB.
Attachment #382868 - Flags: approval1.9.2?
(In reply to comment #17)
> Libpng-1.4.0 will be out in a month or so and will provide a more complete
> solution by allowing us to specify a limit to the size of decompressed data. 
> An open question is what size this limit should be.  Libpng limits it naturally
> to 2GB.  Perhaps 10 MB would be a reasonable limit.  No actual ICC profile is
> really likely to exceed 1 MB.

In qcms we currently ignore profiles larger than 4MB.
This patch has now bitrotted... Bug 504805 removed |#define PNG_NO_HANDLE_AS_UNKNOWN|, but |#define PNG_NO_READ_UNKNOWN_CHUNKS| is still there.

Glenn, can you provide a new patch?
Keywords: checkin-needed
(Assignee)

Comment 20

9 years ago
@reed: With checkin of libpng-1.2.40, the patch is indeed bitrotten for use on the trunk, but it's also obsolete because it was incorporated in the libpng update (see comment #16).  It's only useful now for applying to older branches, and probably is not bitrotten there.
(In reply to comment #20)
> @reed: With checkin of libpng-1.2.40, the patch is indeed bitrotten for use on
> the trunk, but it's also obsolete because it was incorporated in the libpng
> update (see comment #16).  It's only useful now for applying to older branches,
> and probably is not bitrotten there.

Did you incorporate it into the actual libpng source, or is it still a separate Mozilla-specific change, as I don't see it listed on http://mxr.mozilla.org/mozilla-central/source/modules/libimg/png/MOZCHANGES ?
(Assignee)

Comment 22

9 years ago
The change (deleting #define PNG_NO_HANDLE_AS_UNKNOWN) is now in mozpngconf.h so it is a mozilla-specific change (but is not a mozilla-specific change to libpng itself so it didn't appear in MOZCHANGES).

The other deletion (#define PNG_NO_READ_UNKNOWN_CHUNKS) is no longer necessary with libpng-1.2.40, since libpng now separates the HANDLE_AS_UNKNOWN feature and the READ_UNKNOWN_CHUNKS feature.
(Assignee)

Comment 23

9 years ago
(In reply to comment #18)
> (In reply to comment #17)

> In qcms we currently ignore profiles larger than 4MB.

Is there any objection to using a 4MB limit in libpr0n as well?
Definitely not.
(Assignee)

Comment 25

9 years ago
@joe, please clarify: definitely not do it, or definitely no objection?
Definitely no objection :)
(As long as we're talking about colour profiles only!)
(Assignee)

Comment 28

9 years ago
I did a Tryserver run with a 4MB limit and it reduced the time to process and reject hang.png to about 4 seconds.  Another thing we can do (see comment #10) is to increase the compression buffer size.  That's a tradeoff of memory for speed, potentially increasing the speed of reading all large iCCP chunks, not just the bogus ones.  I'm trying a run with zbuf set to 32k.  This also might have a beneficial effect on reading APNG chunks but that remains to be tested.
(Assignee)

Comment 29

9 years ago
Created attachment 420463 [details] [diff] [review]
limit libpng-1.4.0's cache of decompression buffers and expand buffer size

This patch applies a 4MB limit to the size of decompressed ancillary chunks and raises the PNG decompression buffer size from 8 to 32kbytes, when color management is enabled.
(Assignee)

Comment 30

9 years ago
The patched Firefox processes and rejects hang.png in about 1 second.

Successful Tryserver builds are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-bug-497056-v03-Jan06-limit-cache
Comment on attachment 382868 [details] [diff] [review]
Allow the bundled libpng to skip iCCP chunk when color_management is off (v00).

Approved for 1.9.1.8, a=dveditz for release-drivers
Attachment #382868 - Flags: approval1.9.2.1?
Attachment #382868 - Flags: approval1.9.1.8?
Attachment #382868 - Flags: approval1.9.1.8+
(Assignee)

Comment 32

9 years ago
The patch depends on libpng-1.4.0 being installed (bug #532645) which has been checked into mozilla central but has not requested or received approvals for other branches.
Blocks: 532645
(Assignee)

Comment 33

9 years ago
The first patch, which doesn't take care of oversized iCCP chunks when color management is enabled, should work with all branches, but is not needed for any branch which already has libpng-1.2.40.
(Assignee)

Updated

9 years ago
No longer blocks: 532645
Depends on: 532645
Keywords: checkin-needed
Whiteboard: [sg:dos] → [sg:dos][needs 1.9.1 landing]
Attachment #420463 - Flags: review?(joe)
(Assignee)

Comment 34

9 years ago
Re: discussion in bug #532645: There is no "small patch" to fix this bug completely  I could make one that adds the cache_limit stuff to the older embedded libpng.
(Assignee)

Updated

9 years ago
Attachment #420463 - Attachment description: limit libpng's cache of decompression buffers and expand buffer size → limit libpng-1.4.0's cache of decompression buffers and expand buffer size
(Assignee)

Comment 35

9 years ago
Created attachment 421643 [details] [diff] [review]
small patch for 1.9.1 to limit the cache
(Assignee)

Comment 36

9 years ago
Created attachment 421644 [details] [diff] [review]
small patch for 1.9.2 to limit the cache

This patch for 1.9.2 and the small patch in bug #492200 will bit rot each other because both try to update the same part of libimg/png/MOZCHANGES.
(Assignee)

Updated

9 years ago
Attachment #421643 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #421644 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #421643 - Attachment is patch: true
(Assignee)

Updated

9 years ago
Attachment #421644 - Attachment is patch: true
(Assignee)

Comment 37

9 years ago
Comment on attachment 421643 [details] [diff] [review]
small patch for 1.9.1 to limit the cache

I need to build the 1.9.1 patch over again.  It inavertently undoes the 1.9.1 update from bug #497056.
Attachment #421643 - Attachment is obsolete: true
Attachment #421643 - Flags: review?(joe)
(Assignee)

Comment 38

9 years ago
I meant ... inadvertently undoes the 1.9.1 update from bug #492200
A revised patch has been submitted to the Tryserver.
(Assignee)

Comment 39

9 years ago
@joe, would it simplify things if I make a patch for 1.9.2 that combines the simple v0 patch from bug #492200 with the small patch for 1.9.2 that I just uploaded here?
(Assignee)

Comment 40

9 years ago
Created attachment 421682 [details] [diff] [review]
small patch for 1.9.1 to limit the cache (v01)
(Assignee)

Updated

9 years ago
Attachment #421682 - Flags: review?(joe)
(Assignee)

Comment 41

9 years ago
Successfull Tryserver builds of a combined libpng security patch of 1.9.2 (this small 1.9.2 patch and the UMR bugfix patch at bug #492200) are available at
https://build.mozilla.org/tryserver-builds/glennrp@gmail.com-combined-492200-497056-Jan15/
blocking1.9.1: --- → needed
status1.9.1: ? → wanted
(Assignee)

Comment 42

9 years ago
Demo of why this has to be fixed:
html://www.simplesystems.org/users/glennrp/hanger
(Assignee)

Comment 43

9 years ago
Demo of why this has to be fixed:
http://www.simplesystems.org/users/glennrp/hanger
(Assignee)

Comment 44

9 years ago
The small patches don't protect browsers that use the system libpng instead of the embedded one.
(Assignee)

Comment 45

9 years ago
(In reply to comment #44)
> The small patches don't protect browsers that use the system libpng instead of
> the embedded one.

I've developed a patch to address this issue.  Tryserver testing now.
(Assignee)

Updated

9 years ago
Attachment #421644 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #421682 - Flags: review?(joe)
(Assignee)

Comment 46

9 years ago
My tryserver runs with the new patch seem OK but when I download the WINNT executable it won't run (triggers a report to Microsoft).  Neither will a baseline build that changes nothing except a comment.  Builds from january 13 do run.

http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-bug-497056-1.9.2-v02-Jan19-limit-cache
(Assignee)

Comment 47

9 years ago
Created attachment 422321 [details] [diff] [review]
small patch for 1.9.2 to limit the cache (v02)
Attachment #421644 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #422321 - Attachment is patch: true
(Assignee)

Updated

9 years ago
Depends on: 540672
(Assignee)

Comment 48

9 years ago
I am not pleased with the performance of the v02 patch on WinXP  It does reduce the hang time of the example linked to in comment #43 significantly (from hours to minutes) but not enough.  Imposing a smaller cache limit would risk rejecting valid PNG iCCP chunks.
(Assignee)

Comment 49

9 years ago
Libpng-1.4.1beta04 has a much better solution.  Pngcrush built with it reads the hang.png in .07 second, using the default PNG_ZBUF_SIZE == 8192 bytes and a 4MB limit on the growth of the iCCP chunk.
(Assignee)

Comment 50

9 years ago
Created attachment 423282 [details] [diff] [review]
decompress the iCCP chunk faster with the bundled libpng (v03, 1.9.2)

This patch uses the forthcoming libpng-1.4.1 decompression algorithm and a 4MB limit on expanded chunk size.  It can be applied to either 1.9.1 or 1.9.2.

Successful Tryserver builds are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.1-v03a-bug497056-24Jan (for 1.9.1) and at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-1264369591 (for 1.9.2).  These only fix the bug in the embedded libpng, not in the system libpng.
Attachment #421682 - Attachment is obsolete: true
Attachment #422321 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #423282 - Attachment description: small patch for 1.9.2 to limit the cache (v03) → decompress the iCCP chunk faster in 1.9.1 and 1.9.2 (v03)
Attachment #423282 - Attachment is patch: true
(Assignee)

Comment 51

9 years ago
The v03 patch also works against 1.9.3.  Here are the successfull TryServer builds.

http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.3-v03a-bug497056-24Jan
(Assignee)

Updated

9 years ago
Attachment #420463 - Attachment is obsolete: true
Attachment #420463 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #423282 - Attachment description: decompress the iCCP chunk faster in 1.9.1 and 1.9.2 (v03) → decompress the iCCP chunk faster(v03)
(Assignee)

Updated

9 years ago
Attachment #423282 - Attachment description: decompress the iCCP chunk faster(v03) → decompress the iCCP chunk faster with the bundled libpng (v03)
(Assignee)

Updated

9 years ago
Attachment #423282 - Flags: review?(joe)
(Assignee)

Comment 52

9 years ago
All three of the Tryserver sets seem to be 1.9.2.  Ignore the 1.9.1 and 1.9.3 builds in comment #49 and comment #50.  Side note: I wish the text boxes on the Tryserver submission form were wider.
(Assignee)

Updated

9 years ago
Attachment #423282 - Attachment description: decompress the iCCP chunk faster with the bundled libpng (v03) → decompress the iCCP chunk faster with the bundled libpng (v03, 1.9.2)
(Assignee)

Comment 53

9 years ago
Created attachment 423324 [details] [diff] [review]
decompress the iCCP chunk faster with the bundled libpng (v04, 1.9.3)
(Assignee)

Comment 54

9 years ago
Created attachment 423325 [details] [diff] [review]
decompress the iCCP chunk faster with the bundled libpng (v05, 1.9.1)
(Assignee)

Updated

9 years ago
Attachment #423324 - Attachment description: decompress the iCCP chunk fster with the bundled libpng (v04, 1.9.3) → decompress the iCCP chunk faster with the bundled libpng (v04, 1.9.3)
(Assignee)

Comment 55

9 years ago
Separate patches are required for 1.9.1, 1.9.2, and 1.9.3.  Successful Tryserver runs are at

patch v05 for 1.9.1: http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.1-v05a-bug497056-24Jan

patch v03 for 1.9.2: http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.2-v03b-bug497056-24Jan

patch v04 for 1.9.3: http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.3-v04a-bug497056-24Jan

None of these address this bug in decoders that use the system libpng.  That will require a separate set of patches to libpr0n/decoders/png.  Or we could require that the system libpng be at version 1.4.1 or better (libpng-1.4.1 will probably be out in about 2 weeks).
(Assignee)

Comment 56

9 years ago
Created attachment 423947 [details] [diff] [review]
decompress the iCCP chunk faster with the system libpng (v06, 1.9.3)

This patch revises the 1.9.3 PNG decoder in libpr0n to apply a 4MB limit to the size of the decompressed iCCP chunk and uses a 32kbyte buffer.
Tryserver runs with the stock libpng-1.4.0 were successful but of course only showed the first frame of APNG animations and therefore failed several of the unit tests.  The builds (with libpng-1.4.0) are at http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.3-v06g-bug497056-27Jan
(Assignee)

Comment 57

9 years ago
The build from comment #56 deals with the example in comment #43, hanging for about 1 second.
(Assignee)

Comment 58

9 years ago
Libpng-1.4.1 will contain a much-improved png_decompress_chunk() function that makes two passes, one to measure the length of decompressed chunk data and another to allocate memory and store the chunk data.  It's more than twice as fast and doesn't consume any memory when an overlimit chunk is detected.  I'm marking the v03, v04, and v05 patches obsolete and will have replacements using the new function as time permits.
(Assignee)

Updated

9 years ago
Attachment #423282 - Attachment is obsolete: true
Attachment #423282 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #423324 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #423325 - Attachment is obsolete: true
(Assignee)

Comment 59

9 years ago
Comment on attachment 423947 [details] [diff] [review]
decompress the iCCP chunk faster with the system libpng (v06, 1.9.3)

The v06 patch is not affected by comment #58.
Attachment #423947 - Flags: review?(joe)
(Assignee)

Comment 60

9 years ago
Created attachment 424287 [details] [diff] [review]
decompress the iCCP chunk faster with the bundled libpng (v07, 1.9.3)

Patch to port new decompression function from libpng-1.4.1 to mozilla-1.9.3.
Successful Tryserver builds are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.3-v07a-bug497056-29Jan
There is no perceptible delay when decoding the test case and no memory waste.
Attachment #424287 - Flags: review?(joe)
(Assignee)

Comment 61

9 years ago
Created attachment 424430 [details] [diff] [review]
decompress the iCCP chunk faster with the bundled libpng (v08, 1.9.2)

Tryserver builds with the v08 patch for porting png_decompress_chunk() from 1.9.3 to the bundled libpng in 1.9.2 are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.2-v08c-bug497056-30Jan
Attachment #424430 - Flags: review?(joe)
(Assignee)

Comment 62

9 years ago
Created attachment 424431 [details] [diff] [review]
decompress the iCCP chunk faster with the bundled libpng (v09, 1.9.1)

tryserver builds for the v09 patch for porting png_decompress_chunk() from 1.9.3 to the bundled libpng in 1.9.1 are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.1-v09d2-bug497056-30Jan
Attachment #424431 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #424430 - Attachment description: decompress the iCCP chunk faster with the bundled libpng (v08, 1.9.1) → decompress the iCCP chunk faster with the bundled libpng (v08, 1.9.2)
(Assignee)

Updated

9 years ago
Attachment #382868 - Attachment description: Allow the bundled libpng to skip iCCP chunk when color_management is off. → Allow the bundled libpng to skip iCCP chunk when color_management is off (v00).
(Assignee)

Comment 63

9 years ago
Created attachment 424487 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v11, 1.9.3)

This patch combines the v06 and v07 patches.  Tryserver builds are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.3-11e-Jan31
Attachment #423947 - Attachment is obsolete: true
Attachment #424287 - Attachment is obsolete: true
Attachment #424487 - Flags: review?(joe)
Attachment #423947 - Flags: review?(joe)
Attachment #424287 - Flags: review?(joe)
(Assignee)

Comment 64

9 years ago
Created attachment 424489 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v12, 1.9.2)

This patch revises the v06 patch for 1.9.2 and combines it with the v00 patch,
the v08 patch, and the small UMR patch from bug #492200.  Tryserver builds are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.2-v12c-Jan31
Attachment #424430 - Attachment is obsolete: true
Attachment #424489 - Flags: review?(joe)
Attachment #424430 - Flags: review?(joe)
(Assignee)

Comment 65

9 years ago
Created attachment 424490 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v13, 1.9.1)

This patch revises the v06 patch for 1.9.1 and combines it with the v00 patch, the v09 patch, and the patch from bug #397593.  Tryserver builds are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.1-v13c1-Jan31
Attachment #382868 - Attachment is obsolete: true
Attachment #424431 - Attachment is obsolete: true
Attachment #424490 - Flags: review?(joe)
Attachment #382868 - Flags: approval1.9.2.1?
Attachment #424431 - Flags: review?(joe)
(Assignee)

Comment 66

9 years ago
Created attachment 424537 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v14, 1.9.3)

Revised the v11 patch to remove the updates to mozpngconf.h, which are not needed any more with the embedded libpng-1.4.0.  This saves 2.5k in the size of the win32.zip download.  Tryserver builds are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-moz-1.9.3-v14a-Jan31
Attachment #424487 - Attachment is obsolete: true
Attachment #424537 - Flags: review?(joe)
Attachment #424487 - Flags: review?(joe)
Comment on attachment 382868 [details] [diff] [review]
Allow the bundled libpng to skip iCCP chunk when color_management is off (v00).

removing branch approval from obsolete patch, assume we'll want the 1.9.1 version of the patch currently waiting for joe's review.
Attachment #382868 - Flags: approval1.9.1.8+
(Assignee)

Comment 68

9 years ago
Comment on attachment 424489 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v12, 1.9.2)

The v12 patch for 1.9.3 has a typo in png.h (PNG_UNKNOWN_CHUNKS should be PNG_UNKNOWN_CHUNKS_SUPPORTED).  Preparing a replacement patch.
Attachment #424489 - Flags: review?(joe)
(Assignee)

Comment 69

9 years ago
Created attachment 424705 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v15, 1.9.2)
(Assignee)

Comment 70

9 years ago
Created attachment 425142 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v16, 1.9.3)
Attachment #424537 - Attachment is obsolete: true
Attachment #425142 - Flags: review?(joe)
Attachment #424537 - Flags: review?(joe)
(Assignee)

Comment 71

9 years ago
Created attachment 425143 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v17, 1.9.2)
Attachment #424489 - Attachment is obsolete: true
Attachment #424705 - Attachment is obsolete: true
(Assignee)

Comment 72

9 years ago
Created attachment 425144 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v18, 1.9.1)
Attachment #424490 - Attachment is obsolete: true
Attachment #425144 - Flags: review?(joe)
Attachment #424490 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #425143 - Attachment is patch: true
Attachment #425143 - Flags: review?(joe)
(Assignee)

Comment 73

9 years ago
Comment on attachment 425142 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v16, 1.9.3)

v16 still has a #define removed that should not be removed (PNG_NO_READ_USER_CHUNKS)
v19 patch coming tomorrow.
Attachment #425142 - Flags: review?(joe)
(Assignee)

Comment 74

9 years ago
Created attachment 425152 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v19, 1.9.3)
Attachment #425142 - Attachment is obsolete: true
Attachment #425152 - Flags: review?(joe)
(Assignee)

Comment 76

9 years ago
The existence of this bug has been inadvertently leaked to the public png-mng-implement list.

Even though the message body doesn't say anything about the bug, the subject

Subject: RE: [png-mng-implement] [Png-mng-security] vulnerability in png_decompress_chunk()

seems to give enough information for a determined cracker to figure it out in short order.

Please proceed with the reviews.  I'm satisfied with the patches now.
(Assignee)

Comment 77

9 years ago
Fixing bug #544747 fixes this bug and, if checked in to trunk, will make the v19 patch obsolete.  If v19 is checked in first then I'll rebuild the patch for bug #544747; no big deal.
(Assignee)

Comment 78

9 years ago
It is possible to provoke a crash, not just a delay, via this bug.  The code in png_decompress_chunk() seems to be safe, but if it almost fills memory and then png_push_save_buffer() runs out of memory (a reasonable scenario, since they are both running at the same time and both can ask for a lot of memory), there is a png_error() call which kills mozilla without freeing the large save_buffer.
This is fixed in the libpng-1.4.1 upgrade patch in bug #544747.  I recommend applying that patch (in its final form, when libpng-1.4.1 is released) instead of any of the ones posted here in this bug.  I will also prepare similar patches to upgrade 1.9.1 and 1.9.2 to use libpng-1.4.1 over in bug #544747.
(Assignee)

Comment 79

9 years ago
(In reply to comment #78)

> This is fixed in the libpng-1.4.1 upgrade patch in bug #544747.  I recommend
> applying that patch (in its final form, when libpng-1.4.1 is released) instead
> of any of the ones posted here in this bug.

On the other hand, if we are interested in avoiding zero-day exploits, we should apply the three patches here and then upgrade to libpng-1.4.1 shortly after it is released.  Is there someone here who can make such decisions?  I don't mind quickly redoing the libpng upgrade patches in the other bug to accommodate any bitrot caused by checking in the patches here.
(In reply to comment #79)
> if we are interested in avoiding zero-day exploits

What kind of zero-day? "I can annoy the hell out of you (but otherwise safe)" crashes or "I can invoke malware"? Is it throwing an exception from new, calling abort, or getting slapped down by the OS for trying to write somewhere it shouldn't, etc?

> Is there someone here who can make such decisions?

I can for 1.9.1 and probably 1.9.2 once I've got a reviewed patch.
(In reply to comment #79)
> On the other hand, if we are interested in avoiding zero-day exploits, we
> should apply the three patches here and then upgrade to libpng-1.4.1 shortly
> after it is released.  Is there someone here who can make such decisions?  I
> don't mind quickly redoing the libpng upgrade patches in the other bug to
> accommodate any bitrot caused by checking in the patches here.

I will review these patches, and we should get them into their respective branches post haste. We can then upgrade mozilla-central to libpng-1.4.1, if that's okay with you.

Glenn, does this problem exist in 1.9.0/Firefox 3.0?
(Assignee)

Comment 82

9 years ago
@Daniel, It is mostly in the category of annoy-the-hell but as I mentioned in comment #78 it may be possible to design a PNG or a page full of PNGs that causes a crash.  I don't have an exploit handy.  But the annoy-the-hell category consists of easily making a web site that stalls the browser for a specific amount of time (a few seconds, minutes, or hours).  Just make an iCCP chunk that is very tiny but decompresses to 1, 2, 5, 50 megabytes of zeroes.
Even legitimate 500k iCCP chunks will gum up the works.  The malformed one on the santa demo page expands to 60MB and stalls all current versions that don't ignore iCCP by 20-30 minutes, while the compressed iCCP chunk is only 50k.  I would hate to see such things deployed.  I don't think the problem exists in 3.0 but I'll check.

@Joe, your plan expressed in comment #81 is fine with me.  It will be almost trivial to upgrade to 1.4.1 from there, but the patch will of course need to be redone.
Attachment #425143 - Flags: review?(joe) → review-
Comment on attachment 425143 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v17, 1.9.2)


>diff -u8p -r -N -x '.mozconfig*' -x configure src-1.9.2/modules/libpr0n/decoders/png/nsPNGDecoder.cpp src192/modules/libpr0n/decoders/png/nsPNGDecoder.cpp

>-#if defined(PNG_UNKNOWN_CHUNKS_SUPPORTED)
>+#ifndef MOZPNGCONF_H /* We are using the system libpng */
>+#if defined(PNG_UNKNOWN_CHUNKS_SUPPORTED) || \
>+    defined(PNG_HANDLE_AS_UNKNOWN_SUPPORTED)
>   /* Ignore unused chunks */
>   if (gfxPlatform::GetCMSMode() == eCMSMode_Off) {
>     png_set_keep_unknown_chunks(mPNG, 1, color_chunks, 2);
>   }
>   png_set_keep_unknown_chunks(mPNG, 1, unused_chunks,
>      (int)sizeof(unused_chunks)/5);   
> #endif
> 
>+# if PNG_LIBPNG_VER < 10401
>+#  if defined(PNG_WRITE_SUPPORTED) || PNG_LIBPNG_VER > 10400
>+  if (gfxPlatform::GetCMSMode() != eCMSMode_Off) {
>+    /* Increase speed of decompressing large iCCP chunks (default buffer
>+       size is 8192) */
>+    png_set_compression_buffer_size(mPNG, (png_size_t)32768L);
>+  }
>+#  endif
>+# endif
>+#endif

[...snip...]

>+#if PNG_LIBPNG_VER < 10401
>+# ifndef MOZPNGCONF_H
>+#  if defined(PNG_WRITE_SUPPORTED) || PNG_LIBPNG_VER > 10400
>+  /* Revert to the default zlib buffer size */
>+  if (gfxPlatform::GetCMSMode() != eCMSMode_Off) {
>+    /* Increase speed of decompressing large iCCP chunks (default buffer
>+       size is 8192) */
>+    png_set_compression_buffer_size(decoder->mPNG, (png_size_t)8192);
>+  }
>+#  endif
>+# endif
>+#endif
>+

There might be problems in these sections. It looks really wrong - we check for PNG_LIBPNG_VER < 10401, and then for > 10400. Is there a reason for this? (Might just have been C&P from the initial version for 1.9.3, where we do have 1.4.0.)

Also, the second comment should be "Reset buffer size to default" or something like that.
Comment on attachment 425144 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v18, 1.9.1)

Same issues as 1.9.2 patch.
Attachment #425144 - Flags: review?(joe) → review-
Comment on attachment 425152 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v19, 1.9.3)


>diff -u8p -r -N -x '.mozconfig*' -x configure src-1.9/modules/libpr0n/decoders/png/nsPNGDecoder.cpp src/modules/libpr0n/decoders/png/nsPNGDecoder.cpp

>+#ifndef MOZPNGCONF_H
>+# if defined(PNG_WRITE_SUPPORTED) || PNG_LIBPNG_VER > 10400
>+  /* Revert to the default zlib buffer size */
>+  if (gfxPlatform::GetCMSMode() != eCMSMode_Off) {
>+    /* Increase speed of decompressing large iCCP chunks (default buffer
>+       size is 8192) */
>+    png_set_compression_buffer_size(decoder->mPNG, (png_size_t)8192);
>+  }
>+# endif
>+#endif

Only one issue in this patch, and that's the comment here. But I'll mark it r- for consistency, since we do need a new patch.
Attachment #425152 - Flags: review?(joe) → review-
(Assignee)

Comment 86

9 years ago
Yes.  3.0.17, on my wife's laptop, locks up on the demo page, even with gfx.color_management in the default "off" setting.

The reason for checks for 10401 and 10400 is that before 10400, png_set_compression_buffer() was not available if PNG_WRITE is not defined.  The check for 10401 is simply to see if libpng is already taking care of the problem so we need not do anything.  It is ugly but it's correct.  I made the function available in 1.4.0 just for this problem.

You're right about the cut-and-paste problem with the comments.  I'll make new patches to remove the redundant ones.
OK. We are, at least nominally, still supporting 3.0, so are you able to work up a patch for it? Or shall I?
(Assignee)

Comment 88

9 years ago
I'll generate a patch for 1.9.0.

I'm retracting my request in comment #8 to keep this quiet, because of the explicit instructions for exploiting the bug that I gave in comment #82.
(Assignee)

Comment 89

9 years ago
(In reply to comment #83)

> >+#  if defined(PNG_WRITE_SUPPORTED) || PNG_LIBPNG_VER > 10400

> It looks really wrong - we check for
> PNG_LIBPNG_VER < 10401, and then for > 10400. Is there a reason for this?
> (Might just have been C&P from the initial version for 1.9.3, where we do have
> 1.4.0.)

For the sake of clarity I'm changing these lines in all 3 patches to
#  if defined(PNG_WRITE_SUPPORTED) || PNG_LIBPNG_VER == 10400
(In reply to comment #86)
> The reason for checks for 10401 and 10400 is that before 10400,
> png_set_compression_buffer() was not available if PNG_WRITE is not defined.

Ok, but right now the check is strictly greater than, and I think it should be >=.
 
> The check for 10401 is simply to see if libpng is already taking care of the
> problem so we need not do anything.

Can you add a comment to this effect?
(Assignee)

Comment 91

9 years ago
D'oh!  The function was not available before 10401.  But we have already established that we aren't using 10401, because the bug has been fixed in 10401, so the check isn't needed at all.  Thanks for the sharp-eyed review.
(Assignee)

Comment 92

9 years ago
Created attachment 426235 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v20, 1.9.0)
(Assignee)

Comment 93

9 years ago
Created attachment 426236 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v21, 1.9.1)
Attachment #425144 - Attachment is obsolete: true
(Assignee)

Comment 94

9 years ago
Created attachment 426237 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v22, 1.9.2)
Attachment #425143 - Attachment is obsolete: true
(Assignee)

Comment 95

9 years ago
Created attachment 426238 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v23, 1.9.3)

These 4 patches also make a small change in png_push_save_buffer() from libpng-1.4.1 that stops a potential memory leak; I had inadvertently omitted that change from the previous set of patches.  The test for PNG_LIBPNG_VER<10400 is eliminated and the redundant comment is gone.
Attachment #425152 - Attachment is obsolete: true
(Assignee)

Comment 96

9 years ago
(In reply to comment #95)
> The test for PNG_LIBPNG_VER<10400 is eliminated
  /for PNG_LIBPNG_VER > 10400 is eliminated/
(Assignee)

Comment 97

9 years ago
All of the ugly PNG_LIBPNG_VER testing and the substitute memory allocator can be removed once libpng-1.4.1 is out and configure.in requires the system libpng to be 1.4.1 or later.
Comment on attachment 426235 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v20, 1.9.0)

Whoops. you forgot to include the body of malloc_callback in this patch. :(
Attachment #426235 - Flags: review+ → review-
Comment on attachment 426237 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v22, 1.9.2)

r+, but can you remove the || PNG_LIBPNG_VER == 10400 parts as you did in the other patches?
Attachment #426237 - Flags: review+
(Assignee)

Comment 100

9 years ago
There are Tryserver runs for all 4, with embedded and system
libpng.  I won't post the individual links here.  Go to

http://build.mozilla.org/tryserver-builds and look for the
glennrp directories with file prefixes v20b, v21b, v22b, and v23c;
also for v20c, v21c, v22c, and v23d for the "system libpng"
builds.  They aren't really using the system library but
I commented out the 'include "mozpngconf.h"' in pngconf.h
so it uses a non-customized libpng, except for the addition
of APNG support, to see the effect of using a system libpng
with APNG support.
(Assignee)

Comment 101

9 years ago
Created attachment 426317 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v24, 1.9.2)

Removed redundant PNG_LIBPNG_VER test
Attachment #426237 - Attachment is obsolete: true
(Assignee)

Comment 102

9 years ago
Created attachment 426318 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v25, 1.9.0)

Added replacement for png_malloc()
Attachment #426235 - Attachment is obsolete: true
(Assignee)

Comment 103

9 years ago
Tryserver runs in progress, but what could possibly go wrong?
(Assignee)

Updated

9 years ago
Attachment #426317 - Flags: review?(joe)
(Assignee)

Comment 104

9 years ago
Comment on attachment 426318 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v25, 1.9.0)

For some reason I don't get email notifications of the 1.9.0 Tryserver runs but the binaries are available anyway.
Attachment #426318 - Flags: review?(joe)
(Assignee)

Comment 105

9 years ago
One of my regular spammers discovered last night how to get through gmail's "don't show external content" setting (apparently by embedding a binary jpeg in the message).  It was an innocuous v14gr4 ad, but could just has well have been a png with a highly-compressed huge iCCP chunk to make it stall.
(Assignee)

Comment 106

9 years ago
(In reply to comment #88)
> I'll generate a patch for 1.9.0.
> 
> I'm retracting my request in comment #8 to keep this quiet, because of the
> explicit instructions for exploiting the bug that I gave in comment #82.

I submitted a CERT report today:

Subject: huge compressed ancillary chunks stall libpng-based PNG decoders

When libpng is presented with a PNG file containing a highly
compressed huge ancillary chunk (zTXt, iTXt, iCCP) it stalls and
consumes huge amounts of memory.  Libpng-1.4.1 provides a solution
that is available either when the library is built (by imposing a
limit on the amount of expansion of compressed ancillary chunks) and
at run time (by providing a function by which applications can impose
their own limit).

An exploit is demonstrated on a page linked from
<www.simplesystems.org/users/glennrp/hanger>. The animation shown on
the page is fine; it's the separate hang20min.png on the page that
stops the animation and hangs the viewer for 20-30 minutes.

All versions of libpng prior to 1.4.1 and all versions of Firefox from
3.0 are vulnerable.   I think earlier versions of Firefox ignore all
compressed ancillary chunks and would not be vulnerable, but I haven't
checked them.  I haven't tested other gecko-based browsers but they
are most likely to be vulnerable as well.  Internet Explorer ignores
PNG ancillary chunks and is not vulnerable.  All versions of pngcrush,
imagemagick, and graphicsmagick are vulnerable.

Glenn
(Assignee)

Comment 107

9 years ago
Someone has just presented me with a valid PNG file that has
a large (800+ kbyte) iCCP chunk.  Firefox-3.7 with the libpng-1.4.1
patch crashes on it sporadically (2 times out of 4 on WinXPPro).

Back to the drawing board.....
(Assignee)

Comment 108

9 years ago
Further info: the new test case crashes old programs as well,
such as pngcrush built with libpng-1.2.37.  It seems to
be crashing when it attempts to read multiple tEXt chunks
in the end_ptr.  Here are the last few bytes of the
files:

2406540 371 377  \0 267 311   . 341 326 247   w 023  \0  \0  \0 022   t
2406560   E   X   t   c   o   m   m   e   n   t  \0   A   p   p   l   e
2406600   M   a   r   k  \n   y 355  \v 254  \0  \0  \0 017   t   E   X
2406620   t   J   P   E   G   -   Q   u   a   l   i   t   y  \0   8   4
2406640   .   ' 031 311  \0  \0  \0 021   t   E   X   t   J   P   E   G
2406660   -   C   o   l   o   r   s   p   a   c   e  \0   2 201  \r   !
2406700   $  \0  \0  \0 030   t   E   X   t   J   P   E   G   -   C   o
2406720   l   o   r   s   p   a   c   e   -   N   a   m   e  \0   R   G
2406740   B 317 361   @ 356  \0  \0  \0   !   t   E   X   t   J   P   E
2406760   G   -   S   a   m   p   l   i   n   g   -   f   a   c   t   o
2407000   r   s  \0   2   x   2   ,   1   x   1   ,   1   x   1 345   Q
2407020 333 270  \0  \0  \0  \0   I   E   N   D 256   B   ` 202
2407036

Pngcrush gets as far as displaying "AppleMark" and then crashes with
a segfault.

So the problem isn't a bad bugfix for the present bug; it's a newly discovered
somewhat related old bug, having something to do with libpng's system
of cacheing multiple tEXt chunks.

I'll open a new security bug for this once I get approval to publish
the problematic image.  I guess we can proceed with the current patch
set.
(Assignee)

Comment 109

9 years ago
The new test failing is worse than I thought.  It means that although libpng-1.4.1 is correctly avoiding a hang or a crash on malformed iCCP chunks, it is failing to decode valid ones properly.  Something is wrong with the new algorithm.  Cancelling all of the requests for review for now, awaiting the next iteration of libpng-1.4.1beta.
(Assignee)

Updated

9 years ago
Attachment #426236 - Flags: review+ → review-
(Assignee)

Updated

9 years ago
Attachment #426238 - Flags: review+ → review-
(Assignee)

Updated

9 years ago
Attachment #426317 - Flags: review?(joe) → review-
(Assignee)

Updated

9 years ago
Attachment #426318 - Flags: review?(joe) → review-
(Assignee)

Comment 110

9 years ago
I found the culprit.  It is a misplaced closing curley bracket in pngrutil.c/png_decompress_chunk().  New patches and libpng-1.4.1beta11
are in the works.
(Assignee)

Comment 111

9 years ago
Created attachment 426799 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v26, 1.9.2)
Attachment #426317 - Attachment is obsolete: true
(In reply to comment #106)
> I submitted a CERT report today:

Let us know if someone assigns a CVE, we'll add it as the bug's alias.

(In reply to comment #108)
> Further info: the new test case crashes old programs as well [...]
> I'll open a new security bug for this once I get approval to publish
> the problematic image.

Thanks. You could open a placeholder bug without the image if you like ("libpng crash caching multiple tEXt chunks"?) and stick the testcase-wanted keyword on it.
(Assignee)

Comment 113

9 years ago
I can't reproduce the crash I observed yesterday with the new image.  There must have been some kind of pilot error on my part.

We replaced the image data with one that is licensed to redistribute.  The embedded ICC profile doesn't have any explicit copyright marking in it.

Here it is, as indexed and RGB PNG, with and without the color profile: 

   <http://www.simplesystems.org/users/glennrp/patricia>

So far there is no public link to that image.
(Assignee)

Comment 114

9 years ago
Created attachment 426869 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v30, 1.9.0)
Attachment #426318 - Attachment is obsolete: true
(Assignee)

Comment 115

9 years ago
Created attachment 426870 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v31, 1.9.1)
Attachment #426236 - Attachment is obsolete: true
(Assignee)

Comment 116

9 years ago
Created attachment 426871 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v32, 1.9.2)
Attachment #426799 - Attachment is obsolete: true
(Assignee)

Comment 117

9 years ago
Created attachment 426872 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v33, 1.9.3)
Attachment #426238 - Attachment is obsolete: true
(Assignee)

Comment 118

9 years ago
The patches for 1.9.0, 1.9.1, and 1.9.2 are much larger than before, but they are actually much simpler.  All of them bring libpng to the same condition, version 1.4.1 (beta).  Instead of making custom patches with subtle differences for each (time-consuming and very error-prone), I simply copied all of the files in the png directory (except for Makefile.in which is slightly different).  Reviewing should be easier: simply verify that the contents of the modules/libimg/png directory are the same, and only review one of them.  The modules/libpr0n/decoders/png/nsPNGDecoder.cpp
are different in each release, but that cannot be helped.

The Tryserver builds are at
 
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-v30b-1.9.0-497056-Feb13

http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-v31b-1.9.1-497056-Feb13a

http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-v32b-1.9.2-497056-Feb13

http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-v33a-1.9.3-497056-Feb13
(Assignee)

Updated

9 years ago
Attachment #426872 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #426871 - Flags: review?(joe)
I'd really rather not change libpng in the non-mozilla-central (1.9.3) branches, if it's at all possible.

Other than the piecemeal fixes you've had to make, is there a reason underlying your decision to make this change? I thought we were getting pretty close to a patch for all four branches.
(Assignee)

Comment 120

9 years ago
(In reply to comment #119)
> I'd really rather not change libpng in the non-mozilla-central (1.9.3)
> branches, if it's at all possible.
> 
> Other than the piecemeal fixes you've had to make, is there a reason underlying
> your decision to make this change? I thought we were getting pretty close to a
> patch for all four branches.

I thought I gave my reasons.

1. It's easier to make the patches.
2. It's simpler.
3. It's much less error-prone.
4. It's easier to review

also

5. There is a smaller attack surface if they are all alike.
6. Various bugs, mostly with regard to APNG support, might remain
   unfixed in some versions.
7. The patches that I posted work, as far as I can tell.


I can make custom piecemeal patches but I can't think
of a reason for doing it.

You want simplicity.  Simplicity can be measured by the
fact that there are zero lines changed in libpng from
1.9.0 through 1.9.3.  True, there are lots of lines
changed from the current state of each to the fixed
state, but that seems irrelevant to me.

If there is a good reason for doing custom-made patches
for each, I will do it, hopefully before 25 Feb.
The biggest problem with this approach is that our security folk (and I, to be perfectly honest) are not too enthusiastic about introducing a bunch of churn into our old, soon-to-be-unmaintained branches. I totally get that it's easier to prove correct for _this bug_, but it's harder to prove correct _in general_.

So I guess the real question is "what needs to be fixed with the patches just before this revision"? You r-'d them earlier because of problems in libpng-1.4.1b, but I think those turned out to be unrelated?
(Assignee)

Comment 122

9 years ago
They were directly related.  There was a mistake in png_decompress_chunk() that correctly rejected malformed iCCP chunks but also started rejecting some valid ones.  But I can build new patches out of the previous set.  I think it's just a matter of pasting in the latest version of png_decompress_chunk() and adding png_deflate() in pngrutil.c.  I will proceed with that.
Thanks, Glenn. I really, truly appreciate all the work you've put in to this bug (and everything else you've done recently, too!)
(Assignee)

Comment 124

9 years ago
Created attachment 427312 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v34, 1.9.2)

Here is a smaller, targeted patch for 1.9.2.  It leaves the UMR bug from bug #492200 unfixed.

TryServer runs are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-v34b-1.9.2-497056-Feb17
Attachment #427312 - Flags: review?(joe)
(Assignee)

Comment 125

9 years ago
Created attachment 427404 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v35, 1.9.1)
Attachment #426870 - Attachment is obsolete: true
Attachment #427404 - Flags: review?(joe)
(Assignee)

Comment 127

9 years ago
Created attachment 427456 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v36, 1.9.0)
Attachment #426869 - Attachment is obsolete: true
Attachment #427456 - Flags: review?(joe)
(Assignee)

Comment 128

9 years ago
Tryserver builds for the v36 patch (1.9.0) are at
http://build.mozilla.org/tryserver-builds/glennrp@gmail.com-v36a-1.9.0-497056-Feb17
(Assignee)

Comment 129

9 years ago
I hope the patches are satisfactory.  The automatic push of Firefox-3.5.8 this morning seems to have pretty much destroyed my Ubuntu box, where my sources reside.  I'm reduced to posting this from my Windows box.
(Assignee)

Comment 130

9 years ago
Sorry for the off-topic rant.  I am back in business now; had to reboot my cable box and LAN box (which were working fine with the Windows box and laptop) and then the Ubuntu box.  I don't know whether this was the fault of FF-3.5.8 or Ubuntu or something about my LAN, so I don't know where to send a bug report.

Back on topic: we are on track to release libpng-1.4.1 (also 1.2.43 and 1.0.53) on 25 Feb, with disclosure of the exploit of this bug.  The "rc" releases that I posted today and the recent libpng betas do give a hint about the problem, since they contain the new png_decompress_chunk() that is in the v32 through v36 patches, without much of an explanation of why we made the change.
(Assignee)

Comment 131

9 years ago
Libpng-1.4.1 is out.  Checking in the patch from bug #544747 will bit-rot the v33 patch for the trunk/1.9.3 but the rest will of course still be OK.  Likewise, checking in the v33 patch will bit-rot the libpng-1.4.1 upgrade patch.  I'm prepared to replace whichever patch becomes bit-rotten.

Comment 132

9 years ago
There is a typo in patch for 1.9.2
bug #927056 should be bug #497056.
(Assignee)

Comment 133

9 years ago
Here is the libpng security advisory about this issue:
libpng.sf.net/ADVISORY-1.4.1.html
(Assignee)

Comment 134

9 years ago
Created attachment 429577 [details] [diff] [review]
v34 patch for 1.9.2, with typo fixed.
Attachment #427312 - Attachment is obsolete: true
Attachment #429577 - Flags: review?(joe)
Attachment #427312 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Attachment #429577 - Attachment description: v34 patch with typo fixed. → v34 patch for 1.9.2, with typo fixed.
Attachment #427404 - Flags: superreview?(roc)
Attachment #427404 - Flags: review?(joe)
Attachment #427404 - Flags: review+
Attachment #427456 - Flags: superreview?(roc)
Attachment #427456 - Flags: review?(joe)
Attachment #427456 - Flags: review+
Attachment #429577 - Flags: superreview?(roc)
Attachment #429577 - Flags: review?(joe)
Attachment #429577 - Flags: review+
Comment on attachment 426871 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v32, 1.9.2)

Pretty sure this attachment is obsoleted by v34, right?
Attachment #426871 - Attachment is obsolete: true
Attachment #426871 - Flags: review?(joe)
Attachment #427404 - Flags: superreview?(roc) → superreview+
One other question - attachment 426872 [details] [diff] [review], v33 for 1.9.3, contains a bunch of libpng 1.4.1 code. What's your ideal order of operations here? Would you rather we checked in 1.4.1, then made a small targeted fix for system libpng < 1.4.1? Or something else?
Comment on attachment 427456 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v36, 1.9.0)

+/*
+ * Decompress trailing data in a chunk.  The assumption is that chunkdata
+ * points at an allocated area holding the contents of a chunk with a
+ * trailing compressed part.  What we get back is an allocated area
+ * holding the original prefix part and an uncompressed version of the
+ * trailing part (the malloc area passed in is freed).
+ */
+/*
+ * Decompress trailing data in a chunk.  The assumption is that chunkdata
+ * points at an allocated area holding the contents of a chunk with a
+ * trailing compressed part.  What we get back is an allocated area
+ * holding the original prefix part and an uncompressed version of the
+ * trailing part (the malloc area passed in is freed).
+ */

Oops?

Otherwise OK
Attachment #427456 - Flags: superreview?(roc) → superreview+
Attachment #429577 - Flags: superreview?(roc) → superreview+
Attachment #427404 - Flags: approval1.9.1.9?
Attachment #427456 - Flags: approval1.9.0.19?
Attachment #429577 - Flags: approval1.9.2.2?
(Assignee)

Comment 138

9 years ago
(In reply to comment #136)
> One other question - attachment 426872 [details] [diff] [review], v33 for 1.9.3, contains a bunch of
> libpng 1.4.1 code. What's your ideal order of operations here? Would you rather
> we checked in 1.4.1, then made a small targeted fix for system libpng < 1.4.1?
> Or something else?

Let's go with checking 1.4.1 and then
I'll redo the v33 patch.  We still need it because the 1.4.1
patch doesn't address older system libpng.
(Assignee)

Comment 139

9 years ago
(In reply to comment #137)
> (From update of attachment 427456 [details] [diff] [review])
> +/*
> + * Decompress trailing data in a chunk.
> + * Decompress trailing data in a chunk.
> Oops?
> 
> Otherwise OK

Yes, oops.  The Tryserver didn't pick up that cut-n-paste error.
(Assignee)

Comment 140

9 years ago
Created attachment 429605 [details] [diff] [review]
v36 patch for 1.9.0 with redundant comment removed
Attachment #427456 - Attachment is obsolete: true
Attachment #429605 - Flags: review?(joe)
Attachment #427456 - Flags: approval1.9.0.19?
(Assignee)

Comment 141

9 years ago
(In reply to comment #135)
> (From update of attachment 426871 [details] [diff] [review])
> Pretty sure this attachment is obsoleted by v34, right?

It is, if it's decided that we won't do the full
update to libpng-1.4.1 in 1.9.2
(Assignee)

Updated

9 years ago
Flags: blocking1.9.0.19?
Attachment #429605 - Flags: review?(joe)
Attachment #429605 - Flags: review+
Attachment #429605 - Flags: approval1.9.0.19?
Not blocking 1.9.0.19, but we'll take the patch.

Joe: can you make a comment about safety and testing here? Specifically: what testing coverage do we have, and what are the potential risks involved in changing libPNG?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.19?
Flags: blocking1.9.0.19-
We have a number of tests of libpng, including animated and static images, so we won't break libpng totally with these changes. The potential risk is of subtle breakage, which we won't be able to detect until this is out in the wild.

Maybe Glenn can talk about any extra testing he's done on these patches?
(Assignee)

Comment 144

9 years ago
Other than downloading the WIN32 binary from the Tryserver and trying it informally, I haven't much to say.  I am not a great fan of the same person doing the testing who did the building.
Duplicate of this bug: 550159
Alias: CVE-2010-0205
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.2: --- → ?
status2.0: --- → ?
True for ad-hoc testing, but if you had a testsuite knowing you'd run it against that would have value.
blocking1.9.2: ? → needed
status1.9.2: ? → wanted
status2.0: ? → ---
(Assignee)

Comment 147

9 years ago
I think this can be un-hidden now.  I have removed the sample exploits that
were linked to comments 42, 82, 105, 106, and 113.  That still leaves
instructions for building an exploit, though, but anyone can figure that out
from public libpng documents anyhow.  And my exploit pages might be archived
somewhere, although google, yahoo, and bing haven't found them.
(Assignee)

Comment 148

9 years ago
(In reply to comment #146)
> True for ad-hoc testing, but if you had a testsuite knowing you'd run it
> against that would have value.

I only used the Tryserver test suite, plus the exploit pages mentioned in the previous comment, plus a few other more-or-less random pages.  Building a decent test suite is somewhere on my TODO list, but if someone beats me to it that would be fine with me.
Attachment #427404 - Flags: approval1.9.1.9? → approval1.9.1.9+
Attachment #429577 - Flags: approval1.9.2.2? → approval1.9.2.2+
Comment on attachment 429605 [details] [diff] [review]
v36 patch for 1.9.0 with redundant comment removed

a=beltzner for the various branches
Attachment #429605 - Flags: approval1.9.0.19? → approval1.9.0.19+
Checking in modules/libimg/png/MOZCHANGES;
/cvsroot/mozilla/modules/libimg/png/MOZCHANGES,v  <--  MOZCHANGES
new revision: 3.27; previous revision: 3.26
done
Checking in modules/libimg/png/pngrutil.c;
/cvsroot/mozilla/modules/libimg/png/pngrutil.c,v  <--  pngrutil.c
new revision: 3.25; previous revision: 3.24
done
Checking in modules/libpr0n/decoders/png/nsPNGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/png/nsPNGDecoder.cpp,v  <--  nsPNGDecoder.cpp
new revision: 1.84; previous revision: 1.83
done
Checking in modules/libpr0n/encoders/png/nsPNGEncoder.cpp;
/cvsroot/mozilla/modules/libpr0n/encoders/png/nsPNGEncoder.cpp,v  <--  nsPNGEncoder.cpp
new revision: 1.11; previous revision: 1.10
done
Keywords: checkin-needed → fixed1.9.0.19
OK! Now that we've got the branches sorted out, I think our next step should be landing 1.4.1 on mozilla-central.

Glenn, once 1.4.1 is landed, we'll only need to fix this for people who don't use 1.4.1, right? Should we bother?
(Assignee)

Comment 154

9 years ago
The 1.4.1 patch for 1.9.3 modifies nsPNGDecoder.cpp to take care of people using older system libraries.  About all that's left to do after checking in 1.4.1 is some minor cosmetic fixup (folding some long lines) but that is of course not urgent.

There may be people using 1.2.43 which also has the 1.4.1 security fix in pngrutil.c backported to it, except that the replacement memory allocator function must be used with 1.2.43, because png_set_chunk_malloc_max() is not available in 1.2.43.  But that's already taken care of in the 1.4.1 patch.
OK. I've just checked in the 1.4.1 update, so I think we're all done here! Hooray!
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:dos][needs 1.9.1 landing] → [sg:dos]
Comment on attachment 426872 [details] [diff] [review]
decompress the iCCP chunk faster with libpng (v33, 1.9.3)

This patch isn't necessary now that bug 544747 has landed.
Attachment #426872 - Attachment is obsolete: true
Attachment #426872 - Flags: review?(joe)
(Assignee)

Comment 157

9 years ago
(In reply to comment #156)
> (From update of attachment 426872 [details] [diff] [review])
> This patch isn't necessary now that bug 544747 has landed.

Agreed, but the libpng-1.4.1 patch introduced a typo that's not in this patch (png_set_user_chunk_malloc_max should be png_set_chunk_malloc_max), which causes a compile error when attempting to build with a system libpng-1.4.1.
This new problem is addressed in bug #551438.  Fortunately, this problem doesn't exist in the patches for the older mozilla versions (1.9.0, 1.9.1, 1.9.2)
Verified for 1.9.0 using the attached ping with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19pre) Gecko/2010031106 GranParadiso/3.0.19pre (.NET CLR 3.5.30729). 

Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.9pre) Gecko/20100311 Shiretoko/3.5.9pre (.NET CLR 3.5.30729). 

Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100311 Namoroka/3.6.2pre (.NET CLR 3.5.30729).

The png now renders immediately and no longer takes the cpu up to 99%.
Keywords: fixed1.9.0.19 → verified1.9.0.19, verified1.9.1, verified1.9.2
(Assignee)

Comment 160

9 years ago
Is 1.8 vulnerable?  I thought it wasn't decoding the iCCP chunk.
(Assignee)

Comment 161

9 years ago
(In reply to comment #160)
> Is 1.8 vulnerable?  I thought it wasn't decoding the iCCP chunk.

1.8 does indeed decode (but doesn't use) the iCCP chunk and is vulnerable, when using the embedded libpng.  When using a system libpng, it ignores iCCP and
is not vulnerable.
(Assignee)

Comment 162

9 years ago
Created attachment 433312 [details] [diff] [review]
v37: simpler 1.8 patch

This patch merely disables iCCP (and cHRM) chunk reading by the embedded libpng.  The mod to nsPNGEncoder.cpp makes it possible to use libpng-1.4 as the system library.

Updated

9 years ago
Keywords: perf
If I read this bug report correctly, the fix in 1.9.0.19 can't be effective. See bug 556117.
(Assignee)

Comment 164

9 years ago
(In reply to comment #163)
> If I read this bug report correctly, the fix in 1.9.0.19 can't be effective.
> See bug 556117.

It is effective when the bundled libpng is used, but it can't be built with the system library as you reported, due to the patch using eCMSMode() instead of isCMSEnabled() to test whether the iCCP chunk data is needed.  I'll upload a patch to bug #556117.  Thanks.
So I take it that the patch in the bundled libpng is enough by itself.
(Assignee)

Comment 166

9 years ago
(In reply to comment #165)
> So I take it that the patch in the bundled libpng is enough by itself.

It is enough if you are using the bundled libpng.  But if you are using a separate system libpng it isn't, and needs something similar to what you posted in bug #556117.  I'm testing a patch that makes the changes you suggested plus checks for the memory-limiting function that became available in 1.4.1 and uses that instead of setting up a replacement memory-allocation function, if it's available.
(Assignee)

Updated

9 years ago
Attachment #433312 - Flags: review?(joe)
(Assignee)

Updated

9 years ago
Blocks: 556117
Group: core-security
Could someone give me access to the testcase, so that I can validate my security builds ?
blocking2.0: ? → final+
Attachment #433312 - Flags: review?(joe) → review+
You need to log in before you can comment on or make changes to this bug.