[libvirt] [PATCH v3 4/4] util: Make failure to get supplementary group list for a uid non-fatal

[John Ferlan]

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.