Last Comment Bug 441360 - (CVE-2008-2934) Crash [@ CoreFoundation@0x745a4 ] opening GIF file
(CVE-2008-2934)
: Crash [@ CoreFoundation@0x745a4 ] opening GIF file
Status: VERIFIED FIXED
[sg:critical?]
: crash, regression, verified1.9.0.1
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 1.9.0 Branch
: PowerPC Mac OS X
: -- critical (vote)
: mozilla1.9
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://crash-stats.mozilla.com/report...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-23 11:32 PDT by Brandon Sterne (:bsterne)
Modified: 2011-06-13 10:01 PDT (History)
12 users (show)
mbeltzner: blocking1.9.0.1+
vladimir: wanted1.9.0.x+
dveditz: wanted1.8.1.x-
asac: blocking1.8.0.next-
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Crashes Mac trunk. May need to refresh several times. (2.95 KB, image/gif)
2008-06-23 11:32 PDT, Brandon Sterne (:bsterne)
no flags Details
Backtrace from GIF crash (8.38 KB, text/plain)
2008-06-23 13:07 PDT, Brandon Sterne (:bsterne)
no flags Details
crashtest.diff (4.57 KB, patch)
2008-06-23 18:25 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
Patch rev. 1 (787 bytes, patch)
2008-06-23 18:44 PDT, Mats Palmgren (:mats)
vladimir: review+
vladimir: superreview+
mbeltzner: approval1.9.0.1+
Details | Diff | Review

Description Brandon Sterne (:bsterne) 2008-06-23 11:32:00 PDT
Created attachment 326343 [details]
Crashes Mac trunk.  May need to refresh several times.

Drew Yao from Apple Product Security reported this issue to security@mo.  The attached testcase crashes trunk on Mac (after ~2-5 refreshes), but not Windows or Linux, afaict.  The link to the Talkback report is provided above, and I'm doing a DEBUG build to hopefully provide a persistent call stack copy (should the Talkback report get deleted).

From Drew's email:

Free of uninitialized pointer in gfxQuartzSurface::~gfxQuartzSurface()
This probably only affects Mac OS X.  It does not seem to affect Firefox 2, only Firefox 3.

gfxQuartzSurface.cpp

gfxQuartzSurface::gfxQuartzSurface(const gfxSize& size, gfxImageFormat format,
                                   PRBool aForPrinting)
    : mSize(size), mForPrinting(aForPrinting)
{
    unsigned int width = (unsigned int) floor(size.width);
    unsigned int height = (unsigned int) floor(size.height);

    if (!CheckSurfaceSize(gfxIntSize(width, height)))
        return;    <-- the gif triggers a failure, causing a return before mCGContext is initialized

    cairo_surface_t *surf = cairo_quartz_surface_create
        ((cairo_format_t) format, width, height);

    mCGContext = cairo_quartz_surface_get_cg_context (surf);

    CGContextRetain(mCGContext);

    Init(surf);
}

...

gfxQuartzSurface::~gfxQuartzSurface()
{
    CGContextRelease(mCGContext); //<-- an uninitialized pointer can be released, without ever having been retained.
}

To reproduce:
In Firefox 3 RC1 on Mac OS X 10.5.3 Intel,
open the attached gif file and refresh a few times. It should crash.
If you set the environment variable MallocScribble=1 first, it will always crash in CFRelease accessing 0xaaaaaaaa, indicating that it's trying to free uninitialized heap memory.  man malloc to see the behavior of MallocScribble.  It also does what MallocPreScribble claims to do.

Process:         firefox-bin [55007]
Path:            /Volumes/data_apps/obj-i386-apple-darwin9.3.0/dist/MinefieldDebug.app/Contents/MacOS/firefox-bin
Identifier:      org.mozilla.firefox
Version:         3.0pre (3.0pre)
Code Type:       X86 (Native)
Parent Process:  launchd [1]

Date/Time:       2008-06-05 16:19:58.990 -0700
OS Version:      Mac OS X 10.5.3 (9D34)
Report Version:  6

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000aaaaaaaf
Crashed Thread:  0

Thread 0 Crashed:
0   com.apple.CoreFoundation          0x955f15a4 CFRelease + 36
1   libthebes.dylib                   0x12132afb gfxQuartzSurface::~gfxQuartzSurface() + 41
2   libthebes.dylib                   0x1211632e gfxASurface::Release() + 182 (gfxASurface.cpp:102)
3   libgkgfxthebes.dylib              0x120db328 nsRefPtr<gfxASurface>::~nsRefPtr() + 28
4   libgkgfxthebes.dylib              0x120de88a nsThebesImage::Draw(nsIRenderingContext&, gfxRect const&, gfxRect const&, gfxRect const&) + 2318 (nsThebesImage.cpp:515)
5   libgklayout.dylib                 0x122eb9ff nsLayoutUtils::DrawImage(nsIRenderingContext*, imgIContainer*, nsRect const&, nsRect const&, nsRect const*) + 2569 (nsLayoutUtils.cpp:2618)
6   libgklayout.dylib                 0x1237eb68 nsImageFrame::PaintImage(nsIRenderingContext&, nsPoint, nsRect const&, imgIContainer*) + 326 (nsImageFrame.cpp:1185)
7   libgklayout.dylib                 0x1237ecad nsDisplayImage::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) + 95 (nsImageFrame.cpp:1167)
8   libgklayout.dylib                 0x122c0b91 nsDisplayList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) const + 61 (nsDisplayList.cpp:295)
9   libgklayout.dylib                 0x122c1dbb nsDisplayWrapList::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) + 41 (nsDisplayList.cpp:694)
10  libgklayout.dylib                 0x122c2a50 nsDisplayClip::Paint(nsDisplayListBuilder*, nsIRenderingContext*, nsRect const&) + 128 (nsDisplayList.cpp:888)
...

By filling the heap, an attacker may be able to control the pointer that gets freed, potentially leading to memory corruption and/or arbitrary code execution.
Comment 1 Brandon Sterne (:bsterne) 2008-06-23 13:07:40 PDT
Created attachment 326360 [details]
Backtrace from GIF crash
Comment 2 Mats Palmgren (:mats) 2008-06-23 17:16:28 PDT
This is a simple fix, patch coming up...
Comment 3 Mats Palmgren (:mats) 2008-06-23 18:25:16 PDT
Created attachment 326419 [details] [diff] [review]
crashtest.diff
Comment 4 Mats Palmgren (:mats) 2008-06-23 18:44:52 PDT
Created attachment 326420 [details] [diff] [review]
Patch rev. 1

We need to initialize mCGContext to NULL since it's used in the
destructor.  It doesn't really matter in consumers since if we
leave early here mSurfaceValid is false.

I've checked the other Surface classes for similar problems and
they look fine.  (I did find an (edge case) leak in gfxImageSurface
but I'll file that separately.)
Comment 6 Mats Palmgren (:mats) 2008-06-23 19:29:17 PDT
> CFRelease of NULL causes a crash.

Thanks for the notice, but we're using CGContextRelease/Retain here
which handles NULL.
http://developer.apple.com/documentation/GraphicsImaging/Reference/CGContext/Reference/reference.html#//apple_ref/c/func/CGContextRelease
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2008-06-30 14:59:08 PDT
Comment on attachment 326420 [details] [diff] [review]
Patch rev. 1

a=beltzner
Comment 8 Daniel Holbert [:dholbert] 2008-06-30 19:38:37 PDT
Mats, are you going to be able to check this in before the freeze tonight, or should we make this checkin-needed?

CC'ing reed, as he's scheduled to check in a number of other fixes, and he's willing to land this as well.
Comment 9 Reed Loden [:reed] (use needinfo?) 2008-06-30 23:13:22 PDT
Checking in gfx/thebes/src/gfxQuartzSurface.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxQuartzSurface.cpp,v  <--  gfxQuartzSurface.cpp
new revision: 1.17; previous revision: 1.16
done
Comment 10 Marcia Knous [:marcia - use ni] 2008-07-01 10:21:27 PDT
verified fixed on the 1.9.0.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.1pre) Gecko/2008070104 GranParadiso/3.0.1pre. I verified by loading the testcase and was not able to crash. The testcase still crashes on the trunk. Adding verified keyword.
Comment 11 Mats Palmgren (:mats) 2008-07-07 19:17:39 PDT
http://hg.mozilla.org/mozilla-central/index.cgi/rev/e84382e0787e

Will check in the crash test when the bug is public.

-> FIXED
Comment 12 Josh Bressers 2008-07-09 11:12:56 PDT
Please use CVE-2008-2934 for this issue.
Comment 13 Mats Palmgren (:mats) 2008-08-07 11:37:40 PDT
Can we open this bug now?
Comment 14 Mats Palmgren (:mats) 2008-09-16 03:18:49 PDT
Landed the crash test on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/d940395fc107
http://hg.mozilla.org/mozilla-central/rev/ebd45fc4e0eb
http://hg.mozilla.org/mozilla-central/rev/34a7b61d253f

and CVS trunk:
mozilla/gfx/thebes/crashtests/441360.html 	1.1
mozilla/gfx/thebes/crashtests/441360_data.gif 	1.1
mozilla/gfx/thebes/crashtests/crashtests.list 	1.36 
mozilla/gfx/thebes/crashtests/crashtests.list 	1.37 

The test unexpectedly caused an XError on Linux so I disabled
it for that platform for now (filed bug 455463).
Comment 15 Marcia Knous [:marcia - use ni] 2008-09-16 11:17:45 PDT
verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080916020511 Minefield/3.1b1pre.

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