Closed
Bug 1177242
Opened 10 years ago
Closed 9 years ago
Video device access needs to re-verify UX permissions
Categories
(Core :: WebRTC: Audio/Video, defect, P1)
Core
WebRTC: Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: gcp, Assigned: gcp)
References
Details
Attachments
(1 file, 1 obsolete file)
31.87 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
In the case where video camera access/screen sharing is sandboxed, incoming requests to access cameras in the master process need to re-verify against the permissions UI that the device access was actually authorized by the user, as we can no longer guarantee that any approval coming from content-side UI code is benign.
Comment 1•10 years ago
|
||
Gian-Carlo, I think you are taking this bug yourself. If you need me to find another owner, let me know.
Assignee: nobody → gpascutto
backlog: --- → webRTC+
Rank: 15
Priority: -- → P1
Comment 2•9 years ago
|
||
Gian-Carlo - Is this still relevant, or was it fixed along the way? If it is still relevant, what skills/knowledge is needed to resolve it? Do you still plan to deal with this?
Thanks!
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 3•9 years ago
|
||
Nothing has changed wrt to this bug. It requires knowledge of C++ and JS.
Flags: needinfo?(gpascutto)
Comment 4•9 years ago
|
||
Do I/we need to find a new owner? When do you believe this needs to be fixed by?
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 5•9 years ago
|
||
Needs to be fixed before e10s/sandboxing ships.
Flags: needinfo?(gpascutto)
Comment 6•9 years ago
|
||
e10s will likely ship in 45 (it's at least going to beta for a while). Does this need to be resolved for e10s in general, or only for tightening down sandboxing?
Since you didn't indicate above that someone else should take it, where does it sit in your priority queue?
Thanks
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 7•9 years ago
|
||
It's required for sandboxing, not e10s.
(In reply to Randell Jesup [:jesup] from comment #6)
> Since you didn't indicate above that someone else should take it, where does
> it sit in your priority queue?
Needs to be addressed as soon as it's clear which shipping release will have the sandbox enabled. If that's 45 then this is urgent.
Flags: needinfo?(gpascutto)
Assignee | ||
Comment 8•9 years ago
|
||
Per discussion with bholley and billm, a Content process compromise is essentially
equivalent to UXSS, and we (for the forseeable future) can't mitigate this due to
how iframes work (they'd need to be in separate processes too).
This means that even with sandboxing, a compromised process would expose
the camera or microphone to malware sites. That's pretty evil.
So this is what I believe is what we can (and probably should) do for best-effort.
We verify whether the origin that's requesting the camera has permissions to
access the camera or microphone (latter part tbd) at the moment the device is
allocated.
This means that:
- If the user has given persistent permissions to any site, the attacker
must guess the site. This is (unfortunately) likely going to be feasible
in my cases, as he can just try (hangsouts|facebook|discord).
- If the user hasn't given any permissions ever, the attacker will fail.
- If the user has given non-persistent permissions, the attacker must guess
the site that got permission within the same session. This is probably
also feasible, but the exposure is much more limited.
I modified the permissions UI to store the permissions in nsIPermissionManager
even if they're not persistent, and used SESSION lifetimes. It's also possible
to use durations, but specifying one is a bit of a magic number (maximum delay
between the gUM permission being granted and the Camera allocation coming in).
Maybe something like 1 minute?
I'm not sure if grating the SESSION lifetime changes the behavior of our gUM
in a way that's undesirable.
Android UI probably also needs an update, but I want to hear your thoughts on
this approach first.
Attachment #8712694 -
Flags: review?(rjesup)
Comment 9•9 years ago
|
||
Comment on attachment 8712694 [details] [diff] [review]
Verify whether sandboxed Content process has permissions to access the camera/mic
Review of attachment 8712694 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/MediaManager.cpp
@@ +630,5 @@
>
> nsresult VideoDevice::Allocate(const dom::MediaTrackConstraints &aConstraints,
> + const MediaEnginePrefs &aPrefs,
> + const nsACString& aOrigin
> + ) {
nit: put this on the previous line
::: dom/media/systemservices/CamerasParent.cpp
@@ +663,5 @@
> + nsCOMPtr<nsIIOService> ioServ(do_GetIOService());
> + nsCOMPtr<nsIURI> uri;
> + rv = ioServ->NewURI(aOrigin, nullptr, nullptr, getter_AddRefs(uri));
> + if (NS_SUCCEEDED(rv)) {
> + uint32_t video = nsIPermissionManager::UNKNOWN_ACTION;
Some comments explaining what and why, please...
@@ +679,5 @@
> + error = self->mEngines[aCapEngine].mPtrViECapture->AllocateCaptureDevice(
> + unique_id.get(), MediaEngineSource::kMaxUniqueIdLength, numdev);
> + }
> + RefPtr<nsIRunnable> ipc_runnable =
> + media::NewRunnableFrom([self, numdev, error]() -> nsresult {
Ditto
Attachment #8712694 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)
> I modified the permissions UI to store the permissions in
> nsIPermissionManager
> even if they're not persistent, and used SESSION lifetimes. It's also
> possible
> to use durations, but specifying one is a bit of a magic number (maximum
> delay
> between the gUM permission being granted and the Camera allocation coming
> in).
> Maybe something like 1 minute?
>
> I'm not sure if grating the SESSION lifetime changes the behavior of our gUM
> in a way that's undesirable.
So the problem is that this does break our tests, which assume that if you approved gUM on a page, and reload, you get the dialog again. I can't really use time-based permission persistence to fix this either.
I think what I can do is to remove the permission at the moment it is used.
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
The previous patch slightly changed the permission behavior to persists site per session. I was already a little uncomforable with that even though it was r+ed, but it turns out all our tests rely on that not happening. So I went back and made the behavior identical to what it was.
To do this, we need a kind of one-shot permission. I didn't want to modify nsIPermissionManager for those, as so far we're the only user and it may not make much sense outside of our use case. So I implemented it like this: we set EXPIRE_SESSION permissions for our origins (like the original patch), but we *remove* them on use, so they are effectively one-shot permissions.
This means we get the extra security without any change in behavior.
The only thing I don't really like is that all the exceptions (like test prefs, fake sources etc) must be handled in both MediaManager and CamerasParent. Maybe there's some way around that by generating random security tokens and handling those down instead, but then we're duplicating nsIPermissionManager again...
Attachment #8717439 -
Flags: review?(rjesup)
Assignee | ||
Updated•9 years ago
|
Attachment #8712694 -
Attachment is obsolete: true
Comment 17•9 years ago
|
||
Comment on attachment 8717439 [details] [diff] [review]
Verify whether sandboxed Content process has permissions to access the camera/mic
Review of attachment 8717439 [details] [diff] [review]:
-----------------------------------------------------------------
If that case isn't tested, add a patch to do that I think.
::: browser/modules/webrtcUI.jsm
@@ +381,5 @@
> + // Session approval given but never used to allocate a camera, remove
> + // and ask again
> + if (camPerm && !camPermanentPerm) {
> + perms.remove(uri, "camera");
> + camPerm = perms.UNKNOWN_ACTION;
Do the tests test this case? I think it's important to make sure this path works properly.
::: dom/media/systemservices/CamerasParent.cpp
@@ +683,5 @@
> + nsCOMPtr<nsIPrincipal> principal;
> + rv = GetPrincipalFromOrigin(aOrigin, getter_AddRefs(principal));
> + if (NS_SUCCEEDED(rv)) {
> + uint32_t video = nsIPermissionManager::UNKNOWN_ACTION;
> + rv = mgr->TestExactPermissionFromPrincipal(principal, "camera", &video);
Perhaps make "camera" a define/static somewhere
@@ +697,5 @@
> + }
> + }
> + // Session permissions are removed after one use.
> + if (allowed && !permanent) {
> + mgr->RemoveFromPrincipal(principal, "camera");
Is there any chance a session permission could be set up but not immediately used, and then leveraged by something later?
@@ +713,3 @@
> {
> LOG((__PRETTY_FUNCTION__));
> + LOG(("Verifying permissions for %s", aOrigin.get()));
Personal preference would be for
LOG((__PRETTY_FUNCTION__: Verifying ....
up to you
Attachment #8717439 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #17)
> ::: browser/modules/webrtcUI.jsm
> @@ +381,5 @@
> > + // Session approval given but never used to allocate a camera, remove
> > + // and ask again
> > + if (camPerm && !camPermanentPerm) {
> > + perms.remove(uri, "camera");
> > + camPerm = perms.UNKNOWN_ACTION;
>
> Do the tests test this case? I think it's important to make sure this path
> works properly.
browser/base/content/test/general/browser_devices_get_user_media.js
covers this. Any test that pops up the same request multiple times in the same session without using the permission to actually do anything will trigger it.
> @@ +697,5 @@
> > + }
> > + }
> > + // Session permissions are removed after one use.
> > + if (allowed && !permanent) {
> > + mgr->RemoveFromPrincipal(principal, "camera");
>
> Is there any chance a session permission could be set up but not immediately
> used, and then leveraged by something later?
Well, the first part can obviously happen, see the test case above. But it's not clear to me, when you've previously popped up a gUM dialog, got permission, and then reloaded the page without starting the stream (triggering this case), how you're going to actually start the stream (and hence the "leftover" permission), since you don't have a handle on the mediastream after the reload. So you need to gUM again, and that triggers the first code above.
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d6c3e00c91dd0595a12b145c66cf4f2a890591
Bug 1177242 - Verify whether sandboxed Content process has permissions to access the camera/mic. r=jesup
Comment 20•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•