[libvirt] [PATCH 3/3] Replace setuid/setgid/initgroups with virSetUIDGID()

[Laine Stump]

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