124.67 KB, image/png
20.75 KB, image/png
2.96 KB, patch
|Details | Diff | Splinter Review|
Created attachment 771193 [details] repro A buffer overflow (write) occurs when the attached picture is opened in aurora channel asan build from a few days ago. $ opt/firefox-aurora-asan/firefox ff-bofw-rowcallback.png 2>&1 | symbolize | c++filt ================================================================= ==13655== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f60479d1b80 at pc 0x7f606009aeb4 bp 0x7fffd0325910 sp 0x7fffd0325908 WRITE of size 4 at 0x7f60479d1b80 thread T0 #0 0x7f606009aeb3 in mozilla::image::nsPNGDecoder::row_callback(png_struct_def*, unsigned char*, unsigned int, int) /home/aki/src/mozilla-aurora/image/decoders/nsPNGDecoder.cpp:759 #1 0x7f6063c1b52a in MOZ_PNG_push_have_row /home/aki/src/mozilla-aurora/media/libpng/pngpread.c:1460 #2 0x7f6063c1aab9 in MOZ_PNG_proc_IDAT_data /home/aki/src/mozilla-aurora/media/libpng/pngpread.c:1131 0x7f60479d1b80 is located 0 bytes to the right of 224000-byte region [0x7f604799b080,0x7f60479d1b80) allocated by thread T0 here: #0 0x43deca in posix_memalign ??:0 #1 0x7f6063558fb9 in TryAllocAlignedBytes(unsigned long) /home/aki/src/mozilla-aurora/gfx/thebes/gfxImageSurface.cpp:89 #2 0x7f606003b0a2 in imgFrame::Init(int, int, int, int, gfxASurface::gfxImageFormat, unsigned char) /home/aki/src/mozilla-aurora/image/src/imgFrame.cpp:198 #3 0x7f606002114e in mozilla::image::RasterImage::InternalAddFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*, imgFrame**) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:1283 #4 0x7f6060021cd0 in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*, imgFrame**) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:1412 #5 0x7f6060021ee2 in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char**, unsigned int*, imgFrame**) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:1467 #6 0x7f606001bf2f in mozilla::image::RasterImage::InitDecoder(bool, bool) /home/aki/src/mozilla-aurora/image/src/RasterImage.cpp:2161 Shadow byte and word: 0x1fec08f3a370: fa 0x1fec08f3a370: fa fa fa fa fa fa fa fa More shadow bytes: 0x1fec08f3a350: 00 00 00 00 00 00 00 00 0x1fec08f3a358: 00 00 00 00 00 00 00 00 0x1fec08f3a360: 00 00 00 00 00 00 00 00 0x1fec08f3a368: 00 00 00 00 00 00 00 00 =>0x1fec08f3a370: fa fa fa fa fa fa fa fa 0x1fec08f3a378: fa fa fa fa fa fa fa fa 0x1fec08f3a380: fa fa fa fa fa fa fa fa 0x1fec08f3a388: fa fa fa fa fa fa fa fa 0x1fec08f3a390: fa fa fa fa fa fa fa fa Stats: 252M malloced (255M for red zones) by 333799 calls Stats: 49M realloced by 17163 calls Stats: 220M freed by 198590 calls Stats: 83M really freed by 140106 calls Stats: 440M (112722 full pages) mmaped in 110 calls mmaps by size class: 8:212979; 9:32764; 10:12285; 11:16376; 12:2048; 13:1536; 14:1280; 15:384; 16:768; 17:1376; 18:48; 19:40; 20:20; 21:2; mallocs by size class: 8:248243; 9:43800; 10:11322; 11:21445; 12:2360; 13:1984; 14:1609; 15:461; 16:974; 17:1464; 18:72; 19:42; 20:21; 21:2; frees by size class: 8:132541; 9:31130; 10:8119; 11:19784; 12:1559; 13:1323; 14:1412; 15:333; 16:826; 17:1443; 18:61; 19:38; 20:20; 21:1; rfrees by size class: 8:101684; 9:16682; 10:5679; 11:13055; 12:765; 13:636; 14:455; 15:200; 16:613; 17:304; 18:28; 19:3; 20:1; 21:1; Stats: malloc large: 1601 small slow: 2140 ==13655== ABORTING
Keywords: crash, csec-bounds, sec-critical, testcase
Created attachment 771247 [details] testcase-mozilla-small Hm, we hit that as well over the days but hadn't checked the logs yet. I will attach a smaller testcase.
As for attachment 771247 [details] the following field got mutated as the crash occurred: LocateChunk-0.Chunk_IHDR.Core.Data.BitDepth
This looks like a single channel image?
(In reply to Milan Sreckovic [:milan] from comment #3) > This looks like a single channel image? Never mind, gdb was messing with me. It's a regular RGB.
Looks like our image size sniffing is wrong, and we never catch that. Easy enough to fix.
Assignee: nobody → joe
This was probably caused by bug 869125, which implies that this is in everything but release. Please update these flags if I'm wrong.
status-firefox22: --- → unaffected
status-firefox23: --- → affected
status-firefox24: --- → affected
status-firefox25: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?
Created attachment 771464 [details] [diff] [review] check the actual frame rather than image metadata Well, I stupidly put this in in bug 869125 by checking image metadata, which is created by the decoder, rather than the frame itself. This should fix it, though obviously we need to test. (It fixes the crash I see, though.) https://tbpl.mozilla.org/?tree=Try&rev=c2c91d003a2d
Attachment #771464 - Flags: review?
Attachment #771464 - Flags: review? → review?(seth)
It was incredibly hard to review this since bugzilla.js shows thumbnails of attachments, which causes Firefox to immediately crash when you load this page. What fun.
Comment on attachment 771464 [details] [diff] [review] check the actual frame rather than image metadata Review of attachment 771464 [details] [diff] [review]: ----------------------------------------------------------------- Nice.
Attachment #771464 - Flags: review?(seth) → review+
Yeah, I had to turn off bugzilla.js.
Comment on attachment 771464 [details] [diff] [review] check the actual frame rather than image metadata [Security approval request comment] How easily could an exploit be constructed based on the patch? With a little bit of difficulty, though it's not *that* hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The check-in comment could. Which older supported branches are affected by this flaw? If not all supported branches, which bug introduced the flaw? Bug 869125 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Easy. How likely is this patch to cause regressions; how much testing does it need? Not terribly likely to cause regressions.
Attachment #771464 - Flags: sec-approval?
Shoot. Forgot Which older supported branches are affected by this flaw? 23, 24, 25.
But 22 is clear then?
Comment on attachment 771464 [details] [diff] [review] check the actual frame rather than image metadata sec-approval+ after discussion with Dveditz. We should have branch patches made and nominated for after trunk checkin.
Attachment #771464 - Flags: sec-approval? → sec-approval+
Comment on attachment 771464 [details] [diff] [review] check the actual frame rather than image metadata Adding branch approval per discussion with Dveditz.
(In reply to Al Billings [:abillings] from comment #13) > But 22 is clear then? Yep
status-firefox23: affected → fixed
status-firefox24: affected → fixed
status-firefox25: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/50e3c3008b8d Seems like this should have a test?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Definitely, but committing the testcase means we open the vulnerability up to basically anybody to exploit, so I'll leave that to security.
In this situation, we create the test as a separate patch (sometimes even a cloned bug just for that) and then check it in six weeks after we ship the fix in the final release to the public. We would love a checked in test though.
tracking-firefox23: ? → +
tracking-firefox24: ? → +
tracking-firefox25: ? → +
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → unaffected
You need to log in before you can comment on or make changes to this bug.