Re: Possible bug in NFSv4 with krb5p security?

Andrey Simonenko wrote:
> On Wed, Feb 20, 2013 at 06:29:07PM -0500, Rick Macklem wrote:
> > Andrey Simonnenko wrote:
> > >
> > > Another variant. This is a program that can be used for verifying
> > > correctness of the function, just change PWBUF_SIZE_* values and
> > > put
> > > some printfs to see when buffer is reallocated. sizehint is
> > > updated
> > > only when malloc() succeeded.
> > >
> > > ---------------------------------
> > > static int
> > > getpwnam_r_func(const char *name, uid_t *uidp)
> > > {
> > > #define PWBUF_SIZE_INI (2 * MAXLOGNAME + MAXPATHLEN +
> > > _PASSWORD_LEN)
> > > #define PWBUF_SIZE_INC 128
> > >
> > > 	static size_t sizehint = PWBUF_SIZE_INI;
> > >
> > > 	struct passwd pwd;
> > > 	struct passwd *pw;
> > > 	char *buf;
> > > 	size_t size;
> > > 	int error;
> > > 	char lname[MAXLOGNAME];
> > > 	char bufs[PWBUF_SIZE_INI];
> > >
> > > 	strncpy(lname, name, sizeof(lname));
> > >
> > > 	buf = bufs;
> > > 	size = sizeof(bufs);
> > > 	for (;;) {
> > > 		error = getpwnam_r(lname, &pwd, buf, size, &pw);
> > > 		if (buf != bufs)
> > > 			free(buf);
> > > 		if (pw != NULL) {
> > > 			*uidp = pw->pw_uid;
> > > 			return (GSS_S_COMPLETE);
> > > 		} else if (error != ERANGE || size > SIZE_MAX - PWBUF_SIZE_INC)
> > > 			return (GSS_S_FAILURE);
> > > 		if (size != sizehint)
> > > 			size = sizehint;
> > > 		else
> > > 			size += PWBUF_SIZE_INC;
> > > 		buf = malloc(size);
> > > 		if (buf == NULL)
> > > 			return (GSS_S_FAILURE);
> > > 		sizehint = size;
> > > 	}
> > > }
> > >
> > All looks fine to me. (Before my mailer messed with the
> > whitespace;-)
> >
> > Thanks, rick
> > ps: I will commit it in April, unless someone else does so sooner.
> 
> I was thinking about this approach once again and made a conclusion
> that
> it is wrong. Using static buffer at first and then allocate memory for
> next calls can cause slowdown, depends on number of entries and used
> database backend of course. The libc code for getpwnam() allocates
> memory for the buffer and does not free it on exit from the function,
> 
Not sure what you were saying by the last sentence? Using a static
buffer here would make the code unsafe for multiple threads. Although
the gssd is currently single threaded, that might change someday, maybe?
(Using the static as a "hint" should be safe for multiple threads, since
 it is just a hint.)

> If the above written code or any of its modification is going to be
> committed to the source base (by you or by some another committer),
> then I ask and require to not write/mention my name and email address
> neither in source file nor in commit log message.
> 
Ok, that's your choice.

I think the code is fine, since the likelyhood
of the first getpwuid_r() with the buffer on the stack failing is nearly
0, given that it is much bigger than 128. (Although I think a loop on
ERANGE should be in the code, just making the buffer a lot bigger than
128 will fix the problem for 99.99999% of the cases.)
The only change I was planning to the above was moving the free(buf)
down until after "pwd" has been used, so it is safe for fields like pw_name,
which I think will live in "buf". That way the code won't be broken if/when
someone uses it for pw_name.

rick

> Appropriate commit log message for the above written code can be the
> following:
> 
> --------------------------------------------------------------------------
> Since FreeBSD does not support sysconf(_SC_GETPW_R_SIZE_MAX), then
> allocate
> a buffer of sufficient size for getpwnam_r() as it is suggested in
> EXAMPLES
> of SUSv4 documentation for the getpwnam_r() function.
> --------------------------------------------------------------------------
> 
> since this documentation has similar code.
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to
> "freebsd-current-unsubscribe_at_freebsd.org"