Date: Sat, 13 Feb 2016 23:03:28 -0700 From: Warner Losh <imp@bsdimp.com> To: C Turt <cturt@hardenedbsd.org> Cc: "freebsd-arch@freebsd.org" <arch@freebsd.org> Subject: Re: OpenBSD mallocarray Message-ID: <CANCZdfq6ZWgUUmbEmgBeD9aHZ%2Bg=bV_aSbt41PWQU9xYgAyJeA@mail.gmail.com> In-Reply-To: <CAB815Zbx8an18ezSf8yyo=cYMSDvNBKzyJ0M_zj7EK1rXwakJg@mail.gmail.com> References: <CAB815ZafpqJoqr1oH8mDJM=0RxLptQJpoJLexw6P6zOi7oSXTQ@mail.gmail.com> <CAB815Zb9BZcMiHg%2BgkExyCaDbMqqJUNK_X-X2Z8sw54QZ1SRwA@mail.gmail.com> <F165D907-0DD5-48E3-B734-BA5BDD2EBDDA@bsdimp.com> <CAB815Zbx8an18ezSf8yyo=cYMSDvNBKzyJ0M_zj7EK1rXwakJg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, Feb 13, 2016 at 2:18 AM, C Turt <cturt@hardenedbsd.org> wrote:
> "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."
>
> Although most of the PS4 dynamic linker is taken from userland FreeBSD
> rtld-elf, the particular system call which contains the overflow,
> sys_dynlib_prepare_dlclose, is a Sony extension which was not imported an=
d
> so would have been written from scratch with the intention of being kerne=
l
> code. Hence, I continue to believe that if it was common practice to use
> mallocarray in the kernel, it would likely have been used here.
>
> "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?"
>
> You are absolutely right that macros like MALLOC_OVERFLOW should be the
> preferred way of catching integer overflows, and dealing with them
> appropriately according to the unique context where the overflow occurs. =
My
> intention isn't to remove checks and rely on mallocarray to deal with the=
m,
> it is to have both, such that only if the initial checks are faulty they
> will be caught in mallocarray, where the system should then intentionally
> chose to panic rather than continue, leading to a probable heap overflow.
>
We're already seeing places in the kernel where extra cycles being
uselessly consumed are interfering with performance needed to get to
100Gbps off disks and out the network port. While these code paths are
unlikely to have a malloc in them (since they've been optimized in the push
for 10Gbps and 40Gbps), extra, redundant checks, mindlessly applied
throughout the kernel likely aren't the right answer either.
> The problem I have is that certain parts of FreeBSD kernel flat out don't
> live up to FreeBSD's reputation of having clean code written with securit=
y
> in mind. Look at `huft_build` from `sys/kern/inflate.c`, this has to be t=
he
> ugliest function in the whole of FreeBSD kernel, and there is an allocati=
on
> with the described pattern:
>
> if ((q =3D (struct huft *) malloc((z + 1) * sizeof(struct huft), M_GZIP,
> M_WAITOK)) =3D=3D
> (struct huft *) NULL) {
>
> What's more, this code has a history of containing vulnerabilities (
> https://www.freebsd.org/security/advisories/FreeBSD-SA-06:21.gzip.asc and
> CVE-2009-2624).
>
> `z + 1` should be, and probably is, guaranteed by this code to be within
> appropriate bounds. However, my proposal is that pieces of code like this
> should be replaced with `mallocarray` so that there is absolutely no way
> that this can ever overflow from the multiplication at least.
>
I still maintain that a check before this, or as Ed Shouten has pointed
out, using an API that can do the multiplication and report overflow using
the CPU facilities to do so would be a much better API, we'd address the
performance issues I'm worried about.
> Although there are many other ways that an allocation like this could
> potentially be vulnerable: overflowing from the `+ 1`, or the count being
> raced attacked if it wasn't a stack variable, for example, I believe that
> using `mallocarray` would be an excellent start in pro-actively increasin=
g
> the security and code quality of the FreeBSD kernel.
>
I've still not seen an argument why this method is superior to the ones
outlined by Bruce Evans or Ed Shouten. You have to change the code anyway,
it would make more sense to make sure that your changes reflect what you
are doing, rather than add an obfuscating function to the mix with a poor
API for the kernel.
Warner
> On Thu, Feb 11, 2016 at 9:36 PM, Warner Losh <imp@bsdimp.com> wrote:
>
>>
>> > On Feb 11, 2016, at 2:21 PM, C Turt <cturt@hardenedbsd.org> wrote:
>> >
>> > 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
>> >
>> > The significance of this example is that if a `mallocarray` wrapper wa=
s
>> > available, and used here, the bug would not have been exploitable,
>> because
>> > it would have intentionally panicked instead of continuing with an und=
er
>> > 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 hav=
e 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=
.
>> >
>> > 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. The=
re
>> > are some checks on counts and sizes, but they are insufficient:
>> >
>> > [CODE]int
>> > alq_open(struct alq **alqp, const char *file, struct ucred *cred, int
>> cmode,
>> > int size, int count)
>> > {
>> > int ret;
>> >
>> > KASSERT((count >=3D 0), ("%s: count < 0", __func__));
>> >
>> > 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;
>> > }
>> >
>> > ...
>> >
>> > 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]
>> >
>> > The check on `count` being greater than or equal to 0 in `alq_open`, a=
nd
>> > 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`.
>> >
>> > This code path is reachable in several places, such as
>> > `sysctl_debug_ktr_alq_enable`:
>> >
>> > [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]
>> >
>> > With `ktr_alq_depth` being controllable from userland (but only as
>> root):
>> >
>> > [CODE]SYSCTL_INT(_debug_ktr, OID_AUTO, alq_depth, CTLFLAG_RW,
>> > &ktr_alq_depth, 0, "Number of items in the write buffer");[/CODE]
>> >
>> > `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 wh=
en
>> 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 demand=
s
>> 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:
>> >
>> >> I've recently started browsing the OpenBSD kernel source code, and ha=
ve
>> >> found the mallocarray function positively wonderful. I would like to
>> >> discuss the possibility of getting this into FreeBSD kernel.
>> >>
>> >> 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.
>> >>
>> >> The mallocarray is a wrapper for malloc which can be used in this
>> >> situations to detect an integer overflow before allocating:
>> >>
>> >> /*
>> >> * Copyright (c) 2008 Otto Moerbeek <otto@drijf.net>
>> >> *
>> >> * Permission to use, copy, modify, and distribute this software for a=
ny
>> >> * 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 OU=
T
>> OF
>> >> * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>> >> */
>> >>
>> >> /*
>> >> * 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))
>> >>
>> >> 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));
>> >> }
>> >>
>> >>
>> > _______________________________________________
>> > 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=
"
>>
>>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfq6ZWgUUmbEmgBeD9aHZ%2Bg=bV_aSbt41PWQU9xYgAyJeA>