Closed Bug 1163359 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Web Audio, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 + verified
firefox40 + fixed
firefox41 --- fixed
firefox-esr31 - disabled
firefox-esr38 39+ verified
b2g-v2.0 --- disabled
b2g-v2.0M --- disabled
b2g-v2.1 --- disabled
b2g-v2.1S --- disabled
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: bc, Assigned: jya)

References

()

Details

(4 keywords, Whiteboard: [adv-main39+][adv-esr38.1+])

Crash Data

Attachments

(3 files)

Attached file 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.
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.
Flags: needinfo?(padenot)
Attached file 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.
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 ?
Flags: needinfo?(padenot) → needinfo?(cpearce)
Assignee: nobody → padenot
fwiw, valgrind didn't show anything interesting on linux.
Not surprising, the game has support for multiple codecs, and only one of them shows this crash.
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?
Flags: needinfo?(cpearce) → needinfo?(jyavenard)
Group: media-core-security
(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.
Flags: needinfo?(jyavenard)
Always update mMediaInfo after reading the first frame
Attachment #8606621 - Flags: review?(cpearce)
Assignee: padenot → jyavenard
Status: NEW → ASSIGNED
Attachment #8606621 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/685e7ad81dd9
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(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...
Flags: needinfo?(jyavenard)
Flags: needinfo?(cbook)
Flags: needinfo?(abillings)
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+.
Flags: needinfo?(abillings)
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
Flags: needinfo?(jyavenard)
We still need the form filled out for sec-approval?
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
Attachment #8606621 - Flags: sec-approval?
 (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
Flags: needinfo?(cbook)
I suspect people are already getting used to pulsebot commenting their bugs and forgetting that it doesn't do security bugs.
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!
(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.
Attachment #8606621 - Flags: sec-approval? → sec-approval+
We're going to want branch patches, including both ESR branches, for this issue so we can get it fixed everywhere.
So how does that works? I create rebased patches for each branch and ask for each sec review?
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.
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
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?
Flags: needinfo?(jyavenard)
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
Attachment #8606621 - Flags: approval-mozilla-beta?
Attachment #8606621 - Flags: approval-mozilla-aurora?
Attachment #8606621 - Flags: approval-mozilla-beta?
Attachment #8606621 - Flags: approval-mozilla-beta+
Attachment #8606621 - Flags: approval-mozilla-aurora?
Attachment #8606621 - Flags: approval-mozilla-aurora+
(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 on attachment 8606621 [details] [diff] [review]
ensure we read updated audio configuration

sec-high, I am pretty sure we want that in esr38.
Attachment #8606621 - Flags: approval-mozilla-esr38+
Not tracking (thanks Ryan)
Group: media-core-security
Whiteboard: [adv-main39+][adv-esr38.1+]
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
Flags: needinfo?(jyavenard)
no idea really, sorry.
Flags: needinfo?(jyavenard)
(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.
(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
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.