945912 - Crash [@ MOZ_PNG_do_expand_plte] with broken png file

Status: RESOLVED FIXED

Description

[Christian Holler (:decoder)]

Created attachment 8341932 [details]
f427b6bee1acd1fea3ec953bc556a18a.png

The attached PNG file crashes for me in Firefox (built from mozilla-central 4bf430d990e5).


ASan trace:

==25902==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000002fc (pc 0x7fe90ea1eb35 sp 0x7fff61352f90 bp 0x0000000002fc T0)
    #0 0x7fe90ea1eb34 in MOZ_PNG_do_expand_plte media/libpng/pngrtran.c:4675
    #1 0x7fe90ea18af3 in MOZ_PNG_do_read_trans media/libpng/pngrtran.c:2162
    #2 0x7fe90ea174a6 in MOZ_PNG_push_proc_row media/libpng/pngpread.c:1155
    #3 0x7fe90ea16cda in MOZ_PNG_proc_IDAT_data media/libpng/pngpread.c:1108
    #4 0x7fe90ea0f800 in MOZ_PNG_push_read_IDAT media/libpng/pngpread.c:986
    #5 0x7fe90ea0bcdc in MOZ_PNG_proc_some_data media/libpng/pngpread.c:127
    #6 0x7fe90ea0bcdc in MOZ_PNG_process_data media/libpng/pngpread.c:40
    #7 0x7fe90a44d4a4 in mozilla::image::nsPNGDecoder::WriteInternal(char const*, unsigned int) image/decoders/nsPNGDecoder.cpp:356
    #8 0x7fe90a40b13d in mozilla::image::Decoder::Write(char const*, unsigned int) image/src/Decoder.cpp:107
    #9 0x7fe90a3f677a in mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int) image/src/RasterImage.cpp:2150
    #10 0x7fe90a3ff9b4 in mozilla::image::RasterImage::DecodePool::DecodeSomeOfImage(mozilla::image::RasterImage*, mozilla::image::RasterImage::DecodePool::DecodeType, unsigned int) image/src/RasterImage.cpp:3415
    #11 0x7fe90a3fb51a in mozilla::image::RasterImage::DecodePool::DecodeABitOf(mozilla::image::RasterImage*) image/src/RasterImage.cpp:3215
    #12 0x7fe90a3fae47 in mozilla::image::RasterImage::RequestDecodeCore(mozilla::image::RasterImage::RequestDecodeType) image/src/RasterImage.cpp:2319
    #13 0x7fe90c14cb30 in nsImageLoadingContent::OnStopRequest(imgIRequest*, tag_nsresult) content/base/src/nsImageLoadingContent.cpp:251
[...]

Comment 1

[Daniel Veditz [:dveditz]]

crashes on same address on windows: bp-2084589a-45fa-48cf-87a7-27de52131211

Comment 2

[Daniel Veditz [:dveditz]]

From the crashing line I'm assuming palette is null, and that the offset read can be controlled by the size of the image. Can it be arbitrarily large in an attempt to read from some valid section of memory? But if the loop is going to wind down toward zero eventually then maybe this is a guaranteed crash and not a useful information disclosure.

Comment 3

[Glenn Randers-Pehrson]

A zero-length or NULL PLTE chunk should reach pngset.c line 535 which triggers a "benign error", that is treated in libpng-1.6.7 by default as a warning. I'm seeing the crash on Ubuntu (64-bit) with the current nightly (2013-12-10, 29.0a1, using the bundled libpng-1.6.7 in media/libpng).  It seems that this is a libpng design error that will need to be fixed upstream.  Meanwhile, I'm testing a mozilla patch that simply

changes line 530 of pngset.c from 
png_chunk_report(png_ptr, "Invalid palette", PNG_CHUNK_ERROR);
to
png_chunk_error(png_ptr, "Invalid palette");

Firefox 25.0.1-Ubuntu does not appear to have the bug.

Comment 4

[Glenn Randers-Pehrson]

Created attachment 8346300 [details] [diff] [review]
v00-945912 fix handling of incorrect palette length in pngset.c

Current mozilla-central with the v00 patch does not crash on my Ubuntu platform.  It ignores the empty PLTE chunk and displays the image.  There's no libpng warning because we suppress them.

Comment 5

[Glenn Randers-Pehrson]

The bug was introduced to mozilla-central by checkin of libpng-1.6.6, bug #841734, on 26 September 2013.
I think this means that Firefox 27.0 is vulnerable and Firefox 26.0 and earlier are not. Libpng is planning to release the bugfix, in version 1.6.8, on Thursday 19 December 2013.  I have reported the libpng vulnerability to CERT (Tracking ID VRF#HP4TV3J2).

BTW, are the dates in https://wiki.mozilla.org/Releases all off by one year?

Comment 6

[Glenn Randers-Pehrson]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> From the crashing line I'm assuming palette is null, and that the offset
> read can be controlled by the size of the image. Can it be arbitrarily large
> in an attempt to read from some valid section of memory?

png_ptr->palette is always initialized to zero by png_create_read_struct().
"sp" is a png_byte, so "*sp" is in the range 0-255.  The size of the image
controls where "sp" is read from (a valid location in the row data), but reading
from "*sp" (NULL dereference of png_ptr->palette which is a png_colorp struct that
has three one-byte entries, red, green, and blue) is always in the range 0-767.

Comment 7

[Christian Holler (:decoder)]

(In reply to Glenn Randers-Pehrson from comment #6)
> is always in the range 0-767.

That range should still be safe for reading and not provide an opportunity to read valid memory for malicious purposes. Opening up the bug, thanks for the analysis! :)

Comment 8

[Glenn Randers-Pehrson]

This bug is CVE-2013-6954 and VU#650142.  It can be fixed either by applying the v00 patch or by upgrading libpng to version 1.6.8, or both.  I'll open a separate bug to upgrade libpng.

Comment 9

[Glenn Randers-Pehrson]

Needs try server run.  If applied, this will bit rot the libpng-1.6.8 patch slightly.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=5f9eb9087a56

Comment 11

[Glenn Randers-Pehrson]

try server results look pretty good, all green except for one timeout that I can't tell was related to this patch.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Not at all.

Comment 13

[Glenn Randers-Pehrson]

Comment on attachment 8346300 [details] [diff] [review]
v00-945912 fix handling of incorrect palette length in pngset.c

try server results look pretty good, all green except for one timeout that I can't tell was related to this one-line patch.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e96704e1a7ea

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e96704e1a7ea

Please nominate this for uplift to the affected branches.

Comment 16

[Glenn Randers-Pehrson]

Comment on attachment 8346300 [details] [diff] [review]
v00-945912 fix handling of incorrect palette length in pngset.c

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Upgrade to libpng-1.6.6 bug #886499
User impact if declined: DoS via a malformed PNG that has a zero-length PLTE chunk
Testing completed (on m-c, etc.): Tested on m-c with attached test case
Risk to taking this patch (and alternatives if risky): Low; it's a one-line change
String or IDL/UUID changes made by this patch:

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8346300 [details] [diff] [review]
v00-945912 fix handling of incorrect palette length in pngset.c

we can take this up to release so it will ride along with the 27.0.1 dot release going to build today.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/93a51a9585b0

I'll push this to aurora/beta in a little bit with some other uplifts.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/896b9a37ff5b
https://hg.mozilla.org/releases/mozilla-beta/rev/02935dcaa786

Comment 20

[Glenn Randers-Pehrson]

The v00 patch may not have been tested with PR_LOGGING undefined, and it's likely that it won't build
with PR_LOGGING undefined because png_chunk_error() gets defined out of pngerror.c.  If that happens, the fix is to use png_error() instead of png_chunk_error().  In libpng-1.6.9 we are using png_error(); see bug #952505.  I think PR_LOGGING is always set (via FORCE_PR_LOGGING) in the public Firefox releases, so this problem may not happen.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is it worth adding a test for this?

Comment 22

[Glenn Randers-Pehrson]

Created attachment 8374962 [details]
PNG for test case: 64x32 indexed with zero-length palette.

zero-plte-ref.png for testcase

Comment 23

[Glenn Randers-Pehrson]

Created attachment 8375006 [details] [diff] [review]
t00-945912-crashtest.diff

The t00 patch adds a crashtest for this bug.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/02935dcaa786

Comment 25

[Catalin Varga [QA][:VarCat]]

I reproduce the crash using the attached corrupted png file on Mac OS X 10.8.5 and Win 7 x64
Using the FF 27 release build id : 20140127194636 .

The bug was verified using the following environment:

FF 27.0.1 
Build Id : 20140212131424
OS: Win 7 x64, Mac Os x 10.8.5 and Ubuntu 12.04 x32

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8375006 [details] [diff] [review]
t00-945912-crashtest.diff

Confirmed that this crashes pre-fix and doesn't afterwards :)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c1080ed8c03

Thanks, Glenn!

Comment 28

[Catalin Varga [QA][:VarCat]]

I reproduce the crash using the attached corrupted png file on Mac OS X 10.8.5 and Win 7 x64
Using the FF 27 release build id : 20140127194636 .

The bug was verified using the following environment:

FF 28.0b3
Build Id : 20140213172947
OS: Win 7 x64, Mac Os x 10.8.5 and Ubuntu 13.04 x32

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c1080ed8c03

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d9aa8730b36
https://hg.mozilla.org/releases/mozilla-beta/rev/631a07ce77cd

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/631a07ce77cd