1395819 - (CVE-2021-29959) Site can turn cam & hw light back on without permission after cam light goes out, if it keeps recording audio

Status: VERIFIED FIXED

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

Description

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

STRs
 1. Open https://jsfiddle.net/jib1/mtLs2phL/2/
 2. When prompted, share camera and mic, but leave "Remember this decision" unchecked.
 3. Wait for self-view to appear (notice hardware camera light turns on).
 4. Wait 3 seconds, notice video disappears and camera hardware light goes out.

Expected result:
 5. After 6 more seconds, site prompts for camera and mic again. 

Actual result:
 5. After 6 more seconds, camera and light come back on without permission.

It's surprising that the site can turn off the camera hardware light and yet turn the camera back on later without permission.

For it to work, the site has to keep recording using the microphone the whole time, so there's a mic recording indicator in the tab the whole time if you noticed.

Still, even for users who notice this, we think they would not have guessed the camera could come back on after the hardware light and camera live indicator have both gone out.

We should fix this to re-prompt, which would be consistent with expectations, the spec [1], and with how gum(cam+mic) works after gum(mic).

This is a regression since 53 (bug 1270572) - Marking as hidden for now.

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-getusermedia() says:

"For the origin identified by originIdentifier, request permission for use of the devices, while considering all devices attached to a live MediaStreamTrack in the current browsing context to have permission status "granted", resulting in a set of provided media." - We think "devices" here refers to cam and mic as separate devices.

Comment 1

[Daniel Veditz [:dveditz]]

We should follow whatever the spec says, but as a user I wouldn't be surprised that if I granted permission to a page that it could turn the camera on and off at will until I leave that page (or reload).

Comment 2

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

Dan - the working group took a stricter view of this; that the potential privacy invasion (snapping photos) was serious if the website could turn your webcam light off *and* the indicator of access we prominently display for the camera turns off.  This is less serious than if all indicators (i.e. mic) went away and then it turned the camera back on.

Note also that a site could cycle a camera on, take an image, and back off again before a user could notice or a camera LED was visible (we don't assume they're 0-delay, and believe some of them may never be visible to normal users if they're turned on and quickly back off).  Perhaps the WG is wrong in assessing this, but that was the concern (given news reports of things like admins remotely turning on cameras to take occasional stills from school laptops that were taken home and in bedrooms, and people using images and audio obtained from people's computers to blackmail them).

Comment 3

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

SourceListner receives EVENT_FINISHED only when all of the tracks have been finished. [1]
Then the call flow will eventually send a recording-device-stopped message to webrtcUI.jsm [2] and withdraw the temporal permission. [3]

If there is only one track stopped, there won't be EVENT_FINISHED fired and so no any further actions.
Since the temporal permission is not withdrawn, the 2nd gUM will be granted unpromptedly.

We need to create another event, e.g., EVENT_TRACK_FINISHED, and modify MediaStreamGraph / SourceListener / GetUserMediaWindowListener to fix this bug.


[1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/dom/media/MediaStreamGraph.cpp#378-389
[2] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/dom/media/MediaManager.cpp#485
[3] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/browser/modules/webrtcUI.jsm#348

Comment 4

[Andreas Pehrson [:pehrsons]]

No need to patch MediaStreamGraph here. There are already signals for tracks ending, but not in the same event.

Simplest is to keep using the MediaStreamListener, but best would be to switch to MediaStreamTrackListener since we'll soonish be able to remove MediaStreamListener completely.

MediaStreamTrackListener has a NotifyEnded method [1] that would be straight forward to use.

But to avoid risky refactoring in a sec bug, let's use MediaStreamListener::NotifyQueuedTrackChanges [2] instead.
Override it in SourceListener and ignore the queued media. Just make a check for when `aTrackEvents` contains `TRACK_EVENT_ENDED` (it's a flag). Then, like in `NotifyEvent`, schedule an update on main thread.


[1] http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/dom/media/MediaStreamListener.h#157
[2] http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/dom/media/MediaStreamListener.h#103
[3] http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/dom/media/MediaStreamTypes.h#25

Comment 5

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

Mass change P1->P2 to align with new Mozilla triage process.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Andreas, is this still an issue?

Comment 7

[Andreas Pehrson [:pehrsons]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> Andreas, is this still an issue?

Yes. No changes have been made to this logic.

Comment 8

[Nils Ohlmeier [:drno]]

Andreas did your recent changes remove the MediaStreamListener like you announced in comment #4? Is this still a problem?

Comment 9

[Andreas Pehrson [:pehrsons]]

I have removed listeners, yes (bug 1423241). This is still a problem, yes.

Perhaps removing stream listeners has unblocked fixing this in permissions code, but I think it could have been done before too.

Permissions handling, that's where this issue really lies.

Comment 10

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

Cause

It turns out we only fire "recording-device-stopped" to front-end when removing an entire SourceListener here. For historical reasons, it may represent a camera + microphone pair, but only when requested as a pair in getUserMedia.

Front-end listens to "recording-device-stopped" to terminate temporary permissions to device + kind based on track liveliness (an empty message.rawID means stop all, otherwise a specific device only). This is the disconnect.

Effect

Unprompted re-access to a camera or mic post-stop from a pair obtained in a joint gUM call, as long as the other half of the pair is kept live.

STRs: In https://jan-ivar.github.io/dummy/gum_stop.html click Start Both! + Stop cams! + Start cams! = no re-prompt. Or, click Start Both! + Stop mics! + Start mics! = no re-prompt.

Fix

Remove the pair special case from SourceListener. It's arbitrary in the modern stack, which allows pairs of unrelated hardware anyway (e.g. BRIO cam + AirPod mic). . Well-tested. Simple fix.

Why fix it?

On first blush, the buggy behavior may seem desirable. Especially since some web conference sites asymmetrically stop camera on facemute to work around crbug 642785, but never stop the mic on micmute (so they can implement the "Are you speaking? you should unmute" feature).

But it's highly unpredictable in practice: Several sites (Meet, WebEx) obtain camera and microphone separately, so it didn't happen there. Even if we got Meet to use a joint prompt, users might change their cams or mics independently anyway using the site's ⚙️ settings page, and then it wouldn't happen.

But even if we made this reliable, it wouldn't work if the site stops both tracks, which some sites (MeetEcho?) appear to do on mute/unmute. We'd end up causing re-prompts in some sites but not others for seemingly comparably functionality.

Use case

But this does highlight a use-case: lurkers in medium-to-large meetings who spend most of their time facemuted and micmuted, only to unmute when called on to ask some question.

A superior fix to address this use case directly would seem to be to significantly increase the grace period considered in bug 1693677 to minutes not seconds, for as long as the page is not navigated (then trim it down to seconds on navigation). I've filed this as bug 1697284.

Comment 11

[Andreas Pehrson [:pehrsons]]

S3 because this is not blocking any functionality per se. It's a bit too permissive.

Comment 12

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

Note, if we go with a 60 minute grace period in bug 1697284, then it should cover up the effects of this bug most of the time.

I resolved a case where this behavior interfered with a test with https://phabricator.services.mozilla.com/D107769.

I think this would still be good to fix, to simplify our model, but it gives us a bit more breathing room perhaps.

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1395819 - Add b-c test cases for removing temporary permissions per-kind by stopping tracks. r?jib!,r?johannh!

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1395819 - Make SourceListeners per-device and rename them DeviceListener to reflect this. r?jib

This patch makes MediaManager notify of recording-device-events as soon as any
gUM-track has ended, as opposed to when all tracks in a gUM-request have ended.

This effectively makes temporary permissions per-track rather than per-request.

Comment 15

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1395819 - Remove unused method MediaEngineSource::Shutdown. r?jib

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1395819 - Remove recording-device-stopped notifications without device info. r?karlt!,r?johannh!,r?#webidl!

Normally when all tracks capturing a certain device for a window have stopped we
notify of "recording-device-stopped" for that window and device.

On navigation of a window, for instance, this notification is fired for all
active devices for that window before firing "recording-window-ended".

Prior to this patch we have also fired a "recording-device-stopped" without
device info as a catch-all just prior to "recording-window-ended". However,
there is no chance in the content process of missing a device and thus needing
the catch-all.

This patch removes device-less "recording-device-stopped" to simplify both code
and protocol.

Comment 17

[Phabricator Automation]

Comment on attachment 9212852 [details]
Bug 1395819 - Remove recording-device-stopped notifications without device info. r?karlt!,r?johannh!,r?#webidl!

Revision D110430 was moved to bug 1699338. Setting attachment 9212852 [details] to obsolete.

Comment 18

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

Attachment: Bug 1395819 - Handle OS permission failures gracefully.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This had landed:

Add b-c test cases for removing temporary permissions per-kind by stopping tracks. r=johannh,jib
https://hg.mozilla.org/mozilla-central/rev/990441f04a86
Make SourceListeners per-device and rename them DeviceListener to reflect this. r=jib
https://hg.mozilla.org/mozilla-central/rev/46083fddeb0d
Remove unused method MediaEngineSource::Shutdown. r=jib
https://hg.mozilla.org/mozilla-central/rev/813c7ce3607b

But got backed out for causing cpp failures in MediaManager.cpp:
https://hg.mozilla.org/mozilla-central/rev/366a7dcd19f4

Failure log: https://treeherder.mozilla.org/logviewer?job_id=335570691&repo=autoland

dom/media/MediaManager.cpp:3966:17: error: use of undeclared identifier 'MediaDeviceKindValues'; did you mean 'dom::MediaDeviceKindValues'?
dom/media/MediaManager.cpp:4020:32: error: use of undeclared identifier 'MediaDeviceKindValues'; did you mean 'dom::MediaDeviceKindValues'?
and more failures

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Add b-c test cases for removing temporary permissions per-kind by stopping tracks. r=johannh,jib
https://hg.mozilla.org/integration/autoland/rev/6e0fb393c96731102fdeec5cd5e60c9ae764509d
Make SourceListeners per-device and rename them DeviceListener to reflect this. r=jib
https://hg.mozilla.org/integration/autoland/rev/f5ef4c4b1c56147eb8f1e4aabdc411005872c704
Remove unused method MediaEngineSource::Shutdown. r=jib
https://hg.mozilla.org/integration/autoland/rev/3abc76c921f7fbc2909f9d05bb916754b80169d6

https://hg.mozilla.org/mozilla-central/rev/6e0fb393c967
https://hg.mozilla.org/mozilla-central/rev/f5ef4c4b1c56
https://hg.mozilla.org/mozilla-central/rev/3abc76c921f7

Comment 21

[Giorgia Nichita, Release Desktop QA]

We managed to reproduce this issue on an affected Nightly build 2021-04-01 using the steps from comment 0, with a minor difference in step 3:

1. Open https://jsfiddle.net/jib1/mtLs2phL/2/
2. When prompted, share camera and mic, but leave "Remember this decision" unchecked.
3. Hardware camera light turns on (Self-view does not appear, the window remains blank).
4. Wait 3 seconds,camera hardware light goes out (not the case for the self-view as it does not appear in step 3).
5. After 6 more seconds, camera and light start without permission.

We also verified this on the latest Nightly, but it occurs exactly as in the steps above. Can you please take a look at this, what should be the expected result?

Comment 22

[Timea Cernea [:tbabos][inactive]]

Hi Giorgia,
Please allow me to steal the NI? and verification of this bug as it is closely related to one of the features QA is testing. I had a talk with Jan-Ivar about this issue as it gets a bit more complicated to test due to Bug 1697284. Thanks for testing it out so far tho, and sorry for this inconvenience!
Setting up NI? for myself to handle this when I can.

Comment 23

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest Firefox Nightly 89.0a1 on MacOS 10.15 and Windows 10 x 64.
Verified by disabling the grace period ( privacy.webrtc.deviceGracePeriodTimeoutMs set to 0)
The permission panel will request access after following the STR provided in Comment 10.

Comment 24

[Frederik Braun [:freddy]]

Attachment: advisory.txt