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

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 04:52 PM, Eric Blake wrote: 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". 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... :-) Sure, I'll take a stab at it. Oops.

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

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