2011946 - OOB read in BufferMediaResource::ReadFromCache via unsigned underflow when aOffset > mLength

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P3)

Description

[Steven Julian]

Description / Summary:

BufferMediaResource::ReadFromCache (dom/media/BufferMediaResource.h) computes the copy length using:

uint32_t bytes = std::min(mLength - static_cast<uint32_t>(aOffset), aCount);
memcpy(aBuffer, mBuffer + aOffset, bytes);

It only validates aOffset < 0 and does not validate that aOffset is within the buffer length.
If aOffset > mLength, the subtraction (mLength - uint32_t(aOffset)) underflows in unsigned
arithmetic, producing a very large value. std::min(...) then selects aCount, and memcpy()
reads aCount bytes starting from (mBuffer + aOffset), which is beyond the end of the in-memory
buffer. This results in an out-of-bounds (OOB) read.

Additionally, this implementation appears to violate the MediaResource::ReadFromCache contract:
MediaResource::ReadFromCache is documented to return failure if "aCount bytes otherwise can't
be read" (since it does not return a "bytes read" outparam). BufferMediaResource::ReadFromCache
uses std::min(...) and returns NS_OK even when fewer than aCount bytes are copied (e.g. when
aOffset is within range but near EOF). Callers that assume a full read on success may then
consume uninitialized bytes from the destination buffer.

Affected Code (permalinks):

1. BufferMediaResource::ReadFromCache implementation:
   https://searchfox.org/firefox-main/rev/897ad323744cc6d3afe39845cfb94b015e9f65a0/dom/media/BufferMediaResource.h#140-158

2. MediaResource::ReadFromCache contract:
   https://searchfox.org/firefox-main/rev/897ad323744cc6d3afe39845cfb94b015e9f65a0/dom/media/MediaResource.h#245-257

3. Example caller that assumes full read on success (sets *bytes_read = aCount):
   https://searchfox.org/firefox-main/rev/897ad323744cc6d3afe39845cfb94b015e9f65a0/dom/media/mp4/ResourceStream.cpp#111-129

Steps to Reproduce (minimal):
This can be triggered with a minimal direct call to ReadFromCache.

minimal C++:

#include "mozilla/RefPtr.h"
 #include "BufferMediaResource.h"

 using namespace mozilla;

 void Trigger() {
   uint8_t buf[1] = {0x41};
   RefPtr<BufferMediaResource> res = new BufferMediaResource(buf, 1);

   char out[8];
   // aOffset (2) > mLength (1) => unsigned underflow in (mLength - uint32_t(aOffset))
   nsresult rv = res->ReadFromCache(out, /*aOffset*/ 2, /*aCount*/ sizeof(out));
   (void)rv;
 }

Results:

• If aOffset > mLength, memcpy reads past the end of mBuffer (OOB read).
• If aOffset is in range but (aOffset + aCount) exceeds mLength, the function can return NS_OK
  after copying fewer than aCount bytes, which can cause callers to consume uninitialized bytes.

Security Impact:

• OOB read in a media parsing/decoding related code path (in-memory media buffer). Depending
  on memory layout and downstream usage, this can lead to a crash and/or information disclosure.
• The contract violation (returning success on partial read) can also result in uninitialized
  memory being treated as valid input by callers that assume full reads on success.

Suggested Fix:
Harden ReadFromCache with explicit bounds checks and enforce the contract by failing unless
the full requested range [aOffset, aOffset + aCount) is valid:

nsresult ReadFromCache(char* aBuffer, int64_t aOffset, uint32_t aCount) override {
  if (aOffset < 0) {
    return NS_ERROR_FAILURE;
  }

  // mLength is uint32_t so validate aOffset is within [0, mLength] and the full range fits.
  if (static_cast<uint64_t>(aOffset) > mLength ||
      static_cast<uint64_t>(aOffset) + static_cast<uint64_t>(aCount) > mLength) {
    return NS_ERROR_FAILURE;
  }

  // Safe after the checks above.
  memcpy(aBuffer, mBuffer + aOffset, aCount);
  return NS_OK;
}

(Alternatively, use CheckedInt/CheckedUint64 for the addition, and cast aOffset to a safe
size_t/uint32_t after validation.)

Notes:

• BufferMediaResource::ReadAt already checks (aOffset < 0 || aOffset > mLength), but
  ReadFromCache lacks the upper-bound check and currently uses unsigned subtraction that
  can underflow.

Comment 1

[Steven Julian]

I created and build gtest to reproduce ReadFromCache_OffsetBeyondLength_OOBRead confirms the OOB read when aOffset > mLength, and ReadFromCache_PartialRead_ContractViolation demonstrates the contract violation where it returns success on partial reads.

──(steve㉿DESKTOP-1BAC6TN)-[~/firefox]
└─$ ./mach gtest BufferMediaResource.*
 0:02.81 W Warnings in third-party code are being suppressed from the terminal output. Use --show-all-warnings or --verbose to see them.
 0:02.81 W All warnings will still be dumped to /home/steve/firefox/obj-x86_64-pc-linux-gnu/.mozbuild/logs/build/warnings_*.json at the end of the build.
 0:02.85 Clobber not needed.
 0:09.39 Parallelism determined by memory: using 4 jobs for 4 cores based on 5.8 GiB RAM and estimated job size of 1.0 GiB
 0:09.39 /usr/bin/gmake -j4 -s recurse_gtest
 0:09.73 W /home/steve/firefox/config/recurse.mk:94: warning: overriding recipe for target 'toolkit/library/gtest/rust/gtest'
 0:09.73 W /home/steve/firefox/config/recurse.mk:72: warning: ignoring old recipe for target 'toolkit/library/gtest/rust/gtest'
 0:11.71 dom/media/gtest
 0:12.04 dom/media/webaudio
 1:46.65 toolkit/library/gtest/libxul.so
 7:17.90 W 0 compiler warnings present.
 7:19.23 W Overall system resources - Wall time: 436s; CPU: 34%; Read bytes: 10309529600; Write bytes: 4311830528; Read time: 1169802; Write time: 34153601
 7:19.24 W Swap in/out (MB): 32.671875/127.5625
 7:19.26 /usr/bin/notify-send '--app-name=Mozilla Build System' 'Mozilla Build System' 'Build complete'
 7:19.39 To view a profile of the build, run |mach resource-usage|.
 7:19.39 Your build was successful!
One or more Android-only options will be ignored
 7:20.59 Currently running suites: BufferMediaResource
 7:20.67 [BufferMediaResource] *** You are running in headless mode.
 7:20.74 [BufferMediaResource] Running GTest tests...
 7:20.82 [BufferMediaResource] Note: Google Test filter = BufferMediaResource.*-
 7:20.82 [BufferMediaResource] [==========] Running 5 tests from 1 test suite.
 7:20.82 [BufferMediaResource] [----------] Global test environment set-up.
 7:20.82 [BufferMediaResource] [----------] 5 tests from BufferMediaResource
 7:20.82 [BufferMediaResource] [ RUN      ] BufferMediaResource.ReadFromCache_OffsetBeyondLength_OOBRead
 7:20.82 [BufferMediaResource] [       OK ] BufferMediaResource.ReadFromCache_OffsetBeyondLength_OOBRead (0 ms)
 7:20.82 [BufferMediaResource] [ RUN      ] BufferMediaResource.ReadFromCache_PartialRead_ContractViolation
 7:20.82 [BufferMediaResource] [       OK ] BufferMediaResource.ReadFromCache_PartialRead_ContractViolation (0 ms)
 7:20.82 [BufferMediaResource] [ RUN      ] BufferMediaResource.ReadFromCache_ValidRange_ShouldSucceed
 7:20.82 [BufferMediaResource] [       OK ] BufferMediaResource.ReadFromCache_ValidRange_ShouldSucceed (0 ms)
 7:20.82 [BufferMediaResource] [ RUN      ] BufferMediaResource.ReadFromCache_NegativeOffset_ShouldFail
 7:20.82 [BufferMediaResource] [       OK ] BufferMediaResource.ReadFromCache_NegativeOffset_ShouldFail (0 ms)
 7:20.82 [BufferMediaResource] [ RUN      ] BufferMediaResource.ReadFromCache_OffsetAtLength_ShouldFail
 7:20.82 [BufferMediaResource] [       OK ] BufferMediaResource.ReadFromCache_OffsetAtLength_ShouldFail (0 ms)
 7:20.82 [BufferMediaResource] [----------] 5 tests from BufferMediaResource (0 ms total)
 7:20.82 [BufferMediaResource]
 7:20.82 [BufferMediaResource] [----------] Global test environment tear-down
 7:20.83 [BufferMediaResource] [==========] 5 tests from 1 test suite ran. (0 ms total)
 7:20.83 [BufferMediaResource] [  PASSED  ] 5 tests.
 7:20.87 [BufferMediaResource] Finished running GTest tests.
 7:20.90 Ran 5 test(s) from 1 test suite(s) (0 disabled), with 0 failure(s).

/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=2 et sw=2 tw=80: */
/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

/**
 * Bug 2011946 - OOB read in BufferMediaResource::ReadFromCache via unsigned
 * underflow when aOffset > mLength
 */

#include "BufferMediaResource.h"
#include "gtest/gtest.h"
#include "mozilla/RefPtr.h"

using namespace mozilla;

/**
 * Test case matching the minimal reproducer from Bug 2011946.
 * Note: specific casting to MediaResource* is required because ReadFromCache
 * is private in BufferMediaResource but public in the MediaResource base class.
 */
TEST(BufferMediaResource, ReadFromCache_OffsetBeyondLength_OOBRead)
{
  const uint8_t buf[1] = {0x41};
  // We use RefPtr<MediaResource> to access the public ReadFromCache method
  // which is private in BufferMediaResource but accessible via base class.
  RefPtr<MediaResource> res = new BufferMediaResource(buf, sizeof(buf));

  char out[8] = {0};
  // aOffset (2) > mLength (1) => unsigned underflow in (mLength - uint32_t(aOffset))
  // This triggers OOB read in memcpy if not checked.
  // Note: static_cast used to avoid warnings about size_t vs uint32_t
  nsresult rv = res->ReadFromCache(out, 2, static_cast<uint32_t>(sizeof(out)));

  // Should return failure
  EXPECT_NE(rv, NS_OK) << "ReadFromCache should fail when aOffset > mLength "
                       << "(unsigned underflow causes OOB read)";
}

TEST(BufferMediaResource, ReadFromCache_PartialRead_ContractViolation)
{
  const uint8_t buf[4] = {0x41, 0x42, 0x43, 0x44};
  RefPtr<MediaResource> res = new BufferMediaResource(buf, sizeof(buf));

  char out[8] = {0};
  // aOffset=2, aCount=8 => only 2 bytes available
  nsresult rv = res->ReadFromCache(out, 2, 8);

  // Contract violation: returns success on partial read
  EXPECT_NE(rv, NS_OK) << "ReadFromCache should fail when aCount bytes "
                       << "cannot be read (contract violation)";
}

TEST(BufferMediaResource, ReadFromCache_ValidRange_ShouldSucceed)
{
  const uint8_t buf[4] = {0x41, 0x42, 0x43, 0x44};
  RefPtr<MediaResource> res = new BufferMediaResource(buf, sizeof(buf));

  char out[4] = {0};
  nsresult rv = res->ReadFromCache(out, 0, 4);

  EXPECT_EQ(rv, NS_OK);
  EXPECT_EQ(out[0], 0x41);
  EXPECT_EQ(out[1], 0x42);
  EXPECT_EQ(out[2], 0x43);
  EXPECT_EQ(out[3], 0x44);
}

TEST(BufferMediaResource, ReadFromCache_NegativeOffset_ShouldFail)
{
  const uint8_t buf[4] = {0x41, 0x42, 0x43, 0x44};
  RefPtr<MediaResource> res = new BufferMediaResource(buf, sizeof(buf));

  char out[4] = {0};
  nsresult rv = res->ReadFromCache(out, -1, 4);

  EXPECT_NE(rv, NS_OK);
}

TEST(BufferMediaResource, ReadFromCache_OffsetAtLength_ShouldFail)
{
  const uint8_t buf[4] = {0x41, 0x42, 0x43, 0x44};
  RefPtr<MediaResource> res = new BufferMediaResource(buf, sizeof(buf));

  char out[1] = {0};
  nsresult rv = res->ReadFromCache(out, 4, 1);

  EXPECT_NE(rv, NS_OK);
}

Comment 2

[Paul Adenot (:padenot)]

Attachment: (secure)

Add bounds checking to prevent unsigned underflow when aOffset > mLength.
The previous code would underflow when computing mLength - aOffset,
leading to an out-of-bounds read via memcpy.

Comment 3

[Paul Adenot (:padenot)]

I haven't been able to make this fail from script, despite trying lots of permutation, there's quite a lot of validation in the code calling into BufferMediaResource, most notably the demuxers are too strict and either fix the issue (because they are supposed to accept malformed files) or reject the file.

Comment 4

[Andrew McCreight [:mccr8]]

Thanks. I'll change this to sec-other then, unless the reporter can come up with a valid HTML test case that triggers the issue.

Comment 5

[Steven Julian]

Just want to ask, i wondering what happen if i can't provide HTML test case. actually, I'm just curious about the tag sec-other. Will be come to you if there's any update.

Comment 6

[Andrew McCreight [:mccr8]]

This issue will not qualify for a bug bounty unless it can be demonstrated that it can be triggered from web content.

Comment 7

[Paul Adenot (:padenot)]

Attachment: (secure)

Tests by Steven Julian.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/61b2cb729b65
https://hg.mozilla.org/integration/autoland/rev/a1bdb4e189a1
r=media-playback-reviewers,karlt
https://github.com/mozilla-firefox/firefox/commit/a96b2762eaf4
https://hg.mozilla.org/integration/autoland/rev/7975a653208e
Tests. r=media-playback-reviewers,alwu

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/5352fd8cf086
https://hg.mozilla.org/integration/autoland/rev/5fc760f9980b
fix linux build bustages

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/a1bdb4e189a1
https://hg.mozilla.org/mozilla-central/rev/7975a653208e
https://hg.mozilla.org/mozilla-central/rev/5fc760f9980b