A need was found to set the SELinux context label on an open fd (a pipe, as a matter of fact). This patch adds a function to the security driver API that will set the label on an open fd to secdef.label. For all drivers other than the SELinux driver, it's a NOP. For the SElinux driver, it calls fsetfilecon(). If the return is a failure, it only returns error up to the caller if 1) the desired label is different from the existing label, 2) the destination fd is of a type that supports setting the selinux context, and 3) selinux is in enforcing mode. Otherwise it will return success. This follows the pattern of the existing function SELinuxSetFilecon(). --- src/libvirt_private.syms | 1 + src/security/security_apparmor.c | 10 +++++++ src/security/security_dac.c | 10 +++++++ src/security/security_driver.h | 6 +++- src/security/security_manager.c | 11 +++++++ src/security/security_manager.h | 3 ++ src/security/security_nop.c | 9 ++++++ src/security/security_selinux.c | 54 ++++++++++++++++++++++++++++++++++++++ src/security/security_stack.c | 18 ++++++++++++ 9 files changed, 121 insertions(+), 1 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index f60489c..c7e4772 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -723,6 +723,7 @@ virSecurityManagerRestoreAllLabel; virSecurityManagerRestoreHostdevLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; +virSecurityManagerSetFDLabel; virSecurityManagerSetImageLabel; virSecurityManagerSetHostdevLabel; virSecurityManagerSetProcessLabel; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index d82ba73..7dc01ac 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -798,6 +798,14 @@ AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr, return reload_profile(mgr, vm, NULL, false); } +static int +AppArmorSetFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED) +{ + return 0; +} + virSecurityDriver virAppArmorSecurityDriver = { 0, SECURITY_APPARMOR_NAME, @@ -831,4 +839,6 @@ virSecurityDriver virAppArmorSecurityDriver = { AppArmorSetSavedStateLabel, AppArmorRestoreSavedStateLabel, + + AppArmorSetFDLabel, }; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0f24034..52353a3 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -675,6 +675,14 @@ virSecurityDACClearSocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return 0; } +static int +virSecurityDACSetFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED) +{ + return 0; +} + virSecurityDriver virSecurityDriverDAC = { sizeof(virSecurityDACData), @@ -710,4 +718,6 @@ virSecurityDriver virSecurityDriverDAC = { virSecurityDACSetSavedStateLabel, virSecurityDACRestoreSavedStateLabel, + + virSecurityDACSetFDLabel, }; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index e5a8d41..42dfcb8 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -79,7 +79,9 @@ typedef int (*virSecurityDomainSetProcessLabel) (virSecurityManagerPtr mgr, virDomainObjPtr vm); typedef int (*virSecurityDomainSecurityVerify) (virSecurityManagerPtr mgr, virDomainDefPtr def); - +typedef int (*virSecurityDomainSetFDLabel) (virSecurityManagerPtr mgr, + virDomainObjPtr vm, + int fd); struct _virSecurityDriver { size_t privateDataLen; @@ -114,6 +116,8 @@ struct _virSecurityDriver { virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel; virSecurityDomainRestoreSavedStateLabel domainRestoreSavedStateLabel; + + virSecurityDomainSetFDLabel domainSetSecurityFDLabel; }; virSecurityDriverPtr virSecurityDriverLookup(const char *name); diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 6406161..0246dd8 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -323,3 +323,14 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr, virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); return -1; } + +int virSecurityManagerSetFDLabel(virSecurityManagerPtr mgr, + virDomainObjPtr vm, + int fd) +{ + if (mgr->drv->domainSetSecurityFDLabel) + return mgr->drv->domainSetSecurityFDLabel(mgr, vm, fd); + + virSecurityReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; +} diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 189b6b4..3f88801 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -91,5 +91,8 @@ int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, virDomainObjPtr vm); int virSecurityManagerVerify(virSecurityManagerPtr mgr, virDomainDefPtr def); +int virSecurityManagerSetFDLabel(virSecurityManagerPtr mgr, + virDomainObjPtr vm, + int fd); #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 6d7cb47..24d36fe 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -149,6 +149,13 @@ static int virSecurityDomainVerifyNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED return 0; } +static int virSecurityDomainSetFDLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainObjPtr sec ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED) +{ + return 0; +} + virSecurityDriver virSecurityDriverNop = { 0, "none", @@ -182,4 +189,6 @@ virSecurityDriver virSecurityDriverNop = { virSecurityDomainSetSavedStateLabelNop, virSecurityDomainRestoreSavedStateLabelNop, + + virSecurityDomainSetFDLabelNop, }; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7b71fd9..24609bc 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -371,6 +371,45 @@ SELinuxSetFilecon(const char *path, char *tcon) return 0; } +static int +SELinuxFSetFilecon(int fd, char *tcon) +{ + security_context_t econ; + + VIR_INFO("Setting SELinux context on fd %d to '%s'", fd, tcon); + + if (fsetfilecon(fd, tcon) < 0) { + int fsetfilecon_errno = errno; + + if (fgetfilecon(fd, &econ) >= 0) { + if (STREQ(tcon, econ)) { + freecon(econ); + /* It's alright, there's nothing to change anyway. */ + return 0; + } + freecon(econ); + } + + /* if the error complaint is related to an image hosted on + * an nfs mount, or a usbfs/sysfs filesystem not supporting + * labelling, then just ignore it & hope for the best. + * The user hopefully set one of the necessary SELinux + * virt_use_{nfs,usb,pci} boolean tunables to allow it... + */ + if (fsetfilecon_errno != EOPNOTSUPP) { + virReportSystemError(fsetfilecon_errno, + _("unable to set security context '%s' on fd %d"), + tcon, fd); + if (security_getenforce() == 1) + return -1; + } else { + VIR_INFO("Setting security context '%s' on fd %d not supported", + tcon, fd); + } + } + return 0; +} + /* Set fcon to the appropriate label for path and mode, or return -1. */ static int getContext(const char *newpath, mode_t mode, security_context_t *fcon) @@ -1087,6 +1126,19 @@ SELinuxSetSecurityAllLabel(virSecurityManagerPtr mgr, return 0; } +static int +SELinuxSetFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + int fd) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + + if (secdef->imagelabel == NULL) + return 0; + + return SELinuxFSetFilecon(fd, secdef->imagelabel); +} + virSecurityDriver virSecurityDriverSELinux = { 0, SECURITY_SELINUX_NAME, @@ -1120,4 +1172,6 @@ virSecurityDriver virSecurityDriverSELinux = { SELinuxSetSavedStateLabel, SELinuxRestoreSavedStateLabel, + + SELinuxSetFDLabel, }; diff --git a/src/security/security_stack.c b/src/security/security_stack.c index e8bb058..79b3e1f 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -364,6 +364,22 @@ virSecurityStackClearSocketLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackSetFDLabel(virSecurityManagerPtr mgr, + virDomainObjPtr vm, + int fd) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + int rc = 0; + + if (virSecurityManagerSetFDLabel(priv->secondary, vm, fd) < 0) + rc = -1; + if (virSecurityManagerSetFDLabel(priv->primary, vm, fd) < 0) + rc = -1; + + return rc; +} + virSecurityDriver virSecurityDriverStack = { sizeof(virSecurityStackData), @@ -398,4 +414,6 @@ virSecurityDriver virSecurityDriverStack = { virSecurityStackSetSavedStateLabel, virSecurityStackRestoreSavedStateLabel, + + virSecurityStackSetFDLabel, }; -- 1.7.3.4

This patch is a partial resolution to the following bug: https://bugzilla.redhat.com/show_bug.cgi?id=667756 (to complete the fix, an updated selinux-policy package is required, to add the policy that allows libvirt to set the context of a fifo, which was previously not allowed). Explanation : When an incoming migration is over a pipe (for example, if the image was compressed and is being fed through gzip, or was on a root-squash nfs server, so needed to be opened by a child process running as a different uid), qemu cannot read it unless the selinux context label for the pipe has been set properly. The solution is to check the fd used as the source of the migration just before passing it to qemu; if it's a fifo (implying that it's a pipe), we call the newly added virSecurityManagerSetFDLabel() function to set the context properly. --- src/qemu/qemu_driver.c | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 34cc29f..985b062 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2667,6 +2667,24 @@ static int qemudStartVMDaemon(virConnectPtr conn, vm, stdin_path) < 0) goto cleanup; + if (stdin_fd != -1) { + /* if there's an fd to migrate from, and it's a pipe, put the + * proper security label on it + */ + struct stat stdin_sb; + + DEBUG0("setting security label on pipe used for migration"); + + if (fstat(stdin_fd, &stdin_sb) < 0) { + virReportSystemError(errno, + _("cannot stat fd %d"), stdin_fd); + goto cleanup; + } + if (S_ISFIFO(stdin_sb.st_mode) && + virSecurityManagerSetFDLabel(driver->securityManager, vm, stdin_fd) < 0) + goto cleanup; + } + /* Ensure no historical cgroup for this VM is lying around bogus * settings */ DEBUG0("Ensuring no historical cgroup is lying around"); -- 1.7.3.4

On 01/25/2011 12:49 PM, Daniel P. Berrange wrote: That was my first intent too. However, in the case of an image on root-squashed NFS, the knowledge about whether to directly open, or open via a pipe to a child process, is made in qemudOpenAsUID(), which doesn't have access to the domainObj, so cannot call the security driver. In a broader view, qemudOpenAsUID() is really a potentially general purpose function that could be used outside of this context some day, but gumming it up with application-specific things like a domainObj would lock it into being specific to domain-related functions. More specifically (and importantly), the domainObj hasn't even been constructed until after qemudOpenAsUID() is finished and returned, since it's going to be created by the caller from the header of the file that qemudOpenAsUID() has just opened. So by the time we have the domainObj, we no longer have the knowledge that the fd is actually the read side of a pipe - we would still have to call fstat (or clutter up the calling sequence to pass back an "is_fifo" bool or something, which sounds even less right). Compared to that, putting the call to SetFDLabel() in a single place, qualified by fstat() to see if the fd was a fifo, seemed like a much less intrusive change to the code. (The other instance of a pipe being created (for compression) is less problematic, as everything we need is already there. However, since we're already doing the fstat() for the root-squash case, and since doing two FDSetLabels() would be superfluous (in the case of a compressed image stored on a root-squashed share), I figured we might as well have a single call in a common place (which, by the way, is strategically located as late as possible, so that any future additions of pipes will automatically be caught and accounted for).) However, If anyone has a suggestion for dealing with the chicken-egg problem of domainObj vs fifo that is less ugly, please speak up, and I'll be happy to implement it! :-) (NB: I have an idea in the back of my head that all files in libvirt could be opened by a VIR_OPEN() that would automatically handle "the root squash problem", possibly in some other manner than qemudOpenAsUID() (using SCM_RIGHTS maybe?), and in that case we wouldn't have the option of passing the security label down to the innards of the utility function even if we had it (unless we wanted to wire up every single VIR_OPEN() with an extra NULL arg or something).

On Wed, Jan 26, 2011 at 09:00:14AM -0500, Laine Stump wrote: ACK Daniel