q1 Reporter
	Description • 8 years ago

MediaStreamGraphImpl::UnregisterCaptureStreamForWindow (dom\media\MediaStreamGraph.cpp) incorrectly indexes an array, potentially causing it to leave in place an element that it should have deleted.

3820: void
3821: MediaStreamGraphImpl::UnregisterCaptureStreamForWindow(uint64_t aWindowId)
3822: {
3823:   MOZ_ASSERT(NS_IsMainThread());
3824:   for (uint32_t i = 0; i < mWindowCaptureStreams.Length(); i++) {
3825:     if (mWindowCaptureStreams[i].mWindowId == aWindowId) {

The bug is that the following line removes element |i| (e.g., element 3), which causes element |i+1| (e.g., element 4) (if it exists) to be shifted down to element |i| (e.g., element 3). However, when control returns to line 3824, above, |i| is incremented (e.g., to 4), so the next time line 3825 examines an element of mWindowCaptureStreams, it doesn't test the first element that was shifted down (e.g., 3), and so line 3826 doesn't remove it if it should be removed.

3826:       mWindowCaptureStreams.RemoveElementAt(i);
3827:     }
3828:   }
3829: }

I have marked this as a security bug because I'm unsure whether it might allow, e.g., a site opened in a window that previously hosted a different site, to then use a stream object from the previous site.

	Paul Adenot (:padenot) Assignee
	Comment 1 • 8 years ago

This is not enabled on any build and any platform. I'll write a fix soon, though.

	Paul Adenot (:padenot) Assignee
	Comment 3 • 8 years ago

Al, what is the policy for landing a fix like this, in code that is only reachable if you flip a pref, and has never been shipped ?

	q1 Reporter
	Comment 4 • 8 years ago

(In reply to Paul Adenot (:padenot) from comment #2)
> Created attachment 8793666 [details] [diff] [review]
> Iterate backward

The patch invokes undefined behavior by underflowing |i| on the last iteration; see C++11 s.5(4). Probably this causes an infinite loop on all real platforms...

> for (uint32_t i = mWindowCaptureStreams.Length() - 1; i >= 0; i--) {

...because |i| is unsigned, it always satisfies the loop-control condition |i >= 0|.

	Paul Adenot (:padenot) Assignee
	Comment 5 • 8 years ago

True, fix coming up. That's what you get for submitting patches without running the unit tests :-).

	q1 Reporter
	Comment 6 • 8 years ago

One nice form for this kind of loop is:

   for (auto i = array.Length(); // use right type for |i|
             i > 0;) {

      --i;
      ...
      array.RemoveElementAt (i);
      ...
   }

	Andrew McCreight [:mccr8]
	Comment 7 • 8 years ago

I'll just mark this sec-other because we don't ship this anywhere.

	Andrew McCreight [:mccr8]
	Comment 8 • 8 years ago

It could probably also be unhidden if you prefer.

	Al Billings [:abillings - ex-MoCo]
	Comment 9 • 8 years ago

(In reply to Paul Adenot (:padenot) from comment #3)
> Al, what is the policy for landing a fix like this, in code that is only
> reachable if you flip a pref, and has never been shipped ?

The pref thing is an interesting wrinkle. Generally, if the bug is on more branches than just trunk, it needs sec-approval *if* it is rated sec-high or sec-critical. This is functionally disabled though. Realistically, it is probably best to be paranoid and if it had that rating, ask for sec-approval if the code lives on multiple branches. The idea then is that we may want to backport it to Aurora and Beta, for example.

	Paul Adenot (:padenot) Assignee
	Comment 10 • 8 years ago

Fixed.

	Paul Adenot (:padenot) Assignee
	Comment 11 • 8 years ago

Comment on attachment 8796124 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? This could mean cross origin audio data leakage. This code is behind a pref (disabled by default on all releases), and uses a non-standard Web API.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Kind of, although because the feature is non-standard, it's a bit hard to understand the implications.

Which older supported branches are affected by this flaw?
All older branches have the code, but it's disabled everywhere.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply cleanly to all branches.
How likely is this patch to cause regressions; how much testing does it need?

	q1 Reporter
	Comment 12 • 8 years ago

FWIW:

>  for (int32_t i = mWindowCaptureStreams.Length() - 1; i >= 0; i--) {

...implicitly casts the result of nsTArray::Length() (which currently isa size_t; see class nsTArray_base) to int. Technically this truncates on both 32 and 64 bit platforms. Currently this doesn't matter because the maximum number of elements in an nsTArray is limited to 2^31-1 (see struct nsTArrayHeader). If anyone raises that limit, this loop might no longer work properly.

	Paul Adenot (:padenot) Assignee
	Comment 13 • 8 years ago

That's fine, you would OOM long before 2^31-1 capture stream, because that means you would have 2^31-1 HTMLMediaElement or AudioContext playing.

	Al Billings [:abillings - ex-MoCo]
	Comment 14 • 8 years ago

Comment on attachment 8796124 [details] [diff] [review]
patch

Check it in!

	Phil Ringnalda (:philor)
	Comment 15 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/1858c9bbbc83

	Daniel Veditz [:dveditz]
	Comment 16 • 8 years ago

Why is this code behind a pref? Is it just for debugging? Something we abandoned and are going to remove? Or is it a potential future-feature?

	Paul Adenot (:padenot) Assignee
	Comment 17 • 8 years ago

Yes, a potential future feature that got de-prioritized for now.