1163359 - Crash [@ moz_speex_inner_product_single ] | Assertion failure: aIndex < Length() (invalid array index)

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: Assertion crash report

1. http://buu.yle.fi/jul14/14/
2. Crash [@ moz_speex_inner_product_single ]
   bp-5832804a-18c1-454e-ada3-6951e2150510

rated as low exploitability -> s-s

or in debug

Assertion failure: aIndex < Length() (invalid array index), at ../../../dist/include/nsTArray.h:970

Lots of intermittent unittests with this assertion.


Appears to be all branches/OS.

Comment 1

[Daniel Veditz [:dveditz]]

The assertion, which is nowhere near the speex code, worries me. Let's capture this testcase locally if we can so the site doesn't change on us, and run it through an ASAN or valgrind build for more info.

Comment 2

[Bob Clary [:bc] (inactive)]

Attachment: buu.yle.fi.tar.bz2

Saved via firefox/linux. Doesn't reproduce for me locally; neither did a version saved via wget. It does give NS_ERROR_DOM_BAD_URI: Access to restricted URI denied game.js:8:0 in the webconsole locally though on win61 32bit.

Comment 3

[Paul Adenot (:padenot)]

The assertion happens in debug, the crash in libspeex_resampler happens in release, because resampling is exactly the next step in this process.

This is cause by the fact that `mMediaInfo.mAudio.mChannels` is 1 (mono), but when poping the first AudioData from the queue, they are stereo (audioData->mChannels == 2).

This is trivial to fix, but is it expected?

cpearce do you know if I should just work with the fact that an audio file can change the number of channel between filling in `mMediaInfo.mAudio` and the actual decoded packets, or if this is a bug somewhere in the media decoding code ?

Comment 4

[Bob Clary [:bc] (inactive)]

fwiw, valgrind didn't show anything interesting on linux.

Comment 5

[Paul Adenot (:padenot)]

Not surprising, the game has support for multiple codecs, and only one of them shows this crash.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

I imagine the site is using an HEAAC/MP4 file, which can report as being mono in the container, and then turn out to be stereo if the decoder supports HEAAC once decoding has begun. We're supposed to update the MediaInfo once the first sample has decoded, so that we have a stable number of channels and sample rate by the time we create the audio stream.

jya: could that not be happening here? Any ideas?

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #6)
> I imagine the site is using an HEAAC/MP4 file, which can report as being
> mono in the container, and then turn out to be stereo if the decoder
> supports HEAAC once decoding has begun. We're supposed to update the
> MediaInfo once the first sample has decoded, so that we have a stable number
> of channels and sample rate by the time we create the audio stream.
> 
> jya: could that not be happening here? Any ideas?

Doubtful...
HE/AAC is on top of AAC-LC, the AAC-LC should have the same number of channels per spec..

For MP4 do read the number of channels and sample rate after decoding the first frame and update mInfo accordingly. After decoding the first frame, the MDSM call ReadUpdatedMetadata() to get the latest configuration.
AFAIK, only MP4Reader implement it properly.

Having said that, should there be a change of audio config *after* the first frame ; the MP4Reader or new MediaFormatReader will detect it, but there's no mechanism in place to notify the MDSM that the audio configuration has changed. We could then start feeding the MDSM with MediaData with the new config.

Now back to the problem at hand: I can't reproduce the problem with the archive provided in attachment. However, going to http://buu.yle.fi/jul14/14/ I can reproduce the crash instantly.

This is part of the code I didn't know about. We create a MP4Reader, outside the usual fashion via a MediaDecoder / MediaDecoderStateMachine.

It doesn't implement the ReadUpdatedMetadata() after the decoding of the first frame so is only using the audio config as defined in the metadata which here doesn't match the actual AAC content -> boom.

Fix should be fairly simple.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: ensure we read updated audio configuration

Always update mMediaInfo after reading the first frame

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/685e7ad81dd9

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Carsten Book [:Tomcat] from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/685e7ad81dd9

Was there offline agreement to land this on central (only) without s-a?  38+ are affected...

Comment 11

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

No, this needed to have sec-approval? sent (still does) and questions answered. This should not have been checked in (looking at you, Carsten) without sec-approval+.

Comment 12

[Jean-Yves Avenard [:jya]]

oops, I did it again :(

We definitely need something to prevent those checkins. To me that looked like any other bugs really. It causes crashes; I don't see how this could be exploited (other than causing the crash) to make it a security risk

Comment 13

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

We still need the form filled out for sec-approval?

Comment 14

[Jean-Yves Avenard [:jya]]

Comment on attachment 8606621 [details] [diff] [review]
ensure we read updated audio configuration

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Need to craft a MP4 with an init segment (moov) indicating different channel configuration with AAC ADTS content giving a different channel count.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.... Pretty hard fetch for someone looking at the fix to realise you could make it crash. It's not even a special case for MP4.

Which older supported branches are affected by this flaw?
Since MP4 support was added I believe.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Very low, that patch should apply to all branches with little effort (if any)

How likely is this patch to cause regressions; how much testing does it need?
This does what the MediaDecoderStateMachine already does, decoding the first frame and looking at the config.

Comment 15

[Carsten Book [:Tomcat]]

 (In reply to Randell Jesup [:jesup] from comment #10)
> (In reply to Carsten Book [:Tomcat] from comment #9)
> > https://hg.mozilla.org/mozilla-central/rev/685e7ad81dd9
> 
> Was there offline agreement to land this on central (only) without s-a?  38+
> are affected...

seems the inbound comment was missing since this did not land direct on m-c -> https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=685e7ad81dd9

(In reply to Al Billings [:abillings] from comment #11)
> No, this needed to have sec-approval? sent (still does) and questions
> answered. This should not have been checked in (looking at you, Carsten)
> without sec-approval+.

yup but in this case, regarding the "looking at you" this was a missinformation here re landings etc, since my part was just doing the merge from m-i to m-c where this was included

Comment 16

[Ryan VanderMeulen [:RyanVM]]

I suspect people are already getting used to pulsebot commenting their bugs and forgetting that it doesn't do security bugs.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

This rebases easily to 37+. b2g34/b2g32/esr31 will need rebasing, depending on how far back this needs to go.

jya, can you please figure out for sure how far this needs to go, rebase the patch if needed, and request aurora/beta/esr38 (and esr31 if necessary) approval? Thanks!

Comment 18

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

(In reply to Carsten Book [:Tomcat] from comment #15)

> yup but in this case, regarding the "looking at you" this was a
> missinformation here re landings etc, since my part was just doing the merge
> from m-i to m-c where this was included


All right. I didn't realize that Jean-Yves had landed it on inbound first since that wasn't recorded in this bug and I don't normally see inbound landings if people don't put them in the bugs. 

Jean-Yves, we really need you to not do this sort of thing.

Comment 19

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

We're going to want branch patches, including both ESR branches, for this issue so we can get it fixed everywhere.

Comment 20

[Jean-Yves Avenard [:jya]]

So how does that works? I create rebased patches for each branch and ask for each sec review?

Comment 21

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

No, sec-approval is just for trunk. You'd create a patch for each branch (often the same patch will apply) and ask for beta, aurora, etc. approval on the patches.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Per comment 17, b2g34/b2g32/esr31 are all that need any significant rebasing. Don't bother posting patches for 37+.

Also, the wiki link below better details the approval process. Hope it helps:
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 23

[Jean-Yves Avenard [:jya]]

the only place it would matter here (MP4Reader) isn't in use in B2G before 2.2

The MP4Reader was introduced in FF 28. It was pref off by default on everything but windows. webaudio would use the WMFReader however. MP4Reader became the default in 35 for windows only (bug 1057879)
However, the ability to retrieve the updated metadata was introduced in bug 1065827 (FF36), it would be difficult to backport that change beyond 36 then. How much do we want it there?

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8606621 [details] [diff] [review]
ensure we read updated audio configuration

Approval Request Comment
[Feature/regressing bug #]:1057879
[User impact if declined]: Loading a corrupted mp4 could cause a crash
[Describe test coverage new/current, TreeHerder]: In m-c
[Risks and why]: Low, it's doing what other players are doing for almost 1 year.
[String/UUID change made/needed]: none

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jean-Yves Avenard [:jya] from comment #23)
> the only place it would matter here (MP4Reader) isn't in use in B2G before
> 2.2

OK, so no need for b2g32/b2g34.

> The MP4Reader was introduced in FF 28. It was pref off by default on
> everything but windows. webaudio would use the WMFReader however. MP4Reader
> became the default in 35 for windows only (bug 1057879)
> However, the ability to retrieve the updated metadata was introduced in bug
> 1065827 (FF36), it would be difficult to backport that change beyond 36
> then. How much do we want it there?

Sounds like we don't need it for esr31 then either, since the feature is either preffed off or goes through a different backend.

I'm going to mark the older branches as disabled, since the code is there but not in use. Thanks, jya!

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8606621 [details] [diff] [review]
ensure we read updated audio configuration

sec-high, I am pretty sure we want that in esr38.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Not tracking (thanks Ryan)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f7e45db731a
https://hg.mozilla.org/releases/mozilla-beta/rev/3b796baa6b5e
https://hg.mozilla.org/releases/mozilla-esr38/rev/afb01805863a
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/980faa4549ce

Comment 29

[Bogdan Maris, Desktop Test Engineering]

Reproduced with Firefox 38.0.5 under Windows 7x64 → bp-be14588b-9b40-4650-bbf5-3cbc62150701.
Verified fixed with 39.0 RC (Build ID: 20150630154324) and ESR 38.1.0 (Build ID: 20150624141534), across platforms [1] - no crash encountered with the attached URL nor with the sample from comment 2. Although, with the sample from comment 2 I can still see in Browser Console the ‘NS_ERROR_DOM_BAD_URI: Access to restricted URI denied’ message thrown by game.js:8:0. Should this raise any concerns? Thanks in advance!

[1] Windows 7 64-bit, Ubuntu 12.04 64-bit and Mac OS X 10.10

Comment 30

[Jean-Yves Avenard [:jya]]

no idea really, sorry.

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> Although, with the sample
> from comment 2 I can still see in Browser Console the ‘NS_ERROR_DOM_BAD_URI:
> Access to restricted URI denied’ message thrown by game.js:8:0. Should this
> raise any concerns? Thanks in advance!

No, that's unrelated to this bug, thanks.

Comment 32

[Bogdan Maris, Desktop Test Engineering]

(In reply to Randell Jesup [:jesup] from comment #31)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> > Although, with the sample
> > from comment 2 I can still see in Browser Console the ‘NS_ERROR_DOM_BAD_URI:
> > Access to restricted URI denied’ message thrown by game.js:8:0. Should this
> > raise any concerns? Thanks in advance!
> 
> No, that's unrelated to this bug, thanks.

Thank you Randell! Marking this bug as verified fixed on Firefox 39 and ESR 38.1.0