1284680 - Unable to open a video stream from chrome code (NotReadableError)

Status: RESOLVED WORKSFORME

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

Description

[Florian Quèze [:florian]]

Steps to reproduce:
1. Load about:webrtc
2. Tools -> Web Developer -> Web Console
3. Paste this line and press enter:
navigator.mediaDevices.getUserMedia({ video: true }).then(stream => console.log(stream), e => console.error(e));

Expected result: a media stream

Actual result: MediaStreamError { name: "NotReadableError", message: "Failed to allocate videosource", constraint: "", stack: "" }

gcp, jib hinted on IRC that this could be due to sandboxing code you added.

Comment 1

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

It fails here with allowed = false and aOrigin = "about:webrtc":

https://dxr.mozilla.org/mozilla-central/rev/70e05c6832e831374604ac3ce7433971368dffe0/dom/media/systemservices/CamerasParent.cpp#695

Comment 2

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

I don't know if there are any extensions using getUserMedia directly, but they may be broken by this, given that they have no origin. Given that it is in release and we haven't heard anything, maybe not?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> I don't know if there are any extensions using getUserMedia directly, but
> they may be broken by this, given that they have no origin. Given that it is
> in release and we haven't heard anything, maybe not?

No extensions (other than loop, which is special-cased) are using getUserMedia to my knowledge.  Removing regression due to that (in practice it isn't).

As this blocks bug 1284877, bumping priority.

Comment 4

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

Attachment: Bug 1284680 - Skip origin-checks for browser-chrome code so it may call getUserMedia again.

Review commit: https://reviewboard.mozilla.org/r/72394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72394/

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8782204 [details]
Bug 1284680 - Skip origin-checks for browser-chrome code so it may call getUserMedia again.

https://reviewboard.mozilla.org/r/72394/#review70236

If I find a content exploit, I can now bypass the protection by simply zeroing out the origin in my gUM IPC calls and I'll be granted camera access.

The whole point of this check is that a compromised content process must guess an origin that has persistent camera permissions (and is SOL if there aren't any).

Comment 6

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

How different is that from today? An attacker may put in "talky.io", "appear.in", "tokbox.com", or "apprtc.appspot.com" as easily as "" (or keep going down media.getusermedia.screensharing.allowed_domains until something works).

An unverified origin string is security by very little obscurity. What would be satisfactory? A key?

Comment 7

[Gian-Carlo Pascutto [:gcp]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #6)
> or keep going down media.getusermedia.screensharing.allowed_domains until something
> works).

Nothing will work if the user hasn't given camera permission in the UI. That's what the access control checks, and that permission is per origin. You can fake the origin, but only if you know what it is.

>What would be satisfactory? A key?

That works too (in particular, it can solve your use case where there is a persistent permission not tied to an origin). 

But if an origin has persistent permission, you're always SOL, even with keys.

Comment 8

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

In IRC we arrived at a solution where the calling chrome-code sets up a temporary guid-based fake origin with permission granted, uses that, and removes it afterwards.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> and removes it afterwards.

This isn't necessary: if it's a session permission, it will automatically be removed after use. 

See for example https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/browser/modules/webrtcUI.jsm#531 and https://dxr.mozilla.org/mozilla-central/rev/97a52326b06a07930216ebefa5af333271578904/dom/media/systemservices/CamerasParent.cpp#666

Comment 10

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

Attachment: Bug 1284680 - add a temporary permission for chrome code in webrtcUI.jsm

Review commit: https://reviewboard.mozilla.org/r/82124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82124/

Comment 11

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

So after re-reading comment 9 I realized we don't need to add platform code for this at all. This worked for me when applied on top of the patch in bug 1284877.

Florian, wdyt?

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8796222 [details]
Bug 1284680 - add a temporary permission for chrome code in webrtcUI.jsm

https://reviewboard.mozilla.org/r/82124/#review80974

Adding permissions on chrome:// URIs feels wrong, as the nsIPermissionManager interface has several comments saying system principals will always have permissions granted and adding/removing permissions for them will be a no-op. But on the other hand... this is a really simple hack that nicely unblocks things, so I'm really tempted to take it (and will include it in my next version of the patch for bug 1284877 unless a new idea emerges by then).

Comment 13

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

FWIW if we went the platform route we would basically do the same trick, except inside getUserMedia, and for all chrome calls, so this seemed narrower, and webrtcUI.jsm was already doing a similar "temporary session permission" as gcp pointed out in comment 9. gcp, WDYT?

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Might want to check if nsIPermissionManager truly doesn't apply anything special to this case. I think it doesn't or this code wouldn't work, but I'm far from certain.

Bonus points if the URL that's being added isn't guessable easily (on the other hand it's a session permission after user interaction, so not a big "hole").

Also the initial small abuse of using nsIPermissionManager for "Cameras" permissions has been slippery sloping into Domestic Violence territory. I'll give y'all a pass on this one but for the next patch I'm calling the cops and you'll get restraining orders.

Comment 15

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #11)
> So after re-reading comment 9 I realized we don't need to add platform code
> for this at all. This worked for me when applied on top of the patch in bug
> 1284877.
> 
> Florian, wdyt?

Let's go with this hack. Closing the bug, as nothing needs to be done here anymore.