2025281 - MediaIPCUtils.h audit

Status: RESOLVED FIXED

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

Description

[Tom Schuster [:tschuster]]

Motivated by bug 2025179, I let Claude do some very basic audit.

Security Audit: dom/media/ipc/MediaIPCUtils.h

This file defines IPC ParamTraits serializers/deserializers for media types
that cross process boundaries (e.g. content process to parent process). Bugs
here are security-relevant because a compromised content process controls the
serialized data.

Bug 1 (High): DecoderDoctorEvent::Read — arbitrary int cast to enum (UB)

NOTE: I think we only send this from parent to child, so not an issue? The enum only has one value anyway.

Lines 333–341 (in the DecoderDoctorEvent ParamTraits):

static bool Read(MessageReader* aReader, paramType* aResult) {
    int domain = 0;
    if (ReadParam(aReader, &domain) && ReadParam(aReader, &aResult->mResult)) {
      aResult->mDomain = paramType::Domain(domain);  // unchecked cast
      return true;
    }
    return false;
  };

The Domain enum has a single enumerator (eAudioSinkStartup = 0). An
attacker-controlled IPC message can supply any int value. The raw cast
paramType::Domain(domain) produces an enumerator with an out-of-range value.
For unscoped enums this is undefined behavior if the value exceeds the
representable range of the enum's underlying type. Even if the compiler chooses
to store it, downstream switch statements or array indexing keyed on this
enum can misroute control flow or access out-of-bounds memory.

Every other enum in this file uses ContiguousEnumSerializer /
ContiguousEnumSerializerInclusive which validates the range and returns
false on out-of-range values. This one was missed.

Impact: A compromised content process can send a crafted
DecoderDoctorEvent message to the parent process, triggering undefined
behavior. Depending on how mDomain is consumed, this could lead to
out-of-bounds reads, unexpected code paths, or memory corruption.

Fix: Replace the manual ParamTraits with:

template <>
struct ParamTraits<mozilla::DecoderDoctorEvent::Domain>
    : public ContiguousEnumSerializerInclusive<
          mozilla::DecoderDoctorEvent::Domain,
          mozilla::DecoderDoctorEvent::Domain::eAudioSinkStartup,
          mozilla::DecoderDoctorEvent::Domain::eAudioSinkStartup> {};

Then use ReadParam(aReader, &aResult->mDomain) in the DecoderDoctorEvent
reader instead of reading into a raw int.

Bug 2 (Medium): TimeUnit::Read — unvalidated base causes process crash (DoS)

NOTE: This was already reported in bug 2025179

Lines 233–247 (in the TimeUnit ParamTraits):

static bool Read(MessageReader* aReader, paramType* aResult) {
    bool valid;
    int64_t ticks;
    int64_t base;
    if (ReadParam(aReader, &valid) && ReadParam(aReader, &ticks) &&
        ReadParam(aReader, &base)) {
      if (valid) {
        *aResult = mozilla::media::TimeUnit(ticks, base);  // no validation
      } else {
        *aResult = mozilla::media::TimeUnit::Invalid();
      }
      return true;
    }
    return false;
  };

The TimeUnit constructor contains:

constexpr TimeUnit(CheckedInt64 aTicks, int64_t aBase)
    : mTicks(aTicks), mBase(aBase) {
  MOZ_RELEASE_ASSERT(mBase > 0);           // crashes on base <= 0
  MOZ_DIAGNOSTIC_ASSERT(mBase <= UINT32_MAX); // only in debug/diagnostic builds
}

A malicious IPC message with valid=true, base=0 (or any negative value) will
hit the MOZ_RELEASE_ASSERT and intentionally crash the receiving process.
If this message is sent from a compromised content process to the parent (GPU,
RDD, or main) process, it is a denial-of-service against the browser.

Additionally, base > UINT32_MAX is only caught by MOZ_DIAGNOSTIC_ASSERT,
which is compiled out in release builds. Extremely large base values could
cause integer overflow in later TimeUnit arithmetic.

Note: the Write side already handles this (writes 1 when invalid), but the
Read side trusts the incoming data.

Impact: Controlled crash of the parent/utility process from a sandboxed
content process.

Fix: Validate base before constructing the TimeUnit:

if (valid) {
    if (base <= 0 || base > INT64_C(0xFFFFFFFF)) {
      return false;
    }
    *aResult = mozilla::media::TimeUnit(ticks, base);
}

Bug 3 (Low): ConversionRequired enum serializer uses raw integer 0 as lower bound

Lines 214–219:

: public ContiguousEnumSerializerInclusive<
      mozilla::MediaDataDecoder::ConversionRequired,
      mozilla::MediaDataDecoder::ConversionRequired(0),   // raw cast
      mozilla::MediaDataDecoder::ConversionRequired(
          mozilla::MediaDataDecoder::ConversionRequired::kNeedHVCC)> {};

The lower bound should be ConversionRequired::kNeedNone (which happens to
equal 0). Using a raw integer cast is fragile: if someone reorders the enum
or inserts a value before kNeedNone, the serializer would silently accept
invalid values. Every other enum serializer in this file uses named
enumerators for both bounds.

Impact: Not currently exploitable, but a latent defense-in-depth gap.

Fix: Use mozilla::MediaDataDecoder::ConversionRequired::kNeedNone.

Bug 4 (Low): PropertyName enum serializer uses raw integer 0 as lower bound

Lines 222–226:

: public ContiguousEnumSerializerInclusive<
      mozilla::MediaDataDecoder::PropertyName,
      mozilla::MediaDataDecoder::PropertyName(0),   // raw cast
      mozilla::MediaDataDecoder::sHighestPropertyName> {};

Same issue as Bug 3. The upper bound correctly uses the auto-generated
sHighestPropertyName, but the lower bound is a raw 0 cast rather than the
first named enumerator.

Fix: Use the first named enumerator from the PropertyName enum
(MaxNumVideoBuffers if it starts at 0, or the appropriate first value).

Bug 5 (Info): Dead variable in original VideoInfo::Read

In the original code (before recent edits), line 50 declared:

mozilla::gfx::IntRect imageRect;

This variable was never used — the reader wrote directly into
aResult->mImageRect. This has since been cleaned up in the current version
of the file.

---

Summary

#	Severity	Description	Lines
1	High	DecoderDoctorEvent::Domain: unchecked int-to-enum cast = UB	333–341
2	Medium	TimeUnit: unvalidated base crashes process via MOZ_RELEASE_ASSERT	233–247
3	Low	ConversionRequired serializer: raw 0 lower bound	214–219
4	Low	PropertyName serializer: raw 0 lower bound	222–226

Comment 1

[Tom Schuster [:tschuster]]

https://phabricator.services.mozilla.com/D289190

Comment 2

[Andrew McCreight [:mccr8]]

Nothing particularly bad here AFAICT.

Comment 3

[Tom Schuster [:tschuster]]

Attachment: (secure)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0eb69341ab53
https://hg.mozilla.org/integration/autoland/rev/3406a6a44e49
Cleanup MediaIPCUtils. r=media-playback-reviewers,alwu

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/3406a6a44e49

Comment 6

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:tschuster, 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-firefox150 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Prevents content from being able to crash the GPU process. Fixes some minor undefined behavior.
• Code covered by automated testing?: no
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: Trivial patch. Arguably the code got simpler.
• String changes made/needed?: no
• Is Android affected?: yes

Comment 8

[Tom Schuster [:tschuster]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D289190

Comment 9

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/2781d362ed62
https://hg.mozilla.org/releases/mozilla-beta/rev/8a30930a0c0e