[PATCH v2 0/3] qemu_passt: Fixes for passt lifecycle handling
This series implements fixes in the handling of passt's lifecycle. v2: In 1/3, preserve the VM-specific MCS range by explicitly setting a label, as suggested by Daniel, with a temporary workaround sketched by Michal. Stefano Brivio (3): qemu_passt: Don't make passt transition to svirt_t/libvirt_domain on start qemu_passt: Set UID and GID to configured values for qemu driver, if any qemu_passt: Remove passt socket file on exit src/qemu/qemu_passt.c | 46 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 8 deletions(-) -- 2.39.1
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it. That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file. Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613 Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries. Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- src/qemu/qemu_passt.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 1217a6a087..81f48dd630 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -30,6 +30,11 @@ #include "virlog.h" #include "virpidfile.h" +#ifdef WITH_SELINUX +# include <selinux/selinux.h> +# include <selinux/context.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE VIR_LOG_INIT("qemu.passt"); @@ -158,8 +163,11 @@ qemuPasstStart(virDomainObj *vm, g_autofree char *errbuf = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; size_t i; - int exitstatus = 0; - int cmdret = 0; +#ifdef WITH_SELINUX + virSecurityLabelDef *seclabel; + context_t s; + const char *newLabel; +#endif cmd = virCommandNew(PASST); @@ -271,18 +279,31 @@ qemuPasstStart(virDomainObj *vm, if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0) return -1; - if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdret) < 0) - goto error; +#ifdef WITH_SELINUX + /* TODO: Implement a new security manager function for helper binaries, + * possibly deriving domain names from security attributes of the helper + * binary itself. + */ + seclabel = virDomainDefGetSecurityLabelDef(vm->def, "selinux"); + s = context_new(seclabel->label); + context_type_set(s, "passt_t"); + newLabel = context_str(s); + + virCommandSetSELinuxLabel(cmd, newLabel); +#endif - if (cmdret < 0 || exitstatus != 0) { + if (virCommandRun(cmd, NULL)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not start 'passt': %s"), NULLSTR(errbuf)); goto error; } + context_free(s); + return 0; error: - qemuPasstKill(pidfile); + context_free(s); + qemuPasstKill(pidfile, passtSocketName); return -1; } -- 2.39.1
On Wed, Feb 22, 2023 at 02:21:29PM +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
I'd really prefer to see the security manager used from the start, rather than committing code with a TODO that should be practical to implement straight away. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/22/23 9:30 AM, Daniel P. Berrangé wrote:
On Wed, Feb 22, 2023 at 02:21:29PM +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
I'd really prefer to see the security manager used from the start, rather than committing code with a TODO that should be practical to implement straight away.
As the person who started all this by naively believing that we would just need to "add something to the selinux-policy package" to make selinux+libvirt+passt work, and now that Stefano has done the heavy lifting to figure out something that actually *does* work, I will say that I'll help in whatever way I can to make that happen in as short a time as possible.
On Wed, 22 Feb 2023 14:30:21 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Feb 22, 2023 at 02:21:29PM +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
I'd really prefer to see the security manager used from the start, rather than committing code with a TODO that should be practical to implement straight away.
Feel free, by all means. As to myself, I can't realistically implement missing parts of the security manager code that I'm entirely unfamiliar with, and seriously test the whole thing before the 9.1.0 release freeze, which happens to be... today. So I see four alternatives: 1. accept this fix, 2. somebody else fixes this "properly", 3. drop the passt networking back-end, 4. ship a completely broken feature. -- Stefano
On 2/22/23 16:51, Stefano Brivio wrote:
On Wed, 22 Feb 2023 14:30:21 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Feb 22, 2023 at 02:21:29PM +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
I'd really prefer to see the security manager used from the start, rather than committing code with a TODO that should be practical to implement straight away.
Feel free, by all means.
As to myself, I can't realistically implement missing parts of the security manager code that I'm entirely unfamiliar with, and seriously test the whole thing before the 9.1.0 release freeze, which happens to be... today.
So I see four alternatives: 1. accept this fix, 2. somebody else fixes this "properly", 3. drop the passt networking back-end, 4. ship a completely broken feature.
That ship already sailed. Passt was merged in libvirt-9.0.0. We should rather fix this properly, if we can. Michal
On Wed, 22 Feb 2023 17:38:49 +0100 Michal Prívozník <mprivozn@redhat.com> wrote:
On 2/22/23 16:51, Stefano Brivio wrote:
On Wed, 22 Feb 2023 14:30:21 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote:
On Wed, Feb 22, 2023 at 02:21:29PM +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
I'd really prefer to see the security manager used from the start, rather than committing code with a TODO that should be practical to implement straight away.
Feel free, by all means.
As to myself, I can't realistically implement missing parts of the security manager code that I'm entirely unfamiliar with, and seriously test the whole thing before the 9.1.0 release freeze, which happens to be... today.
So I see four alternatives: 1. accept this fix, 2. somebody else fixes this "properly", 3. drop the passt networking back-end, 4. ship a completely broken feature.
That ship already sailed. Passt was merged in libvirt-9.0.0.
...and isn't that a good enough reason to have it actually working in 9.1.0? I really fail to understand the reasoning here. :( Allow me to quote from yourself: On Thu, 16 Feb 2023 17:27:11 +0100 Michal Prívozník <mprivozn@redhat.com> wrote:
And as we are getting close to the release I'd like to make this work with what we have now.
and I think this makes *a lot of sense*. -- Stefano
On Wed, Feb 22, 2023 at 14:21:29 +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- src/qemu/qemu_passt.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 1217a6a087..81f48dd630 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -30,6 +30,11 @@ #include "virlog.h" #include "virpidfile.h"
+#ifdef WITH_SELINUX +# include <selinux/selinux.h> +# include <selinux/context.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE
VIR_LOG_INIT("qemu.passt"); @@ -158,8 +163,11 @@ qemuPasstStart(virDomainObj *vm, g_autofree char *errbuf = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; size_t i; - int exitstatus = 0; - int cmdret = 0; +#ifdef WITH_SELINUX + virSecurityLabelDef *seclabel; + context_t s; + const char *newLabel; +#endif
cmd = virCommandNew(PASST);
@@ -271,18 +279,31 @@ qemuPasstStart(virDomainObj *vm, if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0) return -1;
- if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdret) < 0) - goto error; +#ifdef WITH_SELINUX + /* TODO: Implement a new security manager function for helper binaries, + * possibly deriving domain names from security attributes of the helper + * binary itself. + */ + seclabel = virDomainDefGetSecurityLabelDef(vm->def, "selinux"); + s = context_new(seclabel->label); + context_type_set(s, "passt_t"); + newLabel = context_str(s); + + virCommandSetSELinuxLabel(cmd, newLabel); +#endif
- if (cmdret < 0 || exitstatus != 0) { + if (virCommandRun(cmd, NULL)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not start 'passt': %s"), NULLSTR(errbuf)); goto error; }
+ context_free(s);
This would need to be enclosed in #ifdef too.
+ return 0;
error: - qemuPasstKill(pidfile); + context_free(s);
And here as well.
+ qemuPasstKill(pidfile, passtSocketName); return -1; }
I have to admit I'm not sure whether a proper solution via security manager is feasible with a reasonable short time frame, i.e., ready to be pushed ideally just after the release as I feel it would be too much of a change for the freeze (but as I said, I may be wrong, I'm not familiar with the details of the security manager code). This hacky approach has its downsides apart from the easily fixed nits above. Due to not going through the security manager layers, this code would try to use SELinux even if the driver is actually disabled in runtime (which can happen for various reasons). So the question is, can a proper solution be implemented fast enough or we need some form of this hack to have this new functionality working? Michal, do you have any idea about the feasibility of providing a proper solution to this? Jirka
On Thu, Feb 23, 2023 at 11:40:00AM +0100, Jiri Denemark wrote:
On Wed, Feb 22, 2023 at 14:21:29 +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- src/qemu/qemu_passt.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 1217a6a087..81f48dd630 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -30,6 +30,11 @@ #include "virlog.h" #include "virpidfile.h"
+#ifdef WITH_SELINUX +# include <selinux/selinux.h> +# include <selinux/context.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE
VIR_LOG_INIT("qemu.passt"); @@ -158,8 +163,11 @@ qemuPasstStart(virDomainObj *vm, g_autofree char *errbuf = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; size_t i; - int exitstatus = 0; - int cmdret = 0; +#ifdef WITH_SELINUX + virSecurityLabelDef *seclabel; + context_t s; + const char *newLabel; +#endif
cmd = virCommandNew(PASST);
@@ -271,18 +279,31 @@ qemuPasstStart(virDomainObj *vm, if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0) return -1;
- if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdret) < 0) - goto error; +#ifdef WITH_SELINUX + /* TODO: Implement a new security manager function for helper binaries, + * possibly deriving domain names from security attributes of the helper + * binary itself. + */ + seclabel = virDomainDefGetSecurityLabelDef(vm->def, "selinux"); + s = context_new(seclabel->label); + context_type_set(s, "passt_t"); + newLabel = context_str(s); + + virCommandSetSELinuxLabel(cmd, newLabel); +#endif
- if (cmdret < 0 || exitstatus != 0) { + if (virCommandRun(cmd, NULL)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not start 'passt': %s"), NULLSTR(errbuf)); goto error; }
+ context_free(s);
This would need to be enclosed in #ifdef too.
+ return 0;
error: - qemuPasstKill(pidfile); + context_free(s);
And here as well.
+ qemuPasstKill(pidfile, passtSocketName); return -1; }
I have to admit I'm not sure whether a proper solution via security manager is feasible with a reasonable short time frame, i.e., ready to be pushed ideally just after the release as I feel it would be too much of a change for the freeze (but as I said, I may be wrong, I'm not familiar with the details of the security manager code).
This hacky approach has its downsides apart from the easily fixed nits above. Due to not going through the security manager layers, this code would try to use SELinux even if the driver is actually disabled in runtime (which can happen for various reasons).
So the question is, can a proper solution be implemented fast enough or we need some form of this hack to have this new functionality working? Michal, do you have any idea about the feasibility of providing a proper solution to this?
This really isn't difficult to do in the security manager IMHO. It is just a variation on the existing virSecurityManagerSetChildProcessLabel method, which instead of using the standard QEMU svirt_t labels, queries the label by calling getfilecon on the binary to be execd and appending the MCS label. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/23/23 5:47 AM, Daniel P. Berrangé wrote:
On Thu, Feb 23, 2023 at 11:40:00AM +0100, Jiri Denemark wrote:
On Wed, Feb 22, 2023 at 14:21:29 +0100, Stefano Brivio wrote:
qemuSecurityCommandRun() causes an explicit domain transition of the new process, but passt ships with its own SELinux policy, with external interfaces for libvirtd, so we simply need to transition from virtd_t to passt_t as passt is executed. The qemu type enforcement rules have little to do with it.
That is, if we switch to svirt_t, passt will run in the security context that's intended for QEMU, which allows a number of operations not needed by passt. On the other hand, with a switch to svirt_t, passt won't be able to create its own PID file.
Usage of those new interfaces is implemented by this change in selinux-policy: https://github.com/fedora-selinux/selinux-policy/pull/1613
Replace qemuSecurityCommandRun() with virCommandRun(), and explicitly set the label, preserving the correct MCS range for the given VM instance. This is a temporary measure: eventually, we'll need a more generic and elegant mechanism for helper binaries.
Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- src/qemu/qemu_passt.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 1217a6a087..81f48dd630 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -30,6 +30,11 @@ #include "virlog.h" #include "virpidfile.h"
+#ifdef WITH_SELINUX +# include <selinux/selinux.h> +# include <selinux/context.h> +#endif + #define VIR_FROM_THIS VIR_FROM_NONE
VIR_LOG_INIT("qemu.passt"); @@ -158,8 +163,11 @@ qemuPasstStart(virDomainObj *vm, g_autofree char *errbuf = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; size_t i; - int exitstatus = 0; - int cmdret = 0; +#ifdef WITH_SELINUX + virSecurityLabelDef *seclabel; + context_t s; + const char *newLabel; +#endif
cmd = virCommandNew(PASST);
@@ -271,18 +279,31 @@ qemuPasstStart(virDomainObj *vm, if (qemuExtDeviceLogCommand(driver, vm, cmd, "passt") < 0) return -1;
- if (qemuSecurityCommandRun(driver, vm, cmd, -1, -1, &exitstatus, &cmdret) < 0) - goto error; +#ifdef WITH_SELINUX + /* TODO: Implement a new security manager function for helper binaries, + * possibly deriving domain names from security attributes of the helper + * binary itself. + */ + seclabel = virDomainDefGetSecurityLabelDef(vm->def, "selinux"); + s = context_new(seclabel->label); + context_type_set(s, "passt_t"); + newLabel = context_str(s); + + virCommandSetSELinuxLabel(cmd, newLabel); +#endif
- if (cmdret < 0 || exitstatus != 0) { + if (virCommandRun(cmd, NULL)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not start 'passt': %s"), NULLSTR(errbuf)); goto error; }
+ context_free(s);
This would need to be enclosed in #ifdef too.
+ return 0;
error: - qemuPasstKill(pidfile); + context_free(s);
And here as well.
+ qemuPasstKill(pidfile, passtSocketName); return -1; }
I have to admit I'm not sure whether a proper solution via security manager is feasible with a reasonable short time frame, i.e., ready to be pushed ideally just after the release as I feel it would be too much of a change for the freeze (but as I said, I may be wrong, I'm not familiar with the details of the security manager code).
This hacky approach has its downsides apart from the easily fixed nits above. Due to not going through the security manager layers, this code would try to use SELinux even if the driver is actually disabled in runtime (which can happen for various reasons).
So the question is, can a proper solution be implemented fast enough or we need some form of this hack to have this new functionality working? Michal, do you have any idea about the feasibility of providing a proper solution to this?
This really isn't difficult to do in the security manager IMHO. It is just a variation on the existing virSecurityManagerSetChildProcessLabel method, which instead of using the standard QEMU svirt_t labels, queries the label by calling getfilecon on the binary to be execd and appending the MCS label.
It seems it's not as simple as this. I have an implementation of this, available at https://gitlab.com/lainestump/libvirt/-/commits/active-passt-6 The problem with it is that it doesn't end up setting the label to passt_t. Instead, it sets it to passt_exec_t. My understanding is that, similar to many other binaries (e.g. dnsmasq), the context type of the binary file is passt_exec_t, and the rules in the policies use that as an indicator to transition the process to passt_t. Here's the results of a few different relabelling strategies: In the case of Stefano's patch where libvirt's relabelling is completely removed (it just calls virCommandRun() directly), the passt process looks like this: unconfined_u:unconfined_r:passt_t:s0-s0:c0.c1023 97132 When I allow libvirt to do its normal relabelling, it ends up like this: unconfined_u:unconfined_r:svirt_t:s0:c239,c560 104213 When I run it with my patch (which calls getfilecon, then adds the MCS), it looks like this: system_u:object_r:passt_exec_t:s0:c239,c560 Am I correct to think that what everyone is suggesting is that the process ends up like this? (it's what would happen with Stefano's hard-coded hack that this message is replying to): unconfined_u:unconfined_r:passt_t:s0:c239,c560 104213 If so, how are we to derive "passt_t" from "passt_exec_t", since both those names are arbritrary, and some other selinux policy for some other binary might choose names that don't simply differ by having "_exec" added/removed. Is it possible to get the label of the process after it has been forked, and grab the context type from there? In addition to this, after being able to run passt with selinux enabled, I was reminded that, while I've been doing my testing with privileged libvirt (qemu:///system), Stefano has been using unprivileged libvirt (qemu:///session); I've found that when passt is run by a privileged libvirt: 1) it is unable to write a pidfile to /var/run/libvirt/qemu/passt because its selinux policies restrict it /var/run/$UID for pidfiles (since that's where the pidfiles are expected by unprivileged libvirt). 2) it can't write to a logFile in /tmp, because passt's selinux policy restricts it to $HOME of the uid passt is run under (which is problematic since the user "qemu" doesn't have a $HOME :-)) 3) No <portForward>s are possible, because the passt selinux policy forbids them. I've let Stefano know about these. I'm still unsure what to do about getting the right label though. Any advice is welcome.
On Fri, Mar 03, 2023 at 10:03:02AM -0500, Laine Stump wrote:
On 2/23/23 5:47 AM, Daniel P. Berrangé wrote:
This really isn't difficult to do in the security manager IMHO. It is just a variation on the existing virSecurityManagerSetChildProcessLabel method, which instead of using the standard QEMU svirt_t labels, queries the label by calling getfilecon on the binary to be execd and appending the MCS label.
It seems it's not as simple as this.
I have an implementation of this, available at
https://gitlab.com/lainestump/libvirt/-/commits/active-passt-6
The problem with it is that it doesn't end up setting the label to passt_t. Instead, it sets it to passt_exec_t. My understanding is that, similar to many other binaries (e.g. dnsmasq), the context type of the binary file is passt_exec_t, and the rules in the policies use that as an indicator to transition the process to passt_t.
Here's the results of a few different relabelling strategies:
In the case of Stefano's patch where libvirt's relabelling is completely removed (it just calls virCommandRun() directly), the passt process looks like this:
unconfined_u:unconfined_r:passt_t:s0-s0:c0.c1023 97132
When I allow libvirt to do its normal relabelling, it ends up like this:
unconfined_u:unconfined_r:svirt_t:s0:c239,c560 104213
When I run it with my patch (which calls getfilecon, then adds the MCS), it looks like this:
system_u:object_r:passt_exec_t:s0:c239,c560
Am I correct to think that what everyone is suggesting is that the process ends up like this? (it's what would happen with Stefano's hard-coded hack that this message is replying to):
unconfined_u:unconfined_r:passt_t:s0:c239,c560 104213
If so, how are we to derive "passt_t" from "passt_exec_t", since both those names are arbritrary, and some other selinux policy for some other binary might choose names that don't simply differ by having "_exec" added/removed. Is it possible to get the label of the process after it has been forked, and grab the context type from there?
I'm in no way a SELinux expert, but the idea of figuring out the runtime label for the process based on information found on the filesystem makes me uncomfortable. The idea of using some sort of text transformation to get from one to the other, even more so. Since we know that we're launching passt and not some other random helper, why can't we simply use passt_t directly here? It feels like that would be less prone to issues caused by accidental (or intentional) misconfigurations. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 03, 2023 at 07:23:41AM -0800, Andrea Bolognani wrote:
On Fri, Mar 03, 2023 at 10:03:02AM -0500, Laine Stump wrote:
On 2/23/23 5:47 AM, Daniel P. Berrangé wrote:
This really isn't difficult to do in the security manager IMHO. It is just a variation on the existing virSecurityManagerSetChildProcessLabel method, which instead of using the standard QEMU svirt_t labels, queries the label by calling getfilecon on the binary to be execd and appending the MCS label.
It seems it's not as simple as this.
I have an implementation of this, available at
https://gitlab.com/lainestump/libvirt/-/commits/active-passt-6
The problem with it is that it doesn't end up setting the label to passt_t. Instead, it sets it to passt_exec_t. My understanding is that, similar to many other binaries (e.g. dnsmasq), the context type of the binary file is passt_exec_t, and the rules in the policies use that as an indicator to transition the process to passt_t.
Here's the results of a few different relabelling strategies:
In the case of Stefano's patch where libvirt's relabelling is completely removed (it just calls virCommandRun() directly), the passt process looks like this:
unconfined_u:unconfined_r:passt_t:s0-s0:c0.c1023 97132
When I allow libvirt to do its normal relabelling, it ends up like this:
unconfined_u:unconfined_r:svirt_t:s0:c239,c560 104213
When I run it with my patch (which calls getfilecon, then adds the MCS), it looks like this:
system_u:object_r:passt_exec_t:s0:c239,c560
Am I correct to think that what everyone is suggesting is that the process ends up like this? (it's what would happen with Stefano's hard-coded hack that this message is replying to):
unconfined_u:unconfined_r:passt_t:s0:c239,c560 104213
If so, how are we to derive "passt_t" from "passt_exec_t", since both those names are arbritrary, and some other selinux policy for some other binary might choose names that don't simply differ by having "_exec" added/removed. Is it possible to get the label of the process after it has been forked, and grab the context type from there?
I'm in no way a SELinux expert, but the idea of figuring out the runtime label for the process based on information found on the filesystem makes me uncomfortable. The idea of using some sort of text transformation to get from one to the other, even more so.
Using the label on the filesystem is precisely the right way to do this with SELinux. It is what the kernel does every time a binary is invokved, unless the caller has overriden the target type.
Since we know that we're launching passt and not some other random helper, why can't we simply use passt_t directly here? It feels like that would be less prone to issues caused by accidental (or intentional) misconfigurations.
That ties libvirt's code to a specific policy impl which is not a desirable thing. Same reason we don't hardcode svirt_t as a type for QEMU, but instead query it dynamically from the installed policy. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Mar 03, 2023 at 03:47:23PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 07:23:41AM -0800, Andrea Bolognani wrote:
I'm in no way a SELinux expert, but the idea of figuring out the runtime label for the process based on information found on the filesystem makes me uncomfortable. The idea of using some sort of text transformation to get from one to the other, even more so.
Using the label on the filesystem is precisely the right way to do this with SELinux. It is what the kernel does every time a binary is invokved, unless the caller has overriden the target type.
Since we know that we're launching passt and not some other random helper, why can't we simply use passt_t directly here? It feels like that would be less prone to issues caused by accidental (or intentional) misconfigurations.
That ties libvirt's code to a specific policy impl which is not a desirable thing. Same reason we don't hardcode svirt_t as a type for QEMU, but instead query it dynamically from the installed policy.
Do I understand correctly that this happens in virSecuritySELinuxQEMUInitialize(), by parsing the contents of the file located via a call to selinux_virtual_domain_context_path()? Poking around at the other files present in the same directory I see various formats being used, including... XML? It looks like SELinux implements facilities for exposing arbitrary information about the active policy at well-known locations, with (I assume) the explicit purpose of enabling this kind of interaction. So wouldn't that be the way to go for passt, and other helpers too? Have SELinux expose a virtual_helpers_context file, that we can parse to figure out the appropriate labels to use for passt and friends? -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 03, 2023 at 09:06:38AM -0800, Andrea Bolognani wrote:
On Fri, Mar 03, 2023 at 03:47:23PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 07:23:41AM -0800, Andrea Bolognani wrote:
I'm in no way a SELinux expert, but the idea of figuring out the runtime label for the process based on information found on the filesystem makes me uncomfortable. The idea of using some sort of text transformation to get from one to the other, even more so.
Using the label on the filesystem is precisely the right way to do this with SELinux. It is what the kernel does every time a binary is invokved, unless the caller has overriden the target type.
Since we know that we're launching passt and not some other random helper, why can't we simply use passt_t directly here? It feels like that would be less prone to issues caused by accidental (or intentional) misconfigurations.
That ties libvirt's code to a specific policy impl which is not a desirable thing. Same reason we don't hardcode svirt_t as a type for QEMU, but instead query it dynamically from the installed policy.
Do I understand correctly that this happens in virSecuritySELinuxQEMUInitialize(), by parsing the contents of the file located via a call to selinux_virtual_domain_context_path()?
Yes.
Poking around at the other files present in the same directory I see various formats being used, including... XML? It looks like SELinux implements facilities for exposing arbitrary information about the active policy at well-known locations, with (I assume) the explicit purpose of enabling this kind of interaction.
So wouldn't that be the way to go for passt, and other helpers too? Have SELinux expose a virtual_helpers_context file, that we can parse to figure out the appropriate labels to use for passt and friends?
No, I don't think so. The helpers file is a bit of a special case that was needed because there were multiple contexts we needed to cope with for running QEMU. I don't see any reason not to follow what the kernel already does by relying on the labelled file context. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Mar 03, 2023 at 05:15:43PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 09:06:38AM -0800, Andrea Bolognani wrote:
Since we know that we're launching passt and not some other random helper, why can't we simply use passt_t directly here? It feels like that would be less prone to issues caused by accidental (or intentional) misconfigurations.
That ties libvirt's code to a specific policy impl which is not a desirable thing. Same reason we don't hardcode svirt_t as a type for QEMU, but instead query it dynamically from the installed policy.
Do I understand correctly that this happens in virSecuritySELinuxQEMUInitialize(), by parsing the contents of the file located via a call to selinux_virtual_domain_context_path()?
Yes.
Poking around at the other files present in the same directory I see various formats being used, including... XML? It looks like SELinux implements facilities for exposing arbitrary information about the active policy at well-known locations, with (I assume) the explicit purpose of enabling this kind of interaction.
So wouldn't that be the way to go for passt, and other helpers too? Have SELinux expose a virtual_helpers_context file, that we can parse to figure out the appropriate labels to use for passt and friends?
No, I don't think so. The helpers file is a bit of a special case that was needed because there were multiple contexts we needed to cope with for running QEMU.
I don't see any reason not to follow what the kernel already does by relying on the labelled file context.
Right, but wouldn't the idea of poking at the filesystem to retrieve the label from the binary (passt_exec_t) and then applying a text transformation to obtain the runtime label (passt_t) go directly against the idea of not hardcoding information about a specific policy implementation into libvirt? Looking at the SELinux policy for Fedora, the virt module contains a bunch of instances where an external policy is imported in order to give virtqemud the ability to run external commands. For example, for dnsmasq we have optional_policy(` dnsmasq_domtrans(virtd_t) dnsmasq_signal(virtd_t) dnsmasq_kill(virtd_t) dnsmasq_signull(virtd_t) dnsmasq_create_pid_dirs(virtd_t) dnsmasq_filetrans_named_content_fromdir(virtd_t, virt_var_run_t); dnsmasq_manage_pid_files(virtd_t) ') This looks pretty similar to what Stefano has proposed doing for passt in [1]: optional_policy(` passt_socket(virt_domain) ') optional_policy(` passt_domtrans(virtd_t) passt_kill(virtd_t) ') As I understand it, such a policy would allow virtqemud (virtd_t) to execute passt (passt_exec_t) and automatically result in a transition of the process to the desired context (passt_t). Why would this approach not be okay? [1] https://github.com/fedora-selinux/selinux-policy/pull/1613 -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 03, 2023 at 09:56:55AM -0800, Andrea Bolognani wrote:
On Fri, Mar 03, 2023 at 05:15:43PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 09:06:38AM -0800, Andrea Bolognani wrote:
Since we know that we're launching passt and not some other random helper, why can't we simply use passt_t directly here? It feels like that would be less prone to issues caused by accidental (or intentional) misconfigurations.
That ties libvirt's code to a specific policy impl which is not a desirable thing. Same reason we don't hardcode svirt_t as a type for QEMU, but instead query it dynamically from the installed policy.
Do I understand correctly that this happens in virSecuritySELinuxQEMUInitialize(), by parsing the contents of the file located via a call to selinux_virtual_domain_context_path()?
Yes.
Poking around at the other files present in the same directory I see various formats being used, including... XML? It looks like SELinux implements facilities for exposing arbitrary information about the active policy at well-known locations, with (I assume) the explicit purpose of enabling this kind of interaction.
So wouldn't that be the way to go for passt, and other helpers too? Have SELinux expose a virtual_helpers_context file, that we can parse to figure out the appropriate labels to use for passt and friends?
No, I don't think so. The helpers file is a bit of a special case that was needed because there were multiple contexts we needed to cope with for running QEMU.
I don't see any reason not to follow what the kernel already does by relying on the labelled file context.
Right, but wouldn't the idea of poking at the filesystem to retrieve the label from the binary (passt_exec_t) and then applying a text transformation to obtain the runtime label (passt_t) go directly against the idea of not hardcoding information about a specific policy implementation into libvirt?
I'm not suggesting applying a text transformation. The example code using libselinux I described in the other reply actually askes the kernel to tell us what the target type will be when a process labelled passt_exec_t is execd. The answer we get back (passt_t) will come from the SELinux policy that is currently loaded in the kernel. This is doing exactly the same thing that would happen automatically if we had a normal exec() without the MCS stuff involved.
As I understand it, such a policy would allow virtqemud (virtd_t) to execute passt (passt_exec_t) and automatically result in a transition of the process to the desired context (passt_t).
Yes, and I'm saying we must ask the kernel to tell us what that target context will be for the loaded policy, given the source file context. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Mar 03, 2023 at 06:06:05PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 09:56:55AM -0800, Andrea Bolognani wrote:
Right, but wouldn't the idea of poking at the filesystem to retrieve the label from the binary (passt_exec_t) and then applying a text transformation to obtain the runtime label (passt_t) go directly against the idea of not hardcoding information about a specific policy implementation into libvirt?
I'm not suggesting applying a text transformation. The example code using libselinux I described in the other reply actually askes the kernel to tell us what the target type will be when a process labelled passt_exec_t is execd.
Yeah, that's a lot better.
As I understand it, such a policy would allow virtqemud (virtd_t) to execute passt (passt_exec_t) and automatically result in a transition of the process to the desired context (passt_t).
Yes, and I'm saying we must ask the kernel to tell us what that target context will be for the loaded policy, given the source file context.
I still don't understand why we can't simply execute passt and let the domain transition defined in the policy take care of switching to the appropriate label from us, like we do for dnsmasq and other tools? Why do we need to do things differently for passt? -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 03, 2023 at 10:18:39AM -0800, Andrea Bolognani wrote:
On Fri, Mar 03, 2023 at 06:06:05PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 09:56:55AM -0800, Andrea Bolognani wrote:
Right, but wouldn't the idea of poking at the filesystem to retrieve the label from the binary (passt_exec_t) and then applying a text transformation to obtain the runtime label (passt_t) go directly against the idea of not hardcoding information about a specific policy implementation into libvirt?
I'm not suggesting applying a text transformation. The example code using libselinux I described in the other reply actually askes the kernel to tell us what the target type will be when a process labelled passt_exec_t is execd.
Yeah, that's a lot better.
As I understand it, such a policy would allow virtqemud (virtd_t) to execute passt (passt_exec_t) and automatically result in a transition of the process to the desired context (passt_t).
Yes, and I'm saying we must ask the kernel to tell us what that target context will be for the loaded policy, given the source file context.
I still don't understand why we can't simply execute passt and let the domain transition defined in the policy take care of switching to the appropriate label from us, like we do for dnsmasq and other tools? Why do we need to do things differently for passt?
That won't get the per-VM label applied. It will end up running passt_t:s0:c0.c1023, but we want it to be passt_t:s0:c342,155. To transition from non-MCS to MCS, you have to explicitly tell the kernel what to do instead of relying on the plain automatic transition. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/3/23 1:36 PM, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 10:18:39AM -0800, Andrea Bolognani wrote:
On Fri, Mar 03, 2023 at 06:06:05PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 09:56:55AM -0800, Andrea Bolognani wrote:
Right, but wouldn't the idea of poking at the filesystem to retrieve the label from the binary (passt_exec_t) and then applying a text transformation to obtain the runtime label (passt_t) go directly against the idea of not hardcoding information about a specific policy implementation into libvirt?
I'm not suggesting applying a text transformation. The example code using libselinux I described in the other reply actually askes the kernel to tell us what the target type will be when a process labelled passt_exec_t is execd.
Yeah, that's a lot better.
As I understand it, such a policy would allow virtqemud (virtd_t) to execute passt (passt_exec_t) and automatically result in a transition of the process to the desired context (passt_t).
Yes, and I'm saying we must ask the kernel to tell us what that target context will be for the loaded policy, given the source file context.
I still don't understand why we can't simply execute passt and let the domain transition defined in the policy take care of switching to the appropriate label from us, like we do for dnsmasq and other tools? Why do we need to do things differently for passt?
That won't get the per-VM label applied. It will end up running passt_t:s0:c0.c1023, but we want it to be passt_t:s0:c342,155. To transition from non-MCS to MCS, you have to explicitly tell the kernel what to do instead of relying on the plain automatic transition.
Since you've brought up dnsmasq as an example, note that the dnsmasq processes don't have any MCS (which I guess is the right thing, since a single dnsmasq might be used by many/any/all guests, contrasting with passt, or the slirp-helper or tpm, which have one instance per guest device. This does make dnsmasq a "not ideal" example when figuring out what is needed for passt though (in some ways, but not others)(I think? I still can't claim to fully grok all the details of this).
On Fri, Mar 03, 2023 at 07:46:27PM -0500, Laine Stump wrote:
On 3/3/23 1:36 PM, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 10:18:39AM -0800, Andrea Bolognani wrote:
On Fri, Mar 03, 2023 at 06:06:05PM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 09:56:55AM -0800, Andrea Bolognani wrote:
Right, but wouldn't the idea of poking at the filesystem to retrieve the label from the binary (passt_exec_t) and then applying a text transformation to obtain the runtime label (passt_t) go directly against the idea of not hardcoding information about a specific policy implementation into libvirt?
I'm not suggesting applying a text transformation. The example code using libselinux I described in the other reply actually askes the kernel to tell us what the target type will be when a process labelled passt_exec_t is execd.
Yeah, that's a lot better.
As I understand it, such a policy would allow virtqemud (virtd_t) to execute passt (passt_exec_t) and automatically result in a transition of the process to the desired context (passt_t).
Yes, and I'm saying we must ask the kernel to tell us what that target context will be for the loaded policy, given the source file context.
I still don't understand why we can't simply execute passt and let the domain transition defined in the policy take care of switching to the appropriate label from us, like we do for dnsmasq and other tools? Why do we need to do things differently for passt?
That won't get the per-VM label applied. It will end up running passt_t:s0:c0.c1023, but we want it to be passt_t:s0:c342,155. To transition from non-MCS to MCS, you have to explicitly tell the kernel what to do instead of relying on the plain automatic transition.
Since you've brought up dnsmasq as an example, note that the dnsmasq processes don't have any MCS (which I guess is the right thing, since a single dnsmasq might be used by many/any/all guests, contrasting with passt, or the slirp-helper or tpm, which have one instance per guest device. This does make dnsmasq a "not ideal" example when figuring out what is needed for passt though (in some ways, but not others)(I think? I still can't claim to fully grok all the details of this).
Yes, you are correct. dbus/slirp/swtpm are better matches for the passt scenario, though they are also not useful examples since we've ignored the SELinux labeling needs in all of them, so they all undermine svirt if used. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Mar 06, 2023 at 09:03:42AM +0000, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 07:46:27PM -0500, Laine Stump wrote:
On 3/3/23 1:36 PM, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 10:18:39AM -0800, Andrea Bolognani wrote:
I still don't understand why we can't simply execute passt and let the domain transition defined in the policy take care of switching to the appropriate label from us, like we do for dnsmasq and other tools? Why do we need to do things differently for passt?
That won't get the per-VM label applied. It will end up running passt_t:s0:c0.c1023, but we want it to be passt_t:s0:c342,155. To transition from non-MCS to MCS, you have to explicitly tell the kernel what to do instead of relying on the plain automatic transition.
Since you've brought up dnsmasq as an example, note that the dnsmasq processes don't have any MCS (which I guess is the right thing, since a single dnsmasq might be used by many/any/all guests, contrasting with passt, or the slirp-helper or tpm, which have one instance per guest device. This does make dnsmasq a "not ideal" example when figuring out what is needed for passt though (in some ways, but not others)(I think? I still can't claim to fully grok all the details of this).
Yes, you are correct.
dbus/slirp/swtpm are better matches for the passt scenario, though they are also not useful examples since we've ignored the SELinux labeling needs in all of them, so they all undermine svirt if used.
Got it! The MCS bit is indeed the part that I had been missing. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Mar 03, 2023 at 10:03:02AM -0500, Laine Stump wrote:
On 2/23/23 5:47 AM, Daniel P. Berrangé wrote:
This really isn't difficult to do in the security manager IMHO. It is just a variation on the existing virSecurityManagerSetChildProcessLabel method, which instead of using the standard QEMU svirt_t labels, queries the label by calling getfilecon on the binary to be execd and appending the MCS label.
It seems it's not as simple as this.
I have an implementation of this, available at
https://gitlab.com/lainestump/libvirt/-/commits/active-passt-6
The problem with it is that it doesn't end up setting the label to passt_t. Instead, it sets it to passt_exec_t. My understanding is that, similar to many other binaries (e.g. dnsmasq), the context type of the binary file is passt_exec_t, and the rules in the policies use that as an indicator to transition the process to passt_t.
Oppps, yes, you are correct, I forgot about this distinction. All is not lost, however, we just need to lookup the latter based on the former. This is something the libselinux.so setexecfilecon() is already able todo, but that's not an API that's convenient for us to use. The bit of logic inside it though is easy enough rc = getcon(&mycon); if (rc < 0) goto out; rc = getfilecon(filename, &fcon); if (rc < 0) goto out; rc = security_compute_create(mycon, fcon, string_to_security_class("process"), &newcon); if (rc < 0) goto out; In this snippet of code * 'mycon' is going to be virtd_t as we're inside libvirtd * 'fcon' will get filled with passt_exec_t * After security_compute_create returns, 'newcon' will be filled with 'passt_t'.
1) it is unable to write a pidfile to /var/run/libvirt/qemu/passt because its selinux policies restrict it /var/run/$UID for pidfiles (since that's where the pidfiles are expected by unprivileged libvirt).
2) it can't write to a logFile in /tmp, because passt's selinux policy restricts it to $HOME of the uid passt is run under (which is problematic since the user "qemu" doesn't have a $HOME :-))
Nothing must ever write to /tmp, so definitely needs fixing to use a per-VM log location.
3) No <portForward>s are possible, because the passt selinux policy forbids them.
With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 3/3/23 10:44 AM, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 10:03:02AM -0500, Laine Stump wrote:
On 2/23/23 5:47 AM, Daniel P. Berrangé wrote:
This really isn't difficult to do in the security manager IMHO. It is just a variation on the existing virSecurityManagerSetChildProcessLabel method, which instead of using the standard QEMU svirt_t labels, queries the label by calling getfilecon on the binary to be execd and appending the MCS label.
It seems it's not as simple as this.
I have an implementation of this, available at
https://gitlab.com/lainestump/libvirt/-/commits/active-passt-6
The problem with it is that it doesn't end up setting the label to passt_t. Instead, it sets it to passt_exec_t. My understanding is that, similar to many other binaries (e.g. dnsmasq), the context type of the binary file is passt_exec_t, and the rules in the policies use that as an indicator to transition the process to passt_t.
Oppps, yes, you are correct, I forgot about this distinction.
All is not lost, however, we just need to lookup the latter based on the former. This is something the libselinux.so setexecfilecon() is already able todo, but that's not an API that's convenient for us to use. The bit of logic inside it though is easy enough
rc = getcon(&mycon); if (rc < 0) goto out;
rc = getfilecon(filename, &fcon); if (rc < 0) goto out;
rc = security_compute_create(mycon, fcon, string_to_security_class("process"), &newcon); if (rc < 0) goto out;
In this snippet of code
* 'mycon' is going to be virtd_t as we're inside libvirtd * 'fcon' will get filled with passt_exec_t * After security_compute_create returns, 'newcon' will be filled with 'passt_t'.
Thanks for the example code, that will save a lot of time :-). Something I meant to ask about in my first message but somehow didn't - what about the "role" and "user" part of the label? In each of my examples (which I see aren't in the quoted part here, but I guess you can go back to the previous message), the role and user are different (either unconfined_u:unconfined_r or system_u:object_r). I *think* we want that to be unconfined_u:unconfined_r, right? (In other words, we want everything to be the same as the "common" label (including MCS) that we normally would have set for the child process, but with only the "type" changed, right?)
On Fri, Mar 03, 2023 at 07:56:18PM -0500, Laine Stump wrote:
On 3/3/23 10:44 AM, Daniel P. Berrangé wrote:
On Fri, Mar 03, 2023 at 10:03:02AM -0500, Laine Stump wrote:
On 2/23/23 5:47 AM, Daniel P. Berrangé wrote:
This really isn't difficult to do in the security manager IMHO. It is just a variation on the existing virSecurityManagerSetChildProcessLabel method, which instead of using the standard QEMU svirt_t labels, queries the label by calling getfilecon on the binary to be execd and appending the MCS label.
It seems it's not as simple as this.
I have an implementation of this, available at
https://gitlab.com/lainestump/libvirt/-/commits/active-passt-6
The problem with it is that it doesn't end up setting the label to passt_t. Instead, it sets it to passt_exec_t. My understanding is that, similar to many other binaries (e.g. dnsmasq), the context type of the binary file is passt_exec_t, and the rules in the policies use that as an indicator to transition the process to passt_t.
Oppps, yes, you are correct, I forgot about this distinction.
All is not lost, however, we just need to lookup the latter based on the former. This is something the libselinux.so setexecfilecon() is already able todo, but that's not an API that's convenient for us to use. The bit of logic inside it though is easy enough
rc = getcon(&mycon); if (rc < 0) goto out;
rc = getfilecon(filename, &fcon); if (rc < 0) goto out;
rc = security_compute_create(mycon, fcon, string_to_security_class("process"), &newcon); if (rc < 0) goto out;
In this snippet of code
* 'mycon' is going to be virtd_t as we're inside libvirtd * 'fcon' will get filled with passt_exec_t * After security_compute_create returns, 'newcon' will be filled with 'passt_t'.
Thanks for the example code, that will save a lot of time :-).
Something I meant to ask about in my first message but somehow didn't - what about the "role" and "user" part of the label? In each of my examples (which I see aren't in the quoted part here, but I guess you can go back to the previous message), the role and user are different (either unconfined_u:unconfined_r or system_u:object_r). I *think* we want that to be unconfined_u:unconfined_r, right? (In other words, we want everything to be the same as the "common" label (including MCS) that we normally would have set for the child process, but with only the "type" changed, right?)
I believe that security_compute_create will copy the role/user from the 'mycon' argument, ie it will be the same as libvirtd itself. So I think you can mostly ignore user/role as it'll do the right thing. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
qemuSecurityCommandRun() would have dealt with this (if UID and GID had been passed). With virCommandRun() we need separate, explicit calls. Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- src/qemu/qemu_passt.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 81f48dd630..61e7047354 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -157,6 +157,7 @@ qemuPasstStart(virDomainObj *vm, { qemuDomainObjPrivate *priv = vm->privateData; virQEMUDriver *driver = priv->driver; + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net); g_autoptr(virCommand) cmd = NULL; g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net); @@ -174,6 +175,11 @@ qemuPasstStart(virDomainObj *vm, virCommandClearCaps(cmd); virCommandSetErrorBuffer(cmd, &errbuf); + if (cfg->user != (uid_t) -1) + virCommandSetUID(cmd, cfg->user); + if (cfg->group != (gid_t) -1) + virCommandSetGID(cmd, cfg->group); + virCommandAddArgList(cmd, "--one-off", "--socket", passtSocketName, -- 2.39.1
Just like it can't remove its own PID files, passt can't unlink its own socket upon exit (unless the initialisation fails), because it has no access to the filesystem at runtime. Remove the socket file in qemuPasstKill(). Fixes: a56f0168d576 ("qemu: hook up passt config to qemu domains") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> --- src/qemu/qemu_passt.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 61e7047354..d5df3bb3f7 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -108,7 +108,7 @@ qemuPasstAddNetProps(virDomainObj *vm, static void -qemuPasstKill(const char *pidfile) +qemuPasstKill(const char *pidfile, const char *passtSocketName) { virErrorPtr orig_err; pid_t pid = 0; @@ -120,6 +120,8 @@ qemuPasstKill(const char *pidfile) virProcessKillPainfully(pid, true); unlink(pidfile); + unlink(passtSocketName); + virErrorRestore(&orig_err); } @@ -129,8 +131,9 @@ qemuPasstStop(virDomainObj *vm, virDomainNetDef *net) { g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net); + g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net); - qemuPasstKill(pidfile); + qemuPasstKill(pidfile, passtSocketName); } -- 2.39.1
participants (6)
-
Andrea Bolognani -
Daniel P. Berrangé -
Jiri Denemark -
Laine Stump -
Michal Prívozník -
Stefano Brivio