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

ASSIGNED
Assigned to

Status

()

P2
normal
Rank:
15
ASSIGNED
a year ago
25 days ago

People

(Reporter: jib, Assigned: johannh)

Tracking

({dev-doc-needed, stale-bug})

Trunk
dev-doc-needed, stale-bug
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

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: → bug 1367805
Comment hidden (mozreview-request)
Sounds like something we ought to mention in the docs when it lands.
Keywords: dev-doc-needed
Rank: 15
Priority: -- → P1
Comment hidden (mozreview-request)
Attachment #8876224 - Flags: review?(florian)

Comment 4

a year 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

Comment 5

a year 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?
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 7

a year 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.
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

9 months 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?
Note that there are lots of principals, at all sorts of privilege levels which serialize to "null" but are not GetIsNullPrincipal().
(Assignee)

Updated

6 months ago
See Also: → bug 1438781
What's needed to push this over the line?
Flags: needinfo?(jhofmann)
(Assignee)

Comment 16

28 days 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...
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)
(Assignee)

Comment 18

25 days ago
Assigning to me then but will not get to this before next month.
Assignee: jib → jhofmann
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.