Date: Tue, 19 Feb 2013 20:52:49 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Andrey Simonenko <simon@comsys.ntu-kpi.kiev.ua> Cc: freebsd-current@freebsd.org, Elias Martenson <lokedhs@gmail.com>, Benjamin Kaduk <kaduk@mit.edu> Subject: Re: Possible bug in NFSv4 with krb5p security? Message-ID: <747768617.3137250.1361325169865.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130219102744.GA2426@pm513-1.comsys.ntu-kpi.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Andrey Simonenko wrote:
> On Tue, Feb 19, 2013 at 05:35:50PM +0800, Elias Martenson wrote:
> > On 19 February 2013 17:31, Andrey Simonenko
> > <simon@comsys.ntu-kpi.kiev.ua>wrote:
> >
> > It can require bigger buffer, since root can get the pw_password
> > field
> > > in the struct passwd{}.
> > >
> > > Since sysconf(_SC_GETPW_R_SIZE_MAX) does not work on FreeBSD, the
> > > buffer
> > > for getpwnam_r() call should have at least (2 * MAXLOGNAME + 2 *
> > > MAXPATHLEN +
> > > _PASSWORD_LEN + 1) bytes (it is unclear how much is required for
> > > pw_gecos).
> > >
> > > This buffer can be dynamically reallocated until getpwnam_r() is
> > > not
> > > return ERANGE error (the following code has not been compiled and
> > > verified):
> > >
> >
> > Is this really a better solution than to aim high right away? A
> > series of
> > malloc() calls should certainly have much higher overhead than the
> > previous
> > stack-allocated solution.
> >
> > A better compromise would be to do the lookup in a separate
> > function, that
> > allocates the buffer using alloca() instead, yes?
>
> I cannot find how to get information about maximum buffer size for
> the getpwnam_r() function. This information should be returned by
> sysconf(_SC_GETPW_R_SIZE_MAX), but since it does not work on FreeBSD
> it is necessary to guess its size. Original value is 128 and it works
> for somebody, 1024 works for your environment, but it can fail for
> another environment.
>
> SUSv4 specifies "Storage referenced by the structure is allocated from
> the memory provided with the buffer parameter", but then tells about
> groups
> in EXAMPLE for getpwnam_r() "Note that sysconf(_SC_GETPW_R_SIZE_MAX)
> may
> return -1 if there is no hard limit on the size of the buffer needed
> to
> store all the groups returned".
>
> malloc() can give overhead, but that function can try to call
> getpwnam_r()
> with buffer allocated from stack and if getpwnam_r() failed with
> ERANGE
> use dynamically allocated buffer.
>
> #define PWBUF_SIZE_INI (2 * MAXLOGNAME + 2 * MAXPATHLEN +
> _PASSWORD_LEN + 1)
> #define PWBUF_SIZE_INC 128
>
> char bufs[2 * MAXLOGNAME + MAXPATHLEN + PASSWORD_LEN + 1 + 32];
>
> error = getpwnam_r(lname, &pwd, bufs, sizeof(bufs), &pw);
> if (pw != NULL) {
> *uidp = pw->pw_uid;
> return (GSS_S_COMPLETE);
> } else if (error != ERANGE)
> return (GSS_S_FAILURE);
>
> size = PWBUF_SIZE_INI;
> for (;;) {
> size += PWBUF_SIZE_INC;
> buf = malloc(size);
> if (buf == NULL)
> return (GSS_S_FAILURE);
> error = getpwnam_r(lname, &pwd, buf, size, &pw);
> free(buf);
> if (pw != NULL) {
> *uidp = pw->pw_uid;
> return (GSS_S_COMPLETE);
> } else {
> if (error == ERANGE &&
> size <= SIZE_MAX - PWBUF_SIZE_INC)
> continue;
> return (GSS_S_FAILURE);
> }
> }
Just my opinion, but I think the above is a good approach.
(ie. First trying a fairly large buffer on the stack that
will succeed 99.99% of the time, but check for ERANGE and
loop trying progressively larger malloc'd buffers until
it stops reporting ERANGE.)
I suspect the overheads behind getpwnam_r() are larger than
the difference between using a buffer on the stack vs malloc,
so I think it should use a fairly large buffer the first time.
Personally, I might have coded it as a single do { } while(),
with the first attempt in it, but that's just personal stylistic
taste. (You can check for buf != bufs before doing a free() of it.)
And, if you wanted to be clever, the code could use a static "bufsiz_hint",
which is set to the largest size needed sofar and that is used as
the initial malloc size. That way it wouldn't loop as much for a
site with huge passwd entries. (An entire bio in the GECOS field or ???)
Btw, the same fix is needed in gssd.c, where it calls
getpwuid_r(). { Interesting that for Elias's case, it
must work for 128, although the getpwnam_r() didn't quite fit
in 128. }
Also, FYI, kuserok.c uses a 2048 byte buffer and doesn't check
for ERANGE.
rick
> _______________________________________________
> freebsd-current@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to
> "freebsd-current-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?747768617.3137250.1361325169865.JavaMail.root>