PatchGap
Firefox · ImageLib

CVE-2025-0242

Memory Corruption in ImageLib
High CVSS 6.5 firefox Memory Corruption 🔧 Commit mapped

Overview

High
Severity
6.5
CVSS
No
Exploited ITW
Fixed
Fix Status
Impacthigh
DescriptionMemory safety bugs present in Firefox 133, Thunderbird 133, Firefox ESR 115.18, Firefox ESR 128.5, Thunderbird 115.18, and Thunderbird 128.5. Some of these bugs showed evidence of memory corruption and we presume that with enough effort some of these could have been exploited to run arbitrary code.
ComponentImageLib
Bug ClassMemory Corruption
Tracker1874523
Fix commite1368b4e31dd (firefox) +90/-80
CWECWE-787 (Out-of-bounds write)
CVSS vectorCVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:L/I:L/A:N
CISA KEVNot listed
CreditedAndrew McCreight, Tooru Fujisawa, and the Mozilla Fuzzing Team
Disclosed2025-01-07

Background

`imgFrame`
The ImageLib object that owns the decoded pixel buffer for a single animation frame and exposes it through helpers like GetImageData and RawAccessRef.
Frame recycling
An optimization in AnimationFrameRecyclingQueue where the buffer of an already-displayed frame is reused for a newly decoded frame instead of allocating a fresh surface.
`RawAccessRef`
A method that returns a mapped, raw-pointer view of a frame’s DataSourceSurface, which can fail (return null) when the surface cannot be mapped, e.g. because it was finalized read-only.
`mImageDataLength`
The Decoder member recording the byte length of the current frame’s pixel buffer, used to bound writes made by format decoders (GIF, APNG, etc.).

Root Cause Analysis

During animated-image decoding, Decoder::AllocateFrame gathered the raw destination buffer via imgFrame::GetImageData(&mImageData, &mImageDataLength) and, when recycling, obtained a writable view through imgFrame::RawAccessRef() without checking that the mapping actually succeeded. When a recycled frame’s surface had been finalized (as happens for the first frames produced during an animation), RawAccessRef could return a null or read-only mapping while mImageDataLength was left stale from a previously allocated frame, so decoders continued to write into a buffer whose real size no longer matched the recorded length. This mismatch between the assumed and actual buffer size allowed out-of-bounds writes into the frame’s memory.

The fix threads the true buffer length out of imgFrame::InitForDecoder and imgFrame::InitForDecoderRecycle via a new aImageDataLength out-parameter, explicitly maps recycled surfaces READ_WRITE, resets mImageData/mImageDataLength to null/zero when there is no current frame, and guards the recycle-rect computation on the RawAccessRef result being non-null. The invariant restored is that mImageDataLength always describes the buffer mImageData actually points at.

Key insight
The core mistake was treating mImageDataLength as valid after obtaining a recycled frame buffer whose mapping could silently fail, letting decoders write past the real buffer; the fix always derives the length from the frame that is actually initialized and refuses to proceed when the writable mapping is absent.

Attack Path

  1. Serve a crafted animation An attacker hosts a multi-frame animated image (e.g. GIF or APNG) whose frame sequence triggers the recycling queue’s loop-boundary and first-frame path.
  2. Force surface recycling The animation replays such that AnimationFrameRecyclingQueue::RecycleFrame reuses an early, already-finalized frame whose surface can no longer be mapped writable.
  3. Stale length survives Decoder::AllocateFrame proceeds with a mImageDataLength inherited from a differently sized frame while RawAccessRef() fails to yield a matching writable buffer.
  4. Out-of-bounds write The format decoder writes decoded pixel rows using the stale length bound, overflowing the actual (smaller or unmapped) buffer and corrupting adjacent heap memory.
  5. Escalate to code execution The attacker shapes heap layout so the overflow overwrites exploitable data, potentially leading to arbitrary code execution in the content process.

Impact Assessment

An attacker who can get a victim to load a malicious animated image gains a controlled heap out-of-bounds write in the content process where image decoding runs. Exploitation requires only that the target render the crafted image, with no user interaction beyond viewing a page, and with enough control over heap grooming the memory corruption is presumed exploitable for arbitrary code execution within the sandboxed content process.

Changed Functions

FunctionChangeNotes
if
image/AnimationFrameBuffer.cpp
 modified
for
image/AnimationFrameBuffer.cpp
 modified
if
image/Decoder.cpp
 modified
if
image/imgFrame.cpp
 modified
RawAccessFrameRef
image/imgFrame.h
 modified
if
image/imgFrame.h
 modified

Files Changed

  • image/AnimationFrameBuffer.cpp
  • image/Decoder.cpp
  • image/imgFrame.cpp
  • image/imgFrame.h
  • image/test/gtest/TestAnimationFrameBuffer.cpp

Audit Directions

  • Unchecked raw-buffer mappings
    Look for other callers of RawAccessRef or surface Map/GetImageData that use the resulting pointer or length without verifying the mapping succeeded.
  • Stale length after reuse
    Audit buffer-recycling paths where a size or length member is set once but not refreshed when the underlying buffer is swapped, finalized, or reallocated.
  • Out-parameter length threading
    Review decoder init/allocation APIs that hand back a buffer separately from its length, to confirm the two are always derived from the same object in the same call.

Patch

view commit
diff --git a/image/AnimationFrameBuffer.cpp b/image/AnimationFrameBuffer.cpp
index 6ef855bbd0e..630f59041fc 100644
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -432,32 +432,38 @@ RawAccessFrameRef AnimationFrameRecyclingQueue::RecycleFrame(
 
   RawAccessFrameRef recycledFrame;
   if (mRecycle.front().mFrame) {
-    recycledFrame = mRecycle.front().mFrame->RawAccessRef();
-    MOZ_ASSERT(recycledFrame);
+    recycledFrame = mRecycle.front().mFrame->RawAccessRef(
+        gfx::DataSourceSurface::READ_WRITE);
     mRecycle.pop_front();
 
-    if (mForceUseFirstFrameRefreshArea) {
-      // We are still crossing the loop boundary and cannot rely upon the dirty
-      // rects of entries in mDisplay to be representative. E.g. The first frame
-      // is probably has a full frame dirty rect.
-      aRecycleRect = mFirstFrameRefreshArea;
-    } else {
-      // Calculate the recycle rect for the recycled frame. This is the
-      // cumulative dirty rect of all of the frames ahead of us to be displayed,
-      // and to be used for recycling. Or in other words, the dirty rect between
-      // the recycled frame and the decoded frame which reuses the buffer.
-      //
-      // We know at this point that mRecycle contains either frames from the end
-      // of the animation with the first frame refresh area as the dirty rect
-      // (plus the first frame likewise) and frames with their actual dirty rect
-      // from the start. mDisplay should also only contain frames from the start
-      // of the animation onwards.
-      aRecycleRect.SetRect(0, 0, 0, 0);
-      for (const RefPtr<imgFrame>& frame : mDisplay) {
-        aRecycleRect = aRecycleRect.Union(frame->GetDirtyRect());
-      }
-      for (const RecycleEntry& entry : mRecycle) {
-        aRecycleRect = aRecycleRect.Union(entry.mDirtyRect);
+    // If we couldn't map in the surface, it is probably because the frame was
+    // finalized and we did not expect to need to write into it again. This
+    // happens for the first frames produced during an animation.
+    if (recycledFrame) {
+      if (mForceUseFirstFrameRefreshArea) {
+        // We are still crossing the loop boundary and cannot rely upon the
+        // dirty rects of entries in mDisplay to be representative. E.g. The
+        // first frame is probably has a full frame dirty rect.
+        aRecycleRect = mFirstFrameRefreshArea;
+      } else {
+        // Calculate the recycle rect for the recycled frame. This is the
+        // cumulative dirty rect of all of the frames ahead of us to be
+        // displayed, and to be used for recycling. Or in other words, the dirty
+        // rect between the recycled frame and the decoded frame which reuses
+        // the buffer.
+        //
+        // We know at this point that mRecycle contains either frames from the
+        // end of the animation with the first frame refresh area as the dirty
+        // rect (plus the first frame likewise) and frames with their actual
+        // dirty rect from the start. mDisplay should also only contain frames
+        // from the start of the animation onwards.
+        aRecycleRect.SetRect(0, 0, 0, 0);
+        for (const RefPtr<imgFrame>& frame : mDisplay) {
+          aRecycleRect = aRecycleRect.Union(frame->GetDirtyRect());
+        }
+        for (const RecycleEntry& entry : mRecycle) {
+          aRecycleRect = aRecycleRect.Union(entry.mDirtyRect);
+        }
       }
     }
   } else {
diff --git a/image/Decoder.cpp b/image/Decoder.cpp
index 44b1547e136..abb58eab521 100644
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -316,8 +316,7 @@ nsresult Decoder::AllocateFrame(const gfx::IntSize& aOutputSize,
   if (mCurrentFrame) {
     mHasFrameToTake = true;
 
-    // Gather the raw pointers the decoders will use.
-    mCurrentFrame->GetImageData(&mImageData, &mImageDataLength);
+    mImageData = mCurrentFrame.Data();
 
     // We should now be on |aFrameNum|. (Note that we're comparing the frame
     // number, which is zero-based, with the frame count, which is one-based.)
@@ -329,6 +328,9 @@ nsresult Decoder::AllocateFrame(const gfx::IntSize& aOutputSize,
     // Update our state to reflect the new frame.
     MOZ_ASSERT(!mInFrame, "Starting new frame but not done with old one!");
     mInFrame = true;
+  } else {
+    mImageData = nullptr;
+    mImageDataLength = 0;
   }
 
   return mCurrentFrame ? NS_OK : NS_ERROR_FAILURE;
@@ -389,7 +391,8 @@ RawAccessFrameRef Decoder::AllocateFrameInternal(
       // animation parameters elsewhere. For now we just drop it.
       bool blocked = ref.get() == mRestoreFrame.get();
       if (!blocked) {
-        blocked = NS_FAILED(ref->InitForDecoderRecycle(aAnimParams.ref()));
+        blocked = NS_FAILED(
+            ref->InitForDecoderRecycle(aAnimParams.ref(), &mImageDataLength));
       }
 
       if (blocked) {
@@ -408,12 +411,13 @@ RawAccessFrameRef Decoder::AllocateFrameInternal(
     bool nonPremult = bool(mSurfaceFlags & SurfaceFlags::NO_PREMULTIPLY_ALPHA);
     auto frame = MakeNotNull<RefPtr<imgFrame>>();
     if (NS_FAILED(frame->InitForDecoder(aOutputSize, aFormat, nonPremult,
-                                        aAnimParams, bool(mFrameRecycler)))) {
+                                        aAnimParams, bool(mFrameRecycler),
+                                        &mImageDataLength))) {
       NS_WARNING("imgFrame::Init should succeed");
       return RawAccessFrameRef();
     }
 
-    ref = frame->RawAccessRef();
+    ref = frame->RawAccessRef(gfx::DataSourceSurface::READ_WRITE);
     if (!ref) {
       frame->Abort();
       return RawAccessFrameRef();
diff --git a/image/imgFrame.cpp b/image/imgFrame.cpp
index fc21fb19659..95f23479df9 100644
--- a/image/imgFrame.cpp
+++ b/image/imgFrame.cpp
@@ -145,7 +145,8 @@ imgFrame::~imgFrame() {
 nsresult imgFrame::InitForDecoder(const nsIntSize& aImageSize,
                                   SurfaceFormat aFormat, bool aNonPremult,
                                   const Maybe<AnimationParams>& aAnimParams,
-                                  bool aShouldRecycle) {
+                                  bool aShouldRecycle,
+                                  uint32_t* aImageDataLength) {
   // Assert for properties that should be verified by decoders,
   // warn for properties related to bad content.
   if (!SurfaceCache::IsLegalSize(aImageSize)) {
@@ -217,10 +218,15 @@ nsresult imgFrame::InitForDecoder(const nsIntSize& aImageSize,
     }
   }
 
+  if (aImageDataLength) {
+    *aImageDataLength = GetImageDataLength();
+  }
+
   return NS_OK;
 }
 
-nsresult imgFrame::InitForDecoderRecycle(const AnimationParams& aAnimParams) {
+nsresult imgFrame::InitForDecoderRecycle(const AnimationParams& aAnimParams,
+                                         uint32_t* aImageDataLength) {
   // We want to recycle this frame, but there is no guarantee that consumers are
   // done with it in a timely manner. Let's ensure they are done with it first.
   MonitorAutoLock lock(mMonitor);
@@ -287,6 +293,10 @@ nsresult imgFrame::InitForDecoderRecycle(const AnimationParams& aAnimParams) {
   mDisposalMethod = aAnimParams.mDisposalMethod;
   mDirtyRect = GetRect();
 
+  if (aImageDataLength) {
+    *aImageDataLength = GetImageDataLength();
+  }
+
   return NS_OK;
 }
 
@@ -391,7 +401,10 @@ nsresult imgFrame::InitWithDrawable(gfxDrawable* aDrawable,
 
 DrawableFrameRef imgFrame::DrawableRef() { return DrawableFrameRef(this); }
 
-RawAccessFrameRef imgFrame::RawAccessRef() { return RawAccessFrameRef(this); }
+RawAccessFrameRef imgFrame::RawAccessRef(
+    gfx::DataSourceSurface::MapType aMapType) {
+  return RawAccessFrameRef(this, aMapType);
+}
 
 imgFrame::SurfaceWithFormat imgFrame::SurfaceForDrawing(
     bool aDoPartialDecode, bool aDoTile, ImageRegion& aRegion,
@@ -586,36 +599,6 @@ uint32_t imgFrame::GetImageDataLength() const {
   return GetImageBytesPerRow() * mImageSize.height;
 }
 
-void imgFrame::GetImageData(uint8_t** aData, uint32_t* aLength) const {
-  MonitorAutoLock lock(mMonitor);
-  GetImageDataInternal(aData, aLength);
-}
-
-void imgFrame::GetImageDataInternal(uint8_t** aData, uint32_t* aLength) const {
-  mMonitor.AssertCurrentThreadOwns();
-  MOZ_ASSERT(mRawSurface);
-
-  if (mRawSurface) {
-    // TODO: This is okay for now because we only realloc shared surfaces on
-    // the main thread after decoding has finished, but if animations want to
-    // read frame data off the main thread, we will need to reconsider this.
-    *aData = mRawSurface->GetData();
-    MOZ_ASSERT(*aData,
-               "mRawSurface is non-null, but GetData is null in GetImageData");
-  } else {
-    *aData = nullptr;
-  }
-
-  *aLength = GetImageDataLength();
-}
-
-uint8_t* imgFrame::GetImageData() const {
-  uint8_t* data;
-  uint32_t length;
-  GetImageData(&data, &length);
-  return data;
-}
-
 void imgFrame::FinalizeSurface() {
   MonitorAutoLock lock(mMonitor);
   FinalizeSurfaceInternal();
diff --git a/image/imgFrame.h b/image/imgFrame.h
index 95f219f93e7..8ce0d4bf2c1 100644
--- a/image/imgFrame.h
+++ b/image/imgFrame.h
@@ -55,7 +55,8 @@ class imgFrame {
   nsresult InitForDecoder(const nsIntSize& aImageSize, SurfaceFormat aFormat,
                           bool aNonPremult,
                           const Maybe<AnimationParams>& aAnimParams,
-                          bool aShouldRecycle);
+                          bool aShouldRecycle,
+                          uint32_t* aImageDataLength = nullptr);
 
   /**
    * Reinitialize this imgFrame with the new parameters, but otherwise retain
@@ -65,7 +66,8 @@ class imgFrame {
    * given an IDecoderFrameRecycler object which may yield a recycled imgFrame
    * that was discarded to save memory.
    */
-  nsresult InitForDecoderRecycle(const AnimationParams& aAnimParams);
+  nsresult InitForDecoderRecycle(const AnimationParams& aAnimParams,
+                                 uint32_t* aImageDataLength = nullptr);
 
   /**
    * Initialize this imgFrame with a new surface and draw the provided
@@ -90,7 +92,8 @@ class imgFrame {
   /**
    * Create a RawAccessFrameRef for the frame.
    */
-  RawAccessFrameRef RawAccessRef();
+  RawAccessFrameRef RawAccessRef(
+      gfx::DataSourceSurface::MapType aMapType = gfx::DataSourceSurface::READ);
 
   bool Draw(gfxContext* aContext, const ImageRegion& aRegion,
             SamplingFilter aSamplingFilter, uint32_t aImageFlags,
@@ -160,8 +163,6 @@ class imgFrame {
   BlendMethod GetBlendMethod() const { return mBlendMethod; }
   DisposalMethod GetDisposalMethod() const { return mDisposalMethod; }
   bool FormatHasAlpha() const { return mFormat == SurfaceFormat::OS_RGBA; }
-  void GetImageData(uint8_t** aData, uint32_t* length) const;
-  uint8_t* GetImageData() const;
 
   const IntRect& GetDirtyRect() const { return mDirtyRect; }
   void SetDirtyRect(const IntRect& aDirtyRect) { mDirtyRect = aDirtyRect; }
@@ -186,7 +187,6 @@ class imgFrame {
 
   bool AreAllPixelsWritten() const MOZ_REQUIRES(mMonitor);
   nsresult ImageUpdatedInternal(const nsIntRect& aUpdateRect);
-  void GetImageDataInternal(uint8_t** aData, uint32_t* length) const;
   uint32_t GetImageBytesPerRow() const;
   uint32_t GetImageDataLength() const;
   void FinalizeSurfaceInternal();
@@ -356,13 +356,25 @@ class DrawableFrameRef final {
  */
 class RawAccessFrameRef final {
  public:
-  RawAccessFrameRef() : mData(nullptr) {}
+  RawAccessFrameRef() = default;
 
-  explicit RawAccessFrameRef(imgFrame* aFrame)
-      : mFrame(aFrame), mData(nullptr) {
+  explicit RawAccessFrameRef(imgFrame* aFrame,
+                             gfx::DataSourceSurface::MapType aMapType)
+      : mFrame(aFrame) {
     MOZ_ASSERT(mFrame, "Need a frame");
 
-    mData = mFrame->GetImageData();
+    // Note that we do not use ScopedMap here because it holds a strong
+    // reference to the underlying surface. This affects the reuse logic for
+    // recycling in imgFrame::InitForDecoderRecycle.
+    {
+      MonitorAutoLock lock(mFrame->mMonitor);
+      gfx::DataSourceSurface::MappedSurface map;
+      if (mFrame->mRawSurface && mFrame->mRawSurface->Map(aMapType, &map)) {
+        MOZ_ASSERT(map.mData);
+        mData = map.mData;
+      }
+    }
+
     if (!mData) {
       mFrame = nullptr;
     }
@@ -373,11 +385,15 @@ class RawAccessFrameRef final {
     aOther.mData = nullptr;
   }
 
-  ~RawAccessFrameRef() = default;
+  ~RawAccessFrameRef() { reset(); }
 
   RawAccessFrameRef& operator=(RawAccessFrameRef&& aOther) {
     MOZ_ASSERT(this != &aOther, "Self-moves are prohibited");
 
+    if (mFrame) {
+      MonitorAutoLock lock(mFrame->mMonitor);
+      mFrame->mRawSurface->Unmap();
+    }
     mFrame = std::move(aOther.mFrame);
     mData = aOther.mData;
     aOther.mData = nullptr;
@@ -401,6 +417,10 @@ class RawAccessFrameRef final {
   const imgFrame* get() const { return mFrame; }
 
   void reset() {
+    if (mFrame) {
+      MonitorAutoLock lock(mFrame->mMonitor);
+      mFrame->mRawSurface->Unmap();
+    }
     mFrame = nullptr;
     mData = nullptr;
   }
@@ -412,7 +432,7 @@ class RawAccessFrameRef final {
   RawAccessFrameRef& operator=(const RawAccessFrameRef& aOther) = delete;
 
   RefPtr<imgFrame> mFrame;
-  uint8_t* mData;
+  uint8_t* mData = nullptr;
 };
 
 }  // namespace image
diff --git a/image/test/gtest/TestAnimationFrameBuffer.cpp b/image/test/gtest/TestAnimationFrameBuffer.cpp
index 78186e50665..69e020083a7 100644
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -759,8 +759,7 @@ TEST_F(ImageAnimationFrameBuffer, RecyclingResetBeforeComplete) {
   while (!buffer.Recycle().empty()) {
     gfx::IntRect recycleRect;
     RawAccessFrameRef frameRef = buffer.RecycleFrame(recycleRect);
-    EXPECT_TRUE(frameRef);
-    EXPECT_FALSE(ReinitForRecycle(frameRef));
+    EXPECT_FALSE(frameRef);
   }
 
   // Reinsert the first two frames as recyclable and reset again.
@@ -829,8 +828,7 @@ TEST_F(ImageAnimationFrameBuffer, RecyclingRect) {
   gfx::IntRect recycleRect;
   EXPECT_FALSE(buffer.Recycle().empty());
   RawAccessFrameRef frameRef = buffer.RecycleFrame(recycleRect);
-  EXPECT_TRUE(frameRef);
-  EXPECT_FALSE(ReinitForRecycle(frameRef));
+  EXPECT_FALSE(frameRef);
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Insert a recyclable partial frame. Its dirty rect shouldn't matter since
@@ -842,8 +840,7 @@ TEST_F(ImageAnimationFrameBuffer, RecyclingRect) {
   VerifyAdvance(buffer, 5, true);
   EXPECT_FALSE(buffer.Recycle().empty());
   frameRef = buffer.RecycleFrame(recycleRect);
-  EXPECT_TRUE(frameRef);
-  EXPECT_FALSE(ReinitForRecycle(frameRef));
+  EXPECT_FALSE(frameRef);
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Insert a recyclable partial frame. Its dirty rect should match the recycle
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/image/test/gtest/TestAnimationFrameBuffer.cpp b/image/test/gtest/TestAnimationFrameBuffer.cpp
index 78186e50665..69e020083a7 100644
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -759,8 +759,7 @@ TEST_F(ImageAnimationFrameBuffer, RecyclingResetBeforeComplete) {
   while (!buffer.Recycle().empty()) {
     gfx::IntRect recycleRect;
     RawAccessFrameRef frameRef = buffer.RecycleFrame(recycleRect);
-    EXPECT_TRUE(frameRef);
-    EXPECT_FALSE(ReinitForRecycle(frameRef));
+    EXPECT_FALSE(frameRef);
   }
 
   // Reinsert the first two frames as recyclable and reset again.
@@ -829,8 +828,7 @@ TEST_F(ImageAnimationFrameBuffer, RecyclingRect) {
   gfx::IntRect recycleRect;
   EXPECT_FALSE(buffer.Recycle().empty());
   RawAccessFrameRef frameRef = buffer.RecycleFrame(recycleRect);
-  EXPECT_TRUE(frameRef);
-  EXPECT_FALSE(ReinitForRecycle(frameRef));
+  EXPECT_FALSE(frameRef);
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Insert a recyclable partial frame. Its dirty rect shouldn't matter since
@@ -842,8 +840,7 @@ TEST_F(ImageAnimationFrameBuffer, RecyclingRect) {
   VerifyAdvance(buffer, 5, true);
   EXPECT_FALSE(buffer.Recycle().empty());
   frameRef = buffer.RecycleFrame(recycleRect);
-  EXPECT_TRUE(frameRef);
-  EXPECT_FALSE(ReinitForRecycle(frameRef));
+  EXPECT_FALSE(frameRef);
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Insert a recyclable partial frame. Its dirty rect should match the recycle
Loading diff…
On This Page