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 majordomoFreeBSD.org
with "unsubscribe freebsd-security" in the body of the message

• Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]