passt SELinux labelling (was: Re: [PATCH v2 1/3] qemu_passt: Don't make passt transition to svirt_t/libvirt_domain on start)

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(a)redhat.com&gt; >> --- >> 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.