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

RESOLVED WORKSFORME

Status

()

Core
WebRTC: Audio/Video
P1
normal
Rank:
15
RESOLVED WORKSFORME
2 years ago
2 years ago

People

(Reporter: florian, Assigned: jib)

Tracking

47 Branch
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48 affected, firefox49 affected, firefox50 affected)

Details

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
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?
Blocks: 1177242
Rank: 25
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
Keywords: regression
Priority: -- → P2
Hardware: Unspecified → All
Component: Audio/Video → WebRTC: Audio/Video
(Reporter)

Updated

2 years ago
Blocks: 1284877
Version: unspecified → 47 Branch
(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.
Rank: 25 → 15
Keywords: regression
Priority: P2 → P1
Assignee: nobody → jib
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
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).
Attachment #8782204 - Flags: review?(gpascutto) → review-
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?
Flags: needinfo?(gpascutto)
(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.
Flags: needinfo?(gpascutto)
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.
(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 hidden (mozreview-request)
Attachment #8782204 - Attachment is obsolete: true
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?
Flags: needinfo?(florian)
(Reporter)

Comment 12

2 years ago
mozreview-review
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).
Attachment #8796222 - Flags: review?(florian)
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?
Flags: needinfo?(gpascutto)
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.
Flags: needinfo?(gpascutto)
(Reporter)

Comment 15

2 years ago
(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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(florian)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.