[libvirt] [PATCH v2] util: Make failure to get suplementary group list for a uid non-fatal

15 Jun 2016

Since introduction of the DAC security driver we've documented that
seclabels with a leading + can be used with numerical uid. This would
not work though with the rest of libvirt if the uid was not actually
used in the system as we'd fail when trying to get a list of
suplementary groups for the given uid. Since a uid without entry in
/etc/passwd (or other user database) will not have any suppolementary
groups we can treat the failure to obtain them as such.

This patch modifies virGetGroupList to not report the error of missing
user and tweaks callers to treat the missing list as having 0
supplementary groups.

The only place reporting errors is virt-login-shell as it's used to
determine whether the given user is allowed to access the shell.
---
Although it was ACKed I'm reposting a rebased version including the fixes and
fixed merge conflicts. I'd like to hear feedback from the reporter of this issue.

CC: Roy Keene <rkeene@knightpoint.com>

 src/security/security_dac.c | 13 +++++++------
 src/util/vircommand.c       |  4 +++-
 src/util/virfile.c          | 28 ++++++++++++++++------------
 src/util/virutil.c          | 27 +++++++++++++++++----------
 tools/virt-login-shell.c    |  6 +++++-
 5 files changed, 48 insertions(+), 30 deletions(-)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index 442ce70..9dec201 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -266,14 +266,15 @@ static int
 virSecurityDACPreFork(virSecurityManagerPtr mgr)
 {
     virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
-    int ngroups;

     VIR_FREE(priv->groups);
-    priv->ngroups = 0;
-    if ((ngroups = virGetGroupList(priv->user, priv->group,
-                                   &priv->groups)) < 0)
-        return -1;
-    priv->ngroups = ngroups;
+
+    /* ignore a possible problem in getting supplementary groups just assume
+     * we have none and continue with uid/gid only */
+    if ((priv->ngroups = virGetGroupList(priv->user, priv->group,
+                                         &priv->groups)) < 0)
+        priv->ngroups = 0;
+
     return 0;
 }

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index f5bd7af..58af06a 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -554,8 +554,10 @@ virExec(virCommandPtr cmd)
         childerr = null;
     }

+    /* ignore a possible problem in getting supplementary groups just assume
+     * we have none and continue with uid/gid only */
     if ((ngroups = virGetGroupList(cmd->uid, cmd->gid, &groups)) < 0)
-        goto cleanup;
+        ngroups = 0;

     pid = virFork();

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 9d460b9..4298ec5 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -1978,9 +1978,10 @@ virFileAccessibleAs(const char *path, int mode,
         gid == getegid())
         return access(path, mode);

-    ngroups = virGetGroupList(uid, gid, &groups);
-    if (ngroups < 0)
-        return -1;
+    /* ignore a possible problem in getting supplementary groups just assume
+     * we have none and continue with uid/gid only */
+    if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
+        ngroups = 0;

     pid = virFork();

@@ -2104,9 +2105,10 @@ virFileOpenForked(const char *path, int openflags, mode_t mode,
      * following dance avoids problems caused by root-squashing
      * NFS servers. */

-    ngroups = virGetGroupList(uid, gid, &groups);
-    if (ngroups < 0)
-        return -errno;
+    /* ignore a possible problem in getting supplementary groups just assume
+     * we have none and continue with uid/gid only */
+    if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
+        ngroups = 0;

     if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
         ret = -errno;
@@ -2407,9 +2409,10 @@ virFileRemove(const char *path,
     if (gid == (gid_t) -1)
         gid = getegid();

-    ngroups = virGetGroupList(uid, gid, &groups);
-    if (ngroups < 0)
-        return -errno;
+    /* ignore a possible problem in getting supplementary groups just assume
+     * we have none and continue with uid/gid only */
+    if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
+        ngroups = 0;

     pid = virFork();

@@ -2583,9 +2586,10 @@ virDirCreate(const char *path,
     if (gid == (gid_t) -1)
         gid = getegid();

-    ngroups = virGetGroupList(uid, gid, &groups);
-    if (ngroups < 0)
-        return -errno;
+    /* ignore a possible problem in getting supplementary groups just assume
+     * we have none and continue with uid/gid only */
+    if ((ngroups = virGetGroupList(uid, gid, &groups)) == -1)
+        ngroups = 0;

     pid = virFork();

diff --git a/src/util/virutil.c b/src/util/virutil.c
index ff58054..c11278f 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -755,9 +755,10 @@ virGetUserDirectory(void)

 #ifdef HAVE_GETPWUID_R
 /* Look up fields from the user database for the given user.  On
- * error, set errno, report the error, and return -1.  */
+ * error, set errno, report the error if not instructed otherwise via @quiet,
+ * and return -1.  */
 static int
-virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell)
+virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell, bool quiet)
 {
     char *strbuf;
     struct passwd pwbuf;
@@ -792,12 +793,19 @@ virGetUserEnt(uid_t uid, char **name, gid_t *group, char **dir, char **shell)
         if (VIR_RESIZE_N(strbuf, strbuflen, strbuflen, strbuflen) < 0)
             goto cleanup;
     }
+
     if (rc != 0) {
+        if (quiet)
+            goto cleanup;
+
         virReportSystemError(rc,
                              _("Failed to find user record for uid '%u'"),
                              (unsigned int) uid);
         goto cleanup;
     } else if (pw == NULL) {
+        if (quiet)
+            goto cleanup;
+
         virReportError(VIR_ERR_SYSTEM_ERROR,
                        _("Failed to find user record for uid '%u'"),
                        (unsigned int) uid);
@@ -882,7 +890,7 @@ char *
 virGetUserDirectoryByUID(uid_t uid)
 {
     char *ret;
-    virGetUserEnt(uid, NULL, NULL, &ret, NULL);
+    virGetUserEnt(uid, NULL, NULL, &ret, NULL, false);
     return ret;
 }

@@ -890,7 +898,7 @@ virGetUserDirectoryByUID(uid_t uid)
 char *virGetUserShell(uid_t uid)
 {
     char *ret;
-    virGetUserEnt(uid, NULL, NULL, NULL, &ret);
+    virGetUserEnt(uid, NULL, NULL, NULL, &ret, false);
     return ret;
 }

@@ -940,7 +948,7 @@ char *virGetUserRuntimeDirectory(void)
 char *virGetUserName(uid_t uid)
 {
     char *ret;
-    virGetUserEnt(uid, &ret, NULL, NULL, NULL);
+    virGetUserEnt(uid, &ret, NULL, NULL, NULL, false);
     return ret;
 }

@@ -1113,8 +1121,9 @@ virGetGroupID(const char *group, gid_t *gid)
 /* Compute the list of primary and 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. */
+ * on success, or -1 on failure with no error reported as this usually isn't
+ * a fatal problem for callers. errno is set on error. May not be called
+ * between fork and exec. */
 int
 virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
 {
@@ -1126,14 +1135,12 @@ virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
     if (uid == (uid_t)-1)
         return 0;

-    if (virGetUserEnt(uid, &user, &primary, NULL, NULL) < 0)
+    if (virGetUserEnt(uid, &user, &primary, NULL, NULL, true) < 0)
         return -1;

     ret = mgetgroups(user, primary, list);
     if (ret < 0) {
         sa_assert(!*list);
-        virReportSystemError(errno,
-                             _("cannot get group list for '%s'"), user);
         goto cleanup;
     }

diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
index 38fcb9e..2ad6634 100644
--- a/tools/virt-login-shell.c
+++ b/tools/virt-login-shell.c
@@ -303,8 +303,12 @@ main(int argc, char **argv)
     if (!(conf = virConfReadFile(login_shell_path, 0)))
         goto cleanup;

-    if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
+    if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("failed to query supplementary group list for uid '%u'"),
+                       (unsigned int) uid);
         goto cleanup;
+    }

     if (virLoginShellAllowedUser(conf, name, groups) < 0)
         goto cleanup;
-- 
2.8.3

    

Peter Krempa

John Ferlan

tags

participants (2)