1298107 - Invalid array access in MediaEngineCameraVideoSource::GetCapability()

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-be7e0580-8f63-4352-9b20-0f72c2160823.
=============================================================

There's an assert against this, but it looks like we at least sometimes pass an invalid index. In this report: ElementAt(aIndex = 1, aLength = 1)

Comment 1

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

In addition to finding the root cause, we could also MOZ_RELEASE_ASSERT, or simply "if (index >= array.Length()) { ... }"

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Randell Jesup [:jesup] from comment #1)
> In addition to finding the root cause, we could also MOZ_RELEASE_ASSERT, or
> simply "if (index >= array.Length()) { ... }"

This issue was discovered and reported because nsTArray's ElementAt now does MOZ_RELEASE_ASSERT. Because of this this bug isn't exploitable in Aurora or Nightly (where the nsTArray release assert patch has landed), but is still potentially exploitable in Release, ESR, and Beta.

Comment 4

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

Any ideas?  It seems extremely odd... I can't yet see how it's even possible.

Comment 5

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

https://crash-stats.mozilla.com/report/index/aa05aecf-2e70-48db-9b16-d24ff2161006 is especially confusing.  the invalid index and the number of entries in the array are 8 (most of the crashes are 1 or sometimes 2) .... we have the call to ::GetCapability() in an "if (!mHardcodedCapabilities.IsEmpty())", so we must have entries in the array.   That can only happen if mozilla::camera::GetChildAndCall(&CamerasChild:NumberOfCapabilities) returns no entries - and then normally only on Mac.  Screensharing will append a single item to the mHardCodedCapabilities,and so in theory could be involved.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I don't have a clear case where it can happen, though it may involve adding or removing a camera.

The main issue I think is that GetCapabilities considers mHardcodedCapabilities as the first thing, whereas NumCapabilities considers mHardcodedCapabilities as a last thing. They should agree. The other thing is that a default hardcoded capability should be added based on type, not on coming up empty, and finally I should remove the hardcoded capabilities on OSX where they're no longer needed.

I'll add a minimal patch to clamp the value to the last entry in the array, for uplift, and a second patch to fix up the logic.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Remove hardcoded capabilities on OSX that are no longer used.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Remove hardcoded capabilities on OSX that are no longer used. (2)

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The NumCapabilities code before this patch is susceptible to GetChildAndCall(&CamerasChild::NumberOfCapabilities, ...) failing (returning -1, e.g. from [1] in theory), or returning 0 capabilities, and - having no way to fail itself, and by design - invents one mHardcodedCapabilities entry (sometimes several) when that happens.

The way GetCapabilities is written, if future calls to GetChildAndCall(&CamerasChild::NumberOfCapabilities, ...) were to subsequently succeed with a real number > 0 (I don't know what situation would make it do that, gcp?), it would try to access mHardcodedCapabilities with that number, the problem.

This patch closes that vulnerability now that mHardcodedCapabilities isn't used for anything else in this class (OSX has capabilities through AVFoundation now since bug 1180725).

[1] https://dxr.mozilla.org/mozilla-central/rev/4b9944879c9a60a9aba4a744a7401bc38e0f39c4/dom/media/systemservices/CamerasChild.h#135

Comment 10

[Gian-Carlo Pascutto [:gcp]]

>I don't know what situation would make it do that, gcp?

No idea. The comment suggest it may fail at shutdown, which would preclude the second part of your failure scenario.

Comment 11

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

Comment on attachment 8798944 [details] [diff] [review]
Remove hardcoded capabilities on OSX that are no longer used. (2)

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

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp
@@ +48,5 @@
>  MediaEngineCameraVideoSource::GetCapability(size_t aIndex,
>                                              webrtc::CaptureCapability& aOut) const
>  {
>    MOZ_ASSERT(aIndex < mHardcodedCapabilities.Length());
> +  aOut = mHardcodedCapabilities.SafeElementAt(aIndex, webrtc::CaptureCapability());

This should make it safe; it doesn't really avoid the cause of the bad index however.  I presume this is the "uplift" version of the patch?

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This patch fixes the "cause" inasmuch as it corrects it so an index from CamerasParent is never applied to the mHardCodedCapabilities array, which is the direct cause of the overflow.

We could uplift a smaller version, basically just the SafeElementAt change if we want, which would be the minimum patch, but I recommend uplifting the patch as-is.

Without understanding the situations in which this could happen better, I don't have any additional patch planned atm.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d62eaef0b188&selectedJob=28808827

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Oh, this apparently landed under bug 1304597.

https://hg.mozilla.org/mozilla-central/rev/29c1b972c04d

Comment 16

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

Comment on attachment 8798944 [details] [diff] [review]
Remove hardcoded capabilities on OSX that are no longer used. (2)

Approval Request Comment
[Feature/regressing bug #]: Capability code (some time ago)

[User impact if declined]: Crashes (RELEASE_ASSERTs) in 50 and newer, moderate sec issue in 49 and ESR45 (read access pass end of array) - not requesting uplift to those as this is a moderate.

[Describe test coverage new/current, TreeHerder]: None since this requires plugging and unplugging devices during a run.

[Risks and why]: virtually no risk - removes some no-longer-needed code for mac, and makes the array access safe.

[String/UUID change made/needed]: none

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8798944 [details] [diff] [review]
Remove hardcoded capabilities on OSX that are no longer used. (2)

Sec-mod, Aurora51+, Beta50+

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f69ada08e16
https://hg.mozilla.org/releases/mozilla-beta/rev/3a88afcbfdc4