849330 - camera recording needs AID_SDCARD_RW capability

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #803471 +++

By applying Bug 803471, app process do not need AID_MEDIA/AID_CAMERA permission. Then unprivileged app could access to camera and codec hw. But camra recording still request to an app process to have AID_SDCARD_RW capability.

Comment 1

[Sotaro Ikeda [:sotaro]]

nsGonkCameraControl::StartRecordingImpl() create a recording file by calling open(). It requests AID_SDCARD_RW.

http://mxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#893

Comment 2

[Sotaro Ikeda [:sotaro]]

Permissions are assigned to a privileged process in following.

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#305

Comment 3

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

So bug 803471 introduced a regression?

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Mike Habicher [:mikeh] from comment #3)
> So bug 803471 introduced a regression?

This bug is not about a regression. AID_SDCARD_RW is already necessary current Firefox OS 1.0. Bug 803471 removes all necessary privilege except AID_SDCARD_RW.

Comment 5

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

I see.  So this isn't really a bug then.

Comment 6

[Keven Kuo [:kkuo]]

Assign to Vincent Liu
--
Keven

Comment 7

[Vincent Liu[:vliu]]

Attachment: Patch file for bug-849330

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> (In reply to Mike Habicher [:mikeh] from comment #3)
> > So bug 803471 introduced a regression?
> 
> This bug is not about a regression. AID_SDCARD_RW is already necessary
> current Firefox OS 1.0. Bug 803471 removes all necessary privilege except
> AID_SDCARD_RW.

Hi Sotaro,

Patch was attached removing all necessary privilege except AID_SDCARD_RW. Can you please review it for me? Thanks.

Comment 8

[Sotaro Ikeda [:sotaro]]

Thanks for writing the patch. I am not a correct person to review. cjones and dhylands are correct people to review the patch.

I have comments to the patch.

- PRIVILEGES_VIDEO is not necessary. It can be removed.
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_linux.cc#312

- "deprecated-hwvideo" can be removed from PrivilegesForApp(mozIApplication* aApp)
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Thanks for writing the patch. I am not a correct person to review. cjones
> and dhylands are correct people to review the patch.
> 
> I have comments to the patch.
> 
> - PRIVILEGES_VIDEO is not necessary. It can be removed.
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> process_util_linux.cc#312
> 
> - "deprecated-hwvideo" can be removed from PrivilegesForApp(mozIApplication*
> aApp)
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp

Bug 803471 moves camera hw and hw codec from app process to media server process. By the change, unprivileged app processes can use them.

Comment 10

[Dave Hylands [:dhylands]]

Taking away AID_AUDIO and AID_CAMERA will just restrict apps needing PRIVLEGES_CAMERA, it won't add anything to unprivileged apps (which agrees with comment 9).

Having said that, we should keep things as restrictive as possible.

Which process plays the shutter noise?

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Dave Hylands [:dhylands] from comment #10)
> Taking away AID_AUDIO and AID_CAMERA will just restrict apps needing
> PRIVLEGES_CAMERA, it won't add anything to unprivileged apps (which agrees
> with comment 9).
> 
> Having said that, we should keep things as restrictive as possible.
> 
> Which process plays the shutter noise?

Actual audio output to audio hw is done by AudioFlinger in mediaserver process. Camera app start to play shutter sound. Data of the sound is delivered to AudioFlinger via AudioTrack in sydney audio.

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L676

http://mxr.mozilla.org/mozilla-central/source/media/libsydneyaudio/src/sydney_audio_gonk.cpp#237

Comment 12

[Dave Hylands [:dhylands]]

Comment on attachment 723828 [details] [diff] [review]
Patch file for bug-849330

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

Looks good to me. I tested camera and video and both seem to work.

Comment 13

[Dave Hylands [:dhylands]]

I tested using gaia/master against mozilla-inbound on my unagi.

Comment 14

[Vincent Liu[:vliu]]

Attachment: patch_v2

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Thanks for writing the patch. I am not a correct person to review. cjones
> and dhylands are correct people to review the patch.
> 
> I have comments to the patch.
> 
> - PRIVILEGES_VIDEO is not necessary. It can be removed.
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/
> process_util_linux.cc#312
> 
> - "deprecated-hwvideo" can be removed from PrivilegesForApp(mozIApplication*
> aApp)
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp

Thanks for your comments. Created v2 patch combined the previous one. 

Hi dhylands,

Can you please review for me?  Thanks.

Comment 15

[Dave Hylands [:dhylands]]

Comment on attachment 724295 [details] [diff] [review]
patch_v2

Shouldn't we also remove PRIVLEGES_VIDEO from process_util.h and process_util_linux.cc?

And this wasn't v2 of the patch, it was Part 2 (v2 would have obsoleted an earlier version).

Comment 16

[Vincent Liu[:vliu]]

Attachment: Patch_part2

(In reply to Dave Hylands [:dhylands] from comment #15)
> Comment on attachment 724295 [details] [diff] [review]
> patch_v2
> 
> Shouldn't we also remove PRIVLEGES_VIDEO from process_util.h and
> process_util_linux.cc?
> 
> And this wasn't v2 of the patch, it was Part 2 (v2 would have obsoleted an
> earlier version).

Thanks for the kindly remind of my missing part. The part 2 of the patch was attached.

Comment 17

[Dave Hylands [:dhylands]]

Comment on attachment 724741 [details] [diff] [review]
Patch_part2

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

Looks good. It looks like you need to obsolete the v1 of the patch.

Comment 18

[Vincent Liu[:vliu]]

The result of try server.

https://tbpl.mozilla.org/?tree=Try&rev=6cc5b6c11fab
https://tbpl.mozilla.org/?tree=Try&rev=5ffc562f30d3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Given the useless (and identical) commit messages of the two patches, I went ahead and folded them into one. In the future, commit messages that say what a patch is *doing* is appreciated. Also, please use your full name for commits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd356da2f14

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cd356da2f14

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/95177dcf26aa