Re: [PATCH] libtirpc: handle large numbers of supplemental groups gracefully (try #2)

[Peter Staubach <staubach@redhat.com>]

Jeff Layton wrote:
> On Tue, 16 Feb 2010 14:24:09 -0500
> Peter Staubach <staubach@redhat.com> wrote:
> 
>> Jeff Layton wrote:
>>> This is the second attempt at this patch. The main changes are that this
>>> one doesn't set a floor value for the size of the group list. There are
>>> also a few minor cleanups and comments added.
>>>
>>> If authunix_create_default() is called by a user with more than 16
>>> supplimental groups, it'll abort(), which causes the program to crash
>>> and coredump.
>>>
>>> Fix it to handle this situation gracefully. Get the number of groups
>>> that the user has first, and then allocate a big enough buffer to hold
>>> them. Then, just don't let the lower function use more than the NGRPS
>>> groups.
>>>
>>> Also fix up the error handling in this function so that it just returns
>>> a NULL pointer on error and logs a message via warnx() instead of
>>> calling abort().
>>>
>>> Reported-by: Peter Engel <peter.engel-y6kNeMnOB+c@public.gmane.org>
>>> Signed-off-by: Jeff Layton <jlayton@redhat.com>
>>> ---
>>>  src/auth_unix.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>>>  1 files changed, 56 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/auth_unix.c b/src/auth_unix.c
>>> index 71ca15d..a295e71 100644
>>> --- a/src/auth_unix.c
>>> +++ b/src/auth_unix.c
>>> @@ -49,6 +49,7 @@
>>>  #include <stdlib.h>
>>>  #include <unistd.h>
>>>  #include <string.h>
>>> +#include <errno.h>
>>>  
>>>  #include <rpc/types.h>
>>>  #include <rpc/xdr.h>
>>> @@ -175,20 +176,69 @@ AUTH *
>>>  authunix_create_default()
>>>  {
>>>  	int len;
>>> +	size_t bufsize = 0;
>>>  	char machname[MAXHOSTNAMELEN + 1];
>>>  	uid_t uid;
>>>  	gid_t gid;
>>> -	gid_t gids[NGRPS];
>>> +	gid_t *gids = NULL;
>>> +	AUTH *auth;
>>> +
>>> +	if (gethostname(machname, sizeof machname) == -1) {
>>> +		warnx("%s: gethostname() failed: %s", __func__,
>>> +			strerror(errno));
>>> +		return NULL;
>>> +	}
>>>  
>>> -	if (gethostname(machname, sizeof machname) == -1)
>>> -		abort();
>>>  	machname[sizeof(machname) - 1] = 0;
>>>  	uid = geteuid();
>>>  	gid = getegid();
>>> -	if ((len = getgroups(NGRPS, gids)) < 0)
>>> -		abort();
>>> +
>>> +retry:
>>> +	len = getgroups(0, NULL);
>>> +	if (len < 0) {
>>> +		warnx("%s: failed to get number of groups: %s", __func__,
>>> +			strerror(errno));
>>> +		return NULL;
>>> +	}
>>> +
>>> +	if (len == 0)
>>> +		goto no_groups;
>>> +
>>> +	bufsize = len * sizeof(gid_t);
>>> +	gids = mem_alloc(bufsize);
>>> +	if (gids == NULL) {
>>> +		warnx("%s: memory allocation failed: %s", __func__,
>>> +			strerror(ENOMEM));
>>> +		return NULL;
>>> +	}
>>> +
>>> +	len = getgroups(len, gids);
>>> +	if (len < 0) {
>>> +		mem_free(gids, bufsize);
>>> +		/*
>>> +		 * glibc equivalent routines mention that it's possible for
>>> +		 * the number of groups to change between two getgroups calls.
>>> +		 * If that happens, retry the whole thing again.
>>> +		 */
>>> +		if (len == -EINVAL) {
>>> +			gids = NULL;
>>> +			bufsize = 0;
>>> +			goto retry;
>>> +		}
>>> +		warnx("%s: failed to get group list: %s", __func__,
>>> +			strerror(errno));
>>> +		return NULL;
>>> +	}
>>> +
>>> +	/* AUTH_UNIX has a hard limit of NGRPS supplemental groups */
>>> +	if (len > NGRPS)
>>> +		len = NGRPS;
>>> +
>>> +no_groups:
>>>  	/* XXX: interface problem; those should all have been unsigned */
>>> -	return (authunix_create(machname, uid, gid, len, gids));
>>> +	auth = authunix_create(machname, uid, gid, len, gids);
>>> +	mem_free(gids, bufsize);
>>> +	return auth;
>>>  }
>>>  
>>>  /*
>> This change to restrict the groups used to the first NGRPS
>> groups is one that we have always avoided.  It can be quite
>> confusing to the user, to have an operation fail, but then
>> to look and notice that the correct group is listed.
>>
>> Having the library abort seems odd and wrong, but this will
>> also change semantics.  Is there really a problem here,
>> after all of these years, that must be addressed?
>>
> 
> Yes, we had a bug report against nfs-utils where someone was seeing
> umount.nfs core dump because root was in >NGRPS groups.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=565507
> 
> ...as Chuck points out, this is basically what glibc has done for
> years. While it may be confusing for users, I think it's more
> preferable than having the programs fail outright.
> 
> What sort of behavior would you propose in its place?
> 

Well, I do agree that a library routine should not call abort().
That's just pure and simple, wrong.  That bug must be fixed.

However, making a policy decision for the user also seems
wrong.  Just because glibc did something does not make it right.
It does tend to make it a standard, so the semantic change
argument becomes weaker, but this change was made to a different
RPC library, so the application must have/be changed to use it,
so all bets are off again.

My inclination would be to return NULL and see who complains.

		ps