PNG: heap-overflow crash [@qcms_transform_data_rgba_out_lut_sse2]

VERIFIED FIXED in Firefox 36

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: posidron, Assigned: glennrp+bmo)

Tracking

(4 keywords)

Trunk
mozilla38
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox34 wontfix, firefox35 wontfix, firefox36+ verified, firefox37+ verified, firefox38+ verified, firefox-esr3136+ verified, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main36+][adv-esr31.5+])

Attachments

(8 attachments, 6 obsolete attachments)

61.71 KB, application/octet-stream
Details
14.37 KB, text/plain
Details
922 bytes, text/html
Details
3.44 KB, text/html
Details
2.14 KB, patch
jrmuizel
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
2.19 KB, patch
Details | Diff | Splinter Review
2.14 KB, patch
Details | Diff | Splinter Review
2.16 KB, patch
Details | Diff | Splinter Review
No description provided.
Posted file Testcase
Keywords: testcase
The png file is data_1_output_Output.txt
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.
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.
Flags: needinfo?(cdiehl)
Assignee: nobody → glennrp+bmo
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(cdiehl)
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.
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.
Flags: needinfo?(cdiehl)
Please try this patch.  It removes the special handling of out-of-range tRNS values from nsPNGDecoder.cpp; it was getting it wrong anyway.
Flags: needinfo?(ryanvm)
(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/
Flags: needinfo?(cdiehl)
Verified that the simpler test case also crashes ("Tab crashed" + asan report), tested with downloaded ASAN build of 6 Jan 2015.
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
Attachment #8544530 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
I assume this affects pretty much any supported Gecko revision?
Flags: needinfo?(glennrp+bmo)
(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.
Flags: needinfo?(glennrp+bmo)
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.
Need tryserver run.
Flags: needinfo?(ryanvm)
(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 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?
Attachment #8544801 - Flags: review?(jmuizelaar)
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?
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.
Rebased against last night's update to nsPNGDecoder.cpp
Attachment #8544801 - Attachment is obsolete: true
Attachment #8544801 - Flags: review?(jmuizelaar)
Attachment #8546641 - Flags: review?(jmuizelaar)
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.
Attachment #8546641 - Flags: review?(jmuizelaar) → review+
Revised as requested.
Attachment #8546909 - Flags: review?(jmuizelaar)
The v03 patch omitted the checkin info.  Please transfer "r+" from v02 to v04.
Attachment #8546909 - Attachment is obsolete: true
Attachment #8546909 - Flags: review?(jmuizelaar)
Attachment #8547111 - Flags: review?(jmuizelaar)
Attachment #8546641 - Attachment is obsolete: true
Attachment #8547111 - Flags: review?(jmuizelaar) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
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.
Fix for the off-by-one error mentioned in the previous comment; also fixes the ASAN crash without drawing attention to that.
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.
Need a try run of the v05 patch
Flags: needinfo?(ryanvm)
Attachment #8547111 - Attachment is obsolete: true
Flags: needinfo?(ryanvm)
Attachment #8547126 - Flags: review?(jmuizelaar)
patch for mozilla-release, tested locally on Ubuntu.
patch for mozilla-beta, tested locally on Ubuntu.
The v05-1117406-* patch can also be applied to aurora, without any modification.  Tested locally with Ubuntu 14:04.
I verified that a trivial variation of the "simpler test case" will crash recent regular (non-asan) nightlies with "Tab crashed".
(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+.
Flags: needinfo?(ryanvm)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(ryanvm)
Attachment #8547126 - Flags: review?(jmuizelaar) → review+
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.
Attachment #8547126 - Flags: sec-approval?
Whiteboard: [checkin on 1/26]
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.
Attachment #8547126 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(jmuizelaar)
Attachment #8551434 - Flags: approval-mozilla-aurora?
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.
Attachment #8549294 - Flags: approval-mozilla-beta?
Attachment #8549292 - Attachment is obsolete: true
Attachment #8551434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8549294 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8551510 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
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.
Flags: needinfo?(ryanvm)
We should land an automated test for this once Fx36 ships. I'm just setting the flag as a reminder.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/mozilla-central/rev/0c329304fe62
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Whiteboard: [adv-main36+][adv-esr31.5+]
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);
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.