From owner-svn-src-all@FreeBSD.ORG Wed Jan 13 04:20:33 2010 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9399A1065695; Wed, 13 Jan 2010 04:20:33 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail05.syd.optusnet.com.au (mail05.syd.optusnet.com.au [211.29.132.186]) by mx1.freebsd.org (Postfix) with ESMTP id 26BA68FC23; Wed, 13 Jan 2010 04:20:32 +0000 (UTC) Received: from c122-106-161-214.carlnfd1.nsw.optusnet.com.au (c122-106-161-214.carlnfd1.nsw.optusnet.com.au [122.106.161.214]) by mail05.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id o0D4KTiV020895 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 13 Jan 2010 15:20:30 +1100 Date: Wed, 13 Jan 2010 15:20:29 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Brooks Davis In-Reply-To: <201001120749.o0C7nZ7j019654@svn.freebsd.org> Message-ID: <20100113140806.O61578@delplex.bde.org> References: <201001120749.o0C7nZ7j019654@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed 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 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jan 2010 04:20:33 -0000 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 > #include > #include > #include 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