[libvirt] [PATCH v3 0/4] util: Make failure to get supplementary groups non-fatal
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
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
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
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
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
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
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
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.
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
participants (3)
-
John Ferlan -
Pavel Hrdina -
Peter Krempa