1117406 - PNG: heap-overflow crash [@qcms_transform_data_rgba_out_lut_sse2]

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Comment 1

[Christoph Diehl [:posidron]]

Attachment: Testcase

Comment 2

[Christoph Diehl [:posidron]]

Attachment: BrowserAgent_asan.txt

Comment 3

[Jeff Muizelaar [:jrmuizel]]

The png file is data_1_output_Output.txt

Comment 4

[Glenn Randers-Pehrson]

pngcheck and ImageMagick reports about the test image:

glenn.rp> pngcheck -v data_1_output_Output.txt
File: data_1_output_Output.txt (46657 bytes)
  chunk IHDR at offset 0x0000c, length 13
    200 x 171 image, 24-bit RGB, interlaced
  chunk tEXt at offset 0x00025, length 43, keyword: Creation Time
  chunk tIME at offset 0x0005c, length 7:  5 Apr 2006 13:09:43 UTC
  chunk pHYs at offset 0x0006f, length 9: 2834x2834 pixels/meter (72 dpi)
  chunk gAMA at offset 0x00084, length 4: 0.45455
  chunk tRNS at offset 0x00094, length 6
    red = 0x04be, green = 0xf9c7, blue = 0x4165
  chunk IDAT at offset 0x000a6, length 46471
    zlib: deflated, 32K window, maximum compression
  CRC error in chunk IDAT (computed b5f11d99, expected 6275f80e)
ERRORS DETECTED in data_1_output_Output.txt

glenn.rp> cp data_1_output_Output.txt crash.png
glenn.rp> identify -verbose crash.png # (ImageMagick, built with libpng-1.6.16)
identify: tRNS chunk has out-of-range samples for bit_depth `crash.png' @ warning/png.c/MagickPNGWarningHandler/1656.
identify: IDAT: invalid distance too far back `crash.png' @ error/png.c/MagickPNGErrorHandler/1630.
identify: corrupt image `crash.png' @ error/png.c/ReadPNGImage/3957.

Comment 5

[Glenn Randers-Pehrson]

Can you verify that you were using gfx.color_management.mode=1?  I think that with mode=0 or mode=2, while
reading a PNG with a gAMA chunk but no cHRM chunk, we wouldn't reach the bug.

Comment 6

[Christoph Diehl [:posidron]]

(In reply to Glenn Randers-Pehrson from comment #5)
> Can you verify that you were using gfx.color_management.mode=1?  I think
> that with mode=0 or mode=2, while
> reading a PNG with a gAMA chunk but no cHRM chunk, we wouldn't reach the bug.

user_pref("gfx.color_management.mode", 1);

Confirmed.

Comment 7

[Glenn Randers-Pehrson]

Thanks.  The default configuration (with gfx.color_management.mode=2)
would be vulnerable as well, if the test case had included a valid cHRM chunk or
sRGB chunk.  I'm now setting up to test whether the invalid tRNS chunk is
causing the crash due to some mixup of whether channels is 3 or 4 in the
PNG decoder and/or in libpng.

Comment 8

[Glenn Randers-Pehrson]

Attachment: Simpler test case, bug1117406.html

Here's a simpler test case.  It's a valid PNG except that the tRNS chunk values are out of range.  I verified that when decoding this and the original test case, the PNG decoder calls GetCMSRGBATransform() when it should be calling GetCMSRGBTransform(), which leads to an overwrite beyond the last row of interlacebuf.  I expect to have a patch ready tomorrow.  I wasn't able to get an ASAN build working on my machine, so it would be good to verify that the simple test case also triggers the ASAN crash.

Comment 9

[Glenn Randers-Pehrson]

Attachment: v00-1117406-let-libpng-handle-out-of-range-tRNS

Please try this patch.  It removes the special handling of out-of-range tRNS values from nsPNGDecoder.cpp; it was getting it wrong anyway.

Comment 10

[Christoph Diehl [:posidron]]

(In reply to Glenn Randers-Pehrson from comment #8)
> Created attachment 8544359 [details]
> Simpler test case, bug1117406.html
> I wasn't able to get an ASAN build working on my machine, so it would be good
> to verify that the simple test case also triggers the ASAN crash.

We have ASan builds here: https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/latest/

Comment 11

[Glenn Randers-Pehrson]

Verified that the simpler test case also crashes ("Tab crashed" + asan report), tested with downloaded ASAN build of 6 Jan 2015.

Comment 12

[Glenn Randers-Pehrson]

Comment on attachment 8544530 [details] [diff] [review]
v00-1117406-let-libpng-handle-out-of-range-tRNS

The v00 patch does stop the crash but it also regresses bug #428045

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I assume this affects pretty much any supported Gecko revision?

Comment 14

[Glenn Randers-Pehrson]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #13)
> I assume this affects pretty much any supported Gecko revision?

Yeah, back to Firefox 1.9.1 or so.  I'll upload a v01 patch shortly.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

FYI, this missed Firefox 35 already (the release candidate was built yesterday), so it'll probably need to wait to land until sometime in the middle of the next cycle. The sec team will say for sure when they respond to the sec-approval request on the patch.

Comment 16

[Glenn Randers-Pehrson]

Attachment: v01-1117406-fix-handling-out-of-range-tRNS

Need tryserver run.

Comment 17

[Glenn Randers-Pehrson]

(In reply to Glenn Randers-Pehrson from comment #14)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #13)
> > I assume this affects pretty much any supported Gecko revision?
> 
> Yeah, back to Firefox 1.9.1 or so.  I'll upload a v01 patch shortly.

Actually anything since August 19, 2008, when I introduced the bug, which
is a missing "num_trans = 0;" in nsPNGDecoder.cpp.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=19e62ca8d9a4

Comment 19

[Glenn Randers-Pehrson]

Comment on attachment 8544801 [details] [diff] [review]
v01-1117406-fix-handling-out-of-range-tRNS

The try run looks good.  I downloaded the asan build and it shows the simple test case without a crash, and does not regress bug #42805 (see http://www.simplesystems.org/users/glennrp/tRNS/).  R?

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8544801 [details] [diff] [review]
v01-1117406-fix-handling-out-of-range-tRNS

Review of attachment 8544801 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsPNGDecoder.cpp
@@ +560,5 @@
>  
>    if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
>      int sample_max = (1 << bit_depth);
>      png_color_16p trans_values;
>      png_get_tRNS(png_ptr, info_ptr, &trans, &num_trans, &trans_values);

Is it possible for this call to fail?

Comment 21

[Glenn Randers-Pehrson]

png_get_valid returns 0 or nonzero

sample_max can overflow if sizeof(int) is 16 bits or less but that's harmless;
it would just roll around to zero.  That was fixed a while ago in libpng17
but hasn't been backported to libpng16 yet.

png_get_tRNS returns 0 or nonzero, and fills
in trans (a byte array), num_trans (a png_uint_32 integer),
and trans_values (a png_uint_16 struct), after checking
each of those arguments for NULL first.

But it can provide a wrong answer, if bit_depth is 8 or less but
any of trans_values is greater than 255.  Libpng produces a warning
in that situation, but we ignore all libpng warnings in non-debug
builds.

Comment 22

[Glenn Randers-Pehrson]

Attachment: v02-1117406-fix-handling-out-of-range-tRNS

Rebased against last night's update to nsPNGDecoder.cpp

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8546641 [details] [diff] [review]
v02-1117406-fix-handling-out-of-range-tRNS

Review of attachment 8546641 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsPNGDecoder.cpp
@@ +564,5 @@
>           (int)trans_values->green > sample_max ||
>           (int)trans_values->blue > sample_max))) {
>        // clear the tRNS valid flag and release tRNS memory
>        png_free_data(png_ptr, info_ptr, PNG_FREE_TRNS, 0);
> +      num_trans=0;  // this line added at bug #1117406

Drop this comment and add spaces around '='.

You could also add a comment like:

// reset num_trans so that we don't try to use them.

Comment 24

[Glenn Randers-Pehrson]

Attachment: v03-1117406-fix-handling-out-of-range-tRNS

Revised as requested.

Comment 25

[Glenn Randers-Pehrson]

Attachment: v04-1117406-fix-handling-out-of-range-tRNS

The v03 patch omitted the checkin info.  Please transfer "r+" from v02 to v04.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Per comment 15, this needs to get sec-approval since it's a sec-crit affecting multiple branches.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 27

[Glenn Randers-Pehrson]

Attachment: tRNS_bug_428045.html

Demonstrates that bug #428045 was not completely fixed.  It still accepts invalid trans values that are exactly 2^bit_depth and masks the top byte.
This happens even when the v03 patch is applied.  Different bug, same piece of code.

Comment 28

[Glenn Randers-Pehrson]

Attachment: v05-1117406-fix-handling-out-of-range-tRNS

Fix for the off-by-one error mentioned in the previous comment; also fixes the ASAN crash without drawing attention to that.

Comment 29

[Glenn Randers-Pehrson]

The v05 patch is essentially what's already in libpng-1.7.0beta47.  The equivalent for the "num_trans = 0" fix has been in libpng17 since version 1.7.0alpha01, which was released December 15, 2012.

Comment 30

[Glenn Randers-Pehrson]

Need a try run of the v05 patch

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5262b348c55

Comment 32

[Glenn Randers-Pehrson]

Attachment: v05 (release) -1117406-fix-handling-out-of-range-tRNS

patch for mozilla-release, tested locally on Ubuntu.

Comment 33

[Glenn Randers-Pehrson]

Attachment: v05 (beta) -1117406-fix-handling-out-of-range-tRNS

patch for mozilla-beta, tested locally on Ubuntu.

Comment 34

[Glenn Randers-Pehrson]

The v05-1117406-* patch can also be applied to aurora, without any modification.  Tested locally with Ubuntu 14:04.

Comment 35

[Glenn Randers-Pehrson]

I verified that a trivial variation of the "simpler test case" will crash recent regular (non-asan) nightlies with "Tab crashed".

Comment 36

[Glenn Randers-Pehrson]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #26)
> Per comment 15, this needs to get sec-approval since it's a sec-crit
> affecting multiple branches.

I've got a security-request comment ready for the v05 patch, once v05 gets r+.

Comment 37

[Glenn Randers-Pehrson]

Comment on attachment 8547126 [details] [diff] [review]
v05-1117406-fix-handling-out-of-range-tRNS

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

    Easily, if an attacker notices the addition of "num_trans = 0" and investigates why.  Hopefully the patch draws attention away from that by fixing a closely related but harmless bug at the same time.  The attacker would need to convince the target user to run with the non-default gfx.color_management.mode==1.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

    No.

Which older supported branches are affected by this flaw?

    All.

If not all supported branches, which bug introduced the flaw?

    Bug #428025

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

    Yes, patches for release and beta are attached to this bug. For aurura, use the same patch as for mozilla-central.

How likely is this patch to cause regressions; how much testing does it need?

    Very unlikely.  A patch equivalent to this one has been included in libpng17 since version 1.7.0alpha01, about 2 years ago. There may be a different appearance (not a regression) of images that have a tRNS chunk containing values that are exactly 2^bit_depth (e.g., 256 in a PNG with 8-bit samples); in such images the transparency handling will be done correctly instead of incorrectly.

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8547126 [details] [diff] [review]
v05-1117406-fix-handling-out-of-range-tRNS

sec-approval+ for checkin on January 26. Please nominate Aurora, Beta, and ESR31 patches so we can check them in after everything is green on Trunk.

Comment 39

[Glenn Randers-Pehrson]

Attachment: v05a (aurora) -1117406-fix-handling-out-of-range-tRNS

Approval Request Comment
[Feature/regressing bug #]: 428025
[User impact if declined]: crash via malformed PNG
[Describe test coverage new/current, TBPL]: local tests with malformed PNG only due to security; tested on trunk with usual test set via tryserver
[Risks and why]: Low, patch is simple and this code has been used in libpng17 for 2 years
[String/UUID change made/needed]: none

This patch is identical to the v05-* patch for Trunk.

Comment 40

[Glenn Randers-Pehrson]

Comment on attachment 8549294 [details] [diff] [review]
v05 (beta) -1117406-fix-handling-out-of-range-tRNS


Approval Request Comment
[Feature/regressing bug #]: 428025
[User impact if declined]: crash via malformed PNG
[Describe test coverage new/current, TBPL]: local tests with malformed PNG only due to security; tested on trunk with usual test set via tryserver
[Risks and why]: Low, patch is simple and this code has been used in libpng17 for 2 years
[String/UUID change made/needed]: none

This patch differs from the v05-* patch for trunk only in the offset of the changed code.

Comment 41

[Glenn Randers-Pehrson]

Attachment: v05e (esr31) - 1117406-fix-handling-out-of-range-tRNS

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c329304fe62

Comment 43

[Glenn Randers-Pehrson]

Are you asking for a test case that would "paint a bullseye on the security problem"?  If so, that would change the answer to that question in comment #37.  I can provide one if that's what's required.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

We should land an automated test for this once Fx36 ships. I'm just setting the flag as a reminder.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c329304fe62

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/495caaec91f4
https://hg.mozilla.org/releases/mozilla-beta/rev/a532a2852b2f

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/5ee3807b4bb2

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/495caaec91f4

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/a20cbb3466e4
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ef823d6963e8
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c0613670d6fc

Comment 50

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1420110134/
** user_pref("gfx.color_management.mode", 1);
** data_1_output_Output.txt --> crash.png [Crashed]
** simpler test case bug1117406.html from comment #8 [Crashed]

Went through verification using the following builds:

* fx38: http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1424187631/
* fx37: http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1424185888/
* fx36: http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1424184332/
* fx31.5.0esr: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/31.5.0esr-candidates/build2/

Test Cases Used:

* Opened both POC mentioned above several times in regular windows/tabs
* Opened both POC mentioned above several times in private windows/tabs
* Opened both POC mentioned above several times in e10s windows/tabs (only applies to m-c)
* Ensured that fx didn't crash using both ("gfx.color_management.mode", 1); & ("gfx.color_management.mode", 2);

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/a20cbb3466e4
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/ef823d6963e8