10:24 p.m.
https://bugzilla.redhat.com/show_bug.cgi?id=964358
Posting now to get reviews started. I'd especially like to get
feedback that it doesn't break LXC, and that it works with root-squash
NFS when using qemu:qemu instead of root:root in /etc/libvirt/qemu.conf.
I hope to do more testing myself, and also see if I can try writing an
LD_PRELOAD shim as part of 'make check' to make it easier to test that
the right system calls are made during the sequence.
Eric Blake (2):
util: add virGetGroupList
util: make virSetUIDGID async-signal-safe
configure.ac | 7 +--
src/libvirt_private.syms | 1 +
src/security/security_dac.c | 16 ++++--
src/util/vircommand.c | 10 +++-
src/util/virfile.c | 30 +++++++++--
src/util/virutil.c | 123 ++++++++++++++++++++++++++++++--------------
src/util/virutil.h | 7 ++-
7 files changed, 142 insertions(+), 52 deletions(-)
--
1.8.1.4
10:24 p.m.
New subject: [libvirt] [PATCH 1/2] util: add virGetGroupList
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. This patch adds a nice wrapper around
getgrouplist, which does the lookup by uid instead of name and
which mallocs the result; at least glibc's initgroups() uses
getgrouplist under the hood.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for getgrouplist.
* 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(a)redhat.com>
---
configure.ac | 4 +--
src/libvirt_private.syms | 1 +
src/util/virutil.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virutil.h | 2 ++
4 files changed, 98 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 2055637..d20b90e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -205,8 +205,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 \
+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])
dnl Availability of pthread functions (if missing, win32 threading is
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a81a865..3d9d89f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1933,6 +1933,7 @@ virGetDeviceID;
virGetDeviceUnprivSGIO;
virGetFCHostNameByWWN;
virGetGroupID;
+virGetGroupList;
virGetGroupName;
virGetHostname;
virGetUnprivSGIOSysfsPath;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 239fba8..a3cb64f 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -993,6 +993,99 @@ 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)
+{
+ char *buf = NULL;
+ gid_t *groups = NULL;
+ int ngroups = 0;
+ int ret = -1;
+
+ *list = NULL;
+ if (uid == (uid_t)-1)
+ return 0;
+
+# ifdef HAVE_GETGROUPLIST
+ {
+ 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();
+ goto error;
+ }
+ while ((rc = getpwuid_r(uid, &pwd, buf, bufsize,
+ &pwd_result)) == ERANGE) {
+ if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+ }
+
+ if (rc) {
+ virReportSystemError(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);
+ goto error;
+ }
+
+ if (gid == (gid_t)-1)
+ gid = pwd.pw_gid;
+
+ /* Figure out what size list to expect */
+ getgrouplist(pwd.pw_name, gid, groups, &ngroups);
+
+ if (VIR_ALLOC_N(groups, ngroups) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+
+ if (getgrouplist(pwd.pw_name, gid, groups, &ngroups) < 0) {
+ virReportSystemError(errno,
+ _("cannot getgrouplist(\"%s\",
%d)"),
+ pwd.pw_name, (unsigned int) gid);
+ goto error;
+ }
+ }
+# endif
+
+ if (!ngroups && gid != (gid_t)-1) {
+ if (VIR_ALLOC_N(groups, 1) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+ groups[ngroups++] = gid;
+ }
+
+ *list = groups;
+ groups = NULL;
+ ret = ngroups;
+
+error:
+ VIR_FREE(buf);
+ VIR_FREE(groups);
+ 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,
--
1.8.1.4
11:49 a.m.
New subject: [libvirt] [PATCH 1/2] util: add virGetGroupList
On 05/21/2013 11:24 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. This patch adds a nice wrapper around
getgrouplist, which does the lookup by uid instead of name and
which mallocs the result; at least glibc's initgroups() uses
getgrouplist under the hood.
* configure.ac (AC_CHECK_FUNCS_ONCE): Check for getgrouplist.
* 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(a)redhat.com>
---
configure.ac | 4 +--
src/libvirt_private.syms | 1 +
src/util/virutil.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
src/util/virutil.h | 2 ++
4 files changed, 98 insertions(+), 2 deletions(-)
diff --git a/configure.ac b/configure.ac
index 2055637..d20b90e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -205,8 +205,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 \
+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])
dnl Availability of pthread functions (if missing, win32 threading is
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a81a865..3d9d89f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1933,6 +1933,7 @@ virGetDeviceID;
virGetDeviceUnprivSGIO;
virGetFCHostNameByWWN;
virGetGroupID;
+virGetGroupList;
virGetGroupName;
virGetHostname;
virGetUnprivSGIOSysfsPath;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 239fba8..a3cb64f 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -993,6 +993,99 @@ 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)
+{
+ char *buf = NULL;
+ gid_t *groups = NULL;
+ int ngroups = 0;
+ int ret = -1;
+
+ *list = NULL;
+ if (uid == (uid_t)-1)
+ return 0;
+
+# ifdef HAVE_GETGROUPLIST
+ {
+ 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();
+ goto error;
+ }
+ while ((rc = getpwuid_r(uid, &pwd, buf, bufsize,
+ &pwd_result)) == ERANGE) {
+ if (VIR_RESIZE_N(buf, bufsize, bufsize, bufsize) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+ }
+
+ if (rc) {
+ virReportSystemError(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);
+ goto error;
+ }
Up to here, it's just direct movement from virSetUIDGID.
Now we get to the new stuff:
+
+ if (gid == (gid_t)-1)
+ gid = pwd.pw_gid;
The main use of the gid in the args is to specify one group that will
not be included in the returned group, and it's usually set to the group
listed in the pw record (got that from the manpage :-), so this makes sense.
+
+ /* Figure out what size list to expect */
+ getgrouplist(pwd.pw_name, gid, groups, &ngroups);
Do we need to be concerned about the "BUGS" info in the manpage?
BUGS
In glibc versions before 2.3.3, the implementation of
this function contains a buffer-overrun bug: it returns
the complete list of groups for user in the array
groups, even when the number of groups exceeds *ngroups.
Is anyone running that vintage of glibc? It sounds *kind of* like it
could hit that if ngroups is 0 (but doesn't specifically say that).
+
+ if (VIR_ALLOC_N(groups, ngroups) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+
+ if (getgrouplist(pwd.pw_name, gid, groups, &ngroups) < 0) {
+ virReportSystemError(errno,
+ _("cannot getgrouplist(\"%s\",
%d)"),
+ pwd.pw_name, (unsigned int) gid);
+ goto error;
+ }
+ }
+# endif
+
+ if (!ngroups && gid != (gid_t)-1) {
+ if (VIR_ALLOC_N(groups, 1) < 0) {
+ virReportOOMError();
+ goto error;
+ }
+ groups[ngroups++] = gid;
A reasonable fallback.
+ }
+
+ *list = groups;
+ groups = NULL;
+ ret = ngroups;
+
+error:
+ VIR_FREE(buf);
+ VIR_FREE(groups);
+ 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. I'm curious about whether the bug listed in the manpage could ever
hit us, though (surely it can't be *that* bad, or if it is then everyone
will have a patch for it).
12:02 p.m.
New subject: [libvirt] [PATCH 1/2] util: add virGetGroupList
On 05/22/2013 10:49 AM, Laine Stump wrote:
On 05/21/2013 11:24 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. This patch adds a nice wrapper around
> getgrouplist, which does the lookup by uid instead of name and
> which mallocs the result; at least glibc's initgroups() uses
> getgrouplist under the hood.
>
> * configure.ac (AC_CHECK_FUNCS_ONCE): Check for getgrouplist.
> * src/util/virutil.h (virGetGroupList): New declaration.
> * src/util/virutil.c (virGetGroupList): New function.
> * src/libvirt_private.syms (virutil.h): Export it.
>
> +
> + /* Figure out what size list to expect */
> + getgrouplist(pwd.pw_name, gid, groups, &ngroups);
Do we need to be concerned about the "BUGS" info in the manpage?
BUGS
In glibc versions before 2.3.3, the implementation of
this function contains a buffer-overrun bug: it returns
the complete list of groups for user in the array
groups, even when the number of groups exceeds *ngroups.
Is anyone running that vintage of glibc? It sounds *kind of* like it
could hit that if ngroups is 0 (but doesn't specifically say that).
So I did some digging, and gnulib has an 'mgetgroups' module
(unfortunately GPL, but maybe I could get it relaxed if we wanted to use
that instead) that does the lookup by name rather than by uid_t. Here's
what gnulib's code says about the issue:
#if HAVE_GETGROUPLIST
/* We prefer to use getgrouplist if available, because it has better
performance characteristics.
In glibc 2.3.2, getgrouplist is buggy. If you pass a zero as the
length of the output buffer, getgrouplist will still write to the
buffer. Contrary to what some versions of the getgrouplist
manpage say, this doesn't happen with nonzero buffer sizes.
Therefore our usage here just avoids a zero sized buffer. */
if (username)
{
enum { N_GROUPS_INIT = 10 };
max_n_groups = N_GROUPS_INIT;
g = realloc_groupbuf (NULL, max_n_groups);
So it looks like I could indeed work around the bug by providing a
non-zero buffer in my probing version, and even go so far as to
pre-allocate a reasonable size buffer to get by with a single
getgrouplist() call if the given uid is in a small number of groups to
begin with (and what do you know - our most common "victim" uid will be
"qemu", which on a default-install Fedora system is uid 107 and a member
of exactly two groups: kvm(36) and qemu(107)).
Version 2 coming up with that idea incorporated.
> +
> + if (!ngroups && gid != (gid_t)-1) {
> + if (VIR_ALLOC_N(groups, 1) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> + groups[ngroups++] = gid;
A reasonable fallback.
The gnulib module actually goes one further, and crawls through
getgrent() looking for any group that mentions uid (that is, instead of
ignoring supplementary groups, it actually tries a slower method of
getting at them). Maybe I'll pursue getting the gnulib module relaxed
to LGPL after all.
ACK. I'm curious about whether the bug listed in the manpage could ever
hit us, though (surely it can't be *that* bad, or if it is then everyone
will have a patch for it).
I'll do a v2 that either works around the bug, or which delegates to the
gnulib module (depending on response on the gnulib list about a license
relax request).
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:50 p.m.
New subject: [libvirt] [PATCH 1/2] util: add virGetGroupList
On 05/22/2013 11:02 AM, Eric Blake wrote:
>> + /* Figure out what size list to expect */
>> + getgrouplist(pwd.pw_name, gid, groups, &ngroups);
>
> Do we need to be concerned about the "BUGS" info in the manpage?
>
> BUGS
> In glibc versions before 2.3.3, the implementation of
> this function contains a buffer-overrun bug: it returns
> the complete list of groups for user in the array
> groups, even when the number of groups exceeds *ngroups.
>
> Is anyone running that vintage of glibc? It sounds *kind of* like it
> could hit that if ngroups is 0 (but doesn't specifically say that).
So I did some digging, and gnulib has an 'mgetgroups' module
(unfortunately GPL, but maybe I could get it relaxed if we wanted to use
that instead)
I'll do a v2 that either works around the bug, or which delegates to the
gnulib module (depending on response on the gnulib list about a license
relax request).
Gnulib just relaxed the license[1]; I'll be respinning this patch to
pull in the gnulib module instead.
[1] http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=shortlog;h=612ef3f7
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:24 p.m.
New subject: [libvirt] [PATCH 2/2] util: make virSetUIDGID async-signal-safe
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
1:14 p.m.
New subject: [libvirt] [PATCH 2/2] util: make virSetUIDGID async-signal-safe
On 05/21/2013 11:24 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.
* 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,
ACK.
3973
days inactive
4015
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump