Replace setuid/setgid/initgroups with virSetUIDGID()

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.

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9b538a39527c27f095e05ccd8556086f86f2fc4a..e24ee41bb23b2f113ee1c6316192f60daf907d6d 100644 (file)
--- 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 88fdb8dc81046a8bd593c34270e13bbff5f2420d..b5c52d15e54fdc1f6a923fc279262c72688e7822 100644 (file)
--- 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;
 }