[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 23:25:34 UTC 2017
Ansis Atteka <ansisatteka at gmail.com> writes:
> On 31 August 2017 at 14:57, Aaron Conole <aconole at redhat.com> wrote:
>> 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.
>
> 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 };
>>>>>> + 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.
>
> 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.
More information about the dev
mailing list