Closed Bug 945912 Opened 7 years ago Closed 7 years ago
Crash [@ MOZ
_PNG _do _expand _plte] with broken png file
518 bytes, image/png
1023 bytes, patch
|Details | Diff | Splinter Review|
82 bytes, image/png
959 bytes, patch
|Details | Diff | Splinter Review|
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 [...]
crashes on same address on windows: bp-2084589a-45fa-48cf-87a7-27de52131211
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.
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.
Assignee: nobody → glennrp+bmo
OS: Linux → All
Hardware: x86_64 → All
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.
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?
See Also: → 841734
(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.
(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! :)
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.
Needs try server run. If applied, this will bit rot the libpng-1.6.8 patch slightly.
try server results look pretty good, all green except for one timeout that I can't tell was related to this patch.
Not at all.
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.
Attachment #8346300 - Flags: review?(jmuizelaar)
Attachment #8346300 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/e96704e1a7ea Please nominate this for uplift to the affected branches.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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 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.
Attachment #8346300 - Flags: approval-mozilla-release?
Attachment #8346300 - Flags: approval-mozilla-release+
Attachment #8346300 - Flags: approval-mozilla-beta?
Attachment #8346300 - Flags: approval-mozilla-beta+
Attachment #8346300 - Flags: approval-mozilla-aurora?
Attachment #8346300 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-release/rev/93a51a9585b0 I'll push this to aurora/beta in a little bit with some other uplifts.
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.
Is it worth adding a test for this?
zero-plte-ref.png for testcase
The t00 patch adds a crashtest for this bug.
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 on attachment 8375006 [details] [diff] [review] t00-945912-crashtest.diff Confirmed that this crashes pre-fix and doesn't afterwards :)
Attachment #8375006 - Flags: review+
Flags: in-testsuite? → in-testsuite+
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
You need to log in before you can comment on or make changes to this bug.