Parsing 'user:group' is useful even outside the DAC security driver, so expose the most abstract function which has no DAC security driver bits in itself. Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/security/security_dac.c | 51 +++-------------------------------------- src/util/virutil.c | 56 +++++++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 4 files changed, 62 insertions(+), 48 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9d5f74b..1927451 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1951,6 +1951,7 @@ virIsCapableVport; virIsDevMapperDevice; virManageVport; virParseNumber; +virParseOwnershipIds; virParseVersionString; virPipeReadUntilEOF; virReadFCHost; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b8d1a92..0264c28 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -33,6 +33,7 @@ #include "virscsi.h" #include "virstoragefile.h" #include "virstring.h" +#include "virutil.h" #define VIR_FROM_THIS VIR_FROM_SECURITY #define SECURITY_DAC_NAME "dac" @@ -70,52 +71,6 @@ virSecurityDACSetDynamicOwnership(virSecurityManagerPtr mgr, priv->dynamicOwnership = dynamicOwnership; } -static int -parseIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) -{ - int rc = -1; - uid_t theuid; - gid_t thegid; - char *tmp_label = NULL; - char *sep = NULL; - char *owner = NULL; - char *group = NULL; - - if (VIR_STRDUP(tmp_label, label) < 0) - goto cleanup; - - /* Split label */ - sep = strchr(tmp_label, ':'); - if (sep == NULL) { - virReportError(VIR_ERR_INVALID_ARG, - _("Missing separator ':' in DAC label \"%s\""), - label); - goto cleanup; - } - *sep = '\0'; - owner = tmp_label; - group = sep + 1; - - /* Parse owner and group, error message is defined by - * virGetUserID or virGetGroupID. - */ - if (virGetUserID(owner, &theuid) < 0 || - virGetGroupID(group, &thegid) < 0) - goto cleanup; - - if (uidPtr) - *uidPtr = theuid; - if (gidPtr) - *gidPtr = thegid; - - rc = 0; - -cleanup: - VIR_FREE(tmp_label); - - return rc; -} - /* returns 1 if label isn't found, 0 on success, -1 on error */ static int virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) @@ -133,7 +88,7 @@ virSecurityDACParseIds(virDomainDefPtr def, uid_t *uidPtr, gid_t *gidPtr) return 1; } - if (parseIds(seclabel->label, &uid, &gid) < 0) + if (virParseOwnershipIds(seclabel->label, &uid, &gid) < 0) return -1; if (uidPtr) @@ -194,7 +149,7 @@ virSecurityDACParseImageIds(virDomainDefPtr def, return 1; } - if (parseIds(seclabel->imagelabel, &uid, &gid) < 0) + if (virParseOwnershipIds(seclabel->imagelabel, &uid, &gid) < 0) return -1; if (uidPtr) diff --git a/src/util/virutil.c b/src/util/virutil.c index 028f1d1..450e5e3 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2071,3 +2071,59 @@ virCompareLimitUlong(unsigned long long a, unsigned long b) return -1; } + +/** + * virParseOwnershipIds: + * + * Parse the usual "uid:gid" ownership specification into uid_t and + * gid_t passed as parameters. NULL value for those parameters mean + * the information is not needed. Also, none of those values are + * changed in case of any error. + * + * Returns -1 on error, 0 otherwise. + */ +int +virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr) +{ + int rc = -1; + uid_t theuid; + gid_t thegid; + char *tmp_label = NULL; + char *sep = NULL; + char *owner = NULL; + char *group = NULL; + + if (VIR_STRDUP(tmp_label, label) < 0) + goto cleanup; + + /* Split label */ + sep = strchr(tmp_label, ':'); + if (sep == NULL) { + virReportError(VIR_ERR_INVALID_ARG, + _("Failed to parse uid and gid from '%s'"), + label); + goto cleanup; + } + *sep = '\0'; + owner = tmp_label; + group = sep + 1; + + /* Parse owner and group, error message is defined by + * virGetUserID or virGetGroupID. + */ + if (virGetUserID(owner, &theuid) < 0 || + virGetGroupID(group, &thegid) < 0) + goto cleanup; + + if (uidPtr) + *uidPtr = theuid; + if (gidPtr) + *gidPtr = thegid; + + rc = 0; + +cleanup: + VIR_FREE(tmp_label); + + return rc; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 280a18d..0f6bcc1 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -166,4 +166,6 @@ char *virFindFCHostCapableVport(const char *sysfs_prefix); int virCompareLimitUlong(unsigned long long a, unsigned long b); +int virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); + #endif /* __VIR_UTIL_H__ */ -- 1.8.2.1

Function qemuOpenFile() haven't had any idea about seclabels applied to VMs only, so in case the seclabel differed from the "user:group" from configuration, there might have been issues with opening files. Make qemuOpenFile() VM-aware, but only optionally, passing NULL argument means skipping VM seclabel info completely. However, all current qemuOpenFile() calls look like they should use VM seclabel info in case there is any, so convert these calls as well. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=869053 Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 71 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 16 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4a76f14..87dedef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -147,6 +147,16 @@ static int qemuDomainGetMaxVcpus(virDomainPtr dom); static int qemuDomainManagedSaveLoad(virDomainObjPtr vm, void *opaque); +static int qemuOpenFile(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver); + +static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, + bool dynamicOwnership, + const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver); + virQEMUDriverPtr qemu_driver = NULL; @@ -2551,11 +2561,41 @@ qemuCompressGetCommand(virQEMUSaveFormat compression) } /* Internal function to properly create or open existing files, with - * ownership affected by qemu driver setup. */ + * ownership affected by qemu driver setup and domain DAC label. */ static int -qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, +qemuOpenFile(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, int oflags, bool *needUnlink, bool *bypassSecurityDriver) { + int ret = -1; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + uid_t user = cfg->user; + gid_t group = cfg->group; + bool dynamicOwnership = cfg->dynamicOwnership; + virSecurityLabelDefPtr seclabel; + + virObjectUnref(cfg); + + /* TODO: Take imagelabel into account? */ + if (vm && + (seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL && + (virParseOwnershipIds(seclabel->label, &user, &group) < 0)) + goto cleanup; + + ret = qemuOpenFileAs(user, group, dynamicOwnership, + path, oflags, needUnlink, bypassSecurityDriver); + + cleanup: + return ret; +} + +static int +qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, + bool dynamicOwnership, + const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver) +{ struct stat sb; bool is_reg = true; bool need_unlink = false; @@ -2565,7 +2605,6 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, int path_shared = virStorageFileIsSharedFS(path); uid_t uid = getuid(); gid_t gid = getgid(); - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); /* path might be a pre-existing block dev, in which case * we need to skip the create step, and also avoid unlink @@ -2575,7 +2614,7 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, /* Don't force chown on network-shared FS * as it is likely to fail. */ - if (path_shared <= 0 || cfg->dynamicOwnership) + if (path_shared <= 0 || dynamicOwnership) vfoflags |= VIR_FILE_OPEN_FORCE_OWNER; if (stat(path, &sb) == 0) { @@ -2583,7 +2622,7 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, /* If the path is regular file which exists * already and dynamic_ownership is off, we don't * want to change it's ownership, just open it as-is */ - if (is_reg && !cfg->dynamicOwnership) { + if (is_reg && !dynamicOwnership) { uid = sb.st_uid; gid = sb.st_gid; } @@ -2602,10 +2641,10 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, /* If we failed as root, and the error was permission-denied (EACCES or EPERM), assume it's on a network-connected share where root access is restricted (eg, root-squashed NFS). If the - qemu user (cfg->user) is non-root, just set a flag to + qemu user is non-root, just set a flag to bypass security driver shenanigans, and retry the operation after doing setuid to qemu user */ - if ((fd != -EACCES && fd != -EPERM) || cfg->user == getuid()) + if ((fd != -EACCES && fd != -EPERM) || fallback_uid == getuid()) goto error; /* On Linux we can also verify the FS-type of the directory. */ @@ -2631,11 +2670,11 @@ qemuOpenFile(virQEMUDriverPtr driver, const char *path, int oflags, goto error; } - /* Retry creating the file as cfg->user */ + /* Retry creating the file as qemu user */ if ((fd = virFileOpenAs(path, oflags, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, - cfg->user, cfg->group, + fallback_uid, fallback_gid, vfoflags | VIR_FILE_OPEN_FORK)) < 0) { virReportSystemError(-fd, oflags & O_CREAT ? _("Error from child process creating '%s'") @@ -2656,7 +2695,6 @@ cleanup: *needUnlink = need_unlink; if (bypassSecurityDriver) *bypassSecurityDriver = bypass_security; - virObjectUnref(cfg); return fd; error: @@ -2733,7 +2771,8 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, goto cleanup; } } - fd = qemuOpenFile(driver, path, O_WRONLY | O_TRUNC | O_CREAT | directFlag, + fd = qemuOpenFile(driver, vm, path, + O_WRONLY | O_TRUNC | O_CREAT | directFlag, &needUnlink, &bypassSecurityDriver); if (fd < 0) goto cleanup; @@ -2766,7 +2805,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, if (virFileWrapperFdClose(wrapperFd) < 0) goto cleanup; - if ((fd = qemuOpenFile(driver, path, O_WRONLY, NULL, NULL)) < 0) + if ((fd = qemuOpenFile(driver, vm, path, O_WRONLY, NULL, NULL)) < 0) goto cleanup; memcpy(header.magic, QEMU_SAVE_MAGIC, sizeof(header.magic)); @@ -3178,7 +3217,7 @@ doCoreDump(virQEMUDriverPtr driver, /* Core dumps usually imply last-ditch analysis efforts are * desired, so we intentionally do not unlink even if a file was * created. */ - if ((fd = qemuOpenFile(driver, path, + if ((fd = qemuOpenFile(driver, vm, path, O_CREAT | O_TRUNC | O_WRONLY | directFlag, NULL, NULL)) < 0) goto cleanup; @@ -4628,7 +4667,7 @@ qemuDomainSaveImageOpen(virQEMUDriverPtr driver, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto error; - if ((fd = qemuOpenFile(driver, path, oflags, NULL, NULL)) < 0) + if ((fd = qemuOpenFile(driver, NULL, path, oflags, NULL, NULL)) < 0) goto error; if (bypass_cache && !(*wrapperFd = virFileWrapperFdNew(&fd, path, @@ -11232,7 +11271,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, /* create the stub file and set selinux labels; manipulate disk in * place, in a way that can be reverted on failure. */ if (!reuse) { - fd = qemuOpenFile(driver, source, O_WRONLY | O_TRUNC | O_CREAT, + fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); if (fd < 0) goto cleanup; @@ -13574,7 +13613,7 @@ qemuDomainBlockCopy(virDomainPtr dom, const char *path, } if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { - int fd = qemuOpenFile(driver, dest, O_WRONLY | O_TRUNC | O_CREAT, + int fd = qemuOpenFile(driver, vm, dest, O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); if (fd < 0) goto endjob; -- 1.8.2.1