[libvirt] [PATCH] initgroups() in qemudOpenAsUID()
qemudOpenAsUID is intended to open a file with the credentials of a specified uid. Current implementation fails if the file is accessible to one of uid's groups but not owned by uid. This patch replaces the supplementary group list that the child process inherited from libvirtd with the default group list of uid. --- src/qemu/qemu_driver.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0ce2d40..a1027d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6353,6 +6353,7 @@ parent_cleanup: char *buf = NULL; size_t bufsize = 1024 * 1024; int bytesread; + struct passwd *pwd; /* child doesn't need the read side of the pipe */ close(pipefd[0]); @@ -6365,6 +6366,21 @@ parent_cleanup: goto child_cleanup; } + /* we can avoid getpwuid_r() in threadless child */ + if ((pwd = getpwuid(uid)) == NULL) { + exit_code = errno; + virReportSystemError(errno, + _("cannot setuid(%d) to read '%s'"), + uid, path); + goto child_cleanup; + } + if (initgroups(pwd->pw_name, pwd->pw_gid) != 0) { + exit_code = errno; + virReportSystemError(errno, + _("cannot setuid(%d) to read '%s'"), + uid, path); + goto child_cleanup; + } if (setuid(uid) != 0) { exit_code = errno; virReportSystemError(errno, -- 1.7.2.3
On 10/17/2010 04:58 PM, Dan Kenigsberg wrote:
qemudOpenAsUID is intended to open a file with the credentials of a specified uid. Current implementation fails if the file is accessible to one of uid's groups but not owned by uid.
This patch replaces the supplementary group list that the child process inherited from libvirtd with the default group list of uid.
Urr. Yet another twist in this ugly saga. Thanks for figuring it out!
--- src/qemu/qemu_driver.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0ce2d40..a1027d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6353,6 +6353,7 @@ parent_cleanup: char *buf = NULL; size_t bufsize = 1024 * 1024; int bytesread; + struct passwd *pwd;
/* child doesn't need the read side of the pipe */ close(pipefd[0]); @@ -6365,6 +6366,21 @@ parent_cleanup: goto child_cleanup; }
+ /* we can avoid getpwuid_r() in threadless child */ + if ((pwd = getpwuid(uid)) == NULL) { + exit_code = errno; + virReportSystemError(errno, + _("cannot setuid(%d) to read '%s'"), + uid, path);
The error message is misleading. It should rather be something like: + virReportSystemError(errno, + _("getpwuid(%d) failed reading '%s'"), + uid, path); (BTW, thanks for the comment about getpwuid_r())
+ goto child_cleanup; + } + if (initgroups(pwd->pw_name, pwd->pw_gid) != 0) { + exit_code = errno; + virReportSystemError(errno, + _("cannot setuid(%d) to read '%s'"), + uid, path);
Similarly, the error message should be: + virReportSystemError(errno, + _("initgroups(\"%s\", %d) failed reading '%s'"), + pwd->pw_name, pwd->pw_gid, path);
+ goto child_cleanup; + } if (setuid(uid) != 0) { exit_code = errno; virReportSystemError(errno,
I give my ACK with the above error message fixes, with the reservation that I'm not that familiar with getpwuid and initgroups, and any restrictions they may have when called in a non-root process. (apparently everything is in order in this case, since presumably your testing was successful ;-)
On Mon, Oct 18, 2010 at 01:21:05AM -0400, Laine Stump wrote:
On 10/17/2010 04:58 PM, Dan Kenigsberg wrote:
qemudOpenAsUID is intended to open a file with the credentials of a specified uid. Current implementation fails if the file is accessible to one of uid's groups but not owned by uid.
This patch replaces the supplementary group list that the child process inherited from libvirtd with the default group list of uid.
Urr. Yet another twist in this ugly saga. Thanks for figuring it out!
The saga continues with another chapter. I've incorporated your comments, and had to use the reentrant getpwuid_r after all. I also bumped into another issue with saving to an already-existing file.
qemudOpenAsUID is intended to open a file with the credentials of a specified uid. Current implementation fails if the file is accessible to one of uid's groups but not owned by uid. This patch replaces the supplementary group list that the child process inherited from libvirtd with the default group list of uid. --- src/qemu/qemu_driver.c | 27 ++++++++++++++++++++++----- 1 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0ce2d40..7204ac8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -41,6 +41,7 @@ #include <signal.h> #include <paths.h> #include <pwd.h> +#include <grp.h> #include <stdio.h> #include <sys/wait.h> #include <sys/ioctl.h> @@ -6353,6 +6354,7 @@ parent_cleanup: char *buf = NULL; size_t bufsize = 1024 * 1024; int bytesread; + struct passwd pwd, *pwd_result; /* child doesn't need the read side of the pipe */ close(pipefd[0]); @@ -6365,6 +6367,26 @@ parent_cleanup: goto child_cleanup; } + if (VIR_ALLOC_N(buf, bufsize) < 0) { + exit_code = ENOMEM; + virReportOOMError(); + goto child_cleanup; + } + + exit_code = getpwuid_r(uid, &pwd, buf, bufsize, &pwd_result); + if (pwd_result == NULL) { + virReportSystemError(errno, + _("cannot getpwuid_r(%d) to read '%s'"), + uid, path); + goto child_cleanup; + } + if (initgroups(pwd.pw_name, pwd.pw_gid) != 0) { + exit_code = errno; + virReportSystemError(errno, + _("cannot initgroups(\"%s\", %d) to read '%s'"), + pwd.pw_name, pwd.pw_gid, path); + goto child_cleanup; + } if (setuid(uid) != 0) { exit_code = errno; virReportSystemError(errno, @@ -6379,11 +6401,6 @@ parent_cleanup: path, uid); goto child_cleanup; } - if (VIR_ALLOC_N(buf, bufsize) < 0) { - exit_code = ENOMEM; - virReportOOMError(); - goto child_cleanup; - } /* read from fd and write to pipefd[1] until EOF */ do { -- 1.7.2.3
Over root-squashing nfs, when virFileOperation() is called as uid==0, it may fail with EACCES, but also with EPERM, due to virFileOperationNoFork()'s failed attemp to chown a writable file. qemudDomainSaveFlag() should expect this case, too. --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7204ac8..abd8e9d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5402,13 +5402,13 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, qemudDomainSaveFileOpHook, &hdata, 0)) < 0) { /* If we failed as root, and the error was permission-denied - (EACCES), assume it's on a network-connected share where - root access is restricted (eg, root-squashed NFS). If the + (EACCES or EPERM), assume it's on a network-connected share + where root access is restricted (eg, root-squashed NFS). If the qemu user (driver->user) is non-root, just set a flag to bypass security driver shenanigans, and retry the operation after doing setuid to qemu user */ - if ((rc != -EACCES) || + if (((rc != -EACCES) && (rc != -EPERM)) || driver->user == getuid()) { virReportSystemError(-rc, _("Failed to create domain save file '%s'"), path); -- 1.7.2.3
On Tue, Oct 19, 2010 at 12:11:07PM +0200, Dan Kenigsberg wrote:
Over root-squashing nfs, when virFileOperation() is called as uid==0, it may fail with EACCES, but also with EPERM, due to virFileOperationNoFork()'s failed attemp to chown a writable file.
qemudDomainSaveFlag() should expect this case, too. --- src/qemu/qemu_driver.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 7204ac8..abd8e9d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5402,13 +5402,13 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver, virDomainPtr dom, qemudDomainSaveFileOpHook, &hdata, 0)) < 0) { /* If we failed as root, and the error was permission-denied - (EACCES), assume it's on a network-connected share where - root access is restricted (eg, root-squashed NFS). If the + (EACCES or EPERM), assume it's on a network-connected share + where root access is restricted (eg, root-squashed NFS). If the qemu user (driver->user) is non-root, just set a flag to bypass security driver shenanigans, and retry the operation after doing setuid to qemu user */
- if ((rc != -EACCES) || + if (((rc != -EACCES) && (rc != -EPERM)) || driver->user == getuid()) { virReportSystemError(-rc, _("Failed to create domain save file '%s'"), path);
Looks fine, ACK, applied and commited, I also pushed the new version of 1/2 thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Hi, I might be wrong here, but it seems that when libvirt spawns a new qemu process, it sets its uid and gid (qemu:qemu by deafult) but does not call initgroups(), so the spawned qemu cannot read files that are owned by qemu auxiliary groups. Am I right? How difficult is the fix? Dan.
On 12/19/2010 08:17 AM, Dan Kenigsberg wrote:
Hi,
I might be wrong here, but it seems that when libvirt spawns a new qemu process, it sets its uid and gid (qemu:qemu by deafult) but does not call initgroups(), so the spawned qemu cannot read files that are owned by qemu auxiliary groups.
Am I right? How difficult is the fix? You are correct that initgroups isn't called.
It looks like it could be fixed with a call to initgroups in qemu_security.c:qemuSecurityDACSetProcessLabel(), but I would defer to Dan Berrange as to whether that's the best place to put it.
This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406 If qemu is run as a different uid, it has been unable to access mode 0660 files that are owned by a different user, but with a group that the qemu is a member of (aside from the one group listed in the passwd file). initgroups will change the group membership of the process (and its children) to match the new uid. --- src/qemu/qemu_security_dac.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index 55dc0c6..2e60aec 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -12,6 +12,8 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <pwd.h> +#include <grp.h> #include "qemu_security_dac.h" #include "qemu_conf.h" @@ -558,6 +560,30 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, } } if (driver->user) { + struct passwd pwd, *pwd_result; + char *buf = NULL; + size_t bufsize = 16384; + + if (VIR_ALLOC_N(buf, bufsize) < 0) { + virReportOOMError(); + return -1; + } + getpwuid_r(driver->user, &pwd, buf, bufsize, &pwd_result); + if (pwd_result == NULL) { + virReportSystemError(errno, + _("cannot getpwuid_r(%d)"), driver->user); + VIR_FREE(buf); + return -1; + } + if (initgroups(pwd.pw_name, pwd.pw_gid) != 0) { + virReportSystemError(errno, + _("cannot initgroups(\"%s\", %d)"), + pwd.pw_name, pwd.pw_gid); + VIR_FREE(buf); + return -1; + } + VIR_FREE(buf); + if (setreuid(driver->user, driver->user) < 0) { virReportSystemError(errno, _("cannot change to '%d' user"), @@ -566,6 +592,7 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, } } + return 0; } -- 1.7.3.4
On 12/21/2010 01:45 PM, Laine Stump wrote:
This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406
If qemu is run as a different uid, it has been unable to access mode 0660 files that are owned by a different user, but with a group that the qemu is a member of (aside from the one group listed in the passwd file). initgroups will change the group membership of the process (and its children) to match the new uid. --- src/qemu/qemu_security_dac.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index 55dc0c6..2e60aec 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -12,6 +12,8 @@ #include <sys/types.h> #include <sys/stat.h> #include <fcntl.h> +#include <pwd.h> +#include <grp.h>
#include "qemu_security_dac.h" #include "qemu_conf.h" @@ -558,6 +560,30 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, } } if (driver->user) { + struct passwd pwd, *pwd_result; + char *buf = NULL; + size_t bufsize = 16384;
qemu_driver.c sets this to 1024*1024. Will that matter? For that matter, can't you provide this functionality in a single .c file so that both qemudOpenAsUID and qemuSecurityDACSetProcessLabel can share the benefits of common code? That refactoring probably deserves a v2.
@@ -566,6 +592,7 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, } }
+ return 0;
Spurious whitespace change. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 12/21/2010 04:52 PM, Eric Blake wrote:
On 12/21/2010 01:45 PM, Laine Stump wrote:
This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406
If qemu is run as a different uid, it has been unable to access mode 0660 files that are owned by a different user, but with a group that the qemu is a member of (aside from the one group listed in the passwd file). initgroups will change the group membership of the process (and its children) to match the new uid. --- src/qemu/qemu_security_dac.c | 27 +++++++++++++++++++++++++++ 1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index 55dc0c6..2e60aec 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -12,6 +12,8 @@ #include<sys/types.h> #include<sys/stat.h> #include<fcntl.h> +#include<pwd.h> +#include<grp.h>
#include "qemu_security_dac.h" #include "qemu_conf.h" @@ -558,6 +560,30 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, } } if (driver->user) { + struct passwd pwd, *pwd_result; + char *buf = NULL; + size_t bufsize = 16384; qemu_driver.c sets this to 1024*1024. Will that matter?
The usage in qemu_driver.c re-uses a buffer that was already conveniently allocated for something else. This buffer is used to contain all the strings encountered in a single entry from /etc/passwd. The manpage for getpwuid_r() has example code to learn the maximum possible size for all the strings in one line, but that example falls back to 16384 if it fails to learn the system limit, saying "this should be big enough for anything".
For that matter, can't you provide this functionality in a single .c file so that both qemudOpenAsUID and qemuSecurityDACSetProcessLabel can share the benefits of common code?
If I put it in a separate function, I won't feel so bad about adding in the extra code to detect the absolute max size to allocate. Of course, if I put it in a util file, we'll have to worry about making it compile on all you mother's favorite platforms, so... :-)
That refactoring probably deserves a v2.
Sure, I'll take a stab at it.
@@ -566,6 +592,7 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, } }
+ return 0; Spurious whitespace change.
Oops.
This supercedes a patch I sent yesterday titled: [PATCH] Call initgroups for qemu's uid prior to exec https://www.redhat.com/archives/libvir-list/2010-December/msg00846.html The first patch modifies the lowlevel error logging functions so that they preserve the value of errno. This is necessary because a later patch will rely on errno still being set after returning from a function that logs an error. The second creates a new utility function, for the purpose of centralizing/standardizing use of initgroups to cause a forked process run as a different uid to be a member of *all* groups containing that uid as a member. The 3rd patch changes two separate places in the code to use this new utility function, in both cases fixing some already (or eventually to be) encountered bugs.
There are cases when we want log an error message while still preserving errno for a caller, but the functions that log errors make system calls that will clear errno. This patch preserves errno during those most basic error logging functions (corresponding to virReportSystemError(), virReportOOMError(), networkReportError(), etc, as well as virStrError()). It does *not preserve errno across calls to higher level items such as virDispatchError(), as it's assumed the caller is all finished with any need for errno by the time it dispatches the error. --- src/util/virterror.c | 29 ++++++++++++++++++++++------- 1 files changed, 22 insertions(+), 7 deletions(-) diff --git a/src/util/virterror.c b/src/util/virterror.c index 96a09cc..7543603 100644 --- a/src/util/virterror.c +++ b/src/util/virterror.c @@ -690,6 +690,7 @@ virRaiseErrorFull(virConnectPtr conn ATTRIBUTE_UNUSED, int int2, const char *fmt, ...) { + int save_errno = errno; virErrorPtr to; char *str; int priority; @@ -700,13 +701,17 @@ virRaiseErrorFull(virConnectPtr conn ATTRIBUTE_UNUSED, * to the per-connection error object when necessary */ to = virLastErrorObject(); - if (!to) + if (!to) { + errno = save_errno; return; /* Hit OOM allocating thread error object, sod all we can do now */ + } virResetError(to); - if (code == VIR_ERR_OK) + if (code == VIR_ERR_OK) { + errno = save_errno; return; + } /* * formats the message @@ -749,6 +754,8 @@ virRaiseErrorFull(virConnectPtr conn ATTRIBUTE_UNUSED, to->str3 = strdup(str3); to->int1 = int1; to->int2 = int2; + + errno = save_errno; } /** @@ -1216,6 +1223,7 @@ void virReportErrorHelper(virConnectPtr conn, size_t linenr, const char *fmt, ...) { + int save_errno = errno; va_list args; char errorMessage[1024]; const char *virerr; @@ -1233,7 +1241,7 @@ void virReportErrorHelper(virConnectPtr conn, domcode, errcode, VIR_ERR_ERROR, virerr, errorMessage, NULL, -1, -1, virerr, errorMessage); - + errno = save_errno; } /** @@ -1249,13 +1257,16 @@ void virReportErrorHelper(virConnectPtr conn, */ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) { + int save_errno = errno; + const char *ret; + #ifdef HAVE_STRERROR_R # ifdef __USE_GNU /* Annoying linux specific API contract */ - return strerror_r(theerrno, errBuf, errBufLen); + ret = strerror_r(theerrno, errBuf, errBufLen); # else strerror_r(theerrno, errBuf, errBufLen); - return errBuf; + ret = errBuf; # endif #else /* Mingw lacks strerror_r and its strerror is definitely not @@ -1264,9 +1275,11 @@ const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) * header files for debug purposes */ int n = snprintf(errBuf, errBufLen, "errno=%d", theerrno); - return (0 < n && n < errBufLen - ? errBuf : _("internal error: buffer too small")); + ret = (0 < n && n < errBufLen + ? errBuf : _("internal error: buffer too small")); #endif + errno = save_errno; + return ret; } /** @@ -1288,6 +1301,7 @@ void virReportSystemErrorFull(int domcode, size_t linenr, const char *fmt, ...) { + int save_errno = errno; char strerror_buf[1024]; char msgDetailBuf[1024]; @@ -1318,6 +1332,7 @@ void virReportSystemErrorFull(int domcode, virRaiseErrorFull(NULL, filename, funcname, linenr, domcode, VIR_ERR_SYSTEM_ERROR, VIR_ERR_ERROR, msg, msgDetail, NULL, -1, -1, msg, msgDetail); + errno = save_errno; } /** -- 1.7.3.4
On 12/23/2010 11:39 AM, Laine Stump wrote:
There are cases when we want log an error message while still preserving errno for a caller, but the functions that log errors make system calls that will clear errno. This patch preserves errno during those most basic error logging functions (corresponding to virReportSystemError(), virReportOOMError(), networkReportError(), etc, as well as virStrError()). It does *not preserve errno across calls to higher level items such as virDispatchError(), as it's assumed the caller is all finished with any need for errno by the time it dispatches the error.
const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) { + int save_errno = errno; + const char *ret; + #ifdef HAVE_STRERROR_R # ifdef __USE_GNU /* Annoying linux specific API contract */ - return strerror_r(theerrno, errBuf, errBufLen); + ret = strerror_r(theerrno, errBuf, errBufLen); # else strerror_r(theerrno, errBuf, errBufLen); - return errBuf; + ret = errBuf; # endif #else /* Mingw lacks strerror_r and its strerror is definitely not
Ah, but gnulib now provides the LGPLv2+ strerror_r-posix module, which not only provides strerror_r on mingw, but also sanitizes the Linux interface into the POSIX compatible interface, so that we could use it to simplify the code. But that's a patch for another day. ACK as-is. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 12/23/2010 02:30 PM, Eric Blake wrote:
On 12/23/2010 11:39 AM, Laine Stump wrote:
There are cases when we want log an error message while still preserving errno for a caller, but the functions that log errors make system calls that will clear errno. This patch preserves errno during those most basic error logging functions (corresponding to virReportSystemError(), virReportOOMError(), networkReportError(), etc, as well as virStrError()). It does *not preserve errno across calls to higher level items such as virDispatchError(), as it's assumed the caller is all finished with any need for errno by the time it dispatches the error. const char *virStrerror(int theerrno, char *errBuf, size_t errBufLen) { + int save_errno = errno; + const char *ret; + #ifdef HAVE_STRERROR_R # ifdef __USE_GNU /* Annoying linux specific API contract */ - return strerror_r(theerrno, errBuf, errBufLen); + ret = strerror_r(theerrno, errBuf, errBufLen); # else strerror_r(theerrno, errBuf, errBufLen); - return errBuf; + ret = errBuf; # endif #else /* Mingw lacks strerror_r and its strerror is definitely not Ah, but gnulib now provides the LGPLv2+ strerror_r-posix module, which not only provides strerror_r on mingw, but also sanitizes the Linux interface into the POSIX compatible interface, so that we could use it to simplify the code. But that's a patch for another day.
ACK as-is.
I squashed in a bit of code to preserve errno during VIR_FREE (virFree) as you suggested in the review of patch 3/3 and pushed the result. Thanks!
virSetUIDGID() sets both the real and effective group and user of the process, and additionally calls initgroups() to assure that the process joins all the auxiliary groups that the given uid is a member of. --- src/libvirt_private.syms | 1 + src/util/util.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 + 3 files changed, 66 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0e3033d..befecc1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -853,6 +853,7 @@ virRun; virRunWithHook; virSetCloseExec; virSetNonBlock; +virSetUIDGID; virSkipSpaces; virStrToDouble; virStrToLong_i; diff --git a/src/util/util.c b/src/util/util.c index 7d3377b..a0ab28d 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -2811,6 +2811,60 @@ int virGetGroupID(const char *name, return 0; } + +/* 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. + */ +int +virSetUIDGID(uid_t uid, gid_t gid) +{ + if (gid > 0) { + if (setregid(gid, gid) < 0) { + virReportSystemError(errno, + _("cannot change to '%d' group"), gid); + return -1; + } + } + + if (uid > 0) { + struct passwd pwd, *pwd_result; + char *buf = NULL; + size_t bufsize; + + bufsize = sysconf(_SC_GETPW_R_SIZE_MAX); + if (bufsize == -1) + bufsize = 16384; + + if (VIR_ALLOC_N(buf, bufsize) < 0) { + virReportOOMError(); + return -1; + } + getpwuid_r(uid, &pwd, buf, bufsize, &pwd_result); + if (!pwd_result) { + virReportSystemError(errno, + _("cannot getpwuid_r(%d)"), uid); + VIR_FREE(buf); + return -1; + } + if (initgroups(pwd.pw_name, pwd.pw_gid) < 0) { + virReportSystemError(errno, + _("cannot initgroups(\"%s\", %d)"), + pwd.pw_name, pwd.pw_gid); + VIR_FREE(buf); + return -1; + } + VIR_FREE(buf); + + if (setreuid(uid, uid) < 0) { + virReportSystemError(errno, + _("cannot change to uid to '%d'"), uid); + return -1; + } + } + return 0; +} + #else /* HAVE_GETPWUID_R */ char * @@ -2849,6 +2903,15 @@ int virGetGroupID(const char *name ATTRIBUTE_UNUSED, return 0; } + +int +virSetUIDGID(uid_t uid ATTRIBUTE_UNUSED, + gid_t gid ATTRIBUTE_UNUSED) +{ + virUtilError(VIR_ERR_INTERNAL_ERROR, + "%s", _("virSetUIDGID is not available")); + return -1; +} #endif /* HAVE_GETPWUID_R */ diff --git a/src/util/util.h b/src/util/util.h index ef4fc13..989962f 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -93,6 +93,8 @@ int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf); int virFork(pid_t *pid); +int virSetUIDGID(uid_t uid, gid_t gid); + int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; int virFileReadAll(const char *path, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- 1.7.3.4
On 12/23/2010 11:39 AM, Laine Stump wrote:
virSetUIDGID() sets both the real and effective group and user of the process, and additionally calls initgroups() to assure that the process joins all the auxiliary groups that the given uid is a member of. --- src/libvirt_private.syms | 1 + src/util/util.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 + 3 files changed, 66 insertions(+), 0 deletions(-)
I'm guessing that the only code that called this previously was the qemu driver, in code compiled only for Linux (as the qemu driver is not compiled for mingw). Ultimately, it might be nicer to find portable ways to do the equivalent of initgroups on other platforms that lack the Linux interface, but do have a way to set supplementary groups (POSIX doesn't standardize setting supplementary groups on purpose); or even better, to have gnulib implement initgroups() for as many platforms as possible.
+ if (initgroups(pwd.pw_name, pwd.pw_gid) < 0) { + virReportSystemError(errno, + _("cannot initgroups(\"%s\", %d)"), + pwd.pw_name, pwd.pw_gid); + VIR_FREE(buf); + return -1; + }
My biggest worry is that checking this in will cause compilation failures on other platforms, so here's hoping we can get the word out that we need testing, or even modify this patch to add a configure.ac AC_CHECK_FUNCS_ONCE([initgroups]) and bracket the initgroups() call within #ifdef HAVE_INITGROUPS (non-Linux platforms won't set supplementary groups, just the primary gid, but that's better than failing to compile). Can you handle that, or would you like me to do that as a followup commit? Conditional ACK, based on that answer. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 12/23/2010 03:59 PM, Eric Blake wrote:
virSetUIDGID() sets both the real and effective group and user of the process, and additionally calls initgroups() to assure that the process joins all the auxiliary groups that the given uid is a member of. --- src/libvirt_private.syms | 1 + src/util/util.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 2 + 3 files changed, 66 insertions(+), 0 deletions(-) I'm guessing that the only code that called this previously was the qemu driver, in code compiled only for Linux (as the qemu driver is not compiled for mingw). Ultimately, it might be nicer to find portable ways to do the equivalent of initgroups on other platforms that lack the Linux interface, but do have a way to set supplementary groups (POSIX doesn't standardize setting supplementary groups on purpose); or even better, to have gnulib implement initgroups() for as many platforms as
On 12/23/2010 11:39 AM, Laine Stump wrote: possible.
+ if (initgroups(pwd.pw_name, pwd.pw_gid)< 0) { + virReportSystemError(errno, + _("cannot initgroups(\"%s\", %d)"), + pwd.pw_name, pwd.pw_gid); + VIR_FREE(buf); + return -1; + } My biggest worry is that checking this in will cause compilation failures on other platforms, so here's hoping we can get the word out that we need testing, or even modify this patch to add a configure.ac AC_CHECK_FUNCS_ONCE([initgroups]) and bracket the initgroups() call within #ifdef HAVE_INITGROUPS (non-Linux platforms won't set supplementary groups, just the primary gid, but that's better than failing to compile). Can you handle that, or would you like me to do that as a followup commit?
Conditional ACK, based on that answer.
I squashed in the configure.ac change you suggest, made the call to initgroups conditional, and pushed the result. Thanks!
This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406 If qemu is run as a different uid, it has been unable to access mode 0660 files that are owned by a different user, but with a group that the qemu is a member of (aside from the one group listed in the passwd file), because initgroups() is not being called prior to the exec. initgroups will change the group membership of the process (and its children) to match the new uid. To make this happen, the setregid()/setreuid() code in qemuSecurityDACSetProcessLabel has been replaced with a call to virSetUIDGID(), which does both of those, plus calls initgroups. Similar, but not identical, code in qemudOpenAsUID() has been replaced with virSetUIDGID(). This not only consolidates the functionality to a single location, but also potentially fixes some as-yet unreported bugs. --- src/qemu/qemu_driver.c | 44 +++++++++++++---------------------------- src/qemu/qemu_security_dac.c | 18 +--------------- 2 files changed, 16 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 924446f..8be34d5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -40,8 +40,6 @@ #include <fcntl.h> #include <signal.h> #include <paths.h> -#include <pwd.h> -#include <grp.h> #include <stdio.h> #include <sys/wait.h> #include <sys/ioctl.h> @@ -5499,7 +5497,9 @@ cleanup: after it's finished reading (to avoid a zombie, if nothing else). */ -static int qemudOpenAsUID(const char *path, uid_t uid, pid_t *child_pid) { +static int +qemudOpenAsUID(const char *path, uid_t uid, gid_t gid, pid_t *child_pid) +{ int pipefd[2]; int fd = -1; @@ -5563,7 +5563,6 @@ parent_cleanup: char *buf = NULL; size_t bufsize = 1024 * 1024; int bytesread; - struct passwd pwd, *pwd_result; /* child doesn't need the read side of the pipe */ VIR_FORCE_CLOSE(pipefd[0]); @@ -5576,33 +5575,11 @@ parent_cleanup: goto child_cleanup; } - if (VIR_ALLOC_N(buf, bufsize) < 0) { - exit_code = ENOMEM; - virReportOOMError(); - goto child_cleanup; + if (virSetUIDGID(uid, gid) < 0) { + exit_code = errno; + goto child_cleanup; } - exit_code = getpwuid_r(uid, &pwd, buf, bufsize, &pwd_result); - if (pwd_result == NULL) { - virReportSystemError(errno, - _("cannot getpwuid_r(%d) to read '%s'"), - uid, path); - goto child_cleanup; - } - if (initgroups(pwd.pw_name, pwd.pw_gid) != 0) { - exit_code = errno; - virReportSystemError(errno, - _("cannot initgroups(\"%s\", %d) to read '%s'"), - pwd.pw_name, pwd.pw_gid, path); - goto child_cleanup; - } - if (setuid(uid) != 0) { - exit_code = errno; - virReportSystemError(errno, - _("cannot setuid(%d) to read '%s'"), - uid, path); - goto child_cleanup; - } if ((fd = open(path, O_RDONLY)) < 0) { exit_code = errno; virReportSystemError(errno, @@ -5611,6 +5588,12 @@ parent_cleanup: goto child_cleanup; } + if (VIR_ALLOC_N(buf, bufsize) < 0) { + exit_code = ENOMEM; + virReportOOMError(); + goto child_cleanup; + } + /* read from fd and write to pipefd[1] until EOF */ do { if ((bytesread = saferead(fd, buf, bufsize)) < 0) { @@ -5682,7 +5665,8 @@ qemudDomainSaveImageOpen(struct qemud_driver *driver, that might have better luck. Create a pipe, then fork a child process to run as the qemu user, which will hopefully have the necessary authority to read the file. */ - if ((fd = qemudOpenAsUID(path, driver->user, &read_pid)) < 0) { + if ((fd = qemudOpenAsUID(path, + driver->user, driver->group, &read_pid)) < 0) { /* error already reported */ goto error; } diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c index 88fdb8d..b5c52d1 100644 --- a/src/qemu/qemu_security_dac.c +++ b/src/qemu/qemu_security_dac.c @@ -549,22 +549,8 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED, if (!driver->privileged) return 0; - if (driver->group) { - if (setregid(driver->group, driver->group) < 0) { - virReportSystemError(errno, - _("cannot change to '%d' group"), - driver->group); - return -1; - } - } - if (driver->user) { - if (setreuid(driver->user, driver->user) < 0) { - virReportSystemError(errno, - _("cannot change to '%d' user"), - driver->user); - return -1; - } - } + if (virSetUIDGID(driver->user, driver->group) < 0) + return -1; return 0; } -- 1.7.3.4
On 12/23/2010 11:39 AM, Laine Stump wrote:
This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406
If qemu is run as a different uid, it has been unable to access mode 0660 files that are owned by a different user, but with a group that the qemu is a member of (aside from the one group listed in the passwd file), because initgroups() is not being called prior to the exec. initgroups will change the group membership of the process (and its children) to match the new uid.
To make this happen, the setregid()/setreuid() code in qemuSecurityDACSetProcessLabel has been replaced with a call to virSetUIDGID(), which does both of those, plus calls initgroups.
Similar, but not identical, code in qemudOpenAsUID() has been replaced with virSetUIDGID(). This not only consolidates the functionality to a single location, but also potentially fixes some as-yet unreported bugs. --- src/qemu/qemu_driver.c | 44 +++++++++++++---------------------------- src/qemu/qemu_security_dac.c | 18 +--------------- 2 files changed, 16 insertions(+), 46 deletions(-)
+ if (virSetUIDGID(uid, gid) < 0) { + exit_code = errno; + goto child_cleanup;
Ah, I see why you needed patch 1 - patch two calls virReportSystemError in between the setting of errno and the return. However, one glitch - VIR_FREE(x) does not (yet) guarantee that errno is preserved (some poorly-written free() implementations can modify errno; glibc is nicer in trying to avoid that). You'll need to modify 1/3 accordingly. Which means ACK to this patch as perfect, but the prerequisites aren't there yet :) -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 12/23/2010 04:05 PM, Eric Blake wrote:
On 12/23/2010 11:39 AM, Laine Stump wrote:
This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406
If qemu is run as a different uid, it has been unable to access mode 0660 files that are owned by a different user, but with a group that the qemu is a member of (aside from the one group listed in the passwd file), because initgroups() is not being called prior to the exec. initgroups will change the group membership of the process (and its children) to match the new uid.
To make this happen, the setregid()/setreuid() code in qemuSecurityDACSetProcessLabel has been replaced with a call to virSetUIDGID(), which does both of those, plus calls initgroups.
Similar, but not identical, code in qemudOpenAsUID() has been replaced with virSetUIDGID(). This not only consolidates the functionality to a single location, but also potentially fixes some as-yet unreported bugs. --- src/qemu/qemu_driver.c | 44 +++++++++++++---------------------------- src/qemu/qemu_security_dac.c | 18 +--------------- 2 files changed, 16 insertions(+), 46 deletions(-) + if (virSetUIDGID(uid, gid)< 0) { + exit_code = errno; + goto child_cleanup; Ah, I see why you needed patch 1 - patch two calls virReportSystemError in between the setting of errno and the return.
However, one glitch - VIR_FREE(x) does not (yet) guarantee that errno is preserved (some poorly-written free() implementations can modify errno; glibc is nicer in trying to avoid that). You'll need to modify 1/3 accordingly.
Which means ACK to this patch as perfect, but the prerequisites aren't there yet :)
Pushed. Thanks again for all the last minute pre-holiday reviews!
On Sun, Oct 17, 2010 at 10:58:55PM +0200, Dan Kenigsberg wrote:
qemudOpenAsUID is intended to open a file with the credentials of a specified uid. Current implementation fails if the file is accessible to one of uid's groups but not owned by uid.
This patch replaces the supplementary group list that the child process inherited from libvirtd with the default group list of uid. --- src/qemu/qemu_driver.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0ce2d40..a1027d4 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6353,6 +6353,7 @@ parent_cleanup: char *buf = NULL; size_t bufsize = 1024 * 1024; int bytesread; + struct passwd *pwd;
/* child doesn't need the read side of the pipe */ close(pipefd[0]); @@ -6365,6 +6366,21 @@ parent_cleanup: goto child_cleanup; }
+ /* we can avoid getpwuid_r() in threadless child */ + if ((pwd = getpwuid(uid)) == NULL) {
That may be so, but you're going to hit a 'make syntax-check' failure here, and we don't want to whitelist the entire qemu_driver.c file to get past it.
+ exit_code = errno; + virReportSystemError(errno, + _("cannot setuid(%d) to read '%s'"), + uid, path); + goto child_cleanup; + } + if (initgroups(pwd->pw_name, pwd->pw_gid) != 0) { + exit_code = errno; + virReportSystemError(errno, + _("cannot setuid(%d) to read '%s'"), + uid, path); + goto child_cleanup; + } if (setuid(uid) != 0) { exit_code = errno; virReportSystemError(errno,
Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (5)
-
Dan Kenigsberg -
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Laine Stump