976398 - Control B2G camera access without using Unix group permissions

Status: RESOLVED FIXED

(Firefox OS Graveyard :: GonkIntegration, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Currently, accessing the camera on B2G requires that the content process be in the AID_SDCARD_RW group (or equivalent on newer Android versions).  We need to remove this requirement in order to allow non-privileged apps to access the camera if the user chooses to allow that, and it would also help simplify content process security in general.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Sotaro, do you know where in the system the camera permission checks are performed?

I'm told that access to the camera is remoted through the mediaserver process, and we have DeviceStorage for accessing stored images/videos, but I've observed that the Camera app doesn't work if AID_SDCARD_RW isn't in its group list.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, I'd really like to see us fix this for v1.5. This bug is what prevents us from allowing people to write 3rd party camera apps.

Comment 3

[Sotaro Ikeda [:sotaro]]

IIRC, by Bug 910498 fix, we do not need the permission anymore.

The following are related.
http://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#466
http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#305

Comment 4

[Sotaro Ikeda [:sotaro]]

dhylands, is Comment 3 correct?

Comment 5

[Dave Hylands [:dhylands]]

I *think* that comment 3 is correct, but I haven't tested it to verify that its true.

Comment 6

[Sotaro Ikeda [:sotaro]]

It seems that I accidentally remove "1.5?". But from Comment 5, it could be 1.4.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

E/gonkperm(  578): android.permission.CAMERA for pid=701,uid=10701 denied: missing group
W/ServiceManager(  115): Permission failure: android.permission.CAMERA from uid=10701 pid=701
E/CameraService(  115): Permission Denial: can't use the camera pid=701, uid=10701

578 is the b2g main process; 701 is the Camera app; 115 is mediaserver.

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

GonkPermissionService::checkPermission appears to be parsing the child's /proc/<pid>/status to check its groups for AID_SDCARD_RW.  I've patched it to grant CAMERA to any process (I assume this is bad) and then the camera app works even with an empty group list.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

So, if we want a "quick fix" for 1.4, it should suffice for ContentParent to maintain a thread-safe map from pid to ChildPrivileges, which the GonkPermissionService could check instead of /proc.

But, longer-term, we want to move away from using the pid for this, and integrate it properly with the IPC model.

Comment 10

[Sotaro Ikeda [:sotaro]]

If we want to permit camera access from a web content, need to free camera hw when the content comes to background. Other wise, camera app could be blocked by a web content.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> If we want to permit camera access from a web content, need to free camera
> hw when the content comes to background. Other wise, camera app could be
> blocked by a web content.

I am wondering if current WebRTC does not support this.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> If we want to permit camera access from a web content, need to free camera
> hw when the content comes to background. Other wise, camera app could be
> blocked by a web content.

Hmm, this could be complex than I thought at first...

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I'll take this — I'm pretty sure I know what needs to happen, at least for a short-term solution.

Comment 14

[Preeti Raghunath(:Preeti)]

Hema

Please track for Camera and check if this aligns with the LG work.

Comment 15

[Mike Habicher [:mikeh] (high bugzilla latency)]

One thing to keep in mind is that the video recorder (which runs in the Camera app process) writes directly to the SD card using an fd provided by the parent.

Comment 16

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Mike Habicher [:mikeh] from comment #15)
> One thing to keep in mind is that the video recorder (which runs in the
> Camera app process) writes directly to the SD card using an fd provided by
> the parent.

Permissions are checked on open(); having the fd is sufficient to access the file.

Comment 17

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug976398-camera-ungroup-hg0.diff

This patch removes PRIVILEGES_CAMERA (which we added in bug 822510) and instead has the GonkPermissionService send a runnable to the main thread to check app permissions directly.

As a result, camera apps can no longer directly access the SD card, but recording photos and videos to still works.

Try builds: https://tbpl.mozilla.org/?tree=Try&rev=71a3125fa33b

Comment 18

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug976398-camera-ungroup-hg1.diff

Simplified and/or cleaned up in a few places.

There was some discussion on IRC earlier about how best to accomplish this.  While it would be nice if all the structures we needed to deal with here were thread-safe so that we didn't need the runnable/semaphore, they aren't, and it's nontrivial work to make that happen, and it doesn't change what's fundementally happening here in any case.  Also, while it at first seems simpler to have the permissions manager push its data to the GonkPermissionService instead (which is currently how gUM works), we don't have all the hooks we'd need for that yet, and on some level it would wind up meaning that we'd have two copies of the same data that may or may not be in sync.  So I think this is the least bad approach, at least for now, even if it's a little complicated-looking.

There are also questions about The Right Way to give content processes access to the camera/microphone, but I don't think that should block cleaning up this now-unnecessary use of Unix groups.

Comment 19

[Michael Wu [:mwu]]

Comment on attachment 8385123 [details] [diff] [review]
bug976398-camera-ungroup-hg1.diff

Review of attachment 8385123 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good overall, though we need to be somewhat more strict about the permission we're checking. Otherwise, mostly nits.

::: widget/gonk/GonkPermission.cpp
@@ +50,5 @@
> +  {
> +    sem_init(&mWakeup, 0, 0);
> +  }
> +
> +  void Wait(void) {

You should be able to use TEMP_FAILURE_RETRY.

@@ +64,5 @@
> +    }
> +  }
> +
> +public:
> +  static already_AddRefed<GonkPermissionChecker> Inspect(int32_t pid) {

This file puts { on new lines when starting a function.

@@ +65,5 @@
> +  }
> +
> +public:
> +  static already_AddRefed<GonkPermissionChecker> Inspect(int32_t pid) {
> +    nsCOMPtr<GonkPermissionChecker> that = new GonkPermissionChecker(pid);

Use RefPtr. nsCOMPtr is for interfaces, not concrete classes.

@@ +118,5 @@
>    if (nsTArray<PermissionGrant>::NoIndex != mGrantArray.IndexOf(permGrant)) {
>      mGrantArray.RemoveElement(permGrant);
>      return true;
>    }
>  

Right around this point, we should check if we're looking for android.permission.CAMERA and bail out if we're not.

@@ +121,5 @@
>    }
>  
> +  // Camera/audio record permissions are allowed for apps with the
> +  // "camera" permission.
> +  nsCOMPtr<GonkPermissionChecker> checker =

Use RefPtr.

@@ +125,5 @@
> +  nsCOMPtr<GonkPermissionChecker> checker =
> +    GonkPermissionChecker::Inspect(pid);
> +  bool canUseCamera = checker->CanUseCamera();
> +  if (!canUseCamera) {
> +    ALOGE("%s for pid=%d,uid=%d denied: \"camera\" not granted in app manifest",

Mentioning the app manifest is a bit too specific here, since permission grants can also come from prompts.

@@ +134,3 @@
>  
> +NS_IMETHODIMP
> +GonkPermissionChecker::Run()

I think this should be moved to the top of the file, below the class declaration.

@@ +152,2 @@
>    }
> +  if (contentParent == nullptr) {

if (contentParent) {

@@ +157,5 @@
> +  }
> +
> +  // Now iterate its apps...
> +  for (uint32_t i = 0; i < contentParent->ManagedPBrowserParent().Length(); i++) {
> +    nsRefPtr<dom::TabParent> tabParent =

Can we use a plain pointer here?

@@ +160,5 @@
> +  for (uint32_t i = 0; i < contentParent->ManagedPBrowserParent().Length(); i++) {
> +    nsRefPtr<dom::TabParent> tabParent =
> +      static_cast<dom::TabParent*>(contentParent->ManagedPBrowserParent()[i]);
> +    nsCOMPtr<mozIApplication> mozApp = tabParent->GetOwnOrContainingApp();
> +    if (mozApp == nullptr) {

if (mozApp) {

@@ +168,5 @@
> +    // ...and check if any of them has camera access.
> +    bool appCanUseCamera;
> +    nsresult rv = mozApp->HasPermission("camera", &appCanUseCamera);
> +    if (NS_SUCCEEDED(rv)) {
> +      mCanUseCamera = mCanUseCamera || appCanUseCamera;

Just return if we've found it so we don't need to use the logical or here.

Comment 20

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Michael Wu [:mwu] from comment #19)
> @@ +118,5 @@
> >    if (nsTArray<PermissionGrant>::NoIndex != mGrantArray.IndexOf(permGrant)) {
> >      mGrantArray.RemoveElement(permGrant);
> >      return true;
> >    }
> >  
> 
> Right around this point, we should check if we're looking for
> android.permission.CAMERA and bail out if we're not.

The "camera" permission also grants android.permission.RECORD_AUDIO, which is intentional: the camera app uses it when recording video.  I mentioned this in a comment, but should I rename some identifiers (mCanUseCameraAndMicrophone?) to make it more obvious?

Comment 21

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug976398-camera-ungroup-hg2.diff

I've made the other changes.

(As before: mwu for widget/gonk, bent for the ipc stuff.)

Comment 22

[Michael Wu [:mwu]]

Comment on attachment 8388945 [details] [diff] [review]
bug976398-camera-ungroup-hg2.diff

Review of attachment 8388945 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/GonkPermission.cpp
@@ +50,5 @@
> +  {
> +    sem_init(&mWakeup, 0, 0);
> +  }
> +
> +  void Wait(void) {

void in the parameter list here seems weird.. does this make any difference in c++?

@@ +66,5 @@
> +    that->Wait();
> +    return that.forget();
> +  }
> +
> +  bool CanUseCamera() {

Please fix all cases where { isn't on a new line when starting a function.

Comment 23

[Michael Wu [:mwu]]

(In reply to Jed Davis [:jld] from comment #20)
> The "camera" permission also grants android.permission.RECORD_AUDIO, which
> is intentional: the camera app uses it when recording video.  I mentioned
> this in a comment, but should I rename some identifiers
> (mCanUseCameraAndMicrophone?) to make it more obvious?

Ok. Maybe reword the comment a bit? Up to you.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8388945 [details] [diff] [review]
bug976398-camera-ungroup-hg2.diff

Review of attachment 8388945 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but I'd like to see the next patch with this change:

::: widget/gonk/GonkPermission.cpp
@@ +38,5 @@
>  
> +// Checking permissions needs to happen on the main thread, but the
> +// binder callback is called on a special binder thread, so we use
> +// this runnable for that.  It responds with a POSIX semaphore.
> +class GonkPermissionChecker : public nsRunnable {

I believe you're basically duplicating xpcom/threads/SyncRunnable. Let's try to use that to avoid repeating tricky wait code like this.

Basically you'd keep your runnable almost as-is but lose the semaphore stuff and just call SyncRunnable::DispatchToThread(). That would basically replace your Inspect() function.

Comment 25

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #24)
> I believe you're basically duplicating xpcom/threads/SyncRunnable. Let's try
> to use that to avoid repeating tricky wait code like this.
> 
> Basically you'd keep your runnable almost as-is but lose the semaphore stuff
> and just call SyncRunnable::DispatchToThread(). That would basically replace
> your Inspect() function.

The thread that's sending and waiting for this runnable is (I think?) created by the binder library, not using NSPR.  Will that be a problem?

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Nope, it should be fine.

Comment 27

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Michael Wu [:mwu] from comment #22)
> ::: widget/gonk/GonkPermission.cpp
> @@ +50,5 @@
> > +  {
> > +    sem_init(&mWakeup, 0, 0);
> > +  }
> > +
> > +  void Wait(void) {
> 
> void in the parameter list here seems weird.. does this make any difference
> in c++?

It doesn't, I don't think.  C habit.  (Also, this function is being deleted anyway, because of comment 24.)

> @@ +66,5 @@
> > +    that->Wait();
> > +    return that.forget();
> > +  }
> > +
> > +  bool CanUseCamera() {
> 
> Please fix all cases where { isn't on a new line when starting a function.

That looks like the only one that isn't being deleted; fixed.

Comment 28

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug976398-camera-ungroup-hg3.diff

Fixed.  I think.  I wasn't sure if I needed a strong reference to the main thread, but most of the rest of the codebase seems to be doing it.  Manually tested, and the camera app still works and still has an empty supplementary group list.

Also, trying: https://tbpl.mozilla.org/?tree=Try&rev=c0ae0a6ca7db

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8389594 [details] [diff] [review]
bug976398-camera-ungroup-hg3.diff

Review of attachment 8389594 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great, thanks!

::: widget/gonk/GonkPermission.cpp
@@ +51,5 @@
> +public:
> +  static already_AddRefed<GonkPermissionChecker> Inspect(int32_t pid)
> +  {
> +    nsRefPtr<GonkPermissionChecker> that = new GonkPermissionChecker(pid);
> +    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();

Nit: Please add a MOZ_ASSERT here. It's unlikely that this could fail, but since this is running on a non-XPCOM thread I'd rather we know for sure.

@@ +52,5 @@
> +  static already_AddRefed<GonkPermissionChecker> Inspect(int32_t pid)
> +  {
> +    nsRefPtr<GonkPermissionChecker> that = new GonkPermissionChecker(pid);
> +    nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +    SyncRunnable::DispatchToThread(mainThread.get(), that);

Nit: the |.get()| should not be needed.

@@ +65,5 @@
> +  NS_IMETHOD Run();
> +};
> +
> +NS_IMETHODIMP
> +GonkPermissionChecker::Run()

Nit: This whole method could be cleaner if you added a |using namespace mozilla::dom;| inside this function.

@@ +98,5 @@
> +
> +    // ...and check if any of them has camera access.
> +    nsresult rv = mozApp->HasPermission("camera", &mCanUseCamera);
> +    if (NS_SUCCEEDED(rv) && mCanUseCamera) {
> +      return NS_OK;

This is a little weird because you don't unset mCanUseCamera if rv failed (so in theory a bug in HasPermission would fail but set outval to true)... Might want to tidy this up so that you don't rely on the outval if rv ever fails.

Comment 30

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: bug976398-camera-ungroup-hg4.diff

Carrying over r+(bent,mwu).

Comment 31

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

https://hg.mozilla.org/integration/b2g-inbound/rev/acf4ff844653

Comment 32

[Carsten Book [:Tomcat]]

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

Comment 33

[[:fabrice] Fabrice Desré]

Blocking bug 988731 wich is 1.3T+

Comment 34

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/b6e177d51e67

Comment 35

[Joe Cheng [:jcheng] (please needinfo)]

1.3T+ per comment 33

Comment 36

[Vincent Liu[:vliu]]

Synced with tarako build today, the camera can't do video recording. After the tracking, I found video recording issue is caused by merging the path in this bug into 1.3T.

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Let's use a different bug to track the 1.3T regression.

Comment 38

[Joe Cheng [:jcheng] (please needinfo)]

Follow up Bug 989859 - [Tarako][Camera]: Camera video recording doesn't work