Closed
Bug 1284680
Opened 9 years ago
Closed 8 years ago
Unable to open a video stream from chrome code (NotReadableError)
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Tracking
()
People
(Reporter: florian, Assigned: jib)
References
Details
Attachments
(1 file, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
Details |
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.
Assignee | ||
Comment 1•9 years ago
|
||
It fails here with allowed = false and aOrigin = "about:webrtc":
https://dxr.mozilla.org/mozilla-central/rev/70e05c6832e831374604ac3ce7433971368dffe0/dom/media/systemservices/CamerasParent.cpp#695
Assignee | ||
Comment 2•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Component: Audio/Video → WebRTC: Audio/Video
Updated•9 years ago
|
Version: unspecified → 47 Branch
Comment 3•9 years ago
|
||
(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.
Updated•9 years ago
|
Assignee: nobody → jib
Comment hidden (mozreview-request) |
Comment 5•8 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-
Assignee | ||
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
(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) |
Assignee | ||
Updated•8 years ago
|
Attachment #8782204 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
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•8 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)
Assignee | ||
Comment 13•8 years ago
|
||
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)
Comment 14•8 years ago
|
||
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•8 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
Closed: 8 years ago
Flags: needinfo?(florian)
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•