Ansis Atteka <ansisatt...@gmail.com> writes: > On 31 August 2017 at 14:57, Aaron Conole <acon...@redhat.com> wrote: >> Ansis Atteka <ansisatt...@gmail.com> writes: >> >>> On 31 August 2017 at 11:58, Aaron Conole <acon...@redhat.com> wrote: >>>> Hi Ansis, >>>> >>>> Thanks for the review! >>>> >>>> Ansis Atteka <ansisatt...@gmail.com> writes: >>>> >>>>> On 30 August 2017 at 07:00, Aaron Conole <acon...@redhat.com> wrote: >>>>>> The selinux policy that exists in the repository did not specify access >>>>>> to >>>>>> all of the resources needed for Open vSwitch to properly function with >>>>>> an enforcing selinux policy. This update allows Open vSwitch to operate >>>>>> with selinux set to Enforcing mode, even while running as a non-root >>>>>> user. >>>>>> >>>>>> Acked-by: Flavio Leitner <f...@sysclose.org> >>>>>> Signed-off-by: Aaron Conole <acon...@redhat.com> >>>>>> Tested-by: Jean Hsiao <jhs...@redhat.com> >>>>>> --- >>>>>> selinux/openvswitch-custom.te.in | 40 >>>>>> +++++++++++++++++++++++++++++++++++++++- >>>>>> 1 file changed, 39 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/selinux/openvswitch-custom.te.in >>>>>> b/selinux/openvswitch-custom.te.in >>>>>> index 47ddb56..66cb678 100644 >>>>>> --- a/selinux/openvswitch-custom.te.in >>>>>> +++ b/selinux/openvswitch-custom.te.in >>>>>> @@ -2,15 +2,53 @@ module openvswitch-custom 1.0.1; >>>>>> >>>>>> require { >>>>>> type openvswitch_t; >>>>>> + type openvswitch_rw_t; >>>>>> type openvswitch_tmp_t; >>>>>> + type openvswitch_var_run_t; >>>>>> + >>>>>> type ifconfig_exec_t; >>>>>> type hostname_exec_t; >>>>>> + type tun_tap_device_t; >>>>>> + >>>>>> +@begin_dpdk@ >>>>>> + type hugetlbfs_t; >>>>>> + type kernel_t; >>>>>> + type svirt_image_t; >>>>>> + type vfio_device_t; >>>>>> +@end_dpdk@ >>>>>> + >>>>>> + class capability { dac_override audit_write }; >>>>>> + class dir { write remove_name add_name lock read }; >>>>>> + class file { write getattr read open execute execute_no_trans >>>>>> create unlink }; >>>>>> + class netlink_audit_socket { create nlmsg_relay audit_write >>>>>> read write }; >>>>>> class netlink_socket { setopt getopt create connect getattr >>>>>> write read }; >>>>>> - class file { write getattr read open execute execute_no_trans }; >>>>>> + class unix_stream_socket { write getattr read connectto connect >>>>>> setopt getopt sendto accept bind recvfrom acceptfrom }; >>>>>> + >>>>>> +@begin_dpdk@ >>>>>> + class chr_file { write getattr read open ioctl }; >>>>>> + class tun_socket { relabelfrom relabelto create }; >>>>>> +@end_dpdk@ >>>>>> } >>>>>> >>>>>> #============= openvswitch_t ============== >>>>>> +allow openvswitch_t self:capability { dac_override audit_write }; >>>>>> +allow openvswitch_t self:netlink_audit_socket { create nlmsg_relay >>>>>> audit_write read write }; >>>>>> allow openvswitch_t self:netlink_socket { setopt getopt create connect >>>>>> getattr write read }; >>>>>> + >>>>>> allow openvswitch_t hostname_exec_t:file { read getattr open execute >>>>>> execute_no_trans }; >>>>>> allow openvswitch_t ifconfig_exec_t:file { read getattr open execute >>>>>> execute_no_trans }; >>>>>> + >>>>>> +allow openvswitch_t openvswitch_rw_t:dir { write remove_name add_name >>>>>> lock read }; >>>>>> +allow openvswitch_t openvswitch_rw_t:file { write getattr read open >>>>>> execute execute_no_trans create unlink }; >>>>>> allow openvswitch_t openvswitch_tmp_t:file { execute execute_no_trans }; >>>>>> +allow openvswitch_t openvswitch_tmp_t:unix_stream_socket { write >>>>>> getattr read connectto connect setopt getopt sendto accept bind recvfrom >>>>>> acceptfrom }; >>>>>> + >>>>>> +@begin_dpdk@ >>>>>> +allow openvswitch_t hugetlbfs_t:dir { write remove_name add_name lock >>>>>> read }; >>>>>> +allow openvswitch_t hugetlbfs_t:file { create unlink }; >>>>>> +allow openvswitch_t kernel_t:unix_stream_socket { write getattr read >>>>>> connectto connect setopt getopt sendto accept bind recvfrom acceptfrom }; >>>>>> +allow openvswitch_t self:tun_socket { relabelfrom relabelto create }; >>>>> >>>>> Can you give an example why above rule is required? I believe it >>>>> allows processes running under openvswitch_t type to change labels? I >>>>> believe by having such rule the "Mandatory" access control becomes >>>>> kinda like "Discretionary" in sense that process has now some >>>>> discretion to alter SELinux policy. >>>> >>>> So, as you point out it allows openvswitch_t to relabel from / to on >>>> tun_socket devices. >>>> >>>> Inline are the related AVC message records I have. >>>> >>>> ----------------------------- >8 -------------------------------- >>>> type=AVC msg=audit(1503426353.300:21402): avc: denied { relabelfrom >>>> } for pid=128134 comm="ovs-vswitchd" >>>> scontext=system_u:system_r:openvswitch_t:s0 >>>> tcontext=system_u:system_r:openvswitch_t:s0 tclass=tun_socket >>>> type=AVC msg=audit(1503426353.300:21402): avc: denied { relabelto } >>>> for pid=128134 comm="ovs-vswitchd" >>>> scontext=system_u:system_r:openvswitch_t:s0 >>>> tcontext=system_u:system_r:openvswitch_t:s0 tclass=tun_socket >>>> type=AVC msg=audit(1503426358.919:21406): avc: denied { create } for >>>> pid=128134 comm="ovs-vswitchd" >>>> scontext=system_u:system_r:openvswitch_t:s0 >>>> tcontext=system_u:system_r:openvswitch_t:s0 tclass=tun_socket >>>> ----------------------------- >8 -------------------------------- >>>> >>>> I have to admit, I don't remember the exact details of what triggered >>>> this particular AVC. When I get access to the testbed again, I can >>>> recreate it and let you know (or post a follow up if required). >>> >>> While I could be wrong, I imagine that this rule is required if >>> ovs-vswitchd process would: >>> 1. call chcon() system call itself; OR >>> 2. invoke a script or system utility that calls chcon(). >>> >>> Unfortunately, a quick git-grep did not reveal where chcon could be >>> invoked from OVS code base. >>> >>> For piece of mind I would prefer that I could assure that input >>> arguments are properly validated for this hypothetical chcon call. >> >> Without having a test setup handy, I looked into the call chain. >> Whenever a process opens (read attaches to) a tun device, the LSM hook >> selinux_tun_dev_open() will get called. That call checks that the >> subjective security ID of the current task has the RELABEL_FROM and >> RELABEL_TO permissions (see security/selinux/hooks.c:5067 and >> security/selinux/hooks.c:6431). >> >> This was added as part of ed6d76e4c32d ("selinux: Support for the new >> TUN LSM hooks"). The last sentence of the commit message explains: >> >> The _tun_dev_attach() is unique because it involves a >> domain attaching to an existing TUN device and its associated >> tun_socket object, an operation which does not exist with standard >> sockets and most closely resembles a relabel operation. >> >> This is why the relabel to/from are required specifically for tun_socket >> objects. > > Thanks for looking that up. I think your explanations sounds very plausible. > >> >> Looking at the code, I don't see where that is happening (possibly in >> netdev-linux when a tap is created ... except that I didn't create a tap >> device in any of my tests). It's possibly a label given to a dpdk >> specific communication channel (which seems likely, since they do many >> different types of internal pipes for communication). I thought it >> might be triggered if I used a tap device, which does open >> "/dev/net/tun." >> >> However, trying on my local fedora setup (f25), I can reproduce the >> various tun_tap_device_t create/open/read/write/ioctl errors, so those >> probably should be moved out of the dpdk-specific area in v3 (or if this >> is already applied I can write a quick follow up to address it). But I >> cannot trigger the tun_socket errors, thus far. So I'll keep them as >> dpdk-only. >> >>> >>> >>>> >>>>>> +allow openvswitch_t svirt_image_t:file { getattr read write }; >>>>>> +allow openvswitch_t tun_tap_device_t:chr_file { read write >>>>>> getattr open ioctl }; >>>>>> +allow openvswitch_t vfio_device_t:chr_file { read write open >>>>>> ioctl getattr }; >>>>>> +@end_dpdk@ >>>>> >>>>> IIRC some time ago I mentioned that DPDK policy MAY be extended for >>>>> DPDK via SELinux booleans >>>>> [https://wiki.centos.org/TipsAndTricks/SelinuxBooleans]. I guess there >>>>> was an issue why you did not want to pursue that path? >>>> >>>> It's one of administration. I've been trying to keep the overhead of >>>> enabling dpdk support very low. Using a boolean is an additional step >>>> out of the box. For most other projects (libvirt, etc.) it doesn't seem >>>> like any additional out of the box configuration is needed to get a >>>> working selinux configuration and have virtio (or other subsystems) work >>>> with them, so I wanted to keep it that way. >>>> >>>> There's basically no technical issue I see either way, it's one of >>>> preference. If you are really that concerned that you want a boolean, I >>>> can do that, but it seemed inconvenient - and we can't accidentally open >>>> the selinux policy too wide with non-dpdk enabled OvS since that is a >>>> build-time restriction. >>>> >>>>> Other than that this patch looks good to me. I hope you tested it on >>>>> RHEL and Fedora? >>>> >>>> Yes, RHEL and Fedora (but I didn't test on CentOS). Also, usually I >>>> test with dpdk-enabled. I did try to keep the rules correct for both >>>> cases, though. >>> >>> Great. I am fine with series. Though, it wold be nice if you could >>> triage one more time why the relabeling rule is required. >> >> Hopefully the above explains. Let me know if I should post v3 (and >> maybe have your Acked-by?), or if this is applied and a follow up patch >> is needed. > > I have not applied your patch yet. Feel free to send v3 and then I > will apply your patches.
Awesome. Thanks for all your help! I've CC'd you in the new version. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev