[ovs-dev] [PATCH v2 3/3] selinux: update policy to reflect non-root and dpdk support
Aaron Conole
aconole at redhat.com
Thu Aug 31 21:57:53 UTC 2017
Ansis Atteka <ansisatteka at gmail.com> writes:
> On 31 August 2017 at 11:58, Aaron Conole <aconole at redhat.com> wrote:
>> Hi Ansis,
>>
>> Thanks for the review!
>>
>> Ansis Atteka <ansisatteka at gmail.com> writes:
>>
>>> On 30 August 2017 at 07:00, Aaron Conole <aconole at 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 <fbl at sysclose.org>
>>>> Signed-off-by: Aaron Conole <aconole at redhat.com>
>>>> Tested-by: Jean Hsiao <jhsiao at 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;
>>>> +
>>>> + at begin_dpdk@
>>>> + type hugetlbfs_t;
>>>> + type kernel_t;
>>>> + type svirt_image_t;
>>>> + type vfio_device_t;
>>>> + at 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 };
>>>> +
>>>> + at begin_dpdk@
>>>> + class chr_file { write getattr read open ioctl };
>>>> + class tun_socket { relabelfrom relabelto create };
>>>> + at 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 };
>>>> +
>>>> + at 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 };
>>>> + at 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.
More information about the dev
mailing list