From owner-freebsd-security Mon Aug 12 2:34: 8 2002 Delivered-To: freebsd-security@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 18DD737B400 for ; Mon, 12 Aug 2002 02:34:05 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id E3D7E43E6A for ; Mon, 12 Aug 2002 02:34:03 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id TAA28541; Mon, 12 Aug 2002 19:33:50 +1000 Date: Mon, 12 Aug 2002 19:38:51 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Niels Provos Cc: Kris Kennaway , Subject: Re: [provos@citi.umich.edu: OpenBSD Security Advisory: Select Boundary Condition] In-Reply-To: <20020811223145.GQ22399@citi.citi.umich.edu> Message-ID: <20020812183610.I23649-100000@gamplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-security@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org 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