Closed Bug 1136032 Opened 10 years ago Closed 10 years ago

Create Selinux policy for b2g and its related process

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(feature-b2g:2.5+, b2g-v2.2 wontfix, b2g-v2.2r wontfix, b2g-master fixed)

Status:
RESOLVED FIXED
Milestone:
FxOS-S4 (07Aug)
Project Flags:
feature-b2g 2.5+
Tracking Flags:
Tracking Status
b2g-v2.2 --- wontfix
b2g-v2.2r --- wontfix
b2g-master --- fixed

People

(Reporter: seinlin, Assigned: tedd)

References

Details

Attachments

(4 files, 3 obsolete files)

Part 1: PR gonk-misc - Add generic b2g SELinux policies
49 bytes, text/x-github-pull-request
kang
: review+
kang
: review+
Details | Review
Part 2: PR platform_external_sepolicy - Exclude b2g from some neverallow rules
64 bytes, text/x-github-pull-request
kang
: review+
kang
: review+
Details | Review
Part 3: PR device-hammerhead - Add device specific b2g related SELinux policies
56 bytes, text/x-github-pull-request
kang
: review+
seinlin
: review+
kang
: review+
seinlin
: review+
Details | Review
Part 4: PR b2g-manifest - Use b2g sepolicy fork
52 bytes, text/x-github-pull-request
seinlin
: review+
Details | Review
In build base on lollipop, it is not allowed to disable Selinux in user build. There are two choices to run b2g properly in user build - try to disable Selinux - Add policy for b2g According to bug 1132837 comment 8, 9 and 10, it'd be better to create Selinux policy for b2g and its related process.
<3>[ 3.009135] init: Warning! Service gonksched needs a SELinux domain defined; please fix! <3>[ 3.010082] init: Warning! Service fakeappops needs a SELinux domain defined; please fix! <3>[ 3.011163] init: Warning! Service b2g needs a SELinux domain defined; please fix! <3>[ 3.012284] init: Warning! Service rilproxy needs a SELinux domain defined; please fix! <3>[ 3.013538] init: Warning! Service nfcd needs a SELinux domain defined; please fix! u:r:init:s0 root 188 1 /system/bin/gonksched u:r:init:s0 root 189 1 /system/bin/fakeappops u:r:init:s0 root 190 1 /system/b2g/b2g u:r:init:s0 radio 191 1 /system/bin/rilproxy u:r:init:s0 nfc 192 1 /system/bin/nfcd u:r:init:s0 root 454 190 /system/b2g/b2g u:r:init:s0 u0_a826 826 454 /system/b2g/b2g u:r:init:s0 u0_a945 945 190 /system/b2g/plugin-container u:r:init:s0 u0_a1085 1085 454 /system/b2g/b2g
Hi Jed/Paul: Any candidate to make SELinux policy for about services FxOS created? Thanks!!
Flags: needinfo?(ptheriault)
Flags: needinfo?(jld)
Assignee: nobody → julian.r.hector
I looked at my dmesg output from the flame device, I don't get the warning, but the processes are running with the init context, it is a build from master. Did you get the warnings from a USER build?
Flags: needinfo?(sku)
(In reply to Julian Hector [:tedd] from comment #3) > I looked at my dmesg output from the flame device, I don't get the warning, > but the processes are running with the init context, it is a build from > master. > > Did you get the warnings from a USER build? What version you use on flame? As I know, flame stay in kk base (L (5.0) porting is on going). SELinux is not mandatory on KK, but L. You can use Nexus5-L (Sony Z3C-L will be ready soon) to check the demsg first.
Flags: needinfo?(sku)
Yeah I think that is the problem I had, I was using plain 'flame' config (so jb, not even kk) and when looking at the policy files, everything domain was set to 'permissive', so no enforcing whatsoever.
Attachment #8616730 - Flags: review?(gdestuynder)
Attachment #8616730 - Flags: feedback?(seinlin.maung+bugs)
A fork of git://codeaurora.org/platform/external/sepolicy must be maintained for device specific changes. https://github.com/jhector/platform_external_sepolicy/ -b b2g-nexus-5-l is based on the original sepolicy head for the nexus-5-l configuration and contains the changes necessary for the b2g SELinux policies. Step 3 depends on the presence of this repo in github.com/mozilla-b2g/ with the b2g-nexus-5-l branch
Attachment #8616733 - Flags: review?(gdestuynder)
Attachment #8616733 - Flags: feedback?(seinlin.maung+bugs)
Attachment #8616730 - Flags: feedback?(seinlin.maung+bugs) → review?(seinlin.maung+bugs)
Attachment #8616738 - Flags: review?(seinlin.maung+bugs)
Attachment #8616738 - Flags: review?(gdestuynder)
So gonk-misc contains policies for b2g domains that stay consistent across devices. These policy files will be copied into the external/sepolicy directory by the manifest (Step 3, <copyfile> nodes) this way we don't have to maintain redundant data in device specific repos/branches. During build time those files will then be included in the sepolicy that is present on the device. We still have to maintain a device specific branch of the sepolicy repo because there are some device specific domains (generally found in b2g/device/.../sepolicy) that interact with the b2g domains. These rules can be found inside the b2g-vendor.te file. We also need to exclude the b2g domain from some 'neverallow' rules that are specified by android, those rules can't be overwritten so changes have to made directly to the file where they are declared. I looked at different ways to easily manage generic rules without having too much redundant data across repos. This is the best solution I could come up with. Right now all rules are generated using audit2allow based on denies in dmesg (during test on the device), so that it 'works', at this point the rules can be inspected more carefully to further tighten the privileges of the domains. For example the sys_ptrace capability might not be necessary (currently allowed though, due to audit2allow). So far I only have access to the nexus-5-l and the flame-l which is currently still work in progress (but probably needs to be added as well once it landed). I don't have a Sony Z3C-L (also being ported at the moment) device so I couldn't check the policies there yet.
Flags: needinfo?(jld)
since Julian's working on this, clear paul's flag ;)
Flags: needinfo?(ptheriault)
Comment on attachment 8616730 [details] [review] Step 1: PR gonk-misc - Add generic b2g SELinux policies The content is fine - we need a wiki page that explains how this works/why it's there (and potentially add the link of the page as a comment for discoverability). in particular: -b2g domain has a large set of permissions, this can cause some eye rolling I guess :) -need to explain why it's the case (it's a little similar to the sandbox rules and why some of them still allow things one would assume the sandbox would deny)
Attachment #8616730 - Flags: review?(gdestuynder) → review+
Comment on attachment 8616733 [details] [diff] [review] Step 2: fork github.com/jhector/platform_external_sepolicy into github.com/mozilla-b2g/ Review of attachment 8616733 [details] [diff] [review]: ----------------------------------------------------------------- Similar to step 1. Make sure you check debugging works ok for the ptrace denial ::: .gitignore @@ +1,1 @@ > +# ignore b2g policies add a comment that explains why they're ignored (ie because of the move/avoids erronous commits) ::: domain.te @@ +175,4 @@ > > # Limit ability to ptrace or read sensitive /proc/pid files of processes > # with other UIDs to these whitelisted domains. > +neverallow { domain -debuggerd -vold -dumpstate -system_server -b2g } self:capability sys_ptrace; if this works with engr builds/during debugging im fine with it :)
Attachment #8616733 - Flags: review?(gdestuynder) → review+
Comment on attachment 8616738 [details] [review] Step 3: PR b2g-manifest - Change manifest to use generic b2g and nexus-5-l specific SELinux policies TravisCI failed so you probably want to ensure the owner of the repo knows about the branch issue (which causes the error), maybe directly in https://github.com/mozilla-b2g/b2g-manifest/pull/342 just to be sure.
Attachment #8616738 - Flags: review?(gdestuynder) → review+
Before the pull requests are merged, I would like to check the policies on different configuration first. I might update the pull request or they be fine as is.
Comment on attachment 8616730 [details] [review] Step 1: PR gonk-misc - Add generic b2g SELinux policies Looks good. But I think we still need policy for these two processes which belong to b2g. system/bluetoothd/ system/nfcd/ Thank you for working on this!
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #16) > system/bluetoothd/ > system/nfcd/ Yes, I saw that too, I will probably do a new pull request.
(In reply to Julian Hector [:tedd] from comment #17) > (In reply to Kai-Zhen Li [:kli][:seinlin] from comment #16) > > Yes, I saw that too, I will probably do a new pull request. I think you don't need to make a new pull request. Just need to push -f to jhector:b2g-sepolicy, the pull request will be updated.
I make a fresh build include the following steps - apply three patches manually - modify system/core/init/Android.mk, do not allow disable SELinux in userdebug/eng build But the device didn't boot up correctly and stop at Google logo. I can see the following error log -- E/SELinux ( 173): avc: denied { add } for service=appops scontext=u:r:fakeappops:s0 tcontext=u:object_r:system_server_service:s0 tclass=service_manager E/SELinux ( 173): avc: denied { add } for service=scheduling_policy scontext=u:r:gonksched:s0 tcontext=u:object_r:system_server_service:s0 tclass=service_manager E/SELinux ( 173): avc: denied { add } for service=display.qservice scontext=u:r:b2g:s0 tcontext=u:object_r:surfaceflinger_service:s0 tclass=service_manager E/SELinux ( 173): avc: denied { add } for service=display.qservice scontext=u:r:b2g:s0 tcontext=u:object_r:surfaceflinger_service:s0 tclass=service_manager
Comment on attachment 8616738 [details] [review] Step 3: PR b2g-manifest - Change manifest to use generic b2g and nexus-5-l specific SELinux policies Generally it is fine. But this make manifest file complicated. IMO, we should avoid this if possible. I think we can have all b2g sepolicy in gonk-misc/sepolicy. Then add them in gonk-misc/b2g.mk should work, such as -- BOARD_SEPOLICY_DIRS += \ gonk-misc/sepolicy BOARD_SEPOLICY_UNION += \ b2g-vendor.te \ b2g.te \ fakeappops.te \ gonksched.te \ plugin-container.te \ rilproxy.te \ file_contexts
Attachment #8616738 - Flags: review?(seinlin.maung+bugs) → review-
Comment on attachment 8616733 [details] [diff] [review] Step 2: fork github.com/jhector/platform_external_sepolicy into github.com/mozilla-b2g/ Basically, I think it could be better if we don't need to fork an aosp repo. But I have no idea how to handle the violation problem in keystore.te and domain.te to allow b2g some permissions. If forking external/sepolicy is the only solution, we can fork from caf and rename android-5.1.0_r1 to b2g-5.1.0_r1. Then you can send PR to this branch. I can help to fork it if needed, please ni me.
Attachment #8616733 - Flags: feedback?(seinlin.maung+bugs) → feedback+
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #19) > I make a fresh build include the following steps > - apply three patches manually > - modify system/core/init/Android.mk, do not allow disable SELinux in > userdebug/eng build > > But the device didn't boot up correctly and stop at Google logo. That's why I wanted to halt to process for now, because I configured it wrong in the first place and wanted to fix that. (In reply to Kai-Zhen Li [:kli][:seinlin] from comment #20) > I think we can have all b2g sepolicy in gonk-misc/sepolicy. Then add them in > gonk-misc/b2g.mk should work, such as > -- > > BOARD_SEPOLICY_DIRS += \ > ... > BOARD_SEPOLICY_UNION += \ That's interesting, I was looking at something like that but the only way I saw it working was replacing the BoardConfig.mk which would result in forking some vendor repos (not a good idea). I didn't know we could have BOARD_SEPOLICY_DIRS etc set in b2g.mk inside gonk-misc, but I totally agree this is the better way and we should do that, instead of having it being copied by the manifest. Thank you I didn't know this option existed. (In reply to Kai-Zhen Li [:kli][:seinlin] from comment #21) > Basically, I think it could be better if we don't need to fork an aosp repo. > But I have no idea how to handle the violation problem in keystore.te and > domain.te to allow b2g some permissions. > > If forking external/sepolicy is the only solution, we can fork from caf > and rename android-5.1.0_r1 to b2g-5.1.0_r1. Then you can send PR to this > branch. I can help to fork it if needed, please ni me. So I experimented with BOARD_SEPOLICY_UNION in regards of the neverallow rules, and tried to append a file that would exclude b2g domain from that rule but it didn't work. The way I see it right now is that we will have to maintain a fork in order to exclude b2g from that restriction. What do you think kang?
Flags: needinfo?(gdestuynder)
So if we are going with the BOARD_SEPOLICY_DIRS etc inside gonk-misc approach, what would be the best way to manage policies provided by the vendors that need to interact with the b2g domain. Right now those policies are managed inside b2g-vendor.te, but the content would change depending on the device. I see two ways of managing those right now: 1) have a device specific policy file inside gonk-misc/sepolicy, like b2g-nexus-5-l.te for the nexus-5-l. Then we would have to include these files in the BOARD_SEPOLICY_UNION depending on the device that is being build (e.g. ifeq($SOME_VAR)...), is there an environment variable available in b2g.mk that can be used to determine the target device? 2) if we might have to maintain a fork of platform/external/sepolicy anyway (due to neverallow rules), it might make sense to maintain the device specific policies in the fork inside the file b2g-vendor.te (for example) What would you prefer? Or maybe a different approach.
Flags: needinfo?(seinlin.maung+bugs)
discussed with :tedd IRL - the fork is necessary for neverallow, and we don't want ifdef for every device in b2g.mk anyway (so forking is cleaner and also solution 2 of comment 23 IMO) Also, BOARD_SEPOLICY_UNION is cool, I didn't know this :)
Flags: needinfo?(gdestuynder)
Ok, I also fixed the policies and the phone boots now (also USER builds). So once we have a fork for sepolicy I can make the pull requests. (I would also suggest that we have a branch per device, since the repos are based on different HEADS)
I think it could be better not branching sepolicy per device, and here is an alternative for you reference. - have nexus-5-l specific thing in 'device/lge/hammerhead/sepolicy' and add it to device/lge/hammerhead/BoardConfig.mk. - b2g common things in 'gonk-misc/sepolicy/'. - fork [1] sepolicy for neverallow problem and only for common things. [1] https://github.com/mozilla-b2g/platform_external_sepolicy
Flags: needinfo?(seinlin.maung+bugs)
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #26) > I think it could be better not branching sepolicy per device, and here is an > alternative for you reference. > - have nexus-5-l specific thing in 'device/lge/hammerhead/sepolicy' and add device/lge/hammerhead/BoardConfig.mk Sounds good, since we already have a fork for that. Thanks for the forking of sepolicy.
Comment on attachment 8616730 [details] [review] Step 1: PR gonk-misc - Add generic b2g SELinux policies Disable Overdue Requests notification. Inform me again if you need me to have a reveiw, once you finish it.
Attachment #8616730 - Flags: review?(seinlin.maung+bugs)
b2g-manifest should not be merged before this bug get resolved.
Depends on: 1173299
New pull request that follows the approach of Comment 20
Attachment #8616730 - Attachment is obsolete: true
Attachment #8621053 - Flags: review?(seinlin.maung+bugs)
Attachment #8621053 - Flags: review?(gdestuynder)
Adds rules that allow the interaction between b2g domains and device specific domains as suggested in Comment 26
Attachment #8621058 - Flags: review?(seinlin.maung+bugs)
Attachment #8621058 - Flags: review?(gdestuynder)
Changes the nexus-5-l.xml manifest to use the forked sepolicy repo
Attachment #8616738 - Attachment is obsolete: true
Attachment #8621061 - Flags: review?(seinlin.maung+bugs)
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=669b80c05c04 One of the test failed, but I think it is not related to my changes because the same test succeeded in a previous try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d925afccd695)
Attachment #8621058 - Flags: review?(gdestuynder) → review+
Attachment #8621056 - Flags: review?(gdestuynder) → review+
Attachment #8621053 - Flags: review?(gdestuynder) → review+
Comment on attachment 8621061 [details] [review] Part 4: PR b2g-manifest - Use b2g sepolicy fork LGTM.
Attachment #8621061 - Flags: review?(seinlin.maung+bugs) → review+
Attachment #8621058 - Flags: review?(seinlin.maung+bugs) → review+
Comment on attachment 8621053 [details] [review] Part 1: PR gonk-misc - Add generic b2g SELinux policies :tedd, I have a fresh build to try all wip patch. I got a strange issue, the browser will be terminated unexpectedly. I didn't see and SELinux error log, but I can see some gecko error log instead. such ass -- I/Gecko ( 197): ###!!! [Parent][MessageChannel] Error: (msgtype=0x20005F,name=PBrowser::Msg_UpdateDimensions) Channel error: cannot send/recv I/Gecko ( 197): I/Gecko ( 197): I/Gecko ( 197): ###!!! [Parent][MessageChannel] Error: (msgtype=0x20005F,name=PBrowser::Msg_UpdateDimensions) Channel error: cannot send/recv I/Gecko ( 197): I/Gecko ( 197): I/Gecko ( 197): ###!!! [Parent][MessageChannel] Error: (msgtype=0x20005F,name=PBrowser::Msg_UpdateDimensions) Channel error: cannot send/recv If I build a boot.img which allows to disable SELinux and try again, browser is working fine; So I think some policy for b2g could be missing. Could you have a look? Thanks!
Attachment #8621053 - Flags: review?(seinlin.maung+bugs)
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #36) I will take a look, do you remember what you did that lead to the crash? And if you experience a crash like this again, could you also check dmesg, I am not sure if all the errors are logged in logcat as well.
Flags: needinfo?(seinlin.maung+bugs)
Oh I see the error, but it doesn't crash and I don't see anything in dmesg.
I didn't see any error in dmesg. I saw the following error messages in logcat, when the browser is terminated unexpectedly. -- I/Gecko ( 1523): [Parent 1523] WARNING: pipe error (516): Connection reset by peer: file /home/kaizhen/workspace/l-porting/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459 I/Gecko ( 1523): [Parent 1523] WARNING: pipe error (533): Connection reset by peer: file /home/kaizhen/workspace/l-porting/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459 I/Gecko ( 1523): [Parent 1523] WARNING: pipe error (549): Connection reset by peer: file /home/kaizhen/workspace/l-porting/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459 I/Gecko ( 1523): [Parent 1523] WARNING: pipe error (553): Connection reset by peer: file /home/kaizhen/workspace/l-porting/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 459 I/Gecko ( 1523): I/Gecko ( 1523): ###!!! [Parent][MessageChannel] Error: (msgtype=0x20007A,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv I/Gecko ( 1523):
Flags: needinfo?(seinlin.maung+bugs)
Cervantes, :tedd is working on selinux policy for b2g, now b2g can boot up correctly when selinux is enabled. Everything seems working fine, but there is a strange issue, the browser will be terminated unexpectedly when selinux is enabled. There is no selinux error, and we can see the ipc channel get some error. Do you have some idea or suggestion on this? Thanks!
Flags: needinfo?(cyu)
The IPC log doesn't tell us anything more than just that the process crashed. Can you attach complete log files?
Flags: needinfo?(cyu)
I can see the following crash log when the browser had been terminated. -- I/GeckoDump(10979): Crash reporter : Can't fetch app.reportCrashes. Exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://b2g/content/shell.js :: shell_reportCrash :: line 147" data: no] I/GeckoDump(11867): Crash reporter : Can't fetch app.reportCrashes. Exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://b2g/content/shell.js :: shell_reportCrash :: line 147" data: no]
Julian, I found the cause why browser/app were terminated unexpectedly. When the browser/app is launched from plugin-container instead of b2g, it will hit the error. The browser/app will be launched from plugin-container, when the (Preallocated) process is not ready. So I think we need to allow some more permissions in gonk-misc/sepolicy/plugin-container.te as necessary.
Flags: needinfo?(julian.r.hector)
Cervantes, In case of B2G loader is enabled, app will be launched from plugin-container and not from b2g, if (Preallocated) process is not ready. Is it an expected behaviour?
Flags: needinfo?(cyu)
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #44) > Cervantes, In case of B2G loader is enabled, app will be launched from > plugin-container and not from b2g, if (Preallocated) process is not ready. > Is it an expected behaviour? This isn't what B2G loader does. B2G loader eliminates the exec() call to launch the Nuwa process. You see an app's executable to be plugin-container because it's fork()/exec() from b2g process, this happens when there is no preallocated process available. We plan to fix this in bug 1155547.
Flags: needinfo?(cyu)
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #43) > Julian, I found the cause why browser/app were terminated unexpectedly. When > the browser/app is launched from plugin-container instead of b2g, it will > hit the error. The browser/app will be launched from plugin-container, when > the (Preallocated) process is not ready. So I think we need to allow some > more permissions in gonk-misc/sepolicy/plugin-container.te as necessary. Nice work, so if neither dmesg nor logcat show any log entries from AVC (Access Vector Cache) then it shouldn't be a SELinux problem. Unless it is being logged to a different location (which I don't think it is, but I could be wrong).
Flags: needinfo?(julian.r.hector)
It may be indirectly related to SELinux, like code behaving in a different way when SELinux is running in enforcing mode, but that is just a guess.
Blocks: 1175254
Ok this is very interesting, I did a little test regarding the rules, I added > permissive b2g; > unconfined_domain(b2g); to b2g.te and > permissive plugin-container; > unconfined_domain(plugin-container); to plugin-container.te and built with SELinux in enforcing mode. What this addition does, it basically allows everything for those two domains, without having to explicitly allow it. Although it should be noted that this doesn't add any security whatsoever. But the interesting part is, that it doesn't show the error/warnings anymore, so it could be true that the problem is related to the policies. Maybe something with the SELinux implementation itself is wrong.
No longer blocks: 1175254
So I got strace working (Bug 1176456) but couldn't see anything obvious that would make the send/recv fail. But I think flame-l just landed and I will try and see if the same error exists there as well.
(In reply to Julian Hector [:tedd] from comment #48) > But the interesting part is, that it doesn't show the error/warnings > anymore, so it could be true that the problem is related to the policies. > Maybe something with the SELinux implementation itself is wrong. At least it is proved that the error is related to the policies, but the root cause is still unknown.
Ok, since flame-l recently landed, I started to look into getting the policies running on flame-l. It turns out that the same warnings/errors occur on the flame-l as well when running in enforcing mode. But I still don't know what the cause is. When getting it to run on the flame, I encountered some problems that we need to address in order to get the policies on the flame. The first problem is related to code inside device/qcom/sepolicy/Android.mk. More specifically these two lines are troublesome: >BOARD_SEPOLICY_DIRS := \ >... >BOARD_SEPOLICY_UNION := \ instead of appending to the variables, they are overwritten which discards the changes by b2g.mk which includes the generic b2g policies. This results in the b2g policies not being included when building the policies. In order to fix this we need to maintain our own fork of the qcom sepolicy repo, or need to come up with a totally different approach to maintain generic b2g policies (not using gonk-misc). The second problem is related to a device specific domain that violates a neverallow rule. In particular this rule that is required to make b2g work in enforcing mode: >allow rmt_storage block_device:blk_file { read write open }; rmt_storage needs to be excluded from a neverallow rule, as of right now, the plan was to have a single branch of external/sepolicy that will be used across all devices. But with this approach we can't exclude rmt_storage from the neverallow rule since the domain might not be defined on other devices (not defined on nexus 5 for example). I can think of two ways to solve this: (1) create a dummy rmt_storage domian inside the b2g-vendor.te that is specific to the device (in case it doesn't exist anyways) (2) have device specific branches for the external/sepolicy repo I discussed this with :kang on irc, and we both agreed that (2) is the cleaner solution. So :seinlin, how do you think we should solve the two problems?
Flags: needinfo?(seinlin.maung+bugs)
(In reply to Julian Hector [:tedd] from comment #51) > I can think of two ways to solve this: > > (1) create a dummy rmt_storage domian inside the b2g-vendor.te that is > specific to the device (in case it doesn't exist anyways) > > (2) have device specific branches for the external/sepolicy repo > Both methods seem reasonable and can solve the problem. But from maintainance point of view, I think not branching external/sepolicy per device could be better. Here is an alternative, how do you think? - Remove 'device/qcom/sepolicy' from flame-l manifest file, ".repo/manifests/flame-l.xml". - Move flame-l related policies to "device/t2m/flame/sepolicy" as how nexus-5 does. I think we can also file a new bug for flame-l which depends to this bug.
Flags: needinfo?(seinlin.maung+bugs)
Blocks: 1180216
I think it is a good idea to have a separate bug for each device (since there will likely be other devices that need policies, e.g. Sony Z3C), do you want to change this to a nexus 5 specific bug and create a meta bug to track other devices and make them all block the meta bug? Or change this to a meta bug and create one for nexus 5?
Flags: needinfo?(seinlin.maung+bugs)
There are some WIP for nexus 5 in this bug. I think we can finish b2g common things together with nexus 5 in this bug, and file new bug for other device. Please see also bug 1180216 comment 3. PS: If you prefer to file a separated bug for landing nexus-5 specific thing is also fine :)
Flags: needinfo?(seinlin.maung+bugs)
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #54) > There are some WIP for nexus 5 in this bug. I think we can finish b2g common > things together with nexus 5 in this bug, and file new bug for other device. > Please see also bug 1180216 comment 3. > > PS: If you prefer to file a separated bug for landing nexus-5 specific thing > is also fine :) It is fine to continue the work for nexus-5 in this bug and then create a new one for each new device. Should we create a tracking bug for all of them? Or should this one serve as the tracking bug?
Flags: needinfo?(seinlin.maung+bugs)
Ok good news, I found the cause of the send/recv problem. You (:seinlin) were right that it only occurs when no pre-allocated process is available and plugin-container is executed. It all boils down to the domain transition from u:r:b2g:s0 to u:r:plugin-container:s0. In general such a transition is no problem, but looking at the kernel source[1], I found the following code: > if (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS) > return -EPERM; Since plugin-container serves as a child, which runs inside a seccomp sandbox, we run: > prctl(PR_SET_NO_NEW_PRIVS, 0, 0, 0); in order to prevent a newly spawned process from gaining higher privileges than its parent. PR_SET_NO_NEW_PRIVS also sets LSM_UNSAFE_NO_NEW_PRIVS to allow LSM to follow that restriction as well. In the case of SELinux, this prevents the transition into a new context upon execve() (because the new context could potentially have higher privileges than the original). To solve the issue, I have two solutions: (1) we delete the entire plugin-container domain, and execute plugin-container with the b2g domain. In the long run this could prevent us from further restricting the plugin-container process in case that is required. (2) The Linux kernel introduced a feature in v3.18 which allows to make a domain transition upon execve(), even when LSM_UNSAFE_NO_NEW_PRIVS is set, as long as the new domain's permission are a subset of the original domain's permission. So for example B is subset of A, if A is not allowed to read a file, B shouldn't be allowed to read the file as well. For testing purposes I backported that patch into the hammerhead kernel and got the policies working and the transition into the plugin-container works as well. I would prefer to use solution (2) since it allows to have separate permissions for plugin-container and b2g, but it would require a kernel that supports that option (we compile hammerhead and flame kernel so that shouldn't be a problem), what do you both think is the best solution? If we choose solution 2, I can create a new bug and handle the backport for the required kernels. [1] https://github.com/jhector/codeaurora_kernel_msm/blob/6ea22c33066af66070e3360c68699e219a6c4eab/security/selinux/hooks.c#L2108
Flags: needinfo?(gdestuynder)
discussed this with :tedd yesterday - it makes sense, as unfortunate as it is ;) I would vote for 2 ideally. We have to check if that's ok from a release point of view as this means *requiring* the kernel patch to run Gonk with Android 5/L. If requiring is not ok we'll need a transition path or a different solution - and delee the plugin-container domain in the meantime.
Flags: needinfo?(gdestuynder)
Btw, here is the link to the commit that I would backport (or cherry-pick), which allows to make the domain transition: https://github.com/torvalds/linux/commit/7b0d0b40cd78cadb525df760ee4cac151533c2b5
(In reply to Julian Hector [:tedd] from comment #56) > Since plugin-container serves as a child, which runs inside a seccomp > sandbox, we run: > > > prctl(PR_SET_NO_NEW_PRIVS, 0, 0, 0); But we do that after we exec plugin-container, and immediately before applying a seccomp-bpf policy that doesn't allow execve... > In the case of SELinux, this prevents the transition into a new context upon > execve() (because the new context could potentially have higher privileges > than the original). ...so I'm not seeing why that's causing problems here. One other thing that's different: if we're creating a child process directly we change to a non-zero uid before exec'ing plugin-container, but for Nuwa it's either exec'ed as root or not at all (with the MOZ_B2G_LOADER stuff). I don't know if that can affect SELinux, but it might be changing at some point: bug 930258 would make it possible to chroot content processes, but they'll need to start as root to keep CAP_SYS_CHROOT.
(In reply to Jed Davis [:jld] {UTC-7} from comment #59) > ... > ...so I'm not seeing why that's causing problems here. > ... You are right, I made to many changes during testing, at first I patched the kernel since I was sure that was the problem (I wasn't aware that prctl(PR_SET_NO_NEW_PRIVS,....) is called after the exec), but plugin-container still didn't start all the way, because it couldn't load libmozglue.so. libmozglue.so is loaded using LD_PRELOAD and when SELinux is running it automatically sets AT_SECURE, which clears some environment variables before the linker gets to work, unless allow ...:process { noatsecure } is specified in the policies. So plugin-container couldn't start because of the missing LD_PRELOAD and I missed it in the logs. (noatsecure is usually not audited with 'dontaudit', and the macro that I use for the transition, sets noatsecure to dontaudit (and I didn't see it when I checked dontaudit rules), so it didn't show up in the SELinux logs) :jld you are completely right, NNP isn't the problem here, and :seinlin it was a rule thing after all. Sorry about the confusion, Comment 56 can be ignored here, I will update the PR which should fix it all.
Comment on attachment 8621053 [details] [review] Part 1: PR gonk-misc - Add generic b2g SELinux policies I updated the pull request to include the allow rule for noatsecure, can you give it another review :kang?
Attachment #8621053 - Flags: review?(gdestuynder)
:seinlin, with the updated PR, the problem should be fixed, can you give it another try?
Comment on attachment 8621053 [details] [review] Part 1: PR gonk-misc - Add generic b2g SELinux policies This is the new part compared to previous diff: +# force SELinux to keep environment variables when calling +# execve(plugin-container,...) to preserve LD_PRELOAD which is +# used to load libmozglue.so +allow b2g plugin-container:process { noatsecure };
Attachment #8621053 - Flags: review?(gdestuynder) → review+
In order to avoid maintaining branches as mentioned in Comment 52, I will update the other PR's so that the same branch of platform_external_sepolicy can be used on different devices (not per device branch)
Comment on attachment 8621056 [details] [review] Part 2: PR platform_external_sepolicy - Exclude b2g from some neverallow rules I updated the PR to include that change to domain.te that is required for the flame-l to work, here is the diff: diff --git a/domain.te b/domain.te index bdc338a..ece2bc7 100644 --- a/domain.te +++ b/domain.te @@ -254,7 +254,7 @@ neverallow domain init:binder call; # Don't allow raw read/write/open access to block_device # Rather force a relabel to a more specific type -neverallow { domain -kernel -init -recovery -vold -uncrypt -install_recovery } block_device:blk_file { open read write }; +neverallow { domain -kernel -init -recovery -vold -uncrypt -install_recovery -rmt_storage } block_device:blk_file { open read write }; # Don't allow raw read/write/open access to generic devices. # Rather force a relabel to a more specific type. it is a minimal change, but can you give it another review :kang?
Attachment #8621056 - Flags: review?(gdestuynder)
Comment on attachment 8621058 [details] [review] Part 3: PR device-hammerhead - Add device specific b2g related SELinux policies I also updated this PR which introduces the dummy definition of a domain so that the exclusion rule from Part 2 doesn't fail on the Nexus 5, where 'rmt_storage' is not defined. Here is the diff of the updated PR: diff --git a/BoardConfig.mk b/BoardConfig.mk index c2a2a81..63e1782 100644 --- a/BoardConfig.mk +++ b/BoardConfig.mk @@ -114,6 +114,7 @@ BOARD_SEPOLICY_DIRS += \ BOARD_SEPOLICY_UNION += \ app.te \ b2g-vendor.te \ + b2g-dummy.te \ bluetooth_loader.te \ bridge.te \ camera.te \ diff --git a/sepolicy/b2g-dummy.te b/sepolicy/b2g-dummy.te new file mode 100644 index 0000000..6e03b94 --- /dev/null +++ b/sepolicy/b2g-dummy.te @@ -0,0 +1,5 @@ +# Dummy domain definitions, these domains are defined on +# other devices, but are not required defined on this device. +# Nevertheless, a dummy definition is required for successfully +# building the policies. +type rmt_storage, domain; Can you (:seinlin and :kang) give it another review please?
Attachment #8621058 - Flags: review?(seinlin.maung+bugs)
Attachment #8621058 - Flags: review?(gdestuynder)
Ok just to be sure, because now that I changed the PR's so that we don't have to branch external/sepolicy, it seems like quite some work to maintain that, especially since some devices use a different commit of the repo (flame: LA.BF.1.1.2_rb1.12, nexus: refs/tags/android-5.1.0_r1). If we force every device to use b2g-5.1.0_r1 (android-5.1.0_r1) for external/sepolicy, additional work may be required to make the code work. So :seinlin, are you sure not branching is easier to maintain?
Comment on attachment 8621058 [details] [review] Part 3: PR device-hammerhead - Add device specific b2g related SELinux policies Note: as discussed IRL we also need the wiki page that explains how the logging works, etc. In particular for future work if/when more rules are added/removed.
Attachment #8621058 - Flags: review?(gdestuynder) → review+
(In reply to Julian Hector [:tedd] from comment #62) > :seinlin, with the updated PR, the problem should be fixed, can you give it > another try? I have a try to all PRs on nexus-5, everything looks good. I didn't get an error. device/lge/hammerhead - pull-29 external/sepolicy - pull-1 gonk-misc - pull-239
Flags: needinfo?(seinlin.maung+bugs)
Comment on attachment 8621058 [details] [review] Part 3: PR device-hammerhead - Add device specific b2g related SELinux policies looks good to me.
Attachment #8621058 - Flags: review?(seinlin.maung+bugs) → review+
feature-b2g: --- → 2.5+
status-b2g-master: --- → affected
(In reply to Kai-Zhen Li [:kli][:seinlin] from comment #69) > I have a try to all PRs on nexus-5, everything looks good. I didn't get an > error. > device/lge/hammerhead - pull-29 > external/sepolicy - pull-1 > gonk-misc - pull-239 Thank you very much. I will revert the changes introduced in Comment 65 and Comment 66, because a separate branch of platform_external_sepolicy is needed for Bug 1180216, so these changes are unnecessary. Once this is done and no more errors appear, the PR's should be ready to be merged.
Attachment #8621056 - Flags: review?(gdestuynder) → review+
Ok I updated Part 2 and Part 3 PR (external/sepolicy - pull-1, device/lge/hammerhead - pull-29), I reverted the changes introduced in Comment 65 and Coment 66. Carry over r+ (of previous state) from :kang and :seinlin for external/sepolicy - pull-1, device/lge/hammerhead - pull-29 PR's should be ready to be merged now, I recompiled everything and re-flashed, but if you want to make sure, you can also check it again.
Comment on attachment 8621053 [details] [review] Part 1: PR gonk-misc - Add generic b2g SELinux policies Noticed that some rules are required to physically shutdown the device (not part of the smoke-tests, maybe should be included as well). Carry over r+ from :kang. diff --git a/sepolicy/b2g.te b/sepolicy/b2g.te index 281ab80..4406cd7 100644 --- a/sepolicy/b2g.te +++ b/sepolicy/b2g.te @@ -96,6 +96,8 @@ allow b2g plugin-container:file { write open }; allow b2g plugin-container:process { signal setsched }; allow b2g plugin-container:unix_stream_socket { read write }; allow b2g port:tcp_socket { name_connect name_bind node_bind }; +allow b2g powerctl_prop:property_service set; +allow b2g proc_sysrq:file write; allow b2g property_data_file:dir { read write search getattr open }; allow b2g property_data_file:file { write open getattr }; allow b2g property_socket:sock_file write; @@ -108,10 +110,9 @@ allow b2g sdcard_internal:file { rename unlink read write create open getattr }; allow b2g security_file:dir { read write open }; allow b2g self:binder { transfer call }; allow b2g self:capability2 syslog; -allow b2g self:capability { sys_resource sys_nice sys_ptrace sys_admin sys_module sys_time net_raw net_bind_service net_admin setgid fsetid kill chown setuid fowner dac_override }; +allow b2g self:capability { sys_boot sys_resource sys_nice sys_ptrace sys_admin sys_module sys_time net_raw net_bind_service net_admin setgid fsetid kill chown setuid fowner dac_override }; allow b2g self:netlink_kobject_uevent_socket { setopt create bind read }; allow b2g self:netlink_socket { write bind create read }; -allow b2g self:process execmem; allow b2g self:process { execmem ptrace }; allow b2g self:socket { create read write ioctl }; allow b2g self:tcp_socket { write getattr connect read getopt listen bind create setopt }; diff --git a/sepolicy/plugin-container.te b/sepolicy/plugin-container.te index 047123c..ab30cbb 100644 --- a/sepolicy/plugin-container.te +++ b/sepolicy/plugin-container.te @@ -19,3 +19,4 @@ allow plugin-container shell_data_file:file { read write getattr }; # other domains interacting with plugin-container domain allow mediaserver plugin-container:binder { transfer call }; +allow mediaserver plugin-container:fd use;
Blocks: 1186103
No longer blocks: Emulator_L_Local
Comment on attachment 8621053 [details] [review] Part 1: PR gonk-misc - Add generic b2g SELinux policies During further testing I noticed that a lot of stuff would break because of the defined policies in gonk-misc and because pretty much every device uses gonk-misc master branch. For example in flame-kk the domain bluetoothd is defined outside gonk-misc, but it is also defined inside gonk-misc, which leads to a failed built. Since we want to provide policies for devices that run L or later, I added a check to b2g.mk that only includes the policy files when PLATFORM_VERSION is >= 5. I chose the filter-out function because I couldn't find a way to check >= 5 without invoking $(shell ...). :seinlin is this an acceptable way to do it, or do you have a better idea? I updated the PR, here is the diff of the update: diff --git a/b2g.mk b/b2g.mk index a0d20c1..8cd8a56 100644 --- a/b2g.mk +++ b/b2g.mk @@ -2,6 +2,8 @@ TARGET_PROVIDES_INIT_RC := true CONFIG_ESD := no HTTP := android +MAJOR_VERSION := $(word 1,$(subst ., ,$(PLATFORM_VERSION))) + PRODUCT_PACKAGES += \ b2g.sh \ b2g-info \ @@ -23,6 +25,7 @@ PRODUCT_PACKAGES += \ oom-msg-logger \ $(NULL) +ifneq ($(filter-out 0 1 2 3 4,$(MAJOR_VERSION)),) BOARD_SEPOLICY_DIRS += \ gonk-misc/sepolicy @@ -35,6 +38,7 @@ BOARD_SEPOLICY_UNION += \ plugin-container.te \ rilproxy.te \ file_contexts +endif -include external/svox/pico/lang/all_pico_languages.mk -include gaia/gaia.mk
Flags: needinfo?(seinlin.maung+bugs)
Push to try (with the recent changes): https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfa45734daa4 I only selected build, because none of the tests are actually executed in an SELinux environment (as far as I could tell) so I figured it would be pointless to have them. We don't even support building for the devices (couldn't find anything in trychooser, so that information might be out of date). I can do another push to try if there is a test I should include.
(In reply to Julian Hector [:tedd] from comment #74) > Comment on attachment 8621053 [details] [review] > Part 1: PR gonk-misc - Add generic b2g SELinux policies > > During further testing I noticed that a lot of stuff would break because of > the defined policies in gonk-misc and because pretty much every device uses > gonk-misc master branch. For example in flame-kk the domain bluetoothd is > defined outside gonk-misc, but it is also defined inside gonk-misc, which > leads to a failed built. > > Since we want to provide policies for devices that run L or later, I added a > check to b2g.mk that only includes the policy files when PLATFORM_VERSION is > >= 5. > > I chose the filter-out function because I couldn't find a way to check >= 5 > without invoking $(shell ...). > > :seinlin is this an acceptable way to do it, or do you have a better idea? > > I updated the PR, here is the diff of the update: > > diff --git a/b2g.mk b/b2g.mk > index a0d20c1..8cd8a56 100644 > --- a/b2g.mk > +++ b/b2g.mk > @@ -2,6 +2,8 @@ TARGET_PROVIDES_INIT_RC := true > CONFIG_ESD := no > HTTP := android > > +MAJOR_VERSION := $(word 1,$(subst ., ,$(PLATFORM_VERSION))) > + > PRODUCT_PACKAGES += \ > b2g.sh \ > b2g-info \ > @@ -23,6 +25,7 @@ PRODUCT_PACKAGES += \ > oom-msg-logger \ > $(NULL) > > +ifneq ($(filter-out 0 1 2 3 4,$(MAJOR_VERSION)),) > BOARD_SEPOLICY_DIRS += \ > gonk-misc/sepolicy > > @@ -35,6 +38,7 @@ BOARD_SEPOLICY_UNION += \ > plugin-container.te \ > rilproxy.te \ > file_contexts > +endif > > -include external/svox/pico/lang/all_pico_languages.mk > -include gaia/gaia.mk Under current infrastructure, all platform(gonk) versions use the same branch of gonk-misc. This is reasonable to me, as sepolicy is supported from gonk-l and newer version. But something we need to keep in mind is when the polices get conflicts in newer platform(gonk) version, we'll need to have a mechanism to manage it then.
Flags: needinfo?(seinlin.maung+bugs)
Ok the push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfa45734daa4 shows that it builds. I performed the smoke-tests we have and did some more manual testing. I hope I covered a lot, but in case problems arise, I created a troubleshooting section at: https://wiki.mozilla.org/Security/Sandbox/SELinux which should allow us to resolve policy issues pretty quick.
Keywords: checkin-needed
Pushed a follow-up for B2G v2.2 to pin device-hammerhead to the revision prior to this landing in order to fix bustage. This work is targeted for v2.5 anyway, so I don't think there's any problem in doing that. v2.2: https://github.com/mozilla-b2g/b2g-manifest/commit/fd7b1a9ff7173d25cab1789bd97c3e93f499808f
status-b2g-v2.2: --- → wontfix
status-b2g-v2.2r: --- → wontfix
Ok thanks, sounds good.
Depends on: 1190725
Please help to fix this :D <3>[ 252.210860] c0 init: Warning! Service ril-daemon needs a SELinux domain defined; please fix! <3>[ 252.211950] c0 init: Warning! Service ril-daemon2 needs a SELinux domain defined; please fix! <3>[ 252.213055] c0 init: Warning! Service slog needs a SELinux domain defined; please fix! <3>[ 252.215206] c0 init: Warning! Service at_distributor needs a SELinux domain defined; please fix!
Hey firelord, these warnings indicate that there are no policies defined for these processes. So you basically have to create a domain for each one of them, and then label the appropriate executable on the file system. Some of the B2G SELinux stuff is documented here: https://wiki.mozilla.org/Security/Sandbox/SELinux It also contains references to some external resources regarding SELinux which should get you covered.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: