svn commit: r202143 - in head/sys: boot/forth compat/linux compat/svr4 i386/ibcs2 kern rpc security/audit sys

[Bruce Evans]

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