Crash in InvalidArrayIndex_CRASH | nsTArray_Impl<T>::operator[] | mozilla::TrackBuffersManager::GetSample

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Playback
--
critical
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mccr8, Assigned: jya)

Tracking

({crash, csectype-bounds, sec-other})

Trunk
mozilla52
x86
Windows 7
crash, csectype-bounds, sec-other
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 wontfix, firefox51 affected, firefox52 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main50+][adv-main52-], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-8366c767-19dc-401b-a360-ae5e62160927.
=============================================================

I see a handful of these. It looks like MediaSourceTrackDemuxer::DoGetSamples(int) is calling into TrackBuffersManager::GetSample, and maybe when calling GetTracksData(aTrack).mBuffers.LastElement() it turns out that mBuffers is empty.
I assume we're just reading what we think is media data, so worst case we might display stuff based on private memory?
Keywords: csectype-bounds, sec-moderate
(Assignee)

Comment 2

2 years ago
That crash report doesn't make any sense...
The code starts with:
https://hg.mozilla.org/releases/mozilla-beta/annotate/91faf7ec36cd/dom/media/mediasource/TrackBuffersManager.cpp#l2067


  if (aIndex >= track.Length()) {
  2068
    // reached the end.
  2069
    return nullptr;
  2070
  }
  2071

The first thing it does is checking that the index is less than the size of the array...

So how can we get to a point where we attempt to read past the size of the array?

The only explanation here is that the size of the array reported is nonsensical and greater than 2^32. NsTArray::Length returns a size_t. Which by design of a nsTArray is impossible (maximum number of item is 2^31)
jya: ElementAt(aIndex = 4294967295, aLength = 0)  4294967295 = 0xFFFFFFFF

It appears to be crashing *before* "if (aIndex >= track.Length())" - on GetTrackBuffer(aTrack), which as comment 0 notes, eventally calls GetTracksData(aTrack).mBuffers.LastElement().  mBuffers must be empty.

LastElement() looks like this:
 return ElementAt(Length() - 1);
Component: Audio/Video → Audio/Video: Playback
Flags: needinfo?(jyavenard)
(Assignee)

Comment 4

2 years ago
Interesting. 

Though that would make even less sense as we can only be called to retrieve a particular sample of a track if that track has been created. 

I can't see a way for GetTracksData(aTrack).mBuffers to be an empty array. 

But thanks for the pointer, it gives me a new thing to look at.
(Assignee)

Comment 5

2 years ago
Created attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch
Flags: needinfo?(jyavenard)
Attachment #8800173 - Flags: review?(gsquelart)
(Assignee)

Comment 6

2 years ago
Created attachment 8800174 [details] [diff] [review]
0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch
Attachment #8800174 - Flags: review?(gsquelart)
(Assignee)

Comment 7

2 years ago
Created attachment 8800175 [details] [diff] [review]
0003-Bug-1305876-P3.-Make-function-members-const.-r-geral.patch
Attachment #8800175 - Flags: review?(gsquelart)
(Assignee)

Updated

2 years ago
Assignee: nobody → jyavenard
Attachment #8800173 - Flags: review?(gsquelart) → review+
Comment on attachment 8800174 [details] [diff] [review]
0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch

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

r+ with a suggestion:

::: dom/media/mediasource/TrackBuffersManager.cpp
@@ +325,4 @@
>  
>    // 4. For each track buffer managed by this SourceBuffer, run the following steps:
>    //   1. Let track ranges equal the track buffer ranges for the current track buffer.
> +  for (uint32_t i = 0; i < tracks.Length();  i++) {

I don't think you needed you avoid a range-for.
To make sure it's doing what you want, you could specify the loop-variable type, e.g.:
  for (const TimeIntervals* tR : tracks) {
    TimeIntervals trackRanges = *tR;
Attachment #8800174 - Flags: review?(gsquelart) → review+
Attachment #8800175 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 9

2 years ago
(In reply to Gerald Squelart [:gerald] from comment #8)
> Comment on attachment 8800174 [details] [diff] [review]
> 0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch
> 
> Review of attachment 8800174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a suggestion:
> 
> ::: dom/media/mediasource/TrackBuffersManager.cpp
> @@ +325,4 @@
> >  
> >    // 4. For each track buffer managed by this SourceBuffer, run the following steps:
> >    //   1. Let track ranges equal the track buffer ranges for the current track buffer.
> > +  for (uint32_t i = 0; i < tracks.Length();  i++) {
> 
> I don't think you needed you avoid a range-for.
> To make sure it's doing what you want, you could specify the loop-variable
> type, e.g.:
>   for (const TimeIntervals* tR : tracks) {
>     TimeIntervals trackRanges = *tR;

Yes, but I wanted to avoid the unnecessary copy that would happen most of the time (under most cases the buffered range is queried when the mediasource isn't in ended mode).

Hence why I did it like that...
(Assignee)

Comment 10

2 years ago
Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? very hard, I can't see how in fact

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? it doesn't. Plus we aren't fixing the problem, just adding a release assert

Which older supported branches are affected by this flaw? 42 (and as such ESR45)

If not all supported branches, which bug introduced the flaw? 1171330. I don't think there's any flaw yet though...

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

How likely is this patch to cause regressions; how much testing does it need? It will cause a proper crash if that happens. And I think this is related to other GetSamples crashes we've been getting for a long time.
Attachment #8800173 - Flags: sec-approval?
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> (In reply to Gerald Squelart [:gerald] from comment #8)
> > I don't think you needed you avoid a range-for.
> > To make sure it's doing what you want, you could specify the loop-variable
> > type, e.g.:
> >   for (const TimeIntervals* tR : tracks) {
> >     TimeIntervals trackRanges = *tR;
> 
> Yes, but I wanted to avoid the unnecessary copy that would happen most of
> the time (under most cases the buffered range is queried when the
> mediasource isn't in ended mode).
> 
> Hence why I did it like that...

I'm dubious that there is more copying in the range-for option than with the numbered-for case.
E.g., in the non-ended case, 'intersection.Intersection(*tracks[i]);' still accesses the TimeIntervals pointer (through an index-accessor instead of through the loop iterator) and dereferences it.

I was suggesting the range-for more for clarity than performance.

If performance is that important here, then I would suggest doing some profiling to pick the best solution!
(Assignee)

Comment 12

2 years ago
(In reply to Gerald Squelart [:gerald] from comment #11)
> I'm dubious that there is more copying in the range-for option than with the
> numbered-for case.
> E.g., in the non-ended case, 'intersection.Intersection(*tracks[i]);' still
> accesses the TimeIntervals pointer (through an index-accessor instead of
> through the loop iterator) and dereferences it.

none of which will make a copy. All called code uses const references.

> 
> I was suggesting the range-for more for clarity than performance.
> 
> If performance is that important here, then I would suggest doing some
> profiling to pick the best solution!

it's cheap enough to not do it IMHO, plus i find the code perfectly clear as it is :)
Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

As a sec-moderate rated issue, this doesn't need sec-approval. You can just check it into trunk. If we want to backport this to more than Aurora, we would need to discuss the ramifications of doing so.
Attachment #8800173 - Flags: sec-approval?
(Assignee)

Comment 14

2 years ago
Comment on attachment 8800174 [details] [diff] [review]
0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch

Approval Request Comment
[Feature/regressing bug #]: 1171330
[User impact if declined]: Returning incorrect buffered range for MSE under some circumstances.
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Very low. Spec compliant issue..
[String/UUID change made/needed]: None
Attachment #8800174 - Flags: approval-mozilla-beta?
Attachment #8800174 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 15

2 years ago
Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

Approval Request Comment
[Feature/regressing bug #]: 1305876
[User impact if declined]: Can't find why and where it's crashing. While this bug is low, I have the gutfeel that it's the same as bug 1247189.
[Describe test coverage new/current, TreeHerder]: local test. This change is functionally identical to what was there before, it just crashes if something that can't happen happened 
[Risks and why]: It will crash prior doing a possibly out of bound memory access.
[String/UUID change made/needed]: none
Attachment #8800173 - Flags: approval-mozilla-aurora?

Updated

2 years ago
status-firefox50: --- → affected
status-firefox51: --- → affected
Hi Al, Dan, from this point onwards, I'd prefer to only take sec-high and sec-crits in the Beta50 cycle. This one was nominated for uplift to beta. Also see Al's comment 13. Is this something we ought to take in 50?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(Assignee)

Comment 18

2 years ago
I only nominated patch 2 for beta uplift. It's something I noticed while I was looking at the code. It was a face palm moment really. 

It's very safe really, hence why I thought it could be nominated for beta uplift, despite the lateness. It has nothing to do with the bug at hand.
We probably need a separate bug to uplift only part of this, so we can explain the patch, give it a security rating (or explicitly call it out as not a security bug) so we can write an appropriate advisory, and so it can (maybe) be tested.
Flags: needinfo?(dveditz)
Then again why do that work if it's going to be denied? Maybe just explain the above here, and then create the sub-bug if Ritu will let it into beta.
https://hg.mozilla.org/mozilla-central/rev/4bcd1f78d5cb
https://hg.mozilla.org/mozilla-central/rev/f0b46ebdc828
https://hg.mozilla.org/mozilla-central/rev/ae7d9cd01079
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8800174 [details] [diff] [review]
0002-Bug-1305876-P2.-Do-not-modify-internal-data-when-cal.patch

Jean-Yves emphatically believes this is low risk and I trust him, Beta50+
Attachment #8800174 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Should I uplift these in this bug or are you splitting this off to a sub-bug?
Flags: needinfo?(dveditz)
There's no "these", only patch 2 gets uplifted. We're not creating a sub-bug.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Comment on attachment 8800173 [details] [diff] [review]
0001-Bug-1305876-P1.-Add-diagnostic-assert-to-more-easily.patch

Fix a crash. Take it in 51 aurora.
Attachment #8800173 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

2 years ago
Attachment #8800174 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+]
(Assignee)

Comment 28

2 years ago
This actual crash wasn't uplifted to the various version, only a side bug.

As such, it is still happening in release and beta.
status-firefox50: fixed → wontfix
status-firefox51: fixed → affected
I don't understand. What is affected in 51. What bug is there that was then fixed in 52?
Flags: needinfo?(jyavenard)
Whiteboard: [post-critsmash-triage][adv-main50+] → [post-critsmash-triage][adv-main50+][adv-main52-]
Group: core-security-release
Keywords: sec-moderate → sec-other
(Assignee)

Comment 30

a year ago
(In reply to Al Billings [:abillings] from comment #29)
> I don't understand. What is affected in 51. What bug is there that was then
> fixed in 52?

what was actually fixed was a side problem, with no link to the bug title. data were self-modifying when calling a JS instruction.
Flags: needinfo?(jyavenard)
You need to log in before you can comment on or make changes to this bug.