From owner-freebsd-arch@freebsd.org Sat Feb 13 09:18:29 2016 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 53132AA6496 for ; Sat, 13 Feb 2016 09:18:29 +0000 (UTC) (envelope-from cturt@hardenedbsd.org) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 30DD21AC1 for ; Sat, 13 Feb 2016 09:18:29 +0000 (UTC) (envelope-from cturt@hardenedbsd.org) Received: by mailman.ysv.freebsd.org (Postfix) id 3149EAA6495; Sat, 13 Feb 2016 09:18:29 +0000 (UTC) Delivered-To: arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 16EA7AA6494 for ; Sat, 13 Feb 2016 09:18:29 +0000 (UTC) (envelope-from cturt@hardenedbsd.org) Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 927781ABF for ; Sat, 13 Feb 2016 09:18:28 +0000 (UTC) (envelope-from cturt@hardenedbsd.org) Received: by mail-wm0-x22b.google.com with SMTP id g62so48314457wme.0 for ; Sat, 13 Feb 2016 01:18:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=hardenedbsd-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=dGoeYjFzZ9fLbvh1RB3IVlQ5mtLZgVDqHbRO3z+eg58=; b=NMYlRSTjqBeGuJGR7n20QzDmXnsBlM48dm/5bwPHYKv8hB8x7jqe+sc6k+ngf4imNo Z7Hh/VnRpj89ClCXoaWihJNZVV2JuUXnG2+RpKxN+VmQhWZq39RLbghwtv0yQvLuP2ay h39AMOsXRWsr/7R8AEkq+Xnt8fByJo44KdB8F7UMOkvOihLnHP7EzjEcbn537+2FmpPd syQZWR1+tHb7SR5L//lGidIZ1fyqXFPkGeg2ZFNuqj51JuiBZ0CCXDUo8EwMoEUupWz6 KdLVcx/IICPkhVAQMzLoi3FX8810ayzXuv5l3adgfMoavrEO+DAQFbRvHy5qddvFuYfb ilSA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=dGoeYjFzZ9fLbvh1RB3IVlQ5mtLZgVDqHbRO3z+eg58=; b=caOXMT7HnhbKV5y+6OIAFuYPsvk/9qeGzZ374rb4YtACc/dVTc3eHY5PRA8GyVPUfd RK0E4+BMN0fBfjyYAg1LLB7KDHzYsEABcWQsCn6vYYIgW0L3XSPauPSGkoBC2zyhFCCQ 0jLOn+oI3z0Vyu0P+ok4A1FIkN2d8MtZFq47g8J1Drln0mtaR8X69zuSfwoIWIoA7WPb Hu0EjtPM/1bEE6v8JQr99HbGgJ2Ue4KnySsVTrRB7wWhjozpqxK0ZIa8cYXWMtzoUjz5 3lTqikxAMkS+EINHePV0ZRi+OrD8JNcQt81hnwRRRIUwaxlKG1/siDEvFDHWLmZvqia7 gMFw== X-Gm-Message-State: AG10YOQSof9nDiz5LZmAq6/jQRx3/CwsgJ+bwm1rbkN66mVm1pQ4xG2DWOmKiGpbB6xgsFL9TXQi4tHQw/v2nA0G MIME-Version: 1.0 X-Received: by 10.194.176.170 with SMTP id cj10mr6013751wjc.165.1455355106154; Sat, 13 Feb 2016 01:18:26 -0800 (PST) Received: by 10.27.157.18 with HTTP; Sat, 13 Feb 2016 01:18:26 -0800 (PST) In-Reply-To: References: Date: Sat, 13 Feb 2016 09:18:26 +0000 Message-ID: Subject: Re: OpenBSD mallocarray From: C Turt To: Warner Losh Cc: arch@freebsd.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Feb 2016 09:18:29 -0000 "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 wrote: > > > On Feb 11, 2016, at 2:21 PM, C Turt 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 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 > >> * > >> * 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" > >