Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too
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
Signed-off-by: Michal Privoznik
---
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
#endif
+#ifdef WITH_SELINUX
+# include
+#endif
#include
@@ -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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too
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
>> Signed-off-by: Michal Privoznik
>> ---
>> 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
>> #endif
>> +#ifdef WITH_SELINUX
>> +# include
>> +#endif
>>
>> #include
>>
>> @@ -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.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too
On 01/13/2017 12:26 PM, Andrea Bolognani wrote:
> On Fri, 2017-01-13 at 11:12 +0100, Michal Privoznik wrote:
> [...]
>> @@ -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);
>
> s/get SELinux label on/get SELinux label from/
>
> One more occurrence in the patch.
>
>> +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);
>
> Please decide whether you want the argument to %s on the same
> line as the format string or on the next, and stick with it :)
>
> [...]
>> @@ -7571,6 +7617,9 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
>> ATTRIBUTE_UNUSED,
>>cleanup:
>> if (ret < 0 && delDevice)
>> unlink(data->file);
>> +#ifdef WITH_SELINUX
>> +freecon(data->tcon);
>> +#endif
>
> I don't think you should free the SELinux context...
>
>> virFileFreeACLs(&data->acl);
>
> ... or the ACLs, for that matter, on failure: the caller
> will free them already if the helper fails, which is good
> because whoever allocates the memory should be responsible
> for releasing it.
In fact I need to. This function is ran in a separate process. Therefore
there is no double free. On the other hand, with return from this
function the process ends anyway, so if we don't free it kernel will.
I'm keeping it for the time being.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too
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
Signed-off-by: Michal Privoznik
---
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
#endif
+#ifdef WITH_SELINUX
+# include
+#endif
#include
@@ -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.
For the purpose of this release, let's say I noticed this patch right
after the release and hence didn't have time to NACK it properly on
time.
Martin
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Copy SELinux labels for namespace too
On Fri, 2017-01-13 at 11:12 +0100, Michal Privoznik wrote:
[...]
> @@ -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);
s/get SELinux label on/get SELinux label from/
One more occurrence in the patch.
> +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);
Please decide whether you want the argument to %s on the same
line as the format string or on the next, and stick with it :)
[...]
> @@ -7571,6 +7617,9 @@ qemuDomainAttachDeviceMknodHelper(pid_t pid
> ATTRIBUTE_UNUSED,
> cleanup:
> if (ret < 0 && delDevice)
> unlink(data->file);
> +#ifdef WITH_SELINUX
> +freecon(data->tcon);
> +#endif
I don't think you should free the SELinux context...
> virFileFreeACLs(&data->acl);
... or the ACLs, for that matter, on failure: the caller
will free them already if the helper fails, which is good
because whoever allocates the memory should be responsible
for releasing it.
[...]
> @@ -7619,6 +7677,9 @@ qemuDomainAttachDeviceMknod(virQEMUDriverPtr driver,
>
> ret = 0;
> cleanup:
> +#ifdef WITH_SELINUX
> +freecon(data.tcon);
> +#endif
> virFileFreeACLs(&data.acl);
> return 0;
Existing, but I'm pretty sure you want to return 'ret'
rather than 0 here.
ACK once you deal with the issues mentioned above, and we
definitely want to have this in as soon as possible.
--
Andrea Bolognani / Red Hat / Virtualization
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
