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. 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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev