On 07/09/2013 09:17 PM, Eric Blake wrote:

A future patch needs to look up pw_gid; but it is wasteful to crawl through getpwuid_r twice for two separate pieces of information. Alter the internal-only virGetUserEnt to give us multiple pieces of information on one traversal.

* src/util/virutil.c (virGetUserEnt): Alter signature. (virGetUserDirectory, virGetXDGDirectory, virGetUserName): Adjust callers.

Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/virutil.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/util/virutil.c b/src/util/virutil.c index 569d035..de7b532 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -656,8 +656,12 @@ enum { VIR_USER_ENT_NAME, };

-static char *virGetUserEnt(uid_t uid, - int field) +/* Look up either the name or home directory for a given uid; if group + is non-NULL, also look up the primary group for that user. On + failure, error has been reported, errno is set, and NULL is + returned. */ +static char * +virGetUserEnt(uid_t uid, int field, gid_t *group)

This seems a bit Frankenstein-ish, but I understand the utility. It would look more consistent if you switched it around to provide a pointer for each field in the arglist, and filled in everything that wasn't NULL: static int virGetUserEnt(uid_t uid, char *name, char *directory, gid_t *group) (adding more as necessary). It would make for a very long argument list if a lot of the fields were added, but would look more machined (by someone with OCD) rather than wired together. Or, you could look at how often the caller requiring both name and group is called, and might decide that scanning the entire user list twice isn't really such a big deal in the grand scheme. Or you can leave it as you have it. It does work correctly after all. And somebody else might protest against my suggestion.

{ char *strbuf; char *ret; @@ -698,6 +702,8 @@ static char *virGetUserEnt(uid_t uid, return NULL; }

+ if (group) + *group = pw->pw_gid; ignore_value(VIR_STRDUP(ret, field == VIR_USER_ENT_DIRECTORY ? pw->pw_dir : pw->pw_name)); VIR_FREE(strbuf); @@ -752,7 +758,7 @@ static char *virGetGroupEnt(gid_t gid)

char *virGetUserDirectory(void) { - return virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY); + return virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY, NULL); }

static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) @@ -765,7 +771,7 @@ static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) if (virAsprintf(&ret, "%s/libvirt", path) < 0) goto no_memory; } else { - home = virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY); + home = virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY, NULL); if (virAsprintf(&ret, "%s/%s/libvirt", home, xdgdefdir) < 0) goto no_memory; } @@ -808,7 +814,7 @@ char *virGetUserRuntimeDirectory(void)

char *virGetUserName(uid_t uid) { - return virGetUserEnt(uid, VIR_USER_ENT_NAME); + return virGetUserEnt(uid, VIR_USER_ENT_NAME, NULL); }

char *virGetGroupName(gid_t gid)

ACK, with the reservations/comments above.

A future patch needs to look up pw_gid; but it is wasteful to crawl through getpwuid_r twice for two separate pieces of information, and annoying to copy that much boilerplate code for doing the crawl. The current internal-only virGetUserEnt is also a rather awkward interface; it's easier to just design it to let callers request multiple pieces of data as needed from one traversal. And while at it, I noticed that virGetXDGDirectory could deref NULL if the getpwuid_r lookup fails. * src/util/virutil.c (virGetUserEnt): Alter signature. (virGetUserDirectory, virGetXDGDirectory, virGetUserName): Adjust callers. Signed-off-by: Eric Blake <eblake@redhat.com> --- v3: fold in the changes proposed on-list. I've gone ahead and pushed this, and the rest of the series, now. Next, I'm working on backporting to at least v1.0.5-maint and v0.10.2-maint, since those are active Fedora releases. src/util/virutil.c | 53 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 8f38fb1..d7e61db 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -647,28 +647,30 @@ cleanup: } #ifdef HAVE_GETPWUID_R -enum { - VIR_USER_ENT_DIRECTORY, - VIR_USER_ENT_NAME, -}; - -static char *virGetUserEnt(uid_t uid, - int field) +/* Look up fields from the user database for the given user. On + * error, set errno, report the error, and return -1. */ +static int +virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir) { char *strbuf; - char *ret; struct passwd pwbuf; struct passwd *pw = NULL; long val = sysconf(_SC_GETPW_R_SIZE_MAX); size_t strbuflen = val; int rc; + int ret = -1; + + if (name) + *name = NULL; + if (dir) + *dir = NULL; /* sysconf is a hint; if it fails, fall back to a reasonable size */ if (val < 0) strbuflen = 1024; if (VIR_ALLOC_N(strbuf, strbuflen) < 0) - return NULL; + return -1; /* * From the manpage (terrifying but true): @@ -679,19 +681,27 @@ static char *virGetUserEnt(uid_t uid, */ while ((rc = getpwuid_r(uid, &pwbuf, strbuf, strbuflen, &pw)) == ERANGE) { if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0) - VIR_FREE(strbuf); - return NULL; + goto cleanup; } if (rc != 0 || pw == NULL) { virReportSystemError(rc, _("Failed to find user record for uid '%u'"), (unsigned int) uid); - VIR_FREE(strbuf); - return NULL; + goto cleanup; + } + + if (name && VIR_STRDUP(*name, pw->pw_name) < 0) + goto cleanup; + if (group) + *group = pw->pw_gid; + if (dir && VIR_STRDUP(*dir, pw->pw_dir) < 0) { + if (name) + VIR_FREE(*name); + goto cleanup; } - ignore_value(VIR_STRDUP(ret, field == VIR_USER_ENT_DIRECTORY ? - pw->pw_dir : pw->pw_name)); + ret = 0; +cleanup: VIR_FREE(strbuf); return ret; } @@ -741,7 +751,9 @@ static char *virGetGroupEnt(gid_t gid) char *virGetUserDirectory(void) { - return virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY); + char *ret; + virGetUserEnt(geteuid(), NULL, NULL, &ret); + return ret; } static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) @@ -753,8 +765,9 @@ static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) if (path && path[0]) { ignore_value(virAsprintf(&ret, "%s/libvirt", path)); } else { - home = virGetUserEnt(geteuid(), VIR_USER_ENT_DIRECTORY); - ignore_value(virAsprintf(&ret, "%s/%s/libvirt", home, xdgdefdir)); + home = virGetUserDirectory(); + if (home) + ignore_value(virAsprintf(&ret, "%s/%s/libvirt", home, xdgdefdir)); } VIR_FREE(home); @@ -787,7 +800,9 @@ char *virGetUserRuntimeDirectory(void) char *virGetUserName(uid_t uid) { - return virGetUserEnt(uid, VIR_USER_ENT_NAME); + char *ret; + virGetUserEnt(uid, &ret, NULL, NULL); + return ret; } char *virGetGroupName(gid_t gid) -- 1.8.1.4

On 07/09/2013 09:17 PM, Eric Blake wrote:

Since neither getpwuid_r() nor initgroups() are safe to call in between fork and exec (they obtain a mutex, but if some other thread in the parent also held the mutex at the time of the fork, the child will deadlock), we have to split out the functionality that is unsafe. At least glibc's initgroups() uses getgrouplist under the hood, so the ideal split is to expose getgrouplist for use before a fork. Gnulib already gives us a nice wrapper via mgetgroups; we wrap it once more to look up by uid instead of name.

* .gnulib: Update for mgetgroups. * bootstrap.conf (gnulib_modules): Add mgetgroups. * src/util/virutil.h (virGetGroupList): New declaration. * src/util/virutil.c (virGetGroupList): New function. * src/libvirt_private.syms (virutil.h): Export it.

Signed-off-by: Eric Blake <eblake@redhat.com> --- .gnulib | 2 +- bootstrap.conf | 1 + src/libvirt_private.syms | 1 + src/util/virutil.c | 33 +++++++++++++++++++++++++++++++++ src/util/virutil.h | 2 ++ 5 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/.gnulib b/.gnulib index b72ff2a..da8d59e 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit b72ff2a45efde544c406804186d37a3254728571 +Subproject commit da8d59ee79138b85daee1ad5b22ea12e4fb6ce47

Are you sure you want to update the gnulib version in this same commit? Although it is necessary for the rest of this patch to work, there are probably other changes in gnulib that could potentially cause a regression unrelated to the libvirt parts of the patch. Just for the sake of bisecting, it seems like a good idea to update gnulib in a separate prerequisite patch.

diff --git a/bootstrap.conf b/bootstrap.conf index 410e4f5..1c59c22 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -71,6 +71,7 @@ listen localeconv maintainer-makefile manywarnings +mgetgroups mkdtemp mkostemp mkostemps diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6df5500..d8b03f1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2002,6 +2002,7 @@ virGetDeviceID; virGetDeviceUnprivSGIO; virGetFCHostNameByWWN; virGetGroupID; +virGetGroupList; virGetGroupName; virGetHostname; virGetUnprivSGIOSysfsPath; diff --git a/src/util/virutil.c b/src/util/virutil.c index de7b532..844c694 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -67,6 +67,7 @@ #endif

#include "c-ctype.h" +#include "mgetgroups.h" #include "virerror.h" #include "virlog.h" #include "virbuffer.h" @@ -990,6 +991,38 @@ virGetGroupID(const char *group, gid_t *gid) return 0; }

+ +/* Compute the list of supplementary groups associated with @uid, and + * including @gid in the list (unless it is -1), storing a malloc'd + * result into @list. Return the size of the list on success, or -1 + * on failure with error reported and errno set. May not be called + * between fork and exec. */ +int +virGetGroupList(uid_t uid, gid_t gid, gid_t **list) +{ + int ret = -1; + char *user = NULL; + gid_t group; + + *list = NULL; + if (uid == (uid_t)-1) + return 0; + + if (!(user = virGetUserEnt(uid, VIR_USER_ENT_NAME, &group))) + return -1; + + if (gid == (gid_t)-1) + gid = group; + + ret = mgetgroups(user, gid, list); + if (ret < 0) + virReportSystemError(errno, + _("cannot get group list for '%s'"), user); + VIR_FREE(user); + return ret; +} + + /* Set the real and effective uid and gid to the given values, and call * initgroups so that the process has all the assumed group membership of * that uid. return 0 on success, -1 on failure (the original system error diff --git a/src/util/virutil.h b/src/util/virutil.h index 280a18d..6480b07 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -115,6 +115,8 @@ char *virGetUserCacheDirectory(void); char *virGetUserRuntimeDirectory(void); char *virGetUserName(uid_t uid); char *virGetGroupName(gid_t gid); +int virGetGroupList(uid_t uid, gid_t group, gid_t **groups) + ATTRIBUTE_NONNULL(3); int virGetUserID(const char *name, uid_t *uid) ATTRIBUTE_RETURN_CHECK; int virGetGroupID(const char *name,

ACK to the new function. I do think the gnulib update should be a separate commit, though.

On 07/10/2013 12:23 PM, Eric Blake wrote:

On 07/09/2013 11:20 PM, Laine Stump wrote:

...

On 07/09/2013 09:17 PM, Eric Blake wrote:

...

Since neither getpwuid_r() nor initgroups() are safe to call in between fork and exec (they obtain a mutex, but if some other thread in the parent also held the mutex at the time of the fork, the child will deadlock), we have to split out the functionality that is unsafe. At least glibc's initgroups() uses getgrouplist under the hood, so the ideal split is to expose getgrouplist for use before a fork. Gnulib already gives us a nice wrapper via mgetgroups; we wrap it once more to look up by uid instead of name.

* .gnulib: Update for mgetgroups. * bootstrap.conf (gnulib_modules): Add mgetgroups. * src/util/virutil.h (virGetGroupList): New declaration. * src/util/virutil.c (virGetGroupList): New function. * src/libvirt_private.syms (virutil.h): Export it.

Signed-off-by: Eric Blake <eblake@redhat.com> --- +++ b/.gnulib @@ -1 +1 @@ -Subproject commit b72ff2a45efde544c406804186d37a3254728571 +Subproject commit da8d59ee79138b85daee1ad5b22ea12e4fb6ce47

Are you sure you want to update the gnulib version in this same commit? Although it is necessary for the rest of this patch to work, there are probably other changes in gnulib that could potentially cause a regression unrelated to the libvirt parts of the patch. Just for the sake of bisecting, it seems like a good idea to update gnulib in a separate prerequisite patch. Good point - especially since I _will_ be backporting this series, and we tend to eschew gnulib submodule updates on backports not tied to a stable release branch.

...

ACK to the new function. I do think the gnulib update should be a separate commit, though. Do you need to see a v3, or should I just go ahead and make that split?

Just making the split is fine. We all know what it will look like :-)

On 07/09/2013 09:17 PM, Eric Blake wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=964358

POSIX states that multi-threaded apps should not use functions that are not async-signal-safe between fork and exec, yet we were using getpwuid_r and initgroups. Although rare, it is possible to hit deadlock in the child, when it tries to grab a mutex that was already held by another thread in the parent. I actually hit this deadlock when testing multiple domains being started in parallel with a command hook, with the following backtrace in the child:

Thread 1 (Thread 0x7fd56bbf2700 (LWP 3212)): #0 __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:136 #1 0x00007fd5761e7388 in _L_lock_854 () from /lib64/libpthread.so.0 #2 0x00007fd5761e7257 in __pthread_mutex_lock (mutex=0x7fd56be00360) at pthread_mutex_lock.c:61 #3 0x00007fd56bbf9fc5 in _nss_files_getpwuid_r (uid=0, result=0x7fd56bbf0c70, buffer=0x7fd55c2a65f0 "", buflen=1024, errnop=0x7fd56bbf25b8) at nss_files/files-pwd.c:40 #4 0x00007fd575aeff1d in __getpwuid_r (uid=0, resbuf=0x7fd56bbf0c70, buffer=0x7fd55c2a65f0 "", buflen=1024, result=0x7fd56bbf0cb0) at ../nss/getXXbyYY_r.c:253 #5 0x00007fd578aebafc in virSetUIDGID (uid=0, gid=0) at util/virutil.c:1031 #6 0x00007fd578aebf43 in virSetUIDGIDWithCaps (uid=0, gid=0, capBits=0, clearExistingCaps=true) at util/virutil.c:1388 #7 0x00007fd578a9a20b in virExec (cmd=0x7fd55c231f10) at util/vircommand.c:654 #8 0x00007fd578a9dfa2 in virCommandRunAsync (cmd=0x7fd55c231f10, pid=0x0) at util/vircommand.c:2247 #9 0x00007fd578a9d74e in virCommandRun (cmd=0x7fd55c231f10, exitstatus=0x0) at util/vircommand.c:2100 #10 0x00007fd56326fde5 in qemuProcessStart (conn=0x7fd53c000df0, driver=0x7fd55c0dc4f0, vm=0x7fd54800b100, migrateFrom=0x0, stdin_fd=-1, stdin_path=0x0, snapshot=0x0, vmop=VIR_NETDEV_VPORT_PROFILE_OP_CREATE, flags=1) at qemu/qemu_process.c:3694 ...

The solution is to split the work of getpwuid_r/initgroups into the unsafe portions (getgrouplist, called pre-fork) and safe portions (setgroups, called post-fork).

* src/util/virutil.h (virSetUIDGID, virSetUIDGIDWithCaps): Adjust signature. * src/util/virutil.c (virSetUIDGID): Add parameters. (virSetUIDGIDWithCaps): Adjust clients. * src/util/vircommand.c (virExec): Likewise. * src/util/virfile.c (virFileAccessibleAs, virFileOpenForked) (virDirCreate): Likewise. * src/security/security_dac.c (virSecurityDACSetProcessLabel): Likewise. * src/lxc/lxc_container.c (lxcContainerSetID): Likewise. * configure.ac (AC_CHECK_FUNCS_ONCE): Check for setgroups, not initgroups.

Signed-off-by: Eric Blake <eblake@redhat.com> --- configure.ac | 4 +- src/lxc/lxc_container.c | 9 +++- src/security/security_dac.c | 16 +++++-- src/util/vircommand.c | 10 +++- src/util/virfile.c | 30 ++++++++++-- src/util/virutil.c | 108 ++++++++++++-------------------------------- src/util/virutil.h | 5 +- 7 files changed, 90 insertions(+), 92 deletions(-)

diff --git a/configure.ac b/configure.ac index a6ad6a3..7f53308 100644 --- a/configure.ac +++ b/configure.ac @@ -206,8 +206,8 @@ AC_CHECK_SIZEOF([long]) dnl Availability of various common functions (non-fatal if missing), dnl and various less common threadsafe functions AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \ - getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \ - posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink]) + getpwuid_r getuid kill mmap newlocale posix_fallocate posix_memalign \ + prlimit regexec sched_getaffinity setgroups setns setrlimit symlink])

dnl Availability of pthread functions (if missing, win32 threading is dnl assumed). Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE. diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 595c0f2..c32947b 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2012 Red Hat, Inc. + * Copyright (C) 2008-2013 Red Hat, Inc. * Copyright (C) 2008 IBM Corp. * * lxc_container.c: file description @@ -347,12 +347,17 @@ int lxcContainerWaitForContinue(int control) */ static int lxcContainerSetID(virDomainDefPtr def) { + gid_t *groups; + int ngroups; + /* Only call virSetUIDGID when user namespace is enabled * for this container. And user namespace is only enabled * when nuidmap&ngidmap is not zero */

VIR_DEBUG("Set UID/GID to 0/0"); - if (def->idmap.nuidmap && virSetUIDGID(0, 0) < 0) { + if (def->idmap.nuidmap && + ((ngroups = virGetGroupList(0, 0, &groups) < 0) || + virSetUIDGID(0, 0, groups, ngroups) < 0)) { virReportSystemError(errno, "%s", _("setuid or setgid failed")); return -1; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 0d6defc..b6ddd51 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1001,17 +1001,25 @@ virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, uid_t user; gid_t group; virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + gid_t *groups; + int ngroups; + int ret = -1;

if (virSecurityDACGetIds(def, priv, &user, &group)) return -1; + ngroups = virGetGroupList(user, group, &groups); + if (ngroups < 0) + return -1;

VIR_DEBUG("Dropping privileges of DEF to %u:%u", (unsigned int) user, (unsigned int) group);

- if (virSetUIDGID(user, group) < 0) - return -1; - - return 0; + if (virSetUIDGID(user, group, groups, ngroups) < 0) + goto cleanup; + ret = 0; +cleanup: + VIR_FREE(groups); + return ret; }

diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6e2eb1e..6dc2490 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -402,6 +402,8 @@ virExec(virCommandPtr cmd) const char *binary = NULL; int forkRet, ret; struct sigaction waxon, waxoff; + gid_t *groups = NULL; + int ngroups;

if (cmd->args[0][0] != '/') { if (!(binary = virFindFileInPath(cmd->args[0]))) { @@ -472,6 +474,9 @@ virExec(virCommandPtr cmd) childerr = null; }

+ if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0) + goto cleanup; + forkRet = virFork(&pid);

if (pid < 0) { @@ -497,6 +502,7 @@ virExec(virCommandPtr cmd)

if (binary != cmd->args[0]) VIR_FREE(binary); + VIR_FREE(groups);

return 0; } @@ -651,7 +657,8 @@ virExec(virCommandPtr cmd) cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) { VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx", (int)cmd->uid, (int)cmd->gid, cmd->capabilities); - if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, cmd->capabilities, + if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, groups, ngroups, + cmd->capabilities, !!(cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) { goto fork_error; } @@ -695,6 +702,7 @@ virExec(virCommandPtr cmd) /* This is cleanup of parent process only - child should never jump here on error */

+ VIR_FREE(groups); if (binary != cmd->args[0]) VIR_FREE(binary);

diff --git a/src/util/virfile.c b/src/util/virfile.c index ad01845..e986ee7 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1459,18 +1459,26 @@ virFileAccessibleAs(const char *path, int mode, pid_t pid = 0; int status, ret = 0; int forkRet = 0; + gid_t *groups; + int ngroups;

if (uid == getuid() && gid == getgid()) return access(path, mode);

+ ngroups = virGetGroupList(uid, gid, &groups); + if (ngroups < 0) + return -1; + forkRet = virFork(&pid);

if (pid < 0) { + VIR_FREE(groups); return -1; }

if (pid) { /* parent */ + VIR_FREE(groups); if (virProcessWait(pid, &status) < 0) { /* virProcessWait() already * reported error */ @@ -1499,7 +1507,7 @@ virFileAccessibleAs(const char *path, int mode, goto childerror; }

- if (virSetUIDGID(uid, gid) < 0) { + if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { ret = errno; goto childerror; } @@ -1575,17 +1583,24 @@ virFileOpenForked(const char *path, int openflags, mode_t mode, int fd = -1; int pair[2] = { -1, -1 }; int forkRet; + gid_t *groups; + int ngroups;

/* parent is running as root, but caller requested that the * file be opened as some other user and/or group). The * following dance avoids problems caused by root-squashing * NFS servers. */

+ ngroups = virGetGroupList(uid, gid, &groups); + if (ngroups < 0) + return -errno; + if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) { ret = -errno; virReportSystemError(errno, _("failed to create socket needed for '%s'"), path); + VIR_FREE(groups); return ret; }

@@ -1606,7 +1621,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,

/* set desired uid/gid, then attempt to create the file */

- if (virSetUIDGID(uid, gid) < 0) { + if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { ret = -errno; goto childerror; } @@ -1654,6 +1669,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,

/* parent */

+ VIR_FREE(groups); VIR_FORCE_CLOSE(pair[1]);

do { @@ -1855,6 +1871,8 @@ virDirCreate(const char *path, pid_t pid; int waitret; int status, ret = 0; + gid_t *groups; + int ngroups;

/* allow using -1 to mean "current value" */ if (uid == (uid_t) -1) @@ -1869,15 +1887,21 @@ virDirCreate(const char *path, return virDirCreateNoFork(path, mode, uid, gid, flags); }

+ ngroups = virGetGroupList(uid, gid, &groups); + if (ngroups < 0) + return -errno; + int forkRet = virFork(&pid);

if (pid < 0) { ret = -errno; + VIR_FREE(groups); return ret; }

if (pid) { /* parent */ /* wait for child to complete, and retrieve its exit code */ + VIR_FREE(groups); while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); if (waitret == -1) { ret = -errno; @@ -1904,7 +1928,7 @@ parenterror:

/* set desired uid/gid, then attempt to create the directory */

- if (virSetUIDGID(uid, gid) < 0) { + if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { ret = -errno; goto childerror; }

It's too late for me to be reviewing code - I can tell because I spent several minutes being concerned that you didn't free groups in this code path. Duh. That's really all I can say to myself :-)

diff --git a/src/util/virutil.c b/src/util/virutil.c index 844c694..7e1133c 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1023,87 +1023,37 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list) }

-/* Set the real and effective uid and gid to the given values, and call - * initgroups so that the process has all the assumed group membership of - * that uid. return 0 on success, -1 on failure (the original system error - * remains in errno). +/* Set the real and effective uid and gid to the given values, as well + * as all the supplementary groups, so that the process has all the + * assumed group membership of that uid. Return 0 on success, -1 on + * failure (the original system error remains in errno). */ int -virSetUIDGID(uid_t uid, gid_t gid) +virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups, int ngroups) { - int err; - char *buf = NULL; - - if (gid != (gid_t)-1) { - if (setregid(gid, gid) < 0) { - virReportSystemError(err = errno, - _("cannot change to '%u' group"), - (unsigned int) gid); - goto error; - } + if (gid != (gid_t)-1 && setregid(gid, gid) < 0) { + virReportSystemError(errno, + _("cannot change to '%u' group"), + (unsigned int) gid); + return -1;

Yep. Should have been organized like this to begin with. Join the campaign to eliminate excessive indentation in our lifetime!

}

- if (uid != (uid_t)-1) { -# ifdef HAVE_INITGROUPS - struct passwd pwd, *pwd_result; - size_t bufsize; - int rc; - - bufsize = sysconf(_SC_GETPW_R_SIZE_MAX); - if (bufsize == -1) - bufsize = 16384; - - if (VIR_ALLOC_N(buf, bufsize) < 0) { - virReportOOMError(); - err = ENOMEM; - goto error; - } - while ((rc = getpwuid_r(uid, &pwd, buf, bufsize, - &pwd_result)) == ERANGE) { - if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) { - virReportOOMError(); - err = ENOMEM; - goto error; - } - } - - if (rc) { - virReportSystemError(err = rc, _("cannot getpwuid_r(%u)"), - (unsigned int) uid); - goto error; - } - - if (!pwd_result) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("getpwuid_r failed to retrieve data " - "for uid '%u'"), - (unsigned int) uid); - err = EINVAL; - goto error; - } - - if (initgroups(pwd.pw_name, pwd.pw_gid) < 0) { - virReportSystemError(err = errno, - _("cannot initgroups(\"%s\", %d)"), - pwd.pw_name, (unsigned int) pwd.pw_gid); - goto error; - } +# if HAVE_SETGROUPS + if (ngroups && setgroups(ngroups, groups) < 0) { + virReportSystemError(errno, "%s", + _("cannot set supplemental groups")); + return -1; + } # endif - if (setreuid(uid, uid) < 0) { - virReportSystemError(err = errno, - _("cannot change to uid to '%u'"), - (unsigned int) uid); - goto error; - } + + if (uid != (uid_t)-1 && setreuid(uid, uid) < 0) { + virReportSystemError(errno, + _("cannot change to uid to '%u'"), + (unsigned int) uid); + return -1; }

- VIR_FREE(buf); return 0; - -error: - VIR_FREE(buf); - errno = err; - return -1; }

#else /* ! HAVE_GETPWUID_R */ @@ -1306,7 +1256,9 @@ int virGetGroupID(const char *name ATTRIBUTE_UNUSED,

int virSetUIDGID(uid_t uid ATTRIBUTE_UNUSED, - gid_t gid ATTRIBUTE_UNUSED) + gid_t gid ATTRIBUTE_UNUSED, + gid_t *groups ATTRIBUTE_UNUSED, + int ngroups ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("virSetUIDGID is not available")); @@ -1330,8 +1282,8 @@ virGetGroupName(gid_t gid ATTRIBUTE_UNUSED) * errno). */ int -virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits, - bool clearExistingCaps) +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, + unsigned long long capBits, bool clearExistingCaps) { int ii, capng_ret, ret = -1; bool need_setgid = false, need_setuid = false; @@ -1401,7 +1353,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits, } }

- if (virSetUIDGID(uid, gid) < 0) + if (virSetUIDGID(uid, gid, groups, ngroups) < 0) goto cleanup;

/* Tell it we are done keeping capabilities */ @@ -1445,11 +1397,11 @@ cleanup: */

int -virSetUIDGIDWithCaps(uid_t uid, gid_t gid, +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, unsigned long long capBits ATTRIBUTE_UNUSED, bool clearExistingCaps ATTRIBUTE_UNUSED) { - return virSetUIDGID(uid, gid); + return virSetUIDGID(uid, gid, groups, ngroups); } #endif

diff --git a/src/util/virutil.h b/src/util/virutil.h index 6480b07..0083c88 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -46,8 +46,9 @@ int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK; int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf);

-int virSetUIDGID(uid_t uid, gid_t gid); -int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits, +int virSetUIDGID(uid_t uid, gid_t gid, gid_t *groups, int ngroups); +int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, gid_t *groups, int ngroups, + unsigned long long capBits, bool clearExistingCaps);

int virScaleInteger(unsigned long long *value, const char *suffix,

ACK.