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