10:24 p.m.
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.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for setgroups, not
initgroups.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
configure.ac | 5 +-
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 +-
6 files changed, 84 insertions(+), 90 deletions(-)
diff --git a/configure.ac b/configure.ac
index d20b90e..26deac7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -206,8 +206,9 @@ 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 getgrouplist \
- getmntent_r getpwuid_r getuid initgroups kill mmap newlocale posix_fallocate \
- posix_memalign prlimit regexec sched_getaffinity setns setrlimit symlink])
+ getmntent_r 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/security/security_dac.c b/src/security/security_dac.c
index d922ad2..907dbd5 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1000,17 +1000,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 665f549..ba03802 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 4637919..20ebcbc 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1462,18 +1462,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 */
@@ -1502,7 +1510,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;
}
@@ -1578,17 +1586,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;
}
@@ -1609,7 +1624,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;
}
@@ -1657,6 +1672,7 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
/* parent */
+ VIR_FREE(groups);
VIR_FORCE_CLOSE(pair[1]);
do {
@@ -1858,6 +1874,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)
@@ -1872,15 +1890,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;
@@ -1907,7 +1931,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;
}
diff --git a/src/util/virutil.c b/src/util/virutil.c
index a3cb64f..883672a 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1086,87 +1086,37 @@ error:
}
-/* 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;
}
- 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 */
@@ -1383,7 +1333,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"));
@@ -1407,8 +1359,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;
@@ -1478,7 +1430,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 */
@@ -1522,11 +1474,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,
--
1.8.1.4