Found while investigating bug 1403186. STR: 1 host test.html locally with e.g., `python -m SimpleHTTPServer` 2 `MOZ_LOG=timestamp,MediaManager:5 ./mach run localhost:8000/test.hml` Expected: No gUM permission prompt Actual: gUM permission prompt that asks for permission for audio capture for "mozilla.github.io". Also, clicking "Allow" results in starting audio capture. I have not been able to circumvent the permission prompt. So storing a persistent permission for https://mozilla.github.io still results in a prompt. I have not been able to start video capture this way. There is a guard at [1] which is probably what is catching this. I am not sure whether one can do anything with the audio. It cannot be ruled out. The captured audio stops after a while, presumable because GC collects the track. I have not investigated persisting the capture for longer. [1] http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/dom/media/systemservices/CamerasParent.cpp#640

This adds similar guards to `navigator.mediaDevices.getUserMedia` and `navigator.mediaDevices.enumerateDevices` as are present on `navigator.mozGetUserMedia` and the `navigator.mediaDevices` getter.

Comment on attachment 8915067 [details] [diff] [review] Guard mediaDevices APIs from an invalid window Is there a spec saying to do these checks? If so, what exception does it say to throw, and what actual check does it say to do? I doubt it says to throw NS_ERROR_NOT_AVAILABLE! If there's no spec saying to do this, have you raised a spec issue? The spec should presumably say something here.

Andreas, you've verified this patch fixes the problem? (In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #2) > Is there a spec saying to do these checks? There's no mention of anything like this in the mediacapture spec [1][2]. Note that this situation is the result of fuzzing with window.location.href = newurl; From attachment 8915061 [details]: try { o1 = new XMLHttpRequest({mozAnon: true}) } catch(e) { } try { o2 = window.navigator } catch(e) { } try { o3 = o2.plugins } catch(e) { } try { o4 = o2.mediaDevices } catch(e) { } window.location.href = "https://mozilla.github.io/webrtc-landing/gum_test.html" try { o1.open("T", "",false) } catch(e) { } try { o1.send("") } catch(e) { } try { o4.getUserMedia({audio: true, video: false, fake: false}) } catch(e) { } I don't know enough about how window.location.href works here, but once this run of the event loop has finished, will there even any code left to observe a returned promise from o4.getUserMedia()? > If there's no spec saying to do this, have you raised a spec issue? The spec should presumably say something here. Is this something individual domain specs are expected to handle? Is there a common pattern, or existing spec prose you could point me to as an example? [1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-navigatorusermedia [2] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-navigatorusermedia-getusermedia()

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #3) > will there even any code left to observe a returned promise from o4.getUserMedia()? I mean in the case where this patch would reject with NS_ERROR_NOT_AVAILABLE.

I guess the microtask queue would still run, so nm. Would it be better to return a Promise that never resolves here?

Patch aside, I'm having trouble wrapping my head around what code, if anything, can still be alive from the original test.html page once window.location.href = "https://mozilla.github.io/webrtc-landing/gum_test.html" has gotten far enough that we're seeing that origin in the permission prompt, and users had moused over to click "Accept"? - Is there an vector here for an attacker to record the user?

Also, can we add a test for this?

Why does code have to be alive from the test.html page? You can load a page in a subframe (or window.open it), then grab the relevant objects from it, then navigate the page to a new origin, then call methods on the objects you grabbed, no? > There's no mention of anything like this in the mediacapture spec [1][2]. Ok, that presumably needs fixing. > Is this something individual domain specs are expected to handle? Yes, depending on what "this" is. In general, specs are expected to think about what happens if their APIs are invoked in the context of a document that is not "active" or "fully active" in the sense of https://html.spec.whatwg.org/multipage/browsers.html#active-document and <https://html.spec.whatwg.org/multipage/browsers.html#fully-active>. For many APIs it's not a problem. For some it could be. In any case, it seems like the problem on our end is that the prompt's origin is derived from a WindowProxy somehow, not from a Window. It's entirely possible that the spec simply assumes people won't do that (and indeed, they should not!).

> Ok, that presumably needs fixing. That's _if_ the spec assumes you go through windowproxies at some point. But even if it doesn't. I suspect that having permission prompts coming up for non-fully-active documents is weird and should not happen. Perhaps this should be handled by https://w3c.github.io/permissions/#request-permission-to-use

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #3) > Andreas, you've verified this patch fixes the problem? Yes, as in with the test case the permission prompt no longer appears. Just as getting navigator.mediaDevices for the first time would throw too. FWIW, This is the relevant bit I find in mediacapture-main [1]: > When the getUserMedia() method is called, the User Agent MUST run the following steps: > ... > 6. Run the following steps in parallel: > ... > 4. Let originIdentifier be the [HTML52] top-level browsing context's origin. > > 5. If the current [HTML52] browsing context is a [HTML52] nested browsing context whose origin is different from originIdentifier, let originIdentifier be the result of combining originIdentifier and the current browsing context's origin. > > 6. 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. > ... > If the result of the request is "granted", then for each device that is sourcing the provided media, using the device's deviceId, deviceId, set [[devicesLiveMap]][deviceId] to true, if it isn’t already true, and set the [[devicesAccessibleMap]][deviceId] to true, if it isn’t already true. > > If the result is "denied", jump to the step labeled Permission Failure below. If the user never responds, this algorithm stalls on this step. > > If the user grants permission but a hardware error such as an OS/program/webpage lock prevents access, reject p with a new DOMException object whose name attribute has the value NotReadableError and abort these steps. > > If the result is "granted" but device access fails for any reason other than those listed above, reject p with a new DOMException object whose name attribute has the value AbortError and abort these steps. This is outside my comfort zone, but on a hunch, does it mean anything that "Let originIdentifier be the [HTML52] top-level browsing context's origin." happens in parallel here (e.g., after `window.location.href = "https://mozilla.github.io/webrtc-landing/gum_test.html"`)? [1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediadevices-getusermedia

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #8) > Why does code have to be alive from the test.html page? > > You can load a page in a subframe (or window.open it), then grab the > relevant objects from it, then navigate the page to a new origin, then call > methods on the objects you grabbed, no? Good idea. I can repro that (get the misleading prompt) by hitting "Run" on https://jsfiddle.net/jib1/aLbn85ku/8/ roughly ~1/20th of the time, sometimes much more frequently. But the promise never resolves when I hit "Allow", so maybe no recording risk. More good news, even when I have persistent permission already granted for the good site (webrtc-landing), I still get prompted (even though we haven't implemented bug 1299577 which is step 5 in the prose mentioned in comment 10). In the prompt I still get, if I check the "Remember this decision" box, the prompt UX says "Your connection is not secure...". Not sure how it figured that out since everything is https here, but go principals! Same for camera (s/audio: true/video: true/). > Yes, depending on what "this" is. In general, specs are expected to think > about what happens if their APIs are invoked in the context of a document > that is not "active" or "fully active" in the sense of > https://html.spec.whatwg.org/multipage/browsers.html#active-document and > <https://html.spec.whatwg.org/multipage/browsers.html#fully-active>. For > many APIs it's not a problem. For some it could be. Good point, though from a quick search I see very few specs test for "fully active" on initial invocation. For those that do, I see InvalidStateError, TypeError, SecurityError. I guess we can take our pick? > In any case, it seems like the problem on our end is that the prompt's > origin is derived from a WindowProxy somehow, not from a Window. It's > entirely possible that the spec simply assumes people won't do that (and > indeed, they should not!). How do we tell if it's a proxy? (In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #9) > I suspect that having permission prompts coming up for non-fully-active documents is weird and should not happen. > Perhaps this should be handled by > https://w3c.github.io/permissions/#request-permission-to-use I like that idea. Then we could throw NotAllowedError here.

Cc'ing Florian to see if he thinks comment 9 is an issue that extends to permissions outside of getUserMedia, or it's not if there's precedence on how to handle this.

(In reply to Andreas Pehrson [:pehrsons] from comment #10) > > 6. Run the following steps in parallel: > > ... > > 4. Let originIdentifier be the [HTML52] top-level browsing context's origin. Step 4 should probably be lifted outside of the "in parallel" steps here.

> 4. Let originIdentifier be the [HTML52] top-level browsing context's origin. Oh, boy. 1) A browsing context does not have a concept of "origin". 2) Touching a browsing context in an "in parallel" section is not OK. 3) The top-level browsing context may not have anything to do with the API being invoked, as in the testcases in this bug. Basically, this algorithm needs to capture the origin from a sane place before going parallel. Right now it's not doing any of those things. > does it mean anything that "Let originIdentifier be the [HTML52] top-level browsing > context's origin." happens in parallel Yes, it means that it's bogus spec text (or a threadsafety violation in implementation terms). > But the promise never resolves when I hit "Allow", so maybe no recording risk. Right, behavior of promises in navigated-away-from documents is ... complicated. Also not specified anywhere; the ES spec which defines promises has no concept of navigation. > I guess we can take our pick? Works for me. Just don't use Gecko-internal XPConnect error types like NS_ERROR_NOT_AVAILABLE. ;) > How do we tell if it's a proxy? I'm not sure what you mean. In spec terms, a WindowProxy is the JS object that more or less corresponds to a browsing context, effectively. It's the _only_ JS object for windows exposed to script. It generally forwards access on it to the current Window in the browsing context. A Window is the thing that's actually loaded at any given time. In implementation terms in Firefox, if you're in JS, you have a WindowProxy. If you're in C++, and have a docshell, that's a browsing context. If you have an nsGlobalWindow, it could be an "inner" (spec Window) or an "outer" (spec WindowProxy). Looks to me like the spec quoted above is looking at browsing contexts (though it doesn't define how it does that, exactly). So yes, it's going to get results that may have nothing to do with anything. It needs to not do that unless it's enforcing that those browsing contexts are somehow related to the API being called. If it checks "fully active", that's good enough; then it could examine the origins of the active documents in the relevant browsing contexts.

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #12) > Cc'ing Florian to see if he thinks comment 9 is an issue that extends to > permissions outside of getUserMedia I have not actually verified, but yes I think this would not be specific to getUserMedia.

I've filed https://github.com/w3c/permissions/issues/162 https://github.com/w3c/mediacapture-main/pull/490 https://github.com/w3c/mediacapture-main/pull/491

This should implement the guard per [1]. Let me know if the fully active check is missing something, it's the same as the old mozGetUserMedia one. [1] https://github.com/w3c/mediacapture-main/pull/491/files

Comment on attachment 8918342 [details] [diff] [review] GetUserMedia should reject when document not fully active Sorry, this doesn't work. I was trigger happy here.

This is a MediaStreamError for now, since our OnError() path doesn't support DOMException. We should turn the MediaStreamErrors into DOMExceptions some time in the future.

Comment on attachment 8918778 [details] [diff] [review] GetUserMedia should reject when document not fully active Please don't reinvent nsPIDOMWindowInner::IsCurrentInnerWindow. But also, this is not checking for "fully active". It's checking for "active". Kind of. It's possible for aWindow to not be current but its document to be active (e.g. in a document.open()) case. That's why we have nsIDocument::IsCurrentActiveDocument. And again, that doesn't test "fully active". We don't actually have a convenient way of testing "fully active" right now...

Comment on attachment 8922806 [details] [diff] [review] GetUserMedia should reject when document not fully active The commit message still doesn't match the code, per comment 20. Which one are you actually trying to do?

Best would be to follow the spec and check for fully active. Does this work?

Comment on attachment 8923462 [details] [diff] [review] GetUserMedia should reject when document not fully active >+ auto isFullyActive = [](nsPIDOMWindowInner* win) Is there a good reason for this to be a lambda instead of a static function? >+ if (!win->IsCurrentInnerWindow()) { Again, this is _not_ the right test for the spec concept of "active document". Please use the nsIDocument API I pointed to above. >+ nsCOMPtr<nsIDOMElement> domFrameElement = context->GetFrameElement(); If we know we have an outer, we should really be able to use nsGlobalWindow::GetRealFrameElementOuter. >+ nsIDocument* owner = frameElement->GetOwnerDocument(); frameElement->OwnerDoc(), which is guaranteed to never be null.

Comment on attachment 8923462 [details] [diff] [review] GetUserMedia should reject when document not fully active Review of attachment 8923462 [details] [diff] [review]: ----------------------------------------------------------------- r=me once you get bz's r+ ::: dom/media/MediaManager.cpp @@ +2179,5 @@ > onFailure->OnError(error); > return NS_OK; > } > + > + auto isFullyActive = [](nsPIDOMWindowInner* win) wow, is there really no common helper function for this? @@ +2212,5 @@ > + } > + }; > + if (!isFullyActive(aWindow)) { > + RefPtr<MediaStreamError> error = > + new MediaStreamError(aWindow, NS_LITERAL_STRING("InvalidStateError")); Error looks good.

Hopefully this is it. Manual test is still good.

Comment on attachment 8924930 [details] [diff] [review] GetUserMedia should reject when document not fully active Carries forward r=jib.

Comment on attachment 8924930 [details] [diff] [review] GetUserMedia should reject when document not fully active r=me. Sorry for all this back-and-forth...

AFAICT, this goes back to the Fx42 timeframe? Doesn't seem to meet the uplift criteria for ESR52, but feel free to set the status to affected and request approval if you feel strongly otherwise.