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.

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.

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.

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

dhylands, is Comment 3 correct?

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

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

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.

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.

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.

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.

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

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

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

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

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.

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

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

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

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

I've made the other changes. (As before: mwu for widget/gonk, bent for the ipc stuff.)

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.

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

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

Nope, it should be fine.

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

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

Carrying over r+(bent,mwu).

Blocking bug 988731 wich is 1.3T+

1.3T+ per comment 33

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.

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

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