Closed Bug 1123882 Opened 5 years ago Closed 5 years ago

MediaDecoderStateMachine::SendStreamAudio passes an incorrect buffer size to AudioSegment::AppendFrames when |offset| is non-zero

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed
firefox-esr31 36+ fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: bwc, Assigned: bwc)

Details

(Keywords: sec-high, Whiteboard: [adv-main36+][adv-esr31.5+])

Attachments

(2 files, 1 obsolete file)

Observed multiple ASAN crashes (buffer overflow, read) with the failure here:

https://treeherder.mozilla.org/logviewer.html#?job_id=4289100&repo=try

It looks like our problem is here:

https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#348

aOutput->AppendFrames(buffer.forget(), channels, aAudio->mFrames);

Should probably be

aOutput->AppendFrames(buffer.forget(), channels, aAudio->mFrames - offset);

correct?
Attached patch Fix case where offset != 0 (obsolete) — Splinter Review
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Consolidate some code.
Attachment #8552510 - Attachment is obsolete: true
Attachment #8552512 - Flags: review?(tterribe)
Comment on attachment 8552512 [details] [diff] [review]
Fix case where offset != 0

Review of attachment 8552512 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +332,5 @@
>  
>    if (offset >= aAudio->mFrames)
>      return;
>  
> +  size_t framesToWrite = aAudio->mFrames - offset;

I'd prefer to have this declaration right next to the code that uses it, but that's just personal preference.
Attachment #8552512 - Flags: review?(tterribe) → review+
(In reply to Timothy B. Terriberry (:derf) from comment #3)
> Comment on attachment 8552512 [details] [diff] [review]
> Fix case where offset != 0
> 
> Review of attachment 8552512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +332,5 @@
> >  
> >    if (offset >= aAudio->mFrames)
> >      return;
> >  
> > +  size_t framesToWrite = aAudio->mFrames - offset;
> 
> I'd prefer to have this declaration right next to the code that uses it, but
> that's just personal preference.

Yeah, I was torn between doing that and keeping it near the checking code that ensured the calculation was valid.
try (includes patches from multistream which made this bug happen much more often, plus patches for other bugs the multistream code triggers):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfca214e0135
Also have verified that signaling_unittests works on linux ASAN.
I'll go ahead and do a dedicated try push for this, since it seems we still have more bugs that multistream is tripping over.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a65a9afeb06
Comment on attachment 8552512 [details] [diff] [review]
Fix case where offset != 0

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

   Looks extremely hard, even to cause a segfault.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

   A smart reader could figure out what was being fixed pretty easily, since it is such a small fix. Another matter entirely to figure out how to hit the bug.

Which older supported branches are affected by this flaw?

   All.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

   Not yet, but they should be quite easy.

How likely is this patch to cause regressions; how much testing does it need?

   Seems really unlikely.

Approval Request Comment
[Feature/regressing bug #]:

   Bug 827537

[User impact if declined]:

   Extremely rare crashes while playing back audio, and audio corruption more commonly. Has only happened on e10s mochitests, don't see any sign of this in crash-stats.

[Describe test coverage new/current, TreeHerder]:

   No new tests.

[Risks and why]: 

   Risk seems pretty low, change is simple.

[String/UUID change made/needed]:

   None.
Attachment #8552512 - Flags: sec-approval?
Attachment #8552512 - Flags: approval-mozilla-beta?
Attachment #8552512 - Flags: approval-mozilla-aurora?
We need to figure out a security rating for this before we can checkin. Do you have any ideas on that?
It is possible that we could leak information that lies beyond the end of the buffer to content (granted, that data has been passed through a resampler first, which might make it hard to use, or even determine what it is).
Flags: needinfo?(tterribe)
Given comment 10, I'd suggest sec-moderate or sec-high (sec-high is probably safest).
Flags: needinfo?(tterribe)
sec-approval+ for trunk and I'll approve it for branches. Can we get an ESR31 patch as well, please?
Attachment #8552512 - Flags: sec-approval?
Attachment #8552512 - Flags: sec-approval+
Attachment #8552512 - Flags: approval-mozilla-beta?
Attachment #8552512 - Flags: approval-mozilla-beta+
Attachment #8552512 - Flags: approval-mozilla-aurora?
Attachment #8552512 - Flags: approval-mozilla-aurora+
Patches apply cleanly to aurora and beta, not sure about esr31 yet, will have to wait until I can pull it down.
Is it preferred to allow sheriffs to handle landing on aurora/beta?
Attachment #8553898 - Flags: review+
Attachment #8553898 - Flags: approval-mozilla-esr31?
(In reply to Byron Campen [:bwc] from comment #15)
> Is it preferred to allow sheriffs to handle landing on aurora/beta?

I have no idea. They certainly will land approved things there.
Keywords: checkin-needed
*shrug* If you don't beat me to it, I certainly will :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0cd1d16b95c7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8553898 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Whiteboard: [adv-main36+][adv-esr31.5+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.