From patchwork Sat May 6 02:12:33 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 9714519 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 3B1EF60387 for ; Sat, 6 May 2017 02:12:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 2054628602 for ; Sat, 6 May 2017 02:12:52 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 02D1028650; Sat, 6 May 2017 02:12:52 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.wl.linuxfoundation.org (Postfix) with SMTP id CD29328602 for ; Sat, 6 May 2017 02:12:50 +0000 (UTC) Received: (qmail 11729 invoked by uid 550); 6 May 2017 02:12:48 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Delivered-To: mailing list kernel-hardening@lists.openwall.com Received: (qmail 11694 invoked from network); 6 May 2017 02:12:46 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=G9bsfeGlndrEDxzzW2tyOjKowkCk2yrA2u9Ohs+cRVg=; b=KPujkxJ5hlHoqv0V/5fRe+DDORTZ7kKsw9zaSUTb4WQM3//i8U9pp4f93/RBw1Res1 XA+aU1nmbjer5VLRO8hV3ggnlHjnAH2sTC6LKP6Y8TrvNKJPuxTI5u3otsjKudfY8zSM epdrHI38raDuDMhN2bcBC1Z5gQTgFLb6IKxEQC6DWHVimlj/9GVhuRY8T0fqAScc5Noq FvM7p3vCEzJ+xG0AdoGZSo/5LdnfOHbIrQ/TAHbqTV+Xz/LNYXNuqislcfAD4KcnwKGs AIWg/NzOzmoLoedvXty+g+MOa2zMtQeYTtWP076kJU92Ne+W33KCUokDLDhOQAuomTyX vw+w== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=G9bsfeGlndrEDxzzW2tyOjKowkCk2yrA2u9Ohs+cRVg=; b=d3xgH4n2XA8VHVwYusO7E3hnZl+bBbSKMja/QX9TA8blfhIDFZTFhexm96qNaZdhNm ANVwmSowiff6q5/vAYjxP+B4lUVGllQlgEJwtp7z9u+odLTcWpUfemd8kysUH6xSVhjG 7THJB+iHJ8fA+txZH0AJPpHYcAg0UTA4os2DI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=G9bsfeGlndrEDxzzW2tyOjKowkCk2yrA2u9Ohs+cRVg=; b=Rk+6isdehpZEItfBAVkyVyUV1S/UJQPGU7iGZp1AOnNo3kfl1qPkrawOCqdj0nqj8k rLWu5/7V8noO3qu1a1RGbVLCqvHKQiYSnfrKADXb6pn1+oN4Sp81P4TKg61yuq8sclT1 t/tR9mk9z2xbfedqmViK2ZgG2Qm0i35fGMGSUEwNoXAQ0fd2GUcawReMiTojsl9cizO9 V0prg7KThjugOYyyTkeUxUAMPX9pPa5JciyDs4iTkWHLW4/elS/CKu14147lJo2nf5Ux AVlDI4OV2/O0T8Cwa5U675Z0F35inX6TE905y0rIFtrH1AeY8LzqM1dMkY5rk7RPVeGd y1GA== X-Gm-Message-State: AODbwcDBHK2bLhI2rE8sBdkIwkTV672E/9uNsIBLjjBbef4zzENo9NbF P5sz+tFCm2VNihZNUzW7mgBQbNkTl5Ji X-Received: by 10.36.238.196 with SMTP id b187mr1945052iti.26.1494036754302; Fri, 05 May 2017 19:12:34 -0700 (PDT) MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: <20170504142435.10175-1-danielmicay@gmail.com> References: <20170504142435.10175-1-danielmicay@gmail.com> From: Kees Cook Date: Fri, 5 May 2017 19:12:33 -0700 X-Google-Sender-Auth: q9gfliF_mHggJqKFSptdn4kfHX8 Message-ID: To: Daniel Micay Cc: "kernel-hardening@lists.openwall.com" Subject: [kernel-hardening] Re: [PATCH] add the option of fortified string.h functions X-Virus-Scanned: ClamAV using ClamSMTP On Thu, May 4, 2017 at 7:24 AM, Daniel Micay wrote: > This adds support for compiling with a rough equivalent to the glibc > _FORTIFY_SOURCE=1 feature, providing compile-time and runtime buffer > overflow checks for string.h functions when the compiler determines the > size of the source or destination buffer at compile-time. Unlike glibc, > it covers buffer reads in addition to writes. > > GNU C __builtin_*_chk intrinsics are avoided because they're only > designed to detect write overflows and are overly complex. A single > inline branch works for everything but strncat while those intrinsics > would force the creation of a bunch of extra non-inline wrappers that > aren't able to receive the detected source buffer size. > > This detects various undefined uses of memcpy, etc. at compile-time > outside of non-core kernel code, and in the arm64 vdso code, so there > will need to be a series of fixes applied (mainly memcpy -> strncpy for > copying string constants to avoid copying past the end of the source). > It isn't particularly bad, but there are likely some issues that occur > during regular use at runtime (none found so far). > > Future improvements left out of initial implementation for simplicity, > as it's all quite optional and can be done incrementally: > > The fortified string functions should place a limit on reads from the > source. For strcat/strcpy, these could just be a variant of strlcat / > strlcpy using the size limit as a bound on both the source and > destination, with the return value only reporting whether truncation > occurred rather than providing the source length. It would be an easier > API for developers to use too and not that it really matters but it > would be more efficient for the case where truncation is intended. > > It should be possible to optionally use __builtin_object_size(x, 1) for > some functions (C strings) to detect intra-object overflows (like > glibc's _FORTIFY_SOURCE=2), but for now this takes the conservative > approach to avoid likely compatibility issues. > > The error reporting could be made friendlier by splitting up the > compile-time error for reads and writes. The runtime error could also > directly report the buffer and copy sizes. > > It may make sense to have the compile-time checks in a separate > configuration option since they wouldn't have a runtime performance cost > if there was an ifdef for the runtime check. However, the runtime cost > of these checks is already quite low (much lower than SSP) and as long > as some people are using it with allyesconfig, the issues will be found > for any in-tree code. > > Signed-off-by: Daniel Micay > --- > arch/x86/boot/compressed/misc.c | 5 ++ > include/linux/string.h | 161 ++++++++++++++++++++++++++++++++++++++++ > lib/string.c | 8 ++ > security/Kconfig | 6 ++ > 4 files changed, 180 insertions(+) > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > index b3c5a5f030ce..43691238a21d 100644 > --- a/arch/x86/boot/compressed/misc.c > +++ b/arch/x86/boot/compressed/misc.c > @@ -409,3 +409,8 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap, > debug_putstr("done.\nBooting the kernel.\n"); > return output; > } > + > +void fortify_panic(const char *name) > +{ > + error("detected buffer overflow"); > +} > diff --git a/include/linux/string.h b/include/linux/string.h > index 26b6f6a66f83..3bd429c9593a 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -169,4 +169,165 @@ static inline const char *kbasename(const char *path) > return tail ? tail + 1 : path; > } > > +#define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline)) > +#define __RENAME(x) __asm__(#x) > + > +void fortify_panic(const char *name) __noreturn __cold; To avoid a warning about the compressed boot fortify_panic() not returning, this annotation is needed: -Kees diff --git a/arch/x86/boot/compressed/error.h b/arch/x86/boot/compressed/error.h index 2e59dac07f9e..d732e608e3af 100644 --- a/arch/x86/boot/compressed/error.h +++ b/arch/x86/boot/compressed/error.h @@ -1,7 +1,9 @@ #ifndef BOOT_COMPRESSED_ERROR_H #define BOOT_COMPRESSED_ERROR_H +#include + void warn(char *m); -void error(char *m); +void error(char *m) __noreturn; #endif /* BOOT_COMPRESSED_ERROR_H */