[libvirt] [PATCH] qemu: Copy SELinux labels for namespace too

[Martin Kletzander]

On Fri, Jan 13, 2017 at 03:47:16PM +0100, Michal Privoznik wrote:
>On 01/13/2017 12:43 PM, Martin Kletzander wrote:
>> On Fri, Jan 13, 2017 at 11:12:43AM +0100, Michal Privoznik wrote:
>>> When creating new /dev/* for qemu, we do chown() and copy ACLs to
>>> create the exact copy from the original /dev. I though that
>>> copying SELinux labels is not necessary as SELinux will chose the
>>> sane defaults. Surprisingly, it does not leaving namespace with
>>> the following labels:
>>>
>>> crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0     random
>>> crw-------. root root system_u:object_r:tmpfs_t:s0     rtc0
>>> drwxrwxrwt. root root system_u:object_r:tmpfs_t:s0     shm
>>> crw-rw-rw-. root root system_u:object_r:tmpfs_t:s0     urandom
>>>
>>> As a result, domain is unable to start:
>>>
>>> error: internal error: process exited while connecting to monitor:
>>> Error in GnuTLS initialization: Failed to acquire random data.
>>> qemu-kvm: cannot initialize crypto: Unable to initialize GNUTLS
>>> library: Failed to acquire random data.
>>>
>>> The solution is to copy the SELinux labels as well.
>>>
>>> Reported-by: Andrea Bolognani <abologna at redhat.com>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>> src/qemu/qemu_domain.c | 61
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 61 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>>> index 1399dee0d..a29866673 100644
>>> --- a/src/qemu/qemu_domain.c
>>> +++ b/src/qemu/qemu_domain.c
>>> @@ -63,6 +63,9 @@
>>> #if defined(HAVE_SYS_MOUNT_H)
>>> # include <sys/mount.h>
>>> #endif
>>> +#ifdef WITH_SELINUX
>>> +# include <selinux/selinux.h>
>>> +#endif
>>>
>>> #include <libxml/xpathInternals.h>
>>>
>>> @@ -6958,6 +6961,9 @@ qemuDomainCreateDevice(const char *device,
>>>     char *canonDevicePath = NULL;
>>>     struct stat sb;
>>>     int ret = -1;
>>> +#ifdef WITH_SELINUX
>>> +    char *tcon = NULL;
>>> +#endif
>>>
>>>     if (virFileResolveAllLinks(device, &canonDevicePath) < 0) {
>>>         if (errno == ENOENT && allow_noent) {
>>> @@ -7023,10 +7029,34 @@ qemuDomainCreateDevice(const char *device,
>>>         goto cleanup;
>>>     }
>>>
>>> +#ifdef WITH_SELINUX
>>> +    if (getfilecon_raw(canonDevicePath, &tcon) < 0 &&
>>> +        (errno != ENOTSUP && errno != ENODATA)) {
>>> +        virReportSystemError(errno,
>>> +                             _("Unable to get SELinux label on %s"),
>>> canonDevicePath);
>>> +        goto cleanup;
>>> +    }
>>> +
>>> +    if (tcon &&
>>> +        setfilecon_raw(devicePath, (VIR_SELINUX_CTX_CONST char *)
>>> tcon) < 0) {
>>> +        VIR_WARNINGS_NO_WLOGICALOP_EQUAL_EXPR
>>> +        if (errno != EOPNOTSUPP && errno != ENOTSUP) {
>>> +        VIR_WARNINGS_RESET
>>> +            virReportSystemError(errno,
>>> +                                 _("Unable to set SELinux label on %s"),
>>> +                                 devicePath);
>>> +            goto cleanup;
>>> +        }
>>> +    }
>>> +#endif
>>> +
>>
>> I think, instead of all the ifdefs, this should be a security driver API
>> instead of being hardcoded in places.  That way it will be processed
>> properly by all the security drivers.
>
>I don't think I see what you mean. Firstly, we want to set seclabels for
>some paths that are not touched by secdrivers at all (e.g. /dev/*random,
>/dev/kvm). Secondly, secdrivers should honour norelabel flag and be a
>no-op if that one is set. This would clash with sysadmins handling
>seclabels themselves. Thirdly, no secdriver of ours deals with ACLs. We
>have to in here.
>

It might sound a bit weird to have a security API that does not honour
relabel=no and does label an arbritrary file, but SELinux stuff should
go into SELinux security driver.  Otherwise this will be a mess.  The
only problem I see here is when the driver is disabled in the
configuration file, it will not be triggered.  But we might get around
that.

Just so you know, I'm not special-casing SELinux here.  I think
DAC-related stuff should go to DAC driver and so on.

Just my 2 cents, though.

>Michal
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170116/a2b424d3/attachment-0001.sig>