1177242 - Video device access needs to re-verify UX permissions

Status: RESOLVED FIXED

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

Description

[Gian-Carlo Pascutto [:gcp]]

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

[Maire Reavy [:mreavy]]

Gian-Carlo, I think you are taking this bug yourself.  If you need me to find another owner, let me know.

Comment 2

[Maire Reavy [:mreavy]]

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!

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Nothing has changed wrt to this bug. It requires knowledge of C++ and JS.

Comment 4

[Maire Reavy [:mreavy]]

Do I/we need to find a new owner? When do you believe this needs to be fixed by?

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Needs to be fixed before e10s/sandboxing ships.

Comment 6

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

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

Comment 7

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Verify whether sandboxed Content process has permissions to access the camera/mic

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.

Comment 9

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

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

Comment 10

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95139a37320e

Comment 11

[Gian-Carlo Pascutto [:gcp]]

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

Comment 12

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=670ac7ccf927

Comment 13

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84acd28d51ef

Comment 14

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61aab47c74b8

Comment 15

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27b29c719062

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Attachment: Verify whether sandboxed Content process has permissions to access the camera/mic

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

Comment 17

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

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

Comment 18

[Gian-Carlo Pascutto [:gcp]]

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

Comment 19

[Gian-Carlo Pascutto [:gcp]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5d6c3e00c91