1737816 - Crash in [@ mozilla::MP3TrackDemuxer::FindNextFrame]

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

Crash report: https://crash-stats.mozilla.org/report/index/563bfd45-44d6-42ca-86bb-2246a0211026

Reason: STATUS_STACK_BUFFER_OVERRUN / FAST_FAIL_STACK_COOKIE_CHECK_FAILURE

Top 3 frames of crashing thread:

0 xul.dll _report_gsfailure /builds/worker/workspace/obj-build/toolkit/library/build/d:/agent/_work/1/s/src/vctools/crt/vcstartup/src/gs/gs_report.c:220
1 xul.dll mozilla::MP3TrackDemuxer::FindNextFrame dom/media/mp3/MP3Demuxer.cpp:587
2  @0xcd82a92ab2a2e649 

This bug is only being caught by the Windows Error Reporting runtime module because the failure mode is a call to __fastfail(). The stack cookie check around the crashing function are being corrupted, possibly by a buffer overflow. This seems to have started in version 92 which is odd because I'd expect to see some crashes in the nightly channel first... unless we compile our release builds differently WRT stack-smashing prevention.

Comment 1

[Gabriele Svelto [:gsvelto]]

Adding two signatures that appear to be related:

• @ memcpy | mozilla::MediaResourceIndex::ReadAt is a Windows-only signature and it's a crash involving memcpy()ing into the buffer that appears to be affected by the overflow in comment 0
• @ mozilla::detail::MutexImpl::lock | mozilla::MediaCacheStream::ReadAt is a macOS-only crash and it has to do with a stack-allocated mutex which appears to have been overwritten with zeros. This could have also been caused by a stack-based overflow.

Comment 2

[Gabriele Svelto [:gsvelto]]

And two more related signatures.

Comment 3

[Gabriele Svelto [:gsvelto]]

Stack check failure on macOS, this is a new function which appears to have been introduced in macOS 11. The macOS crash info field tells us what we already know:

{
  "num_records": 1,
  "records": [
    {
      "message": "stack buffer overflow",
      "module": "/usr/lib/system/libsystem_c.dylib"
    }
  ]
}

Comment 4

[Gabriele Svelto [:gsvelto]]

I cracked open a minidump and here's what I found:

1. This memcpy is doing an out-of-bounds access
2. This appears to be because the value of aCount is 4294755389 which looks like a lot like a negative 32-bit integer cast into an unsigned one
3. Indeed we're casting an int32_t into a uint32_t here
4. If I understand this code correctly we might be reading past the end of a stream so aOffset is larger than streamLen here and this leads us to set aSize to a negative value which is ultimately cast into the aCount value seen in point 2

Comment 5

[Gabriele Svelto [:gsvelto]]

And bug 1169142 appears to be the bug where we introduced this code so maybe this is a regression? (albeit a very old one)

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Paul, is this something you could take a look at? Edit: didn't mean to post this, removing some of comment.

Comment 7

[Paul Adenot (:padenot)]

Taking for investigation.

Comment 8

[Paul Adenot (:padenot)]

(In reply to Gabriele Svelto [:gsvelto] from comment #4)

I cracked open a minidump and here's what I found:

1. This memcpy is doing an out-of-bounds access
2. This appears to be because the value of aCount is 4294755389 which looks like a lot like a negative 32-bit integer cast into an unsigned one
3. Indeed we're casting an int32_t into a uint32_t here
4. If I understand this code correctly we might be reading past the end of a stream so aOffset is larger than streamLen here and this leads us to set aSize to a negative value which is ultimately cast into the aCount value seen in point 2

Thanks for the analysis.

What I think is happening is a TOCTOU issue. It's possible for StreamLength() to suddenly change value, for example if a Content-Length header was passed in, and then the socket got closed, resulting in a truncated resource. The mp3 demuxing code first attempts to find a frame further in the stream, because it previously thought that the resource was of a certain length, but in practice this isn't the case when it's trying to actually read from the resource. This plus the negative integer cast to unsigned can explain. It's probably safe to return 0 in this case: this is the same as a failed read (as see by NS_ENSURE_SUCCESS(rv, 0); just below.

Comment 9

[C.M.Chang[:chunmin] | PTO]

Not sure if this is related, but I found that the mp3 time length could overflow by miscalculation in bug 1423834, which is not solved yet

Comment 10

[Paul Adenot (:padenot)]

Attachment: Bug 1737816 - Handle truncated mp3 resources. r?bryce

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 9255047 [details]
Bug 1737816 - Handle truncated mp3 resources. r?bryce

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Somehwat hard, this is an arbitrary read, with cast from negative signed to unsigned (so extremely large number), but the negative number is severely constrained by the way the mp3 format is laid out: the max size of a frame is small, a few thousand bytes max.

We can land this without any message, but it's still going to look like clamping a value just before a subtraction.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Low risk, this is taking a known failure path instead of crashing.

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9255047 [details]
Bug 1737816 - Handle truncated mp3 resources. r?bryce

Approved to land and request uplift

Comment 13

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

Handle truncated mp3 resources. r=bryce
https://hg.mozilla.org/integration/autoland/rev/440bddc72442e469208d2eb532fe9f5f601ee408
https://hg.mozilla.org/mozilla-central/rev/440bddc72442

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Rather unfortunate that this got sec-approval to land before the holidays but only landed today when we're already in RC week. We need release & ESR approval requests on this ASAP because it'll need to into an RC respin now.

Comment 15

[Frederik Braun [:freddy]]

I'm planning to include this in the release notes, as per our tracking flags which assumes that padenot is requesting ESR & Release approval and gets it.

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 9255047 [details]
Bug 1737816 - Handle truncated mp3 resources. r?bryce

Beta/Release Uplift Approval Request

• User impact if declined: Rare crash when decoding mp3
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is taking a known good failure path.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-crit.
• User impact if declined: Rare crash when decoding mp3
• Fix Landed on Version: 97
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is taking a known good failure path.

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9255047 [details]
Bug 1737816 - Handle truncated mp3 resources. r?bryce

Approved for 96.0rc2

Comment 18

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-release/rev/8e359302579c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9255047 [details]
Bug 1737816 - Handle truncated mp3 resources. r?bryce

Approved for 91.5esr.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/d39b3f8cdf1e

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.