| Message ID | 20170504142435.10175-1-danielmicay@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
There are temporary workarounds for the overflows this found in https://github.com/thestinger/linux-hardened/commits/4.11, but not real fixes yet. There are some mostly harmless misuses of memcpy instead of strncpy and memcmp instead of strncmp where the source is a string constant. The arm64 vdso code uses memcmp with the address of 'char vdso_start' so perhaps that can become 'char vdso_start[PAGE_SIZE]' instead. One of the issues looks a bit more concerning. I haven't found any issues at runtime but that doesn't mean much since none of the compile-time issues were in code that's used on my desktop. This has been used on a 3.18 LTS arm64 kernel targeting the Pixel / Pixel XL on CopperheadOS for a while and found a real runtime stack buffer write overflow there in an out-of-tree driver. I'm somewhat surprised by the fact that there are real buffer overflows lying around that are this easily found without any fuzzing, etc. rather than this feature only providing a fair bit of runtime coverage to mitigate bugs that require edge cases.
[adding lkml and rasmus to CC...] On Thu, May 4, 2017 at 7:24 AM, Daniel Micay <danielmicay@gmail.com> 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 <danielmicay@gmail.com> > --- > 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; > +void __buffer_overflow(void) __compiletime_error("buffer overflow"); I've been playing with this with allyesconfig on x86_64, and I've got some suggestions I think would make things nicer for others looking at this. How about splitting the compile-time buffer_overflow() report into the write and read overflow pieces: void __dst_overflow(void) __compiletime_error("writes beyond end of destination buffer"); void __src_overflow(void) __compiletime_error("reads beyond end of source buffer"); With things like memcpy split up: if (__builtin_constant_p(size)) { if (p_size < size) __dst_overflow(); if (q_size < size) __src_overflow(); } I don't think it's worth doing the same for the runtime side since that'll just increase text size, etc. (Or, I wasn't creative enough to think of a way to distinguish them without doing so.) I tried to figure out a way to use __same_type(q, (char *)0) to trigger strncpy automatically and issuing a warning, but I couldn't get it to work for some reason. It never tripped. > +#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) > +__FORTIFY_INLINE char *strcpy(char *p, const char *q) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (p_size == (size_t)-1) > + return __builtin_strcpy(p, q); > + if (strlcpy(p, q, p_size) >= p_size) > + fortify_panic(__func__); > + return p; > +} The other thing, which is cosmetic, would be to swap "p" and "q" for "dst" and "src" everywhere, just to make these more readable. The allyesconfig has tripped over a bunch of places where memcpy needs to be strncpy. I'll send those patches in a bit... -Kees > + > +__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (__builtin_constant_p(size) && p_size < size) > + __buffer_overflow(); > + if (p_size < size) > + fortify_panic(__func__); > + return __builtin_strncpy(p, q, size); > +} > + > +__FORTIFY_INLINE char *strcat(char *p, const char *q) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (p_size == (size_t)-1) > + return __builtin_strcat(p, q); > + if (strlcat(p, q, p_size) >= p_size) > + fortify_panic(__func__); > + return p; > +} > + > +__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count) > +{ > + size_t p_len, copy_len; > + size_t p_size = __builtin_object_size(p, 0); > + if (p_size == (size_t)-1) > + return __builtin_strncat(p, q, count); > + p_len = __builtin_strlen(p); > + copy_len = strnlen(q, count); > + if (p_size < p_len + copy_len + 1) > + fortify_panic(__func__); > + __builtin_memcpy(p + p_len, q, copy_len); > + p[p_len + copy_len] = '\0'; > + return p; > +} > + > +__FORTIFY_INLINE __kernel_size_t strlen(const char *p) > +{ > + __kernel_size_t ret; > + size_t p_size = __builtin_object_size(p, 0); > + if (p_size == (size_t)-1) > + return __builtin_strlen(p); > + ret = strnlen(p, p_size); > + if (p_size <= ret) > + fortify_panic(__func__); > + return ret; > +} > + > +extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen); > +__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size); > + if (p_size <= ret) > + fortify_panic(__func__); > + return ret; > +} > + > +__FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (__builtin_constant_p(size) && p_size < size) > + __buffer_overflow(); > + if (p_size < size) > + fortify_panic(__func__); > + return __builtin_memset(p, c, size); > +} > + > +__FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + size_t q_size = __builtin_object_size(q, 0); > + if (__builtin_constant_p(size) && (p_size < size || q_size < size)) > + __buffer_overflow(); > + if (p_size < size || q_size < size) > + fortify_panic(__func__); > + return __builtin_memcpy(p, q, size); > +} > + > +__FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + size_t q_size = __builtin_object_size(q, 0); > + if (__builtin_constant_p(size) && (p_size < size || q_size < size)) > + __buffer_overflow(); > + if (p_size < size || q_size < size) > + fortify_panic(__func__); > + return __builtin_memmove(p, q, size); > +} > + > +extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan); > +__FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (__builtin_constant_p(size) && p_size < size) > + __buffer_overflow(); > + if (p_size < size) > + fortify_panic(__func__); > + return __real_memscan(p, c, size); > +} > + > +__FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + size_t q_size = __builtin_object_size(q, 0); > + if (__builtin_constant_p(size) && (p_size < size || q_size < size)) > + __buffer_overflow(); > + if (p_size < size || q_size < size) > + fortify_panic(__func__); > + return __builtin_memcmp(p, q, size); > +} > + > +__FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (__builtin_constant_p(size) && p_size < size) > + __buffer_overflow(); > + if (p_size < size) > + fortify_panic(__func__); > + return __builtin_memchr(p, c, size); > +} > + > +void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv); > +__FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (__builtin_constant_p(size) && p_size < size) > + __buffer_overflow(); > + if (p_size < size) > + fortify_panic(__func__); > + return __real_memchr_inv(p, c, size); > +} > + > +extern void *__real_kmemdup(const void *src, size_t len, gfp_t gfp) __RENAME(kmemdup); > +__FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp) > +{ > + size_t p_size = __builtin_object_size(p, 0); > + if (__builtin_constant_p(size) && p_size < size) > + __buffer_overflow(); > + if (p_size < size) > + fortify_panic(__func__); > + return __real_kmemdup(p, size, gfp); > +} > +#endif > + > #endif /* _LINUX_STRING_H_ */ > diff --git a/lib/string.c b/lib/string.c > index b5c9a1168d3a..ca356e537e24 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -19,6 +19,8 @@ > * - Kissed strtok() goodbye > */ > > +#define __NO_FORTIFY > + > #include <linux/types.h> > #include <linux/string.h> > #include <linux/ctype.h> > @@ -952,3 +954,9 @@ char *strreplace(char *s, char old, char new) > return s; > } > EXPORT_SYMBOL(strreplace); > + > +void fortify_panic(const char *name) > +{ > + panic("detected buffer overflow in %s", name); > +} > +EXPORT_SYMBOL(fortify_panic); > diff --git a/security/Kconfig b/security/Kconfig > index 93027fdf47d1..0e5035d720ce 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -154,6 +154,12 @@ config HARDENED_USERCOPY_PAGESPAN > been removed. This config is intended to be used only while > trying to find such users. > > +config FORTIFY_SOURCE > + bool "Harden common functions against buffer overflows" > + help > + Detect overflows of buffers in common functions where the compiler > + can determine the buffer size. > + > config STATIC_USERMODEHELPER > bool "Force all usermode helper calls through a single binary" > help > -- > 2.12.2 >
On 09/05/17 03:57, Daniel Axtens wrote: > (ppc people: this does some compile and run time bounds checking on > string functions. It's cool - currently it picks up a lot of random > things so it will require some more work across the tree, but hopefully > it will eventually hit mainline.) Ooh, nice! > > I've tested this on ppc with pseries_le_defconfig. > > I needed a couple of the fixes from github > (https://github.com/thestinger/linux-hardened/commits/4.11) in order to > build, specifically > https://github.com/thestinger/linux-hardened/commit/c65d6a6f309b06703584a23ac2b2bda4bb363143 > https://github.com/thestinger/linux-hardened/commit/adcec4756574a8c7f7cb5b6fa51ebeaeeae71aae > > Once those were added, I needed to disable fortification in prom_init.c, > as we apparently can't have new symbols there. (I don't understand that > file so I haven't dug into it.) > > We also have problems with the feature fixup tests leading to a panic on > boot. It relates to getting what I think are asm labels(?) and how we > address them. I have just disabled fortify here for now; I think the > code could be rewritten to take the labels as unsigned char *, but I > haven't dug into it. > > With the following fixups, I can boot a LE buildroot initrd (per > https://github.com/linuxppc/linux/wiki/Booting-with-Qemu). Sadly I don't > have access to real hardware any more, so I can't say anything more than > that. (ajd - perhaps relevant to your interests?) I'll test it baremetal when I get the chance, and I'll see if I can investigate the issues you've raised.
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; +void __buffer_overflow(void) __compiletime_error("buffer overflow"); + +#if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) +__FORTIFY_INLINE char *strcpy(char *p, const char *q) +{ + size_t p_size = __builtin_object_size(p, 0); + if (p_size == (size_t)-1) + return __builtin_strcpy(p, q); + if (strlcpy(p, q, p_size) >= p_size) + fortify_panic(__func__); + return p; +} + +__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + if (__builtin_constant_p(size) && p_size < size) + __buffer_overflow(); + if (p_size < size) + fortify_panic(__func__); + return __builtin_strncpy(p, q, size); +} + +__FORTIFY_INLINE char *strcat(char *p, const char *q) +{ + size_t p_size = __builtin_object_size(p, 0); + if (p_size == (size_t)-1) + return __builtin_strcat(p, q); + if (strlcat(p, q, p_size) >= p_size) + fortify_panic(__func__); + return p; +} + +__FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count) +{ + size_t p_len, copy_len; + size_t p_size = __builtin_object_size(p, 0); + if (p_size == (size_t)-1) + return __builtin_strncat(p, q, count); + p_len = __builtin_strlen(p); + copy_len = strnlen(q, count); + if (p_size < p_len + copy_len + 1) + fortify_panic(__func__); + __builtin_memcpy(p + p_len, q, copy_len); + p[p_len + copy_len] = '\0'; + return p; +} + +__FORTIFY_INLINE __kernel_size_t strlen(const char *p) +{ + __kernel_size_t ret; + size_t p_size = __builtin_object_size(p, 0); + if (p_size == (size_t)-1) + return __builtin_strlen(p); + ret = strnlen(p, p_size); + if (p_size <= ret) + fortify_panic(__func__); + return ret; +} + +extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen); +__FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen) +{ + size_t p_size = __builtin_object_size(p, 0); + __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size); + if (p_size <= ret) + fortify_panic(__func__); + return ret; +} + +__FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + if (__builtin_constant_p(size) && p_size < size) + __buffer_overflow(); + if (p_size < size) + fortify_panic(__func__); + return __builtin_memset(p, c, size); +} + +__FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + size_t q_size = __builtin_object_size(q, 0); + if (__builtin_constant_p(size) && (p_size < size || q_size < size)) + __buffer_overflow(); + if (p_size < size || q_size < size) + fortify_panic(__func__); + return __builtin_memcpy(p, q, size); +} + +__FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + size_t q_size = __builtin_object_size(q, 0); + if (__builtin_constant_p(size) && (p_size < size || q_size < size)) + __buffer_overflow(); + if (p_size < size || q_size < size) + fortify_panic(__func__); + return __builtin_memmove(p, q, size); +} + +extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan); +__FORTIFY_INLINE void *memscan(void *p, int c, __kernel_size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + if (__builtin_constant_p(size) && p_size < size) + __buffer_overflow(); + if (p_size < size) + fortify_panic(__func__); + return __real_memscan(p, c, size); +} + +__FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + size_t q_size = __builtin_object_size(q, 0); + if (__builtin_constant_p(size) && (p_size < size || q_size < size)) + __buffer_overflow(); + if (p_size < size || q_size < size) + fortify_panic(__func__); + return __builtin_memcmp(p, q, size); +} + +__FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + if (__builtin_constant_p(size) && p_size < size) + __buffer_overflow(); + if (p_size < size) + fortify_panic(__func__); + return __builtin_memchr(p, c, size); +} + +void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv); +__FORTIFY_INLINE void *memchr_inv(const void *p, int c, size_t size) +{ + size_t p_size = __builtin_object_size(p, 0); + if (__builtin_constant_p(size) && p_size < size) + __buffer_overflow(); + if (p_size < size) + fortify_panic(__func__); + return __real_memchr_inv(p, c, size); +} + +extern void *__real_kmemdup(const void *src, size_t len, gfp_t gfp) __RENAME(kmemdup); +__FORTIFY_INLINE void *kmemdup(const void *p, size_t size, gfp_t gfp) +{ + size_t p_size = __builtin_object_size(p, 0); + if (__builtin_constant_p(size) && p_size < size) + __buffer_overflow(); + if (p_size < size) + fortify_panic(__func__); + return __real_kmemdup(p, size, gfp); +} +#endif + #endif /* _LINUX_STRING_H_ */ diff --git a/lib/string.c b/lib/string.c index b5c9a1168d3a..ca356e537e24 100644 --- a/lib/string.c +++ b/lib/string.c @@ -19,6 +19,8 @@ * - Kissed strtok() goodbye */ +#define __NO_FORTIFY + #include <linux/types.h> #include <linux/string.h> #include <linux/ctype.h> @@ -952,3 +954,9 @@ char *strreplace(char *s, char old, char new) return s; } EXPORT_SYMBOL(strreplace); + +void fortify_panic(const char *name) +{ + panic("detected buffer overflow in %s", name); +} +EXPORT_SYMBOL(fortify_panic); diff --git a/security/Kconfig b/security/Kconfig index 93027fdf47d1..0e5035d720ce 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -154,6 +154,12 @@ config HARDENED_USERCOPY_PAGESPAN been removed. This config is intended to be used only while trying to find such users. +config FORTIFY_SOURCE + bool "Harden common functions against buffer overflows" + help + Detect overflows of buffers in common functions where the compiler + can determine the buffer size. + config STATIC_USERMODEHELPER bool "Force all usermode helper calls through a single binary" help
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 <danielmicay@gmail.com> --- arch/x86/boot/compressed/misc.c | 5 ++ include/linux/string.h | 161 ++++++++++++++++++++++++++++++++++++++++ lib/string.c | 8 ++ security/Kconfig | 6 ++ 4 files changed, 180 insertions(+)