Date: Sat, 13 Feb 2016 09:18:26 +0000 From: C Turt <cturt@hardenedbsd.org> To: Warner Losh <imp@bsdimp.com> Cc: arch@freebsd.org Subject: Re: OpenBSD mallocarray Message-ID: <CAB815Zbx8an18ezSf8yyo=cYMSDvNBKzyJ0M_zj7EK1rXwakJg@mail.gmail.com> In-Reply-To: <F165D907-0DD5-48E3-B734-BA5BDD2EBDDA@bsdimp.com> References: <CAB815ZafpqJoqr1oH8mDJM=0RxLptQJpoJLexw6P6zOi7oSXTQ@mail.gmail.com> <CAB815Zb9BZcMiHg%2BgkExyCaDbMqqJUNK_X-X2Z8sw54QZ1SRwA@mail.gmail.com> <F165D907-0DD5-48E3-B734-BA5BDD2EBDDA@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
"Except that you=E2=80=99d need to change all the code that was imported in=
to
the kernel to use mallocarray. The code Sony imported didn=E2=80=99t have i=
t
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 and
so would have been written from scratch with the intention of being kernel
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 them,
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.
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 security
in mind. Look at `huft_build` from `sys/kern/inflate.c`, this has to be the
ugliest function in the whole of FreeBSD kernel, and there is an allocation
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.
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 increasing
the security and code quality of the FreeBSD kernel.
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 bas=
ed
> > 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 was
> > available, and used here, the bug would not have been exploitable,
> because
> > it would have intentionally panicked instead of continuing with an unde=
r
> > allocated buffer.
>
> Except that you=E2=80=99d need to change all the code that was imported i=
nto
> 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.
> >
> > 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. Ther=
e
> > 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);[/C=
ODE]
> >
> > The check on `count` being greater than or equal to 0 in `alq_open`, an=
d
> > 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 whe=
n
> 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:
> >
> >> I've recently started browsing the OpenBSD kernel source code, and hav=
e
> >> 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 late=
r
> 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 an=
y
> >> * purpose with or without fee is hereby granted, provided that the abo=
ve
> >> * 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 DAMAG=
ES
> >> * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN A=
N
> >> * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT
> 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?CAB815Zbx8an18ezSf8yyo=cYMSDvNBKzyJ0M_zj7EK1rXwakJg>