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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Byron Campen [:bwc]]

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?

Comment 1

[Byron Campen [:bwc]]

Attachment: Fix case where offset != 0

Comment 2

[Byron Campen [:bwc]]

Attachment: Fix case where offset != 0

Consolidate some code.

Comment 3

[Timothy B. Terriberry (:derf)]

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.

Comment 4

[Byron Campen [:bwc]]

(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.

Comment 5

[Byron Campen [:bwc]]

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

Comment 6

[Byron Campen [:bwc]]

Also have verified that signaling_unittests works on linux ASAN.

Comment 7

[Byron Campen [:bwc]]

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 8

[Byron Campen [:bwc]]

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.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

We need to figure out a security rating for this before we can checkin. Do you have any ideas on that?

Comment 10

[Byron Campen [:bwc]]

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).

Comment 11

[Timothy B. Terriberry (:derf)]

Given comment 10, I'd suggest sec-moderate or sec-high (sec-high is probably safest).

Comment 12

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk and I'll approve it for branches. Can we get an ESR31 patch as well, please?

Comment 13

[Byron Campen [:bwc]]

Patches apply cleanly to aurora and beta, not sure about esr31 yet, will have to wait until I can pull it down.

Comment 14

[Byron Campen [:bwc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd1d16b95c7

Comment 15

[Byron Campen [:bwc]]

Is it preferred to allow sheriffs to handle landing on aurora/beta?

Comment 16

[Byron Campen [:bwc]]

Attachment: (ESR31 backport) Fix case where offset != 0

Comment 17

[Al Billings [:abillings - ex-MoCo]]

(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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

*shrug* If you don't beat me to it, I certainly will :)

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0cd1d16b95c7

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b71942fd4bc8
https://hg.mozilla.org/releases/mozilla-beta/rev/228ee06444b5

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b71942fd4bc8

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/08e90874cdf5

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8e1aa59fa872
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/85138cc0dc50
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/b4469bbcea5a

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/8e1aa59fa872
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/85138cc0dc50