Date: Tue, 20 May 2014 18:14:43 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Peter Holm <pho@freebsd.org> Cc: freebsd-bugs@freebsd.org, freebsd-gnats-submit@freebsd.org Subject: Re: kern/189941: getgroups(2) implements first argument as unsigned int Message-ID: <20140520181439.U1106@besplex.bde.org> In-Reply-To: <201405190921.s4J9LtP3089031@cgiserv.freebsd.org> References: <201405190921.s4J9LtP3089031@cgiserv.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 19 May 2014, Peter Holm wrote:
>> Description:
> Passing -1 as gidsetlen is not detected. Discovered by ATF. Caught on Ubuntu and OS/X.
A typical error from abusing an unsigned variable as a counter. This doesn't
even match the API. This bug was in 4.4BSD.
The fix seems to be almost as simple as changing the u_int for getgroups()
in syscalls.master to int. Also change 3 corresponding u_int's in
kern_prot.c
setgroups() has the same API design error, but there it reduces to an
obfuscated way to check for invalid counts (for getgroups(), negative
counts are automatically detected as invalid, since they are too small
to hold any actual number of groups). For setgroups(), negative counts
are are type-punned to become large unsigned counts, so they are
obfuscatedly detected as invalid because they are larger than any actual
number for {NGROUPS_MAX}. The check for that should for negative counts.
Grepping for u_ in syscalls.master shows abuse of u_int for almost all
uses:
- dup, dup2: not even wrong, modulo type puns being benign. These syscalls
take int args, but syscalls. master says that they take u_int args.
This converts the ints to u_ints using a type pun. But the kernel
converts these u_ints back to ints at the top level. Obfuscated
tests for negative values for file descriptors using the unsigned
hack have (all?) been fixed in at least kern_descrip.c too.
- profil: correct
- getlogin: POSIX specifies size_t for getlogin_r(3), but FreeBSD still
uses int. The getlogin syscall is not directly available, but is
closer to getlogin_r(3) than getlogin(3). It uses u_int where the
FreeBSD API says int. This sort of works. It is an obfuscated way
of giving the POSIX semantics instead of the documented semantics.
An arg of -1 doesn't mean -1, but means SIZE_MAX in POSIX and UINT_MAX
in FreeBSD. This is a physically impossible size (except on 64-bit
systems while the punned FreeBSD semantics), but the POSIX semantics
don't allow detecting it as an error, and the FreeBSD behaviour is to
reduce it to MAXLOGNAME. The behaviour is undefined in most cases if
the arg is -1, but in practice the syscall will do the right thing if
the buffer has size >= MAXLOGNAME.
- getgroups, setgroups: see above
- getitimer, setitimer: like setgroups (just the unsigned hack, but the
range checking is slightly more obfuscated: the valid values are 0,
1 and 2, so checking for the unsigned value being <= {the one that
happens to be 2} gives an obfuscated quick check for the value being
one of these three.
- gethostname: POSIX specifies size_t. FreeBSD documents size_t, but
actually uses u_int. On 64-bit arches, the buffer may have the silly
but valid size of 2**32. This is blindly truncated to 0 so the syscall
fails. The arg is converted back to size_t at the top level in the
kernel so as to pass the address of a size_t to another function,
but it has already been truncated then. Similar truncation breaks the
accidental change to POSIX semantics in some cases in getlogin_r.
- sethostname: not in POSIX. FreeBSD still documents int, but still
actually uses u_int. The arg is converted back to size_t too late to
preserve it, as for gethostname.
- readv, writev: POSIX and FreeBSD document that the iovcnt arg is int.
FreeBSD type puns it to u_int, then converts this to size_t before
using it as an arg for copyin*(). No errror checking is done except
in copyin*() where the error checking is adequate. -1 becomes UINT_MAX
vua the type pun, but is not increased further to SIZE_MAX on 64-bit
arches. U_INT is usually too large, so the result is probably EFAULT.
- getrlimit, setrlimit. Like getitimer and setitimer, except the there
is a macro to de-obfuscate the upper limit. In the same file, the
'which' variable for get/setpriority is handled quite differently.
It is left as an int and checked using a case statement.
- getdirentries: just the unsigned hack
- __sysctl: at least matches the documented API. The API uses u_int for
small counts and size_t for large counts. Old APIs don't have the
design error of using u_int where int would do. New APIs mostly
use size_t excessively. My grep doesn't find these. It finds this
one since it has a strange mixture of u_int and size_t.
- poll: correct, I think. POSIX specifies nfds_t, and IIRC has the design
errors of requiring this to be unsigned and having excessive typedefs
for the function. FreeBSD spells it u_int to avoid some namespace
pollution. It should spell it nfds_t except in syscalls.master and
files generated from it.
- preadv, pwritev: like readv, writev (?)
- __getcwd: POSIX specifies size_t and FreeBSD documents size_t for getcwd().
Using u_int probably gives the usual truncation bugs.
- *audit*: not checked. The only set of new APIs that uses u_int. About
25 years anachronistic with C90's and POSIX's converting even old APIs
to use size_t.
- _umtx_op: not checked
- *cap*: not checked. Mostly not count args or plain u_int. It mispells
uint64_t as u_int64_t. The correct spelling uint* is used for just 1
syscall, and my grep for u_ didn't find it. This use seems to be to
avoid namespace pollution, so it is correct. This points to further
uses of unsigned types which were hidden by althernative spellings.
The style bug of using 'unsigned' is used a bit:
- nmount: uses 'unsigned int'. Otherwise like writev (?), except the
documented API also has this bug.
- *ksem*, *kmq*: uses 'unsigned int'. Not checked. Seems to be
undocumented.
- jail_get, jail_set: like nmount.
Stress and regression tests could try to find bugs in all of the above,
but I code inspection only found the harmless truncation bugs for u_int
instead of size_t in addition to the one in the PR. The one in the
PR is a rare case where the unsigned comparison hack doesn't give
fail-safe behaviour.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140520181439.U1106>