Closed Bug 1371741 Opened 3 years ago Closed 10 months ago

Disallow getUserMedia on nullprincipals (sandboxed iframes, top-level data urls).

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jib, Assigned: johannh)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files, 3 obsolete files)

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.
See Also: → 1367805
Sounds like something we ought to mention in the docs when it lands.
Keywords: dev-doc-needed
Rank: 15
Priority: -- → P1
Attachment #8876224 - Flags: review?(florian)
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
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?
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()) {
  ...
Attachment #8876224 - Flags: review?(amarchesini) → 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.
Attachment #8876224 - Flags: review?(florian)
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.
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
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?
Note that there are lots of principals, at all sorts of privilege levels which serialize to "null" but are not GetIsNullPrincipal().
See Also: → 1438781
What's needed to push this over the line?
Flags: needinfo?(jhofmann)
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...
Flags: needinfo?(jhofmann) → needinfo?(jib)
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.
Flags: needinfo?(jib)
Assigning to me then but will not get to this before next month.
Assignee: jib → jhofmann
Status: NEW → ASSIGNED
Johann since you assigned this to you and chance to tackle this any time soon?
Flags: needinfo?(jhofmann)

Yup, got new patches up soon.

Flags: needinfo?(jhofmann)

Let me know when you have a new patch up, or I can take this if you want.

Note that there have been some changes in this area, both in our code and the spec. e.g. the spec no longer mentions throwing SecurityError anywhere.

We should probably reject with NotAllowedError (and the adjacent SecurityErrors should probably either be NotAllowedError or InvalidStateError now according to the spec, not sure which one to pick since they're a bit edge-casy) [1].

Attachment #8932076 - Attachment is obsolete: true
Attachment #8932076 - Flags: review?(jib)
Attachment #8876848 - Attachment is obsolete: true
Attachment #8876848 - Flags: review?(na-g)
Attachment #8876224 - Attachment is obsolete: true
Attachment #8876224 - Flags: review+

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #21)

Let me know when you have a new patch up, or I can take this if you want.

I submitted the updated patches for your review. The test changes mostly cover that data: iframes are null principals now, afaik and I also had to make the test a little more robust (e.g. stopping tracks) to avoid intermittently thrown AbortErrors in my testing.

Note that there have been some changes in this area, both in our code and the spec. e.g. the spec no longer mentions throwing SecurityError anywhere.

We should probably reject with NotAllowedError (and the adjacent SecurityErrors should probably either be NotAllowedError or InvalidStateError now according to the spec, not sure which one to pick since they're a bit edge-casy) [1].

Let's do that in a new bug. :)

about:blank has a null principal and can't run gUM via our new policy.
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
Depends on: 1519415
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Do we need a site compat doc for this? Given that Bug 1367805 was not fixed, getUserMedia() on sandboxed iframes has been broken for a while anyway?

Flags: needinfo?(jhofmann)

(In reply to Kohei Yoshino [:kohei] (Bugzilla UX) (FxSiteCompat) from comment #28)

Do we need a site compat doc for this? Given that Bug 1367805 was not fixed, getUserMedia() on sandboxed iframes has been broken for a while anyway?

Not sure, honestly. Jib, what do you think?

Flags: needinfo?(jhofmann) → needinfo?(jib)

Microphone by itself used to work, so a site compat doc might be good.

https://jan-ivar.github.io/dummy/iframe_gum_sandbox_isolate.html hasn't worked since 52.
https://jan-ivar.github.io/dummy/iframe_gum_audio_sandbox_isolate.html worked until 66.

Flags: needinfo?(jib)
Blocks: 1530917

Documentation updates

BCD updates

You need to log in before you can comment on or make changes to this bug.