Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Description • 7 years ago

Recent replumbing has highlighted (bug 1367805) that granting getUserMedia permission to origins the security architecture considers "unknown origins" is undesirable.

This bug addresses this by having getUserMedia throw SecurityError on nullprincipals, using step 4 in the mediacapture-main [1] spec's  getUserMedia algorithm [2]:

    "If the current settings object's responsible document is NOT allowed to use the feature indicated by attribute name allowusermedia, return a promise rejected with a DOMException object whose name attribute has the value SecurityError."

This affects camera, mic and screen-share from:

 1. Sandboxed iframes without sandbox="allow-same-origin".
 2. Data and blobs urls without a parent principal (e.g. user-entered in url bar).
 3. Anything else with a nullprincipal (srcdoc?)

This means getUserMedia will stop working in e.g. stackoverflow code snippets, matching Chrome behavior. It will continue to work on sites that use "allow-same-origin", e.g. jsfiddle.net.

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

[2] While the "allowusermedia" iframe property is in jeopardy of being replaced by the feature policy proposal's allow="camera" and allow="microphone", both forms appear to be optional permissions for sites to turn on, i.e. off by default. We look forward to this being solidified in the spec.

	Chris Mills [:cmills]
	Comment 2 • 7 years ago

Sounds like something we ought to mention in the docs when it lands.

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8876848 [details]
Bug 1371741 - Test getUserMedia in sandboxed srcdoc, blob, data and regular iframes.

https://reviewboard.mozilla.org/r/148180/#review152570

::: dom/media/tests/mochitest/test_getUserMedia_permission.html:29
(Diff revision 1)
> +createHTML({ title: "Test getUserMedia in iframes", bug: "1371741" });
> +/**
> +  Tests covering enumerateDevices API and deviceId constraint. Exercise code.
> +*/
> +
> +let once = (t, msg) => new Promise(r => t.addEventListener(msg, r, { once: true }));

Nit: once is only used once, and it is declared at broader scope than necessary.

::: dom/media/tests/mochitest/test_getUserMedia_permission.html:69
(Diff revision 1)
> +  is(await iframeGum({ sandbox: "allow-scripts allow-same-origin" }, iframeSrcdoc),
> +     "success", "gUM works in regular srcdoc iframe");
> +
> +  // Test gUM in sandboxed vs regular blob iframe
> +
> +  var blob = new Blob([iframeSrcdoc.srcdoc], {type : 'text/html'});

Nit: ' -> ", consistant qoutes

::: dom/media/tests/mochitest/test_getUserMedia_permission.html:70
(Diff revision 1)
> +     "success", "gUM works in regular srcdoc iframe");
> +
> +  // Test gUM in sandboxed vs regular blob iframe
> +
> +  var blob = new Blob([iframeSrcdoc.srcdoc], {type : 'text/html'});
> +	let src = URL.createObjectURL(blob);

Nit: indent level

	Nico Grunbaum [:ng, @chew:mozilla.org]
	Comment 5 • 7 years ago
mozreview-review-reply

Comment on attachment 8876848 [details]
Bug 1371741 - Test getUserMedia in sandboxed srcdoc, blob, data and regular iframes.

https://reviewboard.mozilla.org/r/148180/#review152570

Looks good, with a few Nits, and a question. Could you point me to into the spec reguarding the blob test?

	Andrea Marchesini [:baku]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8876224 [details]
Bug 1371741 - Disallow getUserMedia on nullprincipals (sandboxed iframes, top-level data urls).

https://reviewboard.mozilla.org/r/147672/#review152594

::: dom/media/MediaManager.cpp:2172
(Diff revision 1)
>    rv = PrincipalToPrincipalInfo(principal, &principalInfo);
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  
> +  if (principalInfo.type() == ipc::PrincipalInfo::TNullPrincipalInfo) {

I would move this block before PrincipalToPrincipalInfo in this way:

if (principal->GetIsNullPrincipal()) {
  ...

	Florian Quèze [:florian]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8876224 [details]
Bug 1371741 - Disallow getUserMedia on nullprincipals (sandboxed iframes, top-level data urls).

https://reviewboard.mozilla.org/r/147672/#review152808

Removing the flag as I don't feel competent to r+ this, but I see no problem with the general idea here :-).

::: dom/media/MediaManager.cpp:2174
(Diff revision 1)
>      return rv;
>    }
>  
> +  if (principalInfo.type() == ipc::PrincipalInfo::TNullPrincipalInfo) {
> +    RefPtr<MediaStreamError> error =
> +        new MediaStreamError(aWindow,

nit: there's for 4 spaces of indent here instead of 2 in the rest of the file.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 8 • 7 years ago

The obstacle here is we don't want to break file:// urls.

I don't see a clean fix for that that won't require passing up some kind of uri again, as a second argument in addition to the principal.

While I would have loved for the principal to be the unique permission identifier, I suspect we'll need a second argument to solve bug 1389198 as well soon.

	Bulk Bug Changes for mreavy's org
	Comment 9 • 7 years ago

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

	Johann Hofmann [:johannh] Assignee
	Comment 13 • 7 years ago

Jib, I believe there's been a bit of a mix-up on principals on file:// urls. While the origin for those principals serializes to "null", they are not null principals AFAIU and as such are not caught by principal->GetIsNullPrincipal(). This patch solves the nits from your path and should work properly in all case (including working on file:// urls).

Can you please test that? :)

In case this is good, someone needs to clean up the tests in the other patch. Do you want me to take the bug or would you like to finish it up?

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 7 years ago

Note that there are lots of principals, at all sorts of privilege levels which serialize to "null" but are not GetIsNullPrincipal().

	:Gijs (he/him)
	Comment 15 • 6 years ago

What's needed to push this over the line?

	Johann Hofmann [:johannh] Assignee
	Comment 16 • 6 years ago

It's been some time and I'm still happy to take this but I think in this case the ball is in Jan-Ivar's court here. See comment 13, I think my patch does what it's supposed to...

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Reporter
	Comment 17 • 6 years ago

Hi, yes I've been sitting on this for too long. I'm glad to hear from comment 13 that the file API is not affected. I just haven't had cycles to look at this. Johann, if you want to take it that would be great!

My only concern would be breaking something we haven't considered or haven't covered sufficiently with tests. If you have time to verify that and fix up the tests that would be super.

Sorry for holding this up.

	Johann Hofmann [:johannh] Assignee
	Comment 18 • 6 years ago

Assigning to me then but will not get to this before next month.

	Nils Ohlmeier [:drno]
	Comment 19 • 6 years ago

Johann since you assigned this to you and chance to tackle this any time soon?

	Johann Hofmann [:johannh] Assignee
	Comment 25 • 5 years ago

about:blank has a null principal and can't run gUM via our new policy.

	Pulsebot
	Comment 26 • 5 years ago

Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/12ad92939684
Disallow getUserMedia on null principals. r=jib
https://hg.mozilla.org/integration/autoland/rev/10f070a6edc0
Test getUserMedia in sandboxed srcdoc, blob, data and regular iframes. r=jib
https://hg.mozilla.org/integration/autoland/rev/897f95b6d3fc
Don't test gUM + autoplay on about:blank. r=padenot