Date: Thu, 11 Feb 2016 14:36:51 -0700 From: Warner Losh <imp@bsdimp.com> To: C Turt <cturt@hardenedbsd.org> Cc: arch@freebsd.org Subject: Re: OpenBSD mallocarray Message-ID: <F165D907-0DD5-48E3-B734-BA5BDD2EBDDA@bsdimp.com> In-Reply-To: <CAB815Zb9BZcMiHg%2BgkExyCaDbMqqJUNK_X-X2Z8sw54QZ1SRwA@mail.gmail.com> References: <CAB815ZafpqJoqr1oH8mDJM=0RxLptQJpoJLexw6P6zOi7oSXTQ@mail.gmail.com> <CAB815Zb9BZcMiHg%2BgkExyCaDbMqqJUNK_X-X2Z8sw54QZ1SRwA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--Apple-Mail=_4D542F93-2169-43C9-A5D6-E0B42B162824 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On Feb 11, 2016, at 2:21 PM, C Turt <cturt@hardenedbsd.org> wrote: >=20 > I just wanted to post some real world examples of bugs which could be > mitigated with `mallocarray` to attract more interest. Let=E2=80=99s play devil=E2=80=99s advocate: since you have to make code = changes, how would mallocarray() be superior to the various MALLOC_OVERFLOW macro suggestions from earlier in the thread? Given that kernel code is somewhat different in what it needs to support? > My most meritable example of a real world attack from this behaviour = would > be the sys_dynlib_prepare_dlclose kernel exploit for PS4 (PS4 OS is = based > on FreeBSD 9.0). You may read my write-up of this exploit here: > http://cturt.github.io/dlclose-overflow.html >=20 > The significance of this example is that if a `mallocarray` wrapper = was > available, and used here, the bug would not have been exploitable, = because > it would have intentionally panicked instead of continuing with an = under > allocated buffer. Except that you=E2=80=99d need to change all the code that was imported = into the kernel to use mallocarray. The code Sony imported didn=E2=80=99t = have it to start with. > You may think that this is clearly Sony's fault for not checking the = count > at all, and that FreeBSD kernel code would never have a bug like this, > which is why I would like to mention that similar overflows can be = possible > even if there initially appear to be sufficient bound checks = performed. >=20 > There are several pieces of vulnerable code present in FreeBSD kernel > (albeit most of them are triggerable only as root so are not = critical), > however I will use the file `/sys/kern/kern_alq.c` to demonstrate. = There > are some checks on counts and sizes, but they are insufficient: >=20 > [CODE]int > alq_open(struct alq **alqp, const char *file, struct ucred *cred, int = cmode, > int size, int count) > { > int ret; >=20 > KASSERT((count >=3D 0), ("%s: count < 0", __func__)); >=20 > if (count > 0) { > if ((ret =3D alq_open_flags(alqp, file, cred, cmode, > size*count, 0)) =3D=3D 0) { > (*alqp)->aq_flags |=3D AQ_LEGACY; > (*alqp)->aq_entmax =3D count; > (*alqp)->aq_entlen =3D size; > } >=20 > ... >=20 > int > alq_open_flags(struct alq **alqp, const char *file, struct ucred = *cred, int > cmode, > int size, int flags) > { > ... > KASSERT((size > 0), ("%s: size <=3D 0", __func__)); > ... > alq->aq_buflen =3D size; > ... > alq->aq_entbuf =3D malloc(alq->aq_buflen, M_ALD, = M_WAITOK|M_ZERO);[/CODE] >=20 > The check on `count` being greater than or equal to 0 in `alq_open`, = and > the check for `size` being greater than 0 in `alq_open_flags` are = cute, but > they don't check for an overflow of `size*count`. >=20 > This code path is reachable in several places, such as > `sysctl_debug_ktr_alq_enable`: >=20 > [CODE]static int > sysctl_debug_ktr_alq_enable(SYSCTL_HANDLER_ARGS) > { > ... > error =3D alq_open(&ktr_alq, (const char *)ktr_alq_file, > req->td->td_ucred, ALQ_DEFAULT_CMODE, > sizeof(struct ktr_entry), ktr_alq_depth); > [/CODE] >=20 > With `ktr_alq_depth` being controllable from userland (but only as = root): >=20 > [CODE]SYSCTL_INT(_debug_ktr, OID_AUTO, alq_depth, CTLFLAG_RW, > &ktr_alq_depth, 0, "Number of items in the write buffer");[/CODE] >=20 > `sizeof(struct ktr_entry)` is 88 bytes. So theoretically if = `ktr_alq_depth` > is set to `48806447`, we'll get an allocation of `0x100000028`, which > truncates to 0x28 =3D 40 bytes. A heap overflow would then possible = when this > buffer is iterated over with `aq_entmax` and `aq_entlen`. These are all good finds. And they are all mitigable with the = MALLOC_OVERFLOW macro that was suggested earlier in this thread. Given the unique = demands of the kernel, why should that not be the preferred method of dealing with this = stuff? Warner > On Mon, Feb 1, 2016 at 7:57 PM, C Turt <cturt@hardenedbsd.org> wrote: >=20 >> I've recently started browsing the OpenBSD kernel source code, and = have >> found the mallocarray function positively wonderful. I would like to >> discuss the possibility of getting this into FreeBSD kernel. >>=20 >> For example, many parts of kernel code in FreeBSD use something like >> malloc(xxx * sizeof(struct xxx)). If xxx is 64bit and controllable by = user, >> this allocation can easily overflow, resulting in a heap overflow = later on. >>=20 >> The mallocarray is a wrapper for malloc which can be used in this >> situations to detect an integer overflow before allocating: >>=20 >> /* >> * Copyright (c) 2008 Otto Moerbeek <otto@drijf.net> >> * >> * Permission to use, copy, modify, and distribute this software for = any >> * purpose with or without fee is hereby granted, provided that the = above >> * copyright notice and this permission notice appear in all copies. >> * >> * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL = WARRANTIES >> * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF >> * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE = FOR >> * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY = DAMAGES >> * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN = AN >> * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING = OUT OF >> * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. >> */ >>=20 >> /* >> * This is sqrt(SIZE_MAX+1), as s1*s2 <=3D SIZE_MAX >> * if both s1 < MUL_NO_OVERFLOW and s2 < MUL_NO_OVERFLOW >> */ >> #define MUL_NO_OVERFLOW (1UL << (sizeof(size_t) * 4)) >>=20 >> void * >> mallocarray(size_t nmemb, size_t size, int type, int flags) >> { >> if ((nmemb >=3D MUL_NO_OVERFLOW || size >=3D MUL_NO_OVERFLOW) && >> nmemb > 0 && SIZE_MAX / nmemb < size) { >> if (flags & M_CANFAIL) >> return (NULL); >> panic("mallocarray: overflow %zu * %zu", nmemb, size); >> } >> return (malloc(size * nmemb, type, flags)); >> } >>=20 >>=20 > _______________________________________________ > freebsd-arch@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-arch > To unsubscribe, send any mail to = "freebsd-arch-unsubscribe@freebsd.org" --Apple-Mail=_4D542F93-2169-43C9-A5D6-E0B42B162824 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP using GPGMail -----BEGIN PGP SIGNATURE----- Comment: GPGTools - https://gpgtools.org iQIcBAEBCgAGBQJWvP70AAoJEGwc0Sh9sBEAvf0QAJLfmMmRyPhvo/oW+wfPRM8o PIZnzALIokBt3ol3m7JiYklSm36lqosqET+eZhJLvrtNAnRC2/uaJjJF1Fo56Bgo bY5eJO5NA3Of1pZspaQ9UmVtmn1i+u8eIdmBmq6FzZAXYGvY4qxEWtTPloEd560H ug9ONA/41zH+gq/oxrIl4G2X5JQtkjLM3iyrXu4zntORvHvU2iLvr25UMSCCVgdo Yd/wj+wJdTRLiemlewYAjBcZy4Y0fBh7IIYw+m1FS2/91T3gPWeOcH4vWcDhwKqM h5kOUk48nocEQFM2tcYVUbsg2K8srAmgyL0GlW05NHuyJ0PvTe4hgLu2HAl/Rz3I +P/VQVZ2UnZKpF87bhyKw46Pu/bGqVuq+V6VTutIv4HlvR4tYvK2aYErBj8qnMW0 YdPZU/2eAGH1GMGHArjX070f6HKilyr0AA9SvcO/ApzMRxRsxquPR+2fCrllhTML c3x0MpYN1JpKh12EetUqKsyxYWO3Q/6+1xTv8b2pja/PbQZlrzJbfkyuTDmkPWbm 8SvWc/ZBk4+wa/KkH3SZIomp47ifshzVOmR1PBKZUGFo6DIdcvHmwXzVHSfjaJK5 btZ2+224uVFeGsVHftenm9v5uo0tgTMO0+0xDnMkFAKrgjVKpONQvrUJWmSVsPUx nKOwv6AzIX6l4MOXz1E8 =lf02 -----END PGP SIGNATURE----- --Apple-Mail=_4D542F93-2169-43C9-A5D6-E0B42B162824--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?F165D907-0DD5-48E3-B734-BA5BDD2EBDDA>