$subj: "supplementary"
On 06/15/2016 11:58 AM, Peter Krempa wrote:
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
ditto
/etc/passwd (or other user database) will not have any suppolementary
ditto
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.
"and tweaks callers that then won't need the supplementary list to
ignore the failure."
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(a)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(-)
You did mix those "quiet" changes into the same patch...
Similarly you could just modify virGetGroupList to add a quiet argument
where quiet (or whatever it is) means don't return an error. Just a
thought...
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)
Since mgetgroups() returns -1 on failure, this could be masking
something different... It wasn't clear why the comparison to -1 in
some/most cases, while others were < 0...
+ 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;
Maybe this should return -2; To allow the callers to distinguish?
-2 means no user
-1 means some error getting group list
0-n is the count of entries on the group list.
ret = mgetgroups(user, primary, list);
Once we get here we know "user"/uid_t exists on this host, right? The
"user" will not be NULL, we'll have some sort of primary, and with any
luck get a list. If getting the list fails, then I would think we'd want
to trap that. I was only half following the IRC discussion though
if (ret < 0) {
sa_assert(!*list);
- virReportSystemError(errno,
- _("cannot get group list for '%s'"),
user);
So removing this could be masking a different problem...
John
BTW: Using the most recent coverity, I tried removing that sa_assert and
coverity didn't complain... Still on a todo list I have somewhere - go
through all those sa_asserts (sigh).
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;