Date: Wed, 13 Jan 2010 15:20:29 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Brooks Davis <brooks@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r202143 - in head/sys: boot/forth compat/linux compat/svr4 i386/ibcs2 kern rpc security/audit sys Message-ID: <20100113140806.O61578@delplex.bde.org> In-Reply-To: <201001120749.o0C7nZ7j019654@svn.freebsd.org> References: <201001120749.o0C7nZ7j019654@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 12 Jan 2010, Brooks Davis wrote: > Log: > Replace the static NGROUPS=NGROUPS_MAX+1=1024 with a dynamic > kern.ngroups+1. kern.ngroups can range from NGROUPS_MAX=1023 to > INT_MAX-1. Given that the Windows group limit is 1024, this range > should be sufficient for most applications. INT_MAX-1 is a bogus limit, since many overflows still occur long before that limit is reached. > Modified: head/sys/compat/linux/linux_misc.c > ============================================================================== > --- head/sys/compat/linux/linux_misc.c Tue Jan 12 07:40:58 2010 (r202142) > +++ head/sys/compat/linux/linux_misc.c Tue Jan 12 07:49:34 2010 (r202143) > @@ -1138,7 +1138,7 @@ linux_setgroups(struct thread *td, struc > struct proc *p; > > ngrp = args->gidsetsize; > - if (ngrp < 0 || ngrp >= NGROUPS) > + if (ngrp < 0 || ngrp > ngroups_max) > return (EINVAL); This seems to have an off-by-1 error, since elsewhere ngrps can be ngroups_max + 1. > linux_gidset = malloc(ngrp * sizeof(*linux_gidset), M_TEMP, M_WAITOK); > error = copyin(args->grouplist, linux_gidset, ngrp * sizeof(l_gid_t)); > > Modified: head/sys/compat/linux/linux_uid16.c > ============================================================================== > --- head/sys/compat/linux/linux_uid16.c Tue Jan 12 07:40:58 2010 (r202142) > +++ head/sys/compat/linux/linux_uid16.c Tue Jan 12 07:49:34 2010 (r202143) > @@ -109,7 +109,7 @@ linux_setgroups16(struct thread *td, str > #endif > > ngrp = args->gidsetsize; > - if (ngrp < 0 || ngrp >= NGROUPS) > + if (ngrp < 0 || ngrp > ngroups_max) > return (EINVAL); > linux_gidset = malloc(ngrp * sizeof(*linux_gidset), M_TEMP, M_WAITOK); > error = copyin(args->gidset, linux_gidset, ngrp * sizeof(l_gid16_t)); Here the multiplication overflows on 32-bit machines when ngroups_max is only about INT_MAX/2. The overflow when ngroups_max equals its bogus limit (INT_MAX - 1) can occur even if no more than 16 groups are ever allocated, since the user can pass in a proposterously large ngrp in places like here. The behaviour is then undefined. In most cases the overflow gives a value so large that the malloc() should fail, but it's an M_WAITOK malloc so it must hang or panic. The overflow can also be to a small value (e.g., 0x40000001 * (size_t)4 = 4. Then the malloc() should succeed, and it is possible for the user buffer to have 1 element although not 0x40000001, so the copyin() and the syscall will succeed too, but the syscall should fail. Even a not so large value of ngroups_max like INT_MAX/2 gives an effectively-unbounded malloc() service. Similarly for other setgroups(), except overflow occurs at INT_MAX/4 with 32-bit gid_t. > Modified: head/sys/kern/kern_prot.c > ============================================================================== > --- head/sys/kern/kern_prot.c Tue Jan 12 07:40:58 2010 (r202142) > +++ head/sys/kern/kern_prot.c Tue Jan 12 07:49:34 2010 (r202143) > @@ -283,7 +283,7 @@ getgroups(struct thread *td, register st > u_int ngrp; > int error; > > - ngrp = MIN(uap->gidsetsize, NGROUPS); > + ngrp = MIN(uap->gidsetsize, ngroups_max + 1); > groups = malloc(ngrp * sizeof(*groups), M_TEMP, M_WAITOK); > error = kern_getgroups(td, &ngrp, groups); > if (error) Use of the MIN() and MAX() macros in the kernel are style bugs. They were removed in 4.4BSD and brought back by FreeBSD. It is a shame to see them used even in kern. Large ngroups_max gives an unbounded malloc() service via getgroups() too. Here the user can pass in an enormous uap->gidsetsize and have it all malloced, subject only to the ngroups_max limit, despite kern_ngroups() only using an array of size precisely cred->cr_ngroups elements. The malloc() is also wasteful (unused) when uap->gidsetsize < cred->cr_ngroups. kern_getgroups() API gets in the way of cleaning this up. > Modified: head/sys/kern/subr_param.c > ============================================================================== > --- head/sys/kern/subr_param.c Tue Jan 12 07:40:58 2010 (r202142) > +++ head/sys/kern/subr_param.c Tue Jan 12 07:49:34 2010 (r202143) > @@ -40,6 +40,7 @@ __FBSDID("$FreeBSD$"); > #include "opt_param.h" > #include "opt_maxusers.h" > > +#include <sys/limits.h> > #include <sys/param.h> > #include <sys/systm.h> > #include <sys/kernel.h> Style bug (unsorting). > @@ -228,6 +230,18 @@ init_param1(void) > TUNABLE_ULONG_FETCH("kern.maxssiz", &maxssiz); > sgrowsiz = SGROWSIZ; > TUNABLE_ULONG_FETCH("kern.sgrowsiz", &sgrowsiz); > + > + /* > + * Let the administrator set {NGROUPS_MAX}, but disallow values > + * less than NGROUPS_MAX which would violate POSIX.1-2008 or > + * greater than INT_MAX-1 which would result in overflow. > + */ > + ngroups_max = NGROUPS_MAX; > + TUNABLE_INT_FETCH("kern.ngroups", &ngroups_max); > + if (ngroups_max < NGROUPS_MAX) > + ngroups_max = NGROUPS_MAX; > + if (ngroups_max > INT_MAX - 1) > + ngroups_max = INT_MAX - 1; > } Limiting parameters is a waste of time and space, especially when it doesn't work. Anyone wanting an unusuable system can easily get it by misconfiguring one of the many unlimited parameters, or by selecting a combination of parameters that requires too man resources to work. It is very difficult to limit all combinations of parameters without excessively limiting individual parameters. Here we could impose some arbitrary reasonable limit like 64K, but that wouldn't do anything except annoy users wanting to test or even use > 64K. A much larger arbitrary limit like 1M wouldn't provide much protection against the unbounded malloc() service, at least when it is unprivileged. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100113140806.O61578>