547143 - (CVE-2010-0164) libpr0n imgContainer Bits-Per-Pixel Change Remote Code Execution Vulnerability (ZDI-CAN-693)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: zdi's PoC

ZDI-CAN-693: Mozilla Firefox libpr0n imgContainer Bits-Per-Pixel Change Remote Code Execution Vulnerability

-- ABSTRACT ------------------------------------------------------------

TippingPoint has identified a vulnerability affecting the following 
products:

    Mozilla Firefox 3.5.x

-- VULNERABILITY DETAILS -----------------------------------------------

This vulnerability allows remote attackers to execute arbitrary code on
vulnerable installations of Mozilla Firefox. User interaction is
required to exploit this vulnerability in that the target must visit a
malicious page or open a malicious file.

The specific flaw exists within the libpr0n library which is responsible
for handling image caching and animation and is due to the way the
application handle animations received from the server via the
multipart/x-mixed-replace mime. During a case where the bits-per-pixel
changes, the application will free a pointer and then can be made to
reuse the freed pointer later. This can lead to code execution under the
context of the application.

Mozilla utilizes libpr0n as it's image renderer. libpr0n is responsible
for providing image encoding/decoding along with image caching and
canvas reuse to the application. This library includes support for
multipart-x-mixed-replace which can be utilized for streaming images or
animation from a server over the http protocol.

The vulnerability exists due to the library's ability to reuse a frame
for animations where a new frame has a different bpp than the old frame.
This leads to a pointer that gets freed, which can then be used in order
to further an attacker into control of the process.

The actual dangling pointer is located in the following code in the
'frame' variable. It occurs when the previous frame can not be reused
for the next frame that gets added. The case can be hit when
frame->GetFormat() != aformat.

modules/libpr0n/src/imgContainer.cpp:539
  imgFrame *frame = GetImgFrame(aFrameNum);
  if (!frame)
    return InternalAddFrame(aFrameNum, aX, aY, aWidth, aHeight,
aFormat,
                            /* aPaletteDepth = */ 0, imageData,
imageLength,
                            /* aPaletteData = */ nsnull,
                            /* aPaletteLength = */ nsnull);

  // See if we can re-use the frame that already exists.
  nsIntRect rect = frame->GetRect();
  if (rect.x != aX || rect.y != aY || rect.width != aWidth ||
rect.height != aHeight ||
      frame->GetFormat() != aFormat) {
    delete frame;                // XXX: pointer we want control of
    return InternalAddFrame(aFrameNum, aX, aY, aWidth, aHeight,
aFormat,
                            /* aPaletteDepth = */ 0, imageData,
imageLength,
                            /* aPaletteData = */ nsnull,
                            /* aPaletteLength = */ nsnull);
  }

Upon parsing an image, the decoder for the image will be initialized
with the object responsible for loading. This code will later call
AppendFrame to append an image to the current list.
/* void init (in imgILoad aLoad); */
NS_IMETHODIMP nsJPEGDecoder::Init(imgILoad *aLoad)
{
  mImageLoad = aLoad;
  mObserver = do_QueryInterface(aLoad);

  /* We set up the normal JPEG error routines, then override error_exit.
*/
  mInfo.err = jpeg_std_error(&mErr.pub);
  /*   mInfo.err = jpeg_std_error(&mErr.pub); */
  mErr.pub.error_exit = my_error_exit;
  /* Establish the setjmp return context for my_error_exit to use. */
  if (setjmp(mErr.setjmp_buffer)) {
    /* If we get here, the JPEG code has signaled an error.
     * We need to clean up the JPEG object, close the input file, and
return.
     */
    return NS_ERROR_FAILURE;
  }


AppendFrame is relaly just a wrapper around InternalAddFrame.
modules/libpr0n/src/imgContainer.cpp:479
NS_IMETHODIMP imgContainer::AppendFrame(PRInt32 aX, PRInt32 aY, PRInt32
aWidth,
                                        PRInt32 aHeight,
                                        gfxASurface::gfxImageFormat
aFormat,
                                        PRUint8 **imageData,
                                        PRUint32 *imageLength)
{
  NS_ENSURE_ARG_POINTER(imageData);
  NS_ENSURE_ARG_POINTER(imageLength);

  return InternalAddFrame(mNumFrames, aX, aY, aWidth, aHeight, aFormat,
                          /* aPaletteDepth = */ 0, imageData,
imageLength,
                          /* aPaletteData = */ nsnull,
                          /* aPaletteLength = */ nsnull);
}

InternalAddFrame will check if the frame length is 2, and then call
StartAnimation as there's more than 1 frame to flip through.
modules/libpr0n/src/imgContainer.cpp:421
nsresult imgContainer::InternalAddFrame(PRUint32 framenum,
                                        PRInt32 aX, PRInt32 aY,
                                        PRInt32 aWidth, PRInt32
aHeight,
                                        gfxASurface::gfxImageFormat
aFormat,
                                        PRUint8 aPaletteDepth,
                                        PRUint8 **imageData,
                                        PRUint32 *imageLength,
                                        PRUint32 **paletteData,
                                        PRUint32 *paletteLength)
{
  if (framenum > PRUint32(mNumFrames))
    return NS_ERROR_INVALID_ARG;

  nsAutoPtr<imgFrame> frame(new imgFrame());
  NS_ENSURE_TRUE(frame, NS_ERROR_OUT_OF_MEMORY);

  nsresult rv = frame->Init(aX, aY, aWidth, aHeight, aFormat,
aPaletteDepth);
  NS_ENSURE_SUCCESS(rv, rv);

  if (mFrames.Length() == 0) {
    return InternalAddFrameHelper(framenum, frame.forget(), imageData,
imageLength,
                                  paletteData, paletteLength);
  }
...
  // If this is our second frame (We've just added our second frame
above),
  // count should now be 2.  This must be called after we AppendObject
  // because StartAnimation checks for > 1 frames
  if (mFrames.Length() == 2)
    StartAnimation();   // XXX

  return rv;
}

StartAnimation will then fetch the current image frame, and then start a
timeout for animating.
modules/libpr0n/src/imgContainer.cpp:699
NS_IMETHODIMP imgContainer::StartAnimation()
{
  if (mAnimationMode == kDontAnimMode ||
      (mAnim && (mAnim->timer || mAnim->animating)))
    return NS_OK;

  if (mNumFrames > 1) {
    if (!ensureAnimExists())
      return NS_ERROR_OUT_OF_MEMORY;

    // Default timeout to 100: the timer notify code will do the right
    // thing, so just get that started.
    PRInt32 timeout = 100;
    imgFrame *currentFrame = GetCurrentImgFrame();  // XXX: calls
::GetImgFrame
    if (currentFrame) {
      timeout = currentFrame->GetTimeout();
      if (timeout <= 0) // -1 means display this frame forever
        return NS_OK;
    }

Once reaching GetImgFrame, RestoreDiscardedData will get called, this
will later called ReloadImages.
modules/libpr0n/src/imgContainer.cpp:192
imgFrame *imgContainer::GetImgFrame(PRUint32 framenum)
{
  nsresult rv = RestoreDiscardedData(); // XXX
  NS_ENSURE_SUCCESS(rv, nsnull);

modules/libpr0n/src/imgContainer.cpp:933
NS_IMETHODIMP imgContainer::Notify(nsITimer *timer)
{
  // Note that as long as the image is animated, it will not be
discarded,
  // so this should never happen...
  nsresult rv = RestoreDiscardedData(); // XXX

modules/libpr0n/src/imgContainer.cpp:1616
nsresult
imgContainer::RestoreDiscardedData(void)
{
  // mRestoreDataDone = PR_TRUE means that we want to timeout and then
discard the image frames
  // So, we only need to restore, if mRestoreDataDone is true, and then
only when the frames are discarded...
  if (!mRestoreDataDone)
    return NS_OK;

  // Reset timer, as the frames are accessed
  nsresult rv = ResetDiscardTimer();
  NS_ENSURE_SUCCESS(rv, rv);

  if (!mDiscarded)
    return NS_OK;

  int num_expected_frames = mNumFrames;

  // To prevent that ReloadImages is called multiple times, reset the
flag before reloading
  mDiscarded = PR_FALSE;

  rv = ReloadImages();  // XXX

Upon reaching ReloadImages, the library will ask the image decoder to
initialize another image, which it will then write data to via the
WriteFrom method of the decoder.
modules/libpr0n/src/imgContainer.cpp:1830
  nsresult result = decoder->Init(loader);
  if (NS_FAILED(result)) {
    PR_LOG(gCompressedImageAccountingLog, PR_LOG_WARNING,
           ("CompressedImageAccounting: imgContainer::ReloadImages()
image container %p "
            "failed to initialize the decoder (%s)",
            this,
            mDiscardableMimeType.get()));
    return result;
  }
...
  // |WriteFrom()| may fail if the original data is broken.
  PRUint32 written;
  (void)decoder->WriteFrom(stream, mRestoreData.Length(), &written); //
XXX

modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp:259
/* unsigned long writeFrom (in nsIInputStream inStr, in unsigned long
count); */
NS_IMETHODIMP nsJPEGDecoder::WriteFrom(nsIInputStream *inStr, PRUint32
count, PRUint32 *writeCount)
{
  NS_ENSURE_ARG_POINTER(inStr);
  NS_ENSURE_ARG_POINTER(writeCount);

  /* necko doesn't propagate the errors from ReadDataOut */
  nsresult rv = inStr->ReadSegments(ReadDataOut, this, count,
writeCount);   // XXX

modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp:240
static NS_METHOD ReadDataOut(nsIInputStream* in,
                             void* closure,
                             const char* fromRawSegment,
                             PRUint32 toOffset,
                             PRUint32 count,
                             PRUint32 *writeCount)
{
  nsJPEGDecoder *decoder = static_cast<nsJPEGDecoder*>(closure);
  nsresult rv = decoder->ProcessData(fromRawSegment, count, writeCount);
 //XXX

modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp:276
nsresult nsJPEGDecoder::ProcessData(const char *data, PRUint32 count,
PRUint32 *writeCount)
{
  LOG_SCOPE_WITH_PARAM(gJPEGlog, "nsJPEGDecoder::ProcessData", "count",
count);

Upon parsing all useful data for the jpeg image, the library will then
call EnsureCleanFrame, to allocate a frame at a particular index.
modules/libpr0n/decoders/jpeg/nsJPEGDecoder.cpp:458
    /* verify that the width and height of the image are the same as
     * the container we're about to put things in to.
     * XXX it might not matter maybe we should just resize the image.
     */
    PRInt32 width, height;
    mImage->GetWidth(&width);
    mImage->GetHeight(&height);
    if (width == 0 && height == 0) {
      mImage->Init(mInfo.image_width, mInfo.image_height, mObserver);
    } else if ((width != (PRInt32)mInfo.image_width) || (height !=
(PRInt32)mInfo.image_height)) {
      mState = JPEG_ERROR;
      return NS_ERROR_UNEXPECTED;
    }

    mImage->Init(mInfo.image_width, mInfo.image_height, mObserver);

    mObserver->OnStartContainer(nsnull, mImage);

    // Use EnsureCleanFrame so we don't create a new frame if we're
being
    // reused for e.g. multipart/x-replace
    PRUint32 imagelength;
    if (NS_FAILED(mImage->EnsureCleanFrame(0, 0, 0, mInfo.image_width,
mInfo.image_height,
                                          
gfxASurface::ImageFormatRGB24,
                                           &mImageData, &imagelength)))
{    // XXX

EnsureCleanFrame will then get called. This will allocate a new frame
via InternalAddFrame at the specified Index..Unless the format is
different.
modules/libpr0n/src/imgContainer.cpp:515
/*  [noscript] void ensureCleanFrame(in unsigned long aFramenum, in
PRInt32 aX, in PRInt32 aY, in PRInt32 aWidth, in PRInt32 aHeight, in
gfxImageFormat aFormat, [array, size_is(imageLength)] out PRUint8
imageData, out unsigned long imageLength); */
NS_IMETHODIMP imgContainer::EnsureCleanFrame(PRUint32 aFrameNum, PRInt32
aX, PRInt32 aY,
                                             PRInt32 aWidth, PRInt32
aHeight, 
                                             gfxASurface::gfxImageFormat
aFormat,
                                             PRUint8 **imageData,
PRUint32 *imageLength)
{
  NS_ENSURE_ARG_POINTER(imageData);
  NS_ENSURE_ARG_POINTER(imageLength);
  if (aFrameNum > PRUint32(mNumFrames))
    return NS_ERROR_INVALID_ARG;

  // Adding a frame that doesn't already exist.
  if (aFrameNum == PRUint32(mNumFrames))
    return InternalAddFrame(aFrameNum, aX, aY, aWidth, aHeight, aFormat,

                            /* aPaletteDepth = */ 0, imageData,
imageLength,
                            /* aPaletteData = */ nsnull, 
                            /* aPaletteLength = */ nsnull);

  imgFrame *frame = GetImgFrame(aFrameNum);
  if (!frame)
    return InternalAddFrame(aFrameNum, aX, aY, aWidth, aHeight, aFormat,

                            /* aPaletteDepth = */ 0, imageData,
imageLength,
                            /* aPaletteData = */ nsnull, 
                            /* aPaletteLength = */ nsnull);

  // See if we can re-use the frame that already exists.
  nsIntRect rect = frame->GetRect();
  if (rect.x != aX || rect.y != aY || rect.width != aWidth ||
rect.height != aHeight ||
      frame->GetFormat() != aFormat) {
    delete frame;
    return InternalAddFrame(aFrameNum, aX, aY, aWidth, aHeight, aFormat,

                            /* aPaletteDepth = */ 0, imageData,
imageLength,
                            /* aPaletteData = */ nsnull, 
                            /* aPaletteLength = */ nsnull);
  }

Due to the format being different, the deleted frame will still remain
in the list of containers at which point an attacker will need to figure
out how to get the frame to be utilized in the current execution path.
One example would be upon image container destruction.
modules/libpr0n/src/imgContainer.cpp:90
imgContainer::~imgContainer()
{
  if (mAnim)
    delete mAnim;

  for (unsigned int i = 0; i < mFrames.Length(); ++i)
    delete mFrames[i];

Version(s)  tested: Mozilla Firefox 3.6
Platform(s) tested: Windows XP SP3


-- CREDIT --------------------------------------------------------------

This vulnerability was discovered by:
    * regenrecht

Comment 1

[Daniel Veditz [:dveditz]]

Blocking for the next security updates. Probably needs to block 3.0.19 as well since that will probably be the last release from that line.

Comment 2

[Joe Drew (not getting mail)]

I am 99% sure that there is no bug here. InternalAddFrame calls InternalAddFrameHelper which changes the pointer held in mFrames, so we don't hold on to a deleted pointer. This is further borne out by my inability to reproduce any problems in Valgrind or by running the PoC myself.

However, InternalAddFrame *does* have failure states, which, though unlikely, could conceivably leave us with a dangling pointer. I can also conceive of cases where we could be left with a leak. I'll whip up a patch to fix those cases.

Also, this doesn't affect 3.0, because imagelib code there is very different there.

Comment 3

[Joe Drew (not getting mail)]

Whoops, I had forgotten that this code wasn't in 3.5 *either*.

Comment 4

[Reed Loden [:reed]]

(In reply to comment #0)
> TippingPoint has identified a vulnerability affecting the following 
> products:
> 
>     Mozilla Firefox 3.5.x
...
> Version(s)  tested: Mozilla Firefox 3.6

~o ~o
One of these things is not like the others,
One of these things just doesn't belong,
Can you tell which thing is not like the others
By the time I finish my song?
~o ~o

Comment 5

[Joe Drew (not getting mail)]

Attachment: Add DeleteImgFrame, which deletes and nulls out a frame in mFrames

There are two cases where we could do the wrong thing here. Both of them are purely theoretical, because they can occur only when we call EnsureCleanFrame() with aFrameNum != 0, which we never do because EnsureCleanFrame() is only called from the JPEG decoder.

The first is bad. Before this patch, if we call EnsureCleanFrame() on an animated image, GetImgFrame() will return mAnim->compositingFrame, which we delete but DO NOT null out. We should never delete mAnim->compositingFrame on our own, because it's purely for display, not storage.

The second is a leak. In the above case, since we delete mAnim->compositingFrame and not mFrames[aFrameNum], InternalAddFrameHelper will unconditionally overwrite mFrames[aFrameNum], and since mFrames is just an nsTArray, overwriting does not imply deletion.

This patch fixes both of these issues.

Comment 6

[Joe Drew (not getting mail)]

This patch also fixes the case where InternalAddFrame or InternalAddFrameHelper returns before it sets mFrame[index] - for example, due to new returning null.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 427850 [details] [diff] [review]
Add DeleteImgFrame, which deletes and nulls out a frame in mFrames

the condition is poor

Comment 8

[Joe Drew (not getting mail)]

Attachment: the condition is better

Comment 9

[Daniel Veditz [:dveditz]]

ZDI will get back to us about our inability to see bad stuff from the PoC, might take a week or two they said which I interpret as meaning they have to get in touch with the original reporter.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 428868 [details] [diff] [review]
the condition is better

I talked with Joe a bunch about this in person and it looks ok to me. We should get test for x-mulitpart-replace jpegs and Joe will file a bug about cleaning this code up so we're less likely to get ourselves in trouble in the future.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Does this still need review from bholley or do should it be landed on trunk and put up for branch approval?

Comment 12

[Joe Drew (not getting mail)]

For all my bugs, you can ignore bholley review requests. I r? him so that he knows what's going on for when he returns from school.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, so is this ready for approval?

Comment 14

[Joe Drew (not getting mail)]

I'm working on a multipart/x-mixed-replace JPEG test right now. Once that's ready, I'll check it in to m-c, and then I'll request branch approval.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, tap me on the shoulder when you do. This blocks our code freeze tomorrow.

Comment 16

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/81e60d14dd49 for the tests; http://hg.mozilla.org/mozilla-central/rev/a1eaad798c2a for the fix.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 428868 [details] [diff] [review]
the condition is better

Whoops; 3.0 is unaffected by this.

Comment 18

[Reed Loden [:reed]]

(In reply to comment #16)
> http://hg.mozilla.org/mozilla-central/rev/81e60d14dd49 for the tests;

Why are we committing tests for a security bug that specifically mention important details such as the multipart/x-mixed-replace mime-type? We usually hold off on committing tests for security bugs until we've shipped the fix to users.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 428868 [details] [diff] [review]
the condition is better

a=beltzner for branches

Comment 20

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/dcf4c045d753

Comment 21

[Joe Drew (not getting mail)]

Comment on attachment 428868 [details] [diff] [review]
the condition is better

This doesn't need to get into 1.9.1.

Comment 22

[Aakash Desai [:aakashd]]

verified FIXED on builds (using the PoC attached to this bug):

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2

and

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Changing this to resolved fixed and adding verified1.9.2 keyword since this was only verified for 1.9.2.

Comment 24

[Daniel Veditz [:dveditz]]

security received this from ZDI last week:

   Because it's a bug related to reusing a pointer that has been previously
   freed and is very likely to contain the same data as before, one would
   have to get an allocation to happen within that code path or hope the OS
   unmaps the page between the free and the reuse, so it's kind of sketchy
   to trigger with that poc. A crash in most cases will happen after the
   fact due to it being possibly minimal heap corruption, so to catch it
   one could breakpoint at the location the pointer is expected used and
   check to see if windows has marked any of those pointers as being
   previously freed. The analyst hasn't written a more reliable poc yet,
   however, if you reload the firefox page a few times after each frame has
   been rendered (1 seconds per frame iirc), the crash should reveal itself
   upon destruction of the surface that everything is being rendered on. It
   took the analyst 3 wait/reloads before anything happened. This was also
   done with fullpageheap enabled.

Users don't run with fullpageheap or other freaky gflags settings. That coupled with the lack of valgrind warnings (comment 2) seems to lower the actual user-facing risk considerably.