1367805 - getUserMedia in sandboxed iframe stopped working in FF53 for camera (regression)!

Status: RESOLVED WONTFIX

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

Description

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

E.g. none of my 28 code snippets on stackoverflow.com showcasing getUserMedia and WebRTC work anymore as of Firefox 53 [1][2]. :-(

19:05.44 INFO: Last good revision: 39efc7bfb839484ef121a2f275481770906d9021
19:05.44 INFO: First bad revision: 5230f39d8f21270414aea3cdfb97914fc6acf4c1
19:05.44 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=39efc7bfb839484ef121a2f275481770906d9021&tochange=5230f39d8f21270414aea3cdfb97914fc6acf4c1

STR:
  1. Open https://jsfiddle.net/jib1/thaqyt42/
  2. When Firefox's permission prompt appears, click "Allow" button.

Expected result:
  Prompt appears, click Allow, then the text "Got gum!" and video of myself in the iframe.

Actual result:
  Prompt appears, click Allow, then "NotReadableError: Failed to allocate videosource" and no video.

Workaround: Access camera or microphone in any other tab from any other domain first, then it works.

[1] https://stackoverflow.com/a/35515536/918910
[1] https://stackoverflow.com/search?q=user%3A918910+%22https+fiddle%22+%22Chrome%22

Comment 1

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

By "first" I mean concurrently.

Comment 2

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

Basically we get a null principal in MediaManager::GetUserMedia in this case.

From reading the spec [1] the new behavior seems more correct, we just haven't implemented allowUserMedia [2] yet.

I see a couple of options here:

  A) Detect gUM from sandboxed iframe and kludge back previous behavior, or
  B) Implement allowUserMedia asap.

It's worth noting jsfiddle still works, because it uses allow-same-origin:

  sandbox="allow-forms allow-scripts allow-same-origin allow-modals allow-popups"

while stackoverflow uses:

  sandbox="allow-forms allow-modals allow-scripts"

allow-scripts + allow-same-origin is considered unsafe [3], so I wouldn't expect SO to add it.

They might consider adding allowusermedia however, at least for questions tagged [getusermedia].

[1] https://html.spec.whatwg.org/multipage/browsers.html#sandboxOrigin
[2] https://html.spec.whatwg.org/multipage/embedded-content.html#attr-iframe-allowusermedia
[3] See note under https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#Attributes

Baku, any thoughts on option A?

Comment 3

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

I should add this works in Edge, but not in Chrome. In fact, the STRs here tab-crash Chrome.

Comment 4

[Andrea Marchesini [:baku]]

>   A) Detect gUM from sandboxed iframe and kludge back previous behavior, or

> Baku, any thoughts on option A?

Do you know if chrome is going to implement/support this behavior?
I would like to see option B implemented instead of this work-around.
Do you know how complex would be to implement allowusermedia? It should not take too much, I guess.

Comment 5

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

I would expect Chrome to implement B, not A. Even before stackoverflow went full https, I remember I switching individual question urls to https manually, yet getUserMedia in code snippets still wouldn't work in Chrome (it didn't crash then).

On B, from https://w3c.github.io/mediacapture-main/#dom-mediadevices-getusermedia() <-- don't forget the ()

    "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."

But this only tells us what error to throw.

[1] https://dxr.mozilla.org/mozilla-central/rev/f7adbf457ee20eeffde72694e0d17d73616e3cfd/dom/media/systemservices/CamerasParent.cpp#645-646,663,671

Comment 6

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

Actually, looking closer, I'm not sure one option is easier than the other, as the real obstacle appears to be our e10s permission sandboxing in HasCameraPermission(). [1] It wants to independently verify permissions in a way that cannot be faked easily by a compromised content process.

For normal temporary (non-persistent) permissions, the chrome permission UI (webrtcUI.jsm) code today adds a temporary "MediaManagerVideo" persistent camera permission (to the right origin judging by the STRs), which gets revoked right after, a bit of a kludge.

The trouble is, all this permission code plumbing in HasCameraPermission relies on the principal associated with that origin to be passed in, so it can verify the permission. Instead it gets a nullPrincipal.

Perhaps I should ask Florian what the permission code does here, as it appears to be getting it right, and somehow duplicated what it does.

Comment 7

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

In short, it looks like the webrtcUI.jsm code operates on the origin, not the principal. I suspect we need to plumb up both the origin AND principalInfo to HasCameraPermission to solve this.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late to fix for 53, but we may still be able to take a last minute patch for 54.

Comment 9

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> In short, it looks like the webrtcUI.jsm code operates on the origin, not
> the principal. I suspect we need to plumb up both the origin AND
> principalInfo to HasCameraPermission to solve this.

How difficult would it be to include the requesting principal for getUserMedia requests sent to the UI? It seems this would also help when showing permission prompts for requests from subframes in data: urls.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 1 - allowusermedia attribute

This patch just introduces allowusermedia attribute.
We still need to deal with the principal vs origin in the permission UI.

Comment 11

[Andrea Marchesini [:baku]]

With this patch https://jsfiddle.net/jib1/thaqyt42/ shows a security error because jsfiddle runs the script into an iframe without allowusermedia attribute set.

Comment 12

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

Discussing offline we realized allowusermedia A) has larger compat issues, and B) is likely being removed from the spec [1] in favor of feature policy.

Ideally we'd prefer to break things a little later, like once sites can use feature policy to enable this functionality. Our options seem to be:

 A) Close this as invalid since our new 53 behavior follows what Chrome does, or
 B) Try to kludge in 52 behavior (allow gUM in sandboxed iframes) again until feature policy is implemented.

[1] https://github.com/w3c/mediacapture-main/pull/440

Comment 13

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

Comment on attachment 8873331 [details] [diff] [review]
part 1 - allowusermedia attribute

We're not doing allowusermedia in the end, per comment 12.

Comment 14

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

The plan is to let the current behavior ride for 53, and after that disallow getUserMedia in this case, to match Chrome and spec (bug 1371741).

The patch in bug 1371741 should take care of the remaining issues here which are:

 1. we should fail before prompting, not after (i.e. not prompt).
 2. we should fail mic and screensharing as well.

Comment 15

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

Sorry I meant let the current behavior ride for 54.

Comment 16

[Mike Taylor [:miketaylr]]

Given Comment #14, what's the status of this bug these days Jan-Ivar?

Comment 17

[Bulk Bug Changes for mreavy's org]

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

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for a fix in 57 since we're a week away from release.

Comment 19

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 20

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

Note that comment 0 is a bit confusing. As comment 2 points out, this bug is about sandboxed iframes without allow-same-origin (like stackoverflow and codepen, not jsfiddle).

STR:

1. Open https://jan-ivar.github.io/dummy/iframe_gum_sandbox_isolate.html

Expected result:
NotAllowedError, I think?

With bug 1371741 fixed, this is what I get, and it matches Chrome.

Andrea, is this expected behavior? Is allow-same-origin generally required for allow attributes to work?

I half-expected allow="camera" to make camera work even in an isolated sandbox (though this might pose a unique challenge for our permission prompt: "who's asking?").

Comment 21

[Paul Adenot (:padenot)]

jib, please follow-up and close this is you think we're doing the right thing, maybe pasting here links to the relevant specs. I tried quickly searching but didn't find anything immediately so I stopped.

Comment 22

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

I think the remaining question is, with the availability of the new wildcard form allow="camera *; microphone *" e.g. in https://jan-ivar.github.io/dummy/iframe_gum_sandbox_starcross_isolate.html, if we'd ever consider supporting getUserMedia in a sandboxed iframe without allow-same-origin. Since we're trying to get rid of * in the spec, maybe no.

The answer in bug 1371741 is no. This matches Chrome and Edge. Safari surprisingly supports it, but that doesn't seem like a good idea to me.

Other developments: codepen now appears to use allow-same-origin (see https://codepen.io/jib1/pen/QWwBgMz?editors=1010) so it works fine now.

I'm closing this. If anyone wants this behavior they need to open a new bug.