9:44 p.m.
Changes:
- New patches fixing few problems in virt-login-shell found while modifying the original
patch.
- split out addition of @quiet
- rest of review feedback
Peter Krempa (4):
tools: virt-login-shell: Fix group list bounds checking
tools: virt-login-shell: Fix cut'n'paste mistake in error message
util: Add option not to report errors in virGetUserEnt
util: Make failure to get supplementary group list for a uid non-fatal
src/util/virutil.c | 49 ++++++++++++++++++++++++++++--------------------
tools/virt-login-shell.c | 9 +++++----
2 files changed, 34 insertions(+), 24 deletions(-)
--
2.8.3
9:44 p.m.
New subject: [libvirt] [PATCH v3 1/4] tools: virt-login-shell: Fix group list bounds checking
The list certainly isn't zero terminated and it would isallow usage of
group 'root'. Pass in the array size and match against it.
---
tools/virt-login-shell.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
index 38fcb9e..96ca410 100644
--- a/tools/virt-login-shell.c
+++ b/tools/virt-login-shell.c
@@ -47,7 +47,8 @@ static const char *conf_file = SYSCONFDIR
"/libvirt/virt-login-shell.conf";
static int virLoginShellAllowedUser(virConfPtr conf,
const char *name,
- gid_t *groups)
+ gid_t *groups,
+ size_t ngroups)
{
virConfValuePtr p;
int ret = -1;
@@ -74,7 +75,7 @@ static int virLoginShellAllowedUser(virConfPtr conf,
ptr = &pp->str[1];
if (!*ptr)
continue;
- for (i = 0; groups[i]; i++) {
+ for (i = 0; i < ngroups; i++) {
if (!(gname = virGetGroupName(groups[i])))
continue;
if (fnmatch(ptr, gname, 0) == 0) {
@@ -306,7 +307,7 @@ main(int argc, char **argv)
if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0)
goto cleanup;
- if (virLoginShellAllowedUser(conf, name, groups) < 0)
+ if (virLoginShellAllowedUser(conf, name, groups, ngroups) < 0)
goto cleanup;
if (virLoginShellGetShellArgv(conf, &shargv, &shargvlen) < 0)
--
2.8.3
8:09 p.m.
New subject: [libvirt] [PATCH v3 1/4] tools: virt-login-shell: Fix group list bounds checking
On Fri, Jun 17, 2016 at 03:44:09PM +0200, Peter Krempa wrote:
The list certainly isn't zero terminated and it would isallow
usage of
s/isallow/disallow/
group 'root'. Pass in the array size and match against it.
---
tools/virt-login-shell.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
ACK
9:44 p.m.
New subject: [libvirt] [PATCH v3 2/4] tools: virt-login-shell: Fix cut'n'paste mistake in error message
Whine about 'allowed_users' having wrong format rather than 'shell'
---
tools/virt-login-shell.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
index 96ca410..e8474a4 100644
--- a/tools/virt-login-shell.c
+++ b/tools/virt-login-shell.c
@@ -64,7 +64,7 @@ static int virLoginShellAllowedUser(virConfPtr conf,
for (pp = p->list; pp; pp = pp->next) {
if (pp->type != VIR_CONF_STRING) {
virReportSystemError(EINVAL, "%s",
- _("shell must be a list of strings"));
+ _("allowed_users must be a list of
strings"));
goto cleanup;
} else {
/*
--
2.8.3
8:10 p.m.
New subject: [libvirt] [PATCH v3 2/4] tools: virt-login-shell: Fix cut'n'paste mistake in error message
On Fri, Jun 17, 2016 at 03:44:10PM +0200, Peter Krempa wrote:
Whine about 'allowed_users' having wrong format rather than
'shell'
---
tools/virt-login-shell.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK
9:44 p.m.
New subject: [libvirt] [PATCH v3 3/4] util: Add option not to report errors in virGetUserEnt
In some cases it will be necessary to ignore errors reported from this
function. This allows suppressing them to avoid spamming logs.
---
src/util/virutil.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index ff58054..392091b 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;
}
@@ -1126,7 +1134,7 @@ 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, false) < 0)
return -1;
ret = mgetgroups(user, primary, list);
--
2.8.3
9:44 p.m.
New subject: [libvirt] [PATCH v3 4/4] util: Make failure to get supplementary group list for a uid non-fatal
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
supplementary groups for the given uid. Since a uid without entry in
/etc/passwd (or other user database) will not have any supplementary
groups we can treat the failure to obtain them as such.
This patch modifies virGetGroupList to not report the error for missing
users and makes it return an empty list or just the group specified in
@gid.
All callers will grand less permissions in case of failure and thus this
change is safe.
---
src/util/virutil.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 392091b..60da17b 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1120,29 +1120,30 @@ 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. */
+ * storing a malloc'd result into @list. If uid is -1 or doesn't exist in the
+ * system database querying of the supplementary groups is skipped.
+ *
+ * Returns 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.
+ * */
int
virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
{
- int ret = -1;
+ int ret = 0;
char *user = NULL;
gid_t primary;
*list = NULL;
- if (uid == (uid_t)-1)
- return 0;
- if (virGetUserEnt(uid, &user, &primary, NULL, NULL, false) < 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;
+ /* invalid users have no supplementary groups */
+ if (uid != (uid_t)-1 &&
+ virGetUserEnt(uid, &user, &primary, NULL, NULL, true) >= 0) {
+ if ((ret = mgetgroups(user, primary, list)) < 0) {
+ virReportSystemError(errno,
+ _("cannot get group list for '%s'"),
user);
+ ret = -1;
+ goto cleanup;
+ }
}
if (gid != (gid_t)-1) {
--
2.8.3
8:10 p.m.
New subject: [libvirt] [PATCH v3 4/4] util: Make failure to get supplementary group list for a uid non-fatal
On 06/17/2016 09:44 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
supplementary groups for the given uid. Since a uid without entry in
/etc/passwd (or other user database) will not have any supplementary
groups we can treat the failure to obtain them as such.
This patch modifies virGetGroupList to not report the error for missing
users and makes it return an empty list or just the group specified in
@gid.
All callers will grand less permissions in case of failure and thus this
change is safe.
"grand less"? did you mean "gain less"?
ACK series (just typed out loud below)
John
---
src/util/virutil.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 392091b..60da17b 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -1120,29 +1120,30 @@ 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. */
+ * storing a malloc'd result into @list. If uid is -1 or doesn't exist in the
+ * system database querying of the supplementary groups is skipped.
+ *
+ * Returns 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.
+ * */
int
virGetGroupList(uid_t uid, gid_t gid, gid_t **list)
{
- int ret = -1;
+ int ret = 0;
char *user = NULL;
gid_t primary;
*list = NULL;
- if (uid == (uid_t)-1)
- return 0;
- if (virGetUserEnt(uid, &user, &primary, NULL, NULL, false) < 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;
+ /* invalid users have no supplementary groups */
+ if (uid != (uid_t)-1 &&
+ virGetUserEnt(uid, &user, &primary, NULL, NULL, true) >= 0) {
+ if ((ret = mgetgroups(user, primary, list)) < 0) {
+ virReportSystemError(errno,
+ _("cannot get group list for '%s'"),
user);
+ ret = -1;
+ goto cleanup;
+ }
}
Playing the "what if" game here. If for some reason uid == -1 OR
virGetUserEnt fails, then
gid - still to be determined
ret = 0
if (gid != (gid_t)-1) {
If we enter this "if", then the for loop will exit immediately and the
VIR_APPEND_ELEMENT will append that gid onto *list which is IIRC what we
want. Then ret will be set to 1 (i) and we return. Otherwise if gid ==
-1, then we return 0, which is still fine.
11:37 p.m.
New subject: [libvirt] [PATCH v3 4/4] util: Make failure to get supplementary group list for a uid non-fatal
On Mon, Jun 20, 2016 at 08:10:10 -0400, John Ferlan wrote:
On 06/17/2016 09:44 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
> supplementary groups for the given uid. Since a uid without entry in
> /etc/passwd (or other user database) will not have any supplementary
> groups we can treat the failure to obtain them as such.
>
> This patch modifies virGetGroupList to not report the error for missing
> users and makes it return an empty list or just the group specified in
> @gid.
>
> All callers will grand less permissions in case of failure and thus this
> change is safe.
"grand less"? did you mean "gain less"?
I've explained it a bit more:
All callers will grant less permissions to a user in case of failure of
this function and thus this change is safe.
ACK series (just typed out loud below)
John
> ---
> src/util/virutil.c | 31 ++++++++++++++++---------------
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/src/util/virutil.c b/src/util/virutil.c
> index 392091b..60da17b 100644
> --- a/src/util/virutil.c
> +++ b/src/util/virutil.c
[...]
> + }
> }
Playing the "what if" game here. If for some reason uid == -1 OR
virGetUserEnt fails, then
gid - still to be determined
ret = 0
>
> if (gid != (gid_t)-1) {
>
If we enter this "if", then the for loop will exit immediately and the
VIR_APPEND_ELEMENT will append that gid onto *list which is IIRC what we
want. Then ret will be set to 1 (i) and we return. Otherwise if gid ==
-1, then we return 0, which is still fine.
Yep I wanted to achieve exactly this logic as it makes most sense IMO.
I've pushed this now. Thanks.
Peter
3055
days inactive
3058
days old
8 comments
3 participants
participants (3)
-
John Ferlan -
Pavel Hrdina -
Peter Krempa