Last Comment Bug 758200 - (CVE-2012-4202) ASAN: Heap-buffer-overflow at image::RasterImage::DrawFrameTo
(CVE-2012-4202)
: ASAN: Heap-buffer-overflow at image::RasterImage::DrawFrameTo
Status: VERIFIED FIXED
[asan][adv-track-main17+][adv-track-e...
: crash, csectype-bounds, sec-critical, testcase
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Other Branch
: x86_64 Linux
: -- critical (vote)
: mozilla18
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 06:56 PDT by Atte Kettunen
Modified: 2014-07-02 12:47 PDT (History)
18 users (show)
rforbes: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
-
wontfix
+
fixed
+
fixed
17+
fixed


Attachments
Repro-gifs and html (71.43 KB, application/java-archive)
2012-05-24 06:56 PDT, Atte Kettunen
no flags Details
AddressSanitizer Debug Log (11.71 KB, text/plain)
2012-05-24 08:51 PDT, Christian Holler (:decoder)
no flags Details
ASAN-report as use-after-free (3.68 KB, text/plain)
2012-06-07 11:41 PDT, Atte Kettunen
no flags Details
Repro-files for uaf (982.77 KB, application/java-archive)
2012-06-11 09:31 PDT, Atte Kettunen
no flags Details
zero out buffers when creating new frames for GIFs (1.48 KB, patch)
2012-09-14 15:46 PDT, Joe Drew (not getting mail)
justin.lebar+bug: review+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
dveditz: sec‑approval+
Details | Diff | Splinter Review

Description Atte Kettunen 2012-05-24 06:56:58 PDT
Created attachment 626785 [details]
Repro-gifs and html

This bug seems to be some sort of race condition. The bug is fairly hard to reproduce, but I was able to reproduce it with the files in the attached zip and with the build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/decoder@own-hero.net-9751d29c325
d/try-linux64-debug/firefox-15.0a1.en-US.linux-x86_64.tar.bz2
 


ASAN report: ( Using build from http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/decoder@own-hero.net-9751d29c325
d/try-linux64-debug/firefox-15.0a1.en-US.linux-x86_64.tar.bz2 )

==31319== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fd2b6d89e80 at pc 0x7fd305566383 bp 0x7fff393c7c10 sp 0x7fff393c7c08
READ of size 4 at 0x7fd2b6d89e80 thread T0
    #0 0x7fd305566383 in mozilla::image::RasterImage::DrawFrameTo(imgFrame*, imgFrame*, nsIntRect&) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:2070
    #1 0x7fd30555c2ec in mozilla::image::RasterImage::DoComposite(nsIntRect*, imgFrame*, imgFrame*, int) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:1885
    #2 0x7fd30555af67 in mozilla::image::RasterImage::AdvanceFrame(mozilla::TimeStamp, nsIntRect*) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:369
    #3 0x7fd30555c96b in mozilla::image::RasterImage::RequestRefresh(mozilla::TimeStamp const&) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:412
    #4 0x7fd30575563d in nsRefreshDriver::ImageRequestEnumerator(nsISupportsHashKey*, void*) /builds/slave/try-lnx64-dbg/build/layout/base/nsRefreshDriver.cpp:454
    #5 0x7fd3075e906b in PL_DHashTableEnumerate /builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/build/pldhash.cpp:715
    #6 0x7fd30575545a in nsTHashtable<nsISupportsHashKey>::EnumerateEntries(PLDHashOperator (*)(nsISupportsHashKey*, void*), void*) /builds/slave/try-lnx64-dbg/build/../../dist/include/nsTHashtable.h:237
    #7 0x7fd305754dda in nsRefreshDriver::Notify(nsITimer*) /builds/slave/try-lnx64-dbg/build/layout/base/nsRefreshDriver.cpp:410
    #8 0x7fd30769fdff in nsTimerImpl::Fire() /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsTimerImpl.cpp:476
    #9 0x7fd3076a07fd in nsTimerEvent::Run() /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsTimerImpl.cpp:559
    #10 0x7fd30769332d in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsThread.cpp:624
    #11 0x7fd3075e49e3 in NS_ProcessNextEvent_P(nsIThread*, bool) /builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:213
    #12 0x7fd3073f3cae in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/try-lnx64-dbg/build/ipc/glue/MessagePump.cpp:113
    #13 0x7fd307718442 in MessageLoop::RunInternal() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:209
    #14 0x7fd30771832f in MessageLoop::Run() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:176
    #15 0x7fd3071b8c52 in nsBaseAppShell::Run() /builds/slave/try-lnx64-dbg/build/widget/xpwidgets/nsBaseAppShell.cpp:165
    #16 0x7fd306d77b41 in nsAppStartup::Run() /builds/slave/try-lnx64-dbg/build/toolkit/components/startup/nsAppStartup.cpp:256
    #17 0x7fd305187d43 in XREMain::XRE_mainRun() /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3765
    #18 0x7fd3051890e8 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3842
    #19 0x7fd305189b62 in XRE_main /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3918
    #20 0x4079c2 in do_main(int, char**) /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:157
    #21 0x4071ba in main /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:244
    #22 0x7fd30eaf5eff in __libc_start_main /build/buildd/eglibc-2.13/csu/libc-start.c:258
0x7fd2b6d89e80 is located 0 bytes to the right of 513-byte region [0x7fd2b6d89c80,0x7fd2b6d89e81)
allocated by thread T0 here:
    #0 0x4276a2 in malloc ??:0
    #1 0x7fd305579808 in imgFrame::Init(int, int, int, int, gfxASurface::gfxImageFormat, unsigned char) /builds/slave/try-lnx64-dbg/build/image/src/imgFrame.cpp:169
    #2 0x7fd305561b6e in mozilla::image::RasterImage::InternalAddFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:1044
    #3 0x7fd3055623be in mozilla::image::RasterImage::EnsureFrame(unsigned int, int, int, int, int, gfxASurface::gfxImageFormat, unsigned char, unsigned char**, unsigned int*, unsigned int**, unsigned int*) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:1145
    #4 0x7fd3055b4e96 in mozilla::image::nsGIFDecoder2::BeginImageFrame(unsigned short) /builds/slave/try-lnx64-dbg/build/image/decoders/nsGIFDecoder2.cpp:185
    #5 0x7fd3055b8ecd in mozilla::image::nsGIFDecoder2::WriteInternal(char const*, unsigned int) /builds/slave/try-lnx64-dbg/build/image/decoders/nsGIFDecoder2.cpp:922
    #6 0x7fd305563fde in mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:2378
    #7 0x7fd305568374 in mozilla::image::RasterImage::DecodeSomeData(unsigned int) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:2731
    #8 0x7fd305569105 in mozilla::image::RasterImage::DecodeWorker::DecodeSomeOfImage(mozilla::image::RasterImage*, mozilla::image::RasterImage::DecodeWorker::DecodeType) /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:3028
    #9 0x7fd305568a7d in mozilla::image::RasterImage::DecodeWorker::Run() /builds/slave/try-lnx64-dbg/build/image/src/RasterImage.cpp:2957
    #10 0x7fd30769332d in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsThread.cpp:624
    #11 0x7fd3075e49e3 in NS_ProcessNextEvent_P(nsIThread*, bool) /builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:213
    #12 0x7fd3073f3cc9 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/try-lnx64-dbg/build/ipc/glue/MessagePump.cpp:82
    #13 0x7fd307718442 in MessageLoop::RunInternal() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:209
    #14 0x7fd30771832f in MessageLoop::Run() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:176
    #15 0x7fd3071b8c52 in nsBaseAppShell::Run() /builds/slave/try-lnx64-dbg/build/widget/xpwidgets/nsBaseAppShell.cpp:165
    #16 0x7fd306d77b41 in nsAppStartup::Run() /builds/slave/try-lnx64-dbg/build/toolkit/components/startup/nsAppStartup.cpp:256
    #17 0x7fd305187d43 in XREMain::XRE_mainRun() /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3765
    #18 0x7fd3051890e8 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3842
    #19 0x7fd305189b62 in XRE_main /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3918
    #20 0x4079c2 in do_main(int, char**) /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:157
    #21 0x4071ba in main /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:244
==31319== ABORTING
Stats: 1296M malloced (1238M for red zones) by 2598427 calls
Stats: 340M realloced by 249611 calls
Stats: 1243M freed by 2350356 calls
Stats: 1114M really freed by 2040296 calls
Stats: 812M (208022 full pages) mmaped in 203 calls
  mmaps   by size class: 8:442341; 9:90101; 10:28665; 11:16376; 12:5120; 13:5120; 14:2048; 15:1024; 16:1088; 17:160; 18:208; 19:72; 20:140; 21:78; 22:1;
  mallocs by size class: 8:1938088; 9:399426; 10:123808; 11:66502; 12:21500; 13:26796; 14:10485; 15:6225; 16:4102; 17:434; 18:590; 19:92; 20:300; 21:78; 22:1;
  frees   by size class: 8:1733745; 9:371387; 10:116404; 11:61308; 12:20229; 13:25807; 14:9918; 15:6176; 16:3939; 17:423; 18:552; 19:91; 20:299; 21:77; 22:1;
  rfrees  by size class: 8:1508469; 9:319321; 10:101173; 11:53186; 12:17227; 13:22292; 14:8562; 15:5231; 16:3485; 17:382; 18:522; 19:89; 20:279; 21:77; 22:1;
Stats: malloc large: 1495 small slow: 14726
Shadow byte and word:
  0x1ffa56db13d0: 1
  0x1ffa56db13d0: 01 fb fb fb fb fb fb fb
More shadow bytes:
  0x1ffa56db13b0: 00 00 00 00 00 00 00 00
  0x1ffa56db13b8: 00 00 00 00 00 00 00 00
  0x1ffa56db13c0: 00 00 00 00 00 00 00 00
  0x1ffa56db13c8: 00 00 00 00 00 00 00 00
=>0x1ffa56db13d0: 01 fb fb fb fb fb fb fb
  0x1ffa56db13d8: fb fb fb fb fb fb fb fb
  0x1ffa56db13e0: fa fa fa fa fa fa fa fa
  0x1ffa56db13e8: fa fa fa fa fa fa fa fa
  0x1ffa56db13f0: fa fa fa fa fa fa fa fa
Comment 1 Christian Holler (:decoder) 2012-05-24 08:51:31 PDT
Created attachment 626829 [details]
AddressSanitizer Debug Log

I reproduced this with the same build (I had to wait maybe 20 seconds though), but the trace differs a little for me (shows more information where previous memory was freed it seems). I'll attach my trace here in addition :)
Comment 2 Atte Kettunen 2012-06-07 11:41:40 PDT
Created attachment 631060 [details]
ASAN-report as use-after-free

My tests also caught this bug as use-after-free. ASAN report as attachment.
Comment 3 Atte Kettunen 2012-06-11 09:31:53 PDT
Created attachment 631917 [details]
Repro-files for uaf

New simplified test-case. HTML-file now includes two png-files and one gif. This combination is with highest possibility for use-after-free my tests have found so far.
Comment 4 Daniel Veditz [:dveditz] 2012-08-09 14:11:37 PDT
There's been a little source drift, but if this is complaining that dstRect is deleted when we're reading dstRect.width then we're in trouble, because we're about to write into aDst. Maybe it's just the source that's gone and instead we're about to write garbage and hand it back to the content script? not quite as bad, but I'll assume the worst to begin.
Comment 5 David Bolter [:davidb] 2012-08-23 13:06:27 PDT
Joe can you reproduce this one?
Comment 6 Atte Kettunen 2012-08-23 14:14:23 PDT
I can still reproduce it. I used repro-files from comment 3 and ASAN-build of the current Mozilla-central.

==31395== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f5b2d8255c4 at pc 0x7f5b26628a96 bp 0x7fffde940e50 sp 0x7fffde940e48
READ of size 4 at 0x7f5b2d8255c4 thread T0
    #0 0x7f5b26628a96 in mozilla::image::RasterImage::DrawFrameTo(imgFrame*, imgFrame*, nsIntRect&) /home/attekett/firefox/src/image/src/RasterImage.cpp:2059
    #1 0x7f5b2661f62c in mozilla::image::RasterImage::DoComposite(nsIntRect*, imgFrame*, imgFrame*, int) /home/attekett/firefox/src/image/src/RasterImage.cpp:1868
    #2 0x7f5b2661e1a2 in mozilla::image::RasterImage::AdvanceFrame(mozilla::TimeStamp, nsIntRect*) /home/attekett/firefox/src/image/src/RasterImage.cpp:377

==31490== ERROR: AddressSanitizer heap-use-after-free on address 0x7f5d2a104ca4 at pc 0x7f5d4d29ca96 bp 0x7fffdbaaf870 sp 0x7fffdbaaf868
READ of size 4 at 0x7f5d2a104ca4 thread T0
    #0 0x7f5d4d29ca96 in mozilla::image::RasterImage::DrawFrameTo(imgFrame*, imgFrame*, nsIntRect&) /home/attekett/firefox/src/image/src/RasterImage.cpp:2059
    #1 0x7f5d4d29362c in mozilla::image::RasterImage::DoComposite(nsIntRect*, imgFrame*, imgFrame*, int) /home/attekett/firefox/src/image/src/RasterImage.cpp:1868
    #2 0x7f5d4d2921a2 in mozilla::image::RasterImage::AdvanceFrame(mozilla::TimeStamp, nsIntRect*) /home/attekett/firefox/src/image/src/RasterImage.cpp:377
Comment 7 Atte Kettunen 2012-08-31 11:37:00 PDT
Any progress with this issue?
Comment 9 Joe Drew (not getting mail) 2012-09-06 13:31:19 PDT
No direct progress, but it's on my semi-immediate to-do list.
Comment 10 Josh Aas 2012-09-12 03:07:03 PDT
(In reply to Joe Drew (:JOEDREW! \o/) from comment #9)
> No direct progress, but it's on my semi-immediate to-do list.

This is a reproducible sec-critical bug. Can we please make it a higher priority?
Comment 11 Joe Drew (not getting mail) 2012-09-14 08:23:54 PDT
It could not have been made a higher priority, no. I was working on a Firefox 15 regression and (separately) a topcrash.

I'm working on this now, as I promised.
Comment 12 Joe Drew (not getting mail) 2012-09-14 15:46:26 PDT
Created attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

I've had trouble reproducing this with ASAN, I think due to old versions of clang in Ubuntu (I'm currently rebuilding ASAN-Firefox with clang 3.2-r161152), but I did find something with Valgrind that I believe this patch should fix. I'll update as to whether it actually does fix the problem on Monday, but any testing you want to do would be ok too :)
Comment 13 Joe Drew (not getting mail) 2012-09-17 07:30:24 PDT
Comment on attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

Yep, this fixes the bug. We could probably fine a more targeted fix, but this feels incredibly safe and better all around anyways.

Brian, I'm r? you because you've dealt with decoder security fixes before. Feel free to ignore/unset review.
Comment 14 Brian R. Bondy [:bbondy] 2012-09-17 07:57:06 PDT
Comment on attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

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

I'm fine with this, but it does concern me that we'll be fixing whatever the reproducible case is currently and not fixing all cases of whatever the core problem is.
Comment 15 Justin Lebar (not reading bugmail) 2012-09-17 08:17:33 PDT
I'm not at all familiar with this code, but I'm not convinced this is right.
In EndImageFrame, we do 

    mColormap[mGIFStruct.tpixel] = mOldColor;

so clearly we care about the value of mColormap between frames.

Then in RasterImage::EnsureFrame, we may end up setting mColormap to the frame's saved palette data:

    if (paletteData) { // paletteData is &mColormap
      frame->GetPaletteData(paletteData, paletteLength);
    }

so why should we then erase this after EnsureFrame returns?

It may be that I'm just being thick here.  Sorry if that's the case!
Comment 16 Brian R. Bondy [:bbondy] 2012-09-17 08:24:56 PDT
ya seems like the color table memset should be inside the  "if (mGIFStruct.images_decoded)" block if rv is success.
Comment 17 Justin Lebar (not reading bugmail) 2012-09-17 08:33:50 PDT
(In reply to Brian R. Bondy [:bbondy] from comment #16)
> ya seems like the color table memset should be inside the  "if
> (mGIFStruct.images_decoded)" block if rv is success.

That doesn't seem quite right either.  After the call to EnsureFrame where we pass &mColormap, mColormap may have important data inside it (see the second piece of code in comment 15).  It's not clear to me that we should nuke it.
Comment 18 Joe Drew (not getting mail) 2012-09-17 09:56:40 PDT
The way the GIF decoder uses EnsureFrame, though, we never reuse frames. Note that mGIFStruct.images_decoded is unconditionally incremented in EndImageFrame.

The reason mOldColor is restored is because GIFs have two available colour maps: the global one (for the whole image) and a local one (for each individual frame), and the transparent colour need not be the same from frame to frame, so if two frames have different transparent colours and both use the global colourmap, you're going to have a bad time unless you keep the colourmap consistent.
Comment 19 Justin Lebar (not reading bugmail) 2012-09-17 11:06:37 PDT
> The way the GIF decoder uses EnsureFrame, though, we never reuse frames. Note that 
> mGIFStruct.images_decoded is unconditionally incremented in EndImageFrame

Okay, thanks I think I understand that now.  But then why not have imgFrame::Init clear the palette data?  I guess I'm now confused as to why the GIF decoder needs zeroed palette and image-data buffers here but every other image decoder can presumably live with uninitialized buffers.
Comment 20 Joe Drew (not getting mail) 2012-09-17 11:46:57 PDT
Palette is a special case. If we don't initialize image frames, worst case we display uninitialized data - not sec-critical. But in the case of a paletted image, the image's "colour" is in fact an index into the palette array. Therefore, if a malformed image causes us to not write to all of the image's pixels, we can overrun different buffers.
Comment 21 Justin Lebar (not reading bugmail) 2012-09-17 12:22:20 PDT
> Therefore, if a malformed image causes us to not write to all of the image's pixels, we can overrun 
> different buffers.

I assume you mean /palette's pixels/ here?

If the specific case is a paletted image, not a GIF image, shouldn't imgFrame::Init clear the palette data after mallocing it?

Separately, memset(0)'ing the image data is not to fix a security bug, but just to fix a potential bug where we display uninitialized data, right?  Is that issue specific to the GIF decoder for some reason?
Comment 22 Joe Drew (not getting mail) 2012-09-17 12:29:21 PDT
(In reply to Justin Lebar [:jlebar] from comment #21)
> > Therefore, if a malformed image causes us to not write to all of the image's pixels, we can overrun 
> > different buffers.
> 
> I assume you mean /palette's pixels/ here?

No. The image data (addressed as uint8_t*) is an index into the palette.

> If the specific case is a paletted image, not a GIF image, shouldn't
> imgFrame::Init clear the palette data after mallocing it?

Could do. I'm not picky.

> Separately, memset(0)'ing the image data is not to fix a security bug, but
> just to fix a potential bug where we display uninitialized data, right?  Is
> that issue specific to the GIF decoder for some reason?

As above, no, you've got it backwards.
Comment 23 Justin Lebar (not reading bugmail) 2012-09-17 12:37:15 PDT
> As above, no, you've got it backwards.

Oooh, I see.  Now I understand why this feels like a GIF-specific issue.

Thanks.  :)
Comment 24 Justin Lebar (not reading bugmail) 2012-09-17 12:38:16 PDT
Comment on attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

r=me, but please consider adding a comment about how we're not nuking any useful data (because we always increment images_decoded, and therefore RasterImage/imgFrame always creates a new buffer for us).
Comment 27 Joe Drew (not getting mail) 2012-09-17 17:13:34 PDT
(In reply to Justin Lebar [:jlebar] from comment #24)
> r=me, but please consider adding a comment about how we're not nuking any
> useful data (because we always increment images_decoded, and therefore
> RasterImage/imgFrame always creates a new buffer for us).

I'll do so, and also put the zeroing inside the conditional.
Comment 28 Justin Lebar (not reading bugmail) 2012-09-17 17:20:03 PDT
Sorry for dragging this one out; thanks for convincing me.  :)
Comment 29 Alex Keybl [:akeybl] 2012-09-19 17:35:28 PDT
If we think this is low risk enough to uplift into FF16, please land and nominate for aurora/beta approval prior to Tuesday's Beta 5 go to build. If not, we'll untrack for release.
Comment 30 Joe Drew (not getting mail) 2012-09-19 17:39:08 PDT
I don't think we should land this for beta - there's a chance (however minute) to break certain gifs, which I don't want to do.
Comment 31 Alex Keybl [:akeybl] 2012-09-20 15:41:32 PDT
(In reply to Joe Drew (:JOEDREW! \o/) from comment #30)
> I don't think we should land this for beta - there's a chance (however
> minute) to break certain gifs, which I don't want to do.

Sounds good. If anybody disagrees, please re-nominate for tracking.
Comment 32 Joe Drew (not getting mail) 2012-09-21 15:34:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e36ba60ece12
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-09-22 05:48:21 PDT
https://hg.mozilla.org/mozilla-central/rev/e36ba60ece12
Comment 34 Robert Kaiser 2012-10-17 14:00:41 PDT
This is sec-critical and tracking-firefox17+ - are we planning to land this on beta?
Comment 35 Joe Drew (not getting mail) 2012-10-17 14:19:39 PDT
Comment on attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

[Approval Request Comment]
Bug caused by (feature/regressing bug #): stuart, probably
User impact if declined: sec bug
Testing completed (on m-c, etc.): On m-c and aurora
Risk to taking this patch (and alternatives if risky): not terribly risky, but not without risk.
String or UUID changes made by this patch: none
Fix Landed on Version: 18
Comment 36 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-18 10:35:52 PDT
Comment on attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

I'm pre-approving these for branch uplift (and ESR) but I believe that this bug falls into our new process for needing to request sec-approval first to ensure it can land at any time so please set sec-approval?  and fill out the form for that triage.
Comment 37 Joe Drew (not getting mail) 2012-10-18 10:40:38 PDT
Comment on attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

[Security approval request comment]
How easily can the security issue be deduced from the patch?

Not necessarily easily, since we're only zeroing out buffers, but this would probably make a malicious person think in the right direction. Note that this has already landed on mozilla-central.

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 AFAICT.

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

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

Very easy to apply this fix if it's not already applicable.

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

It has the potential to cause regressions in animated GIF drawing, though I have convinced myself and others that it does not in fact cause those regressions.
Comment 38 Daniel Veditz [:dveditz] 2012-10-19 13:49:42 PDT
Comment on attachment 661389 [details] [diff] [review]
zero out buffers when creating new frames for GIFs

sec-approval+, although the flag is really for landing on m-c (which in this bug happened before we started requiring explicit approval)
Comment 39 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:51:18 PDT
(In reply to Daniel Veditz [:dveditz] from comment #38)
> Comment on attachment 661389 [details] [diff] [review]
> zero out buffers when creating new frames for GIFs
> 
> sec-approval+, although the flag is really for landing on m-c (which in this
> bug happened before we started requiring explicit approval)

Ya, I was mis-reading the process for the flag last week, apologies.
Comment 40 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-21 09:51:51 PDT
Joe - please land as soon as you can, ideally before Monday EOD PT so this gets into 17.0 Beta 3
Comment 41 Joe Drew (not getting mail) 2012-10-22 11:58:54 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/eaf3765dbed9
https://hg.mozilla.org/releases/mozilla-esr10/rev/94971f5e22e7

Presumably this is now wontfix for 16; marking thusly.
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-06 14:22:25 PST
RE: in-testsuite?
Is QA verification of this fix required, or will verification be addressed by an in-testsuite testcase?
Comment 43 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-15 13:49:33 PST
Marking this as qa- due to lack of response. Please add verifyme keyword if there's action needed from QA on this bug.

Atte, please feel free to test and verify this is fixed for you. It should now be fixed in Firefox 17b6, 18.0a2, 19.0a1, and 10.0.11esr. If you need help finding the builds, please let me know.

Thanks
Comment 44 Atte Kettunen 2012-11-21 12:43:50 PST
ashughes: Are there ASAN-builds available for those Firefox versions? I'm only building m-c.
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-21 12:50:32 PST
Atte, no. We only have ASan builds (on decoder's people account) for the last 30 days of Aurora and Nightly builds. If you can verify this against mozilla-central then we'll have to accept that as "good enough".
Comment 46 Atte Kettunen 2012-11-21 12:56:59 PST
The repro-files I have attached in here are not causing crash for me on ASAN-build of current m-c.
Comment 47 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-11-21 13:00:27 PST
Thank you Atte. I'm marking this verified fixed.

Note You need to log in before you can comment on or make changes to this bug.