Date: Thu, 19 May 2016 06:05:36 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: yBryan Drewery <bdrewery@freebsd.org>, Gleb Smirnoff <glebius@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-releng@freebsd.org Subject: Re: svn commit: r300088 - in releng/9.3: . sys/conf sys/dev/kbd Message-ID: <20160519060448.B1270@besplex.bde.org> In-Reply-To: <20160518124147.V7042@besplex.bde.org> References: <201605172228.u4HMSbhj012124@repo.freebsd.org> <14a8d29d-bc14-3f96-57a4-81f1b6dfdd82@FreeBSD.org> <20160517230710.GB1015@FreeBSD.org> <38ca6091-5607-5796-9f6e-7f2d6c117707@FreeBSD.org> <20160518124147.V7042@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 18 May 2016, Bruce Evans wrote: > On Tue, 17 May 2016, Bryan Drewery wrote: > >> On 5/17/16 4:07 PM, Gleb Smirnoff wrote: >>> On Tue, May 17, 2016 at 03:59:26PM -0700, Bryan Drewery wrote: >>> B> > Author: glebius >>> B> > Date: Tue May 17 22:28:36 2016 >>> B> > New Revision: 300088 >>> B> > URL: https://svnweb.freebsd.org/changeset/base/300088 >>> B> > >>> B> > Log: >>> B> > - Use unsigned version of min() when handling arguments of SETFKEY >>> ioctl. >>> B> > - Validate that user supplied control message length in sendmsg(2) >>> B> > is not negative. >>> B> >>> B> The sendmsg(2) change is not included here (9.3) nor in the advisory >>> but >>> B> is in the commit log. Was it intended to be changed in 9.3? >>> >>> That was my failure to mention SA-16:19 in commit message for 9.3. It >>> doesn't >>> apply to 9.x. >>> >>> B> Plus the only consumer I see is sendit() which seems to be protected >>> B> already from negative values when not using COMPAT_43: >>> B> >>> B> > if (mp->msg_controllen < sizeof(struct cmsghdr) >>> B> > #ifdef COMPAT_OLDSOCK >>> B> > && mp->msg_flags != MSG_COMPAT >>> B> > #endif >>> B> > ) { >>> B> > error = EINVAL; >>> B> > goto bad; >>> B> > } >>> B> > error = sockargs(&control, mp->msg_control, >>> B> > mp->msg_controllen, MT_CONTROL); >>> >>> No, it isn't protected. In the comparison (mp->msg_controllen < >>> sizeof(struct cmsghdr)) >>> both values are unsigned. Later in sockargs() it is treated as signed. > > But it is protected (except on exotic unsupported arches). The above is > a complete bounds check for mp->msg_controllen, written in an obfuscated > way using an unsigned type botch/hack. Negative values are normally > promoted to large unsigned values, so they fail this check and sockargs() > is never called with them. > > On exotic arches, the analysis is more complicated and the hack doesn't > work. ... Oops. I read this sort of backwards. The unsign botches are somewhat larger, and more like the one in kbd.c: - this only checks the lower bound, so if the operands were negative then they would pass instead of fail the check after bogus unsign extension - the operands are actually both unsigned, since msg_controllen already has unsigned poisoning. It was poisoned even in FreeBSD-1. It was u_int then. It is still u_int in 4.4BSD-Lite*. Now it is socklen_t, which is uint32_t. POSIX has messes for this. At least in the 2001 version, it doesn't seem to require the poisoning, but recommends that applications not use values above 2**31-1 [since these values require the poisoning to represent, and are not very useful except for opening security holes like here]. - so after passing the lower bound check, msg_controllen may have a large unsigned 32-bit value. Undefined behaviour occurs when we pass this to sockargs() which doesn't have unsign poisoning. Except on unsupported exotic arches, the behaviour is to overflow to a negative value. - sockargs() then checks the overflowed value. This is robust enough if the overflow gives any value at all, but still bogus. Correct code would do bounds checks before calling sockargs (of the form val >= min && val <= INT_MAX), but there are several callers and it is convenient to check only in sockargs(). For that, sockargs must take a parameter with the same type as msg_controllen, although old unsign botches force this to be unsigned (precisely socklen_t). It is too late to change socklen_t back to int. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160519060448.B1270>