Date: Mon, 12 Aug 2002 19:38:51 +1000 (EST) From: Bruce Evans <bde@zeta.org.au> To: Niels Provos <provos@citi.umich.edu> Cc: Kris Kennaway <kris@obsecurity.org>, <security@FreeBSD.ORG> Subject: Re: [provos@citi.umich.edu: OpenBSD Security Advisory: Select Boundary Condition] Message-ID: <20020812183610.I23649-100000@gamplex.bde.org> In-Reply-To: <20020811223145.GQ22399@citi.citi.umich.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 11 Aug 2002, Niels Provos wrote:
> On Sun, Aug 11, 2002 at 02:47:23PM -0700, Kris Kennaway wrote:
> > In case anyone is wondering, it looks like FreeBSD fixed this security
> > hole 6 years ago, in the following commit:
> >
> > ---
> > Revision 1.19 / (download) - annotate - [select for diffs], Tue Aug 20 07:17:48 1996 UTC (5 years, 11 months ago) by smpatel
> > Branch: MAIN
> > Changes since 1.18: +43 -15 lines
> > Diff to previous 1.18 (colored)
> >
> > Remove the kernel FD_SETSIZE limit for select().
> > Make select()'s first argument 'int' not 'u_int'.
> >
> > Reviewed by: bde
> > ---
> Read that commit message carefully. That problem was introduced into
> FreeBSD six years ago. It was fixed last year.
>
> revision 1.74
> date: 2001/02/27 00:50:20; author: jlemon; state: Exp; lines: +3 -2
> Cast nfds to u_int before range checking it in order to catch negative
> values.
>
> PR: 25393
>
> NetBSD fixed it somewhat later.
It is necessary to read all of the patches and the originally code carefully
to understand the history of this bug :-). FreeBSD doesn't seem to have
ever had it for select(), but it had it for poll():
(1) There was no actual problem in rev.1.18, since uap->nd was (bogusly)
u_int and is compared with FD_SETSIZE. If userland passes a small
negative value, then it gets converted to a large unsigned one via
a type pun. The type pun even works the same as a cast would since
all supported machines are 2's complement. E.g., -1 gets converted
to UINT_MAX which is > FD_SETSIZE so select() fails. The possible
values for the bogus conversion of non- small negative values are
also clear on all supported machines, since they all have 32-bit
ints -- the values are (u_int)INT_MAX + 1 through UINT_MAX. Anyway,
the value of uip->nd is guaranteed to be between 0 and FD_SETSIZE.
(2) Rev.1.19 did not introduce a problem. The relevant part of it just
removed the type puns by changing uap->nd to int and adding an
explicit check that uap->nd >= 0. This also improves the error
handling -- we return EINVAL immediately if uap-nd < 0 instead of
treating it as a too-large value and using the "forgiving; slightly
wrong" code.
The hack of optimizing the check for negative values together with
checking for small positive values by casting to u_int should not
be used in code written in the last 15 years or so, since compilers
have been doing it automatically if possible, and its correctness
is time-consuming to check especially if the types are typedefed
types instead of just plain int/u_int.
(3) poll() was obtained from NetBSD in rev.1.29. This version was
correct since the corresponding variable uap->nfds actually has
type u_int (at least in -current where it has type nfds_t = u_int)
it is compared early with p->p_fd->fd_nfiles. Omitting the explicit
comparison with 0 is non-bogus here since u_int's are inherently
nonnegative. However the proving the correctness of the comparision
of a u_int with the signed (int) p->p_fd_fd_nfiles involves the same
considerations.
(4) poll() was broken in rev.1.71 by clobbering uap->nfds by converting
it to a a variable of the wrong type (int nfds) without checking
that it fits first.
(5) poll() was "fixed" in rev.1.74 (20 days after it was broken) by
casting nfds back to u_int. The casts back and forth don't change
the value on 2's complement machines but are obfuscations.
(6) I complained about the bogus types and casts in rev.1.74 and they
were fixed a few hours later in rev.1.75.
> I did not contact anyone at FreeBSD or NetBSD because it was not a
> problem there in case you were wondering.
No problem.
014_scarg.patch seems to be correct and non-bogus except it uses the
cast-to-u_int hack for select().
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-security" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20020812183610.I23649-100000>