| Message ID | 20230223-nolibc-stackprotector-v1-4-3e74d81b3f21@weissschuh.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/nolibc: add support for stack protector | expand |
Hi Thomas, thanks for this patchset. I must confess it's not very clear to me which class of programs using nolibc could benefit from stack protection, but if you think it can improve the overall value (even if just by allowing to test more combinations), I'm fine with this given that it doesn't remove anything. I'm having a few comments below: On Tue, Mar 07, 2023 at 10:22:33PM +0000, Thomas Weißschuh wrote: > diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h > new file mode 100644 > index 000000000000..ca1360b7afd8 > --- /dev/null > +++ b/tools/include/nolibc/stackprotector.h > @@ -0,0 +1,48 @@ > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ > +/* > + * Stack protector support for NOLIBC > + * Copyright (C) 2023 Thomas Weißschuh <linux@weissschuh.net> > + */ > + > +#ifndef _NOLIBC_STACKPROTECTOR_H > +#define _NOLIBC_STACKPROTECTOR_H > + > +#include "arch.h" > + > +#if defined(NOLIBC_STACKPROTECTOR) > + > +#if !defined(__ARCH_SUPPORTS_STACK_PROTECTOR) > +#error "nolibc does not support stack protectors on this arch" > +#endif > + > +#include "sys.h" > +#include "stdlib.h" > + > +__attribute__((weak,noreturn,section(".text.nolibc_stack_chk"))) > +void __stack_chk_fail(void) > +{ > + write(STDERR_FILENO, "!!Stack smashing detected!!\n", 28); > + abort(); > +} Don't you think you should call the syscall directly here like you did for __stack_chk_init() and/or declare the function with the no_stackprotector attribute ? I'm wondering if there could be a risk that it fails again if called from a bad condition. If you're certain it cannot, maybe just explain it in a 2-line comment above the function so that others don't ask the same in the future. > +__attribute__((weak,no_stack_protector,section(".text.nolibc_stack_chk"))) > +void __stack_chk_init(void) > +{ > + // raw syscall assembly as calling a function would trigger the > + // stackprotector itself > + my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0); For full-line comments, the regular C-style "/* */" is preferred (and please also use the multi-line format when needed). "//" tends to be reserved for short ones at the end of a line. > + // a bit more randomness in case getrandom() fails > + __stack_chk_guard |= (uintptr_t) &__stack_chk_guard; Using |= will in fact remove randomness rather than add, because it will turn some zero bits to ones but not the opposite. Maybe you'd want to use "^=" or "+=" instead ? Thanks, Willy
On Sun, Mar 12, 2023 at 01:56:43PM +0100, Willy Tarreau wrote: > Hi Thomas, > > thanks for this patchset. I must confess it's not very clear to me which > class of programs using nolibc could benefit from stack protection, but > if you think it can improve the overall value (even if just by allowing > to test more combinations), I'm fine with this given that it doesn't > remove anything. I forgot the rationale, will add it properly to the next revision: This is useful when using nolibc for security-critical tools. Using nolibc has the advantage that the code is easily auditable and sandboxable with seccomp as no unexpected syscalls are used. Using compiler-assistent stack protection provides another security mechanism. > I'm having a few comments below: > > On Tue, Mar 07, 2023 at 10:22:33PM +0000, Thomas Weißschuh wrote: > > diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h > > new file mode 100644 > > index 000000000000..ca1360b7afd8 > > --- /dev/null > > +++ b/tools/include/nolibc/stackprotector.h > > @@ -0,0 +1,48 @@ > > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ > > +/* > > + * Stack protector support for NOLIBC > > + * Copyright (C) 2023 Thomas Weißschuh <linux@weissschuh.net> > > + */ > > + > > +#ifndef _NOLIBC_STACKPROTECTOR_H > > +#define _NOLIBC_STACKPROTECTOR_H > > + > > +#include "arch.h" > > + > > +#if defined(NOLIBC_STACKPROTECTOR) > > + > > +#if !defined(__ARCH_SUPPORTS_STACK_PROTECTOR) > > +#error "nolibc does not support stack protectors on this arch" > > +#endif > > + > > +#include "sys.h" > > +#include "stdlib.h" > > + > > +__attribute__((weak,noreturn,section(".text.nolibc_stack_chk"))) > > +void __stack_chk_fail(void) > > +{ > > + write(STDERR_FILENO, "!!Stack smashing detected!!\n", 28); > > + abort(); > > +} > > Don't you think you should call the syscall directly here like you > did for __stack_chk_init() and/or declare the function with the > no_stackprotector attribute ? I'm wondering if there could be a > risk that it fails again if called from a bad condition. If you're > certain it cannot, maybe just explain it in a 2-line comment above > the function so that others don't ask the same in the future. Good point. It probably works because the compiler decided to inline the call. But syscalls are more robust, I'll change that. > > +__attribute__((weak,no_stack_protector,section(".text.nolibc_stack_chk"))) > > +void __stack_chk_init(void) > > +{ > > + // raw syscall assembly as calling a function would trigger the > > + // stackprotector itself > > + my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0); > > For full-line comments, the regular C-style "/* */" is preferred (and > please also use the multi-line format when needed). "//" tends to be > reserved for short ones at the end of a line. Of course, will be changed. > > + // a bit more randomness in case getrandom() fails > > + __stack_chk_guard |= (uintptr_t) &__stack_chk_guard; > > Using |= will in fact remove randomness rather than add, because it > will turn some zero bits to ones but not the opposite. Maybe you'd > want to use "^=" or "+=" instead ? Indeed, will change that.
On Sun, Mar 12, 2023 at 11:06:58PM +0000, Thomas Weißschuh wrote: > On Sun, Mar 12, 2023 at 01:56:43PM +0100, Willy Tarreau wrote: > > Hi Thomas, > > > > thanks for this patchset. I must confess it's not very clear to me which > > class of programs using nolibc could benefit from stack protection, but > > if you think it can improve the overall value (even if just by allowing > > to test more combinations), I'm fine with this given that it doesn't > > remove anything. > > I forgot the rationale, will add it properly to the next revision: > > This is useful when using nolibc for security-critical tools. > Using nolibc has the advantage that the code is easily auditable and > sandboxable with seccomp as no unexpected syscalls are used. > Using compiler-assistent stack protection provides another security > mechanism. I hadn't thought about such a use case at all. Till now the code has been developped in a more or less lenient way because it was aimed at tiny tools (a small preinit code, and regtests) with no particular focus on security. I'm fine with such use cases but I think we need to place the cursor at the right place in terms of responsibilities between the lib and the application. For example IMHO we should make sure it's never the lib's responsibility to erase some buffers that might have contained a password, to provide constant-time memcmp(), nor to pad/memset the structures in functions stacks, otherwise it will significantly complicate contributions and reviews in the future. This means the lib should continue to focus on providing convenient access to syscalls and very basic functions and if certain security- sensitive functions are ever needed, we should probably refrain from implementing them so that users know it's their job to provide them for their application. I don't have any such function in mind but I prefer that we can draw this line early. But I definitely understand how such a model based on inlined code can provide some benefits in terms of code auditing! You can even copy the code in the application's repository and have everything available without even depending on any version so that once the code has been audited, you know it will not change by a iota. Makes sense! Thanks for the background, Willy
diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile index ec57d3932506..9839feafd38a 100644 --- a/tools/include/nolibc/Makefile +++ b/tools/include/nolibc/Makefile @@ -25,8 +25,8 @@ endif nolibc_arch := $(patsubst arm64,aarch64,$(ARCH)) arch_file := arch-$(nolibc_arch).h -all_files := ctype.h errno.h nolibc.h signal.h std.h stdint.h stdio.h stdlib.h \ - string.h sys.h time.h types.h unistd.h +all_files := ctype.h errno.h nolibc.h signal.h stackprotector.h std.h stdint.h \ + stdio.h stdlib.h string.h sys.h time.h types.h unistd.h # install all headers needed to support a bare-metal compiler all: headers diff --git a/tools/include/nolibc/arch-i386.h b/tools/include/nolibc/arch-i386.h index e8d0cf545bf1..a8deb123edca 100644 --- a/tools/include/nolibc/arch-i386.h +++ b/tools/include/nolibc/arch-i386.h @@ -180,6 +180,9 @@ struct sys_stat_struct { char **environ __attribute__((weak)); const unsigned long *_auxv __attribute__((weak)); +void __stack_chk_init(void) __attribute__((weak)); + +#define __ARCH_SUPPORTS_STACK_PROTECTOR /* startup code */ /* @@ -188,9 +191,12 @@ const unsigned long *_auxv __attribute__((weak)); * 2) The deepest stack frame should be set to zero * */ -void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void) +void __attribute__((weak,noreturn,optimize("omit-frame-pointer"),no_stack_protector)) _start(void) { __asm__ volatile ( +#ifdef NOLIBC_STACKPROTECTOR + "call __stack_chk_init\n" // initialize stack protector +#endif "pop %eax\n" // argc (first arg, %eax) "mov %esp, %ebx\n" // argv[] (second arg, %ebx) "lea 4(%ebx,%eax,4),%ecx\n" // then a NULL then envp (third arg, %ecx) diff --git a/tools/include/nolibc/arch-x86_64.h b/tools/include/nolibc/arch-x86_64.h index 17f6751208e7..f7f2a11d4c3b 100644 --- a/tools/include/nolibc/arch-x86_64.h +++ b/tools/include/nolibc/arch-x86_64.h @@ -181,6 +181,8 @@ struct sys_stat_struct { char **environ __attribute__((weak)); const unsigned long *_auxv __attribute__((weak)); +#define __ARCH_SUPPORTS_STACK_PROTECTOR + /* startup code */ /* * x86-64 System V ABI mandates: @@ -191,6 +193,9 @@ const unsigned long *_auxv __attribute__((weak)); void __attribute__((weak,noreturn,optimize("omit-frame-pointer"))) _start(void) { __asm__ volatile ( +#ifdef NOLIBC_STACKPROTECTOR + "call __stack_chk_init\n" // initialize stack protector +#endif "pop %rdi\n" // argc (first arg, %rdi) "mov %rsp, %rsi\n" // argv[] (second arg, %rsi) "lea 8(%rsi,%rdi,8),%rdx\n" // then a NULL then envp (third arg, %rdx) diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h index b2bc48d3cfe4..04739a6293c4 100644 --- a/tools/include/nolibc/nolibc.h +++ b/tools/include/nolibc/nolibc.h @@ -104,6 +104,7 @@ #include "string.h" #include "time.h" #include "unistd.h" +#include "stackprotector.h" /* Used by programs to avoid std includes */ #define NOLIBC diff --git a/tools/include/nolibc/stackprotector.h b/tools/include/nolibc/stackprotector.h new file mode 100644 index 000000000000..ca1360b7afd8 --- /dev/null +++ b/tools/include/nolibc/stackprotector.h @@ -0,0 +1,48 @@ +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ +/* + * Stack protector support for NOLIBC + * Copyright (C) 2023 Thomas Weißschuh <linux@weissschuh.net> + */ + +#ifndef _NOLIBC_STACKPROTECTOR_H +#define _NOLIBC_STACKPROTECTOR_H + +#include "arch.h" + +#if defined(NOLIBC_STACKPROTECTOR) + +#if !defined(__ARCH_SUPPORTS_STACK_PROTECTOR) +#error "nolibc does not support stack protectors on this arch" +#endif + +#include "sys.h" +#include "stdlib.h" + +__attribute__((weak,noreturn,section(".text.nolibc_stack_chk"))) +void __stack_chk_fail(void) +{ + write(STDERR_FILENO, "!!Stack smashing detected!!\n", 28); + abort(); +} + +__attribute__((weak,noreturn,section(".text.nolibc_stack_chk"))) +void __stack_chk_fail_local(void) +{ + __stack_chk_fail(); +} + +__attribute__((weak,section(".data.nolibc_stack_chk"))) +uintptr_t __stack_chk_guard; + +__attribute__((weak,no_stack_protector,section(".text.nolibc_stack_chk"))) +void __stack_chk_init(void) +{ + // raw syscall assembly as calling a function would trigger the + // stackprotector itself + my_syscall3(__NR_getrandom, &__stack_chk_guard, sizeof(__stack_chk_guard), 0); + // a bit more randomness in case getrandom() fails + __stack_chk_guard |= (uintptr_t) &__stack_chk_guard; +} +#endif // defined(NOLIBC_STACKPROTECTOR) + +#endif // _NOLIBC_STACKPROTECTOR_H diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile index ea2b82a3cd86..749a09c9a012 100644 --- a/tools/testing/selftests/nolibc/Makefile +++ b/tools/testing/selftests/nolibc/Makefile @@ -1,6 +1,8 @@ # SPDX-License-Identifier: GPL-2.0 # Makefile for nolibc tests include ../../../scripts/Makefile.include +# We need this for the "cc-option" macro. +include ../../../build/Build.include # we're in ".../tools/testing/selftests/nolibc" ifeq ($(srctree),) @@ -74,7 +76,13 @@ else Q=@ endif +CFLAGS_STACKPROTECTOR = -DNOLIBC_STACKPROTECTOR \ + $(call cc-option,-mstack-protector-guard=global) \ + $(call cc-option,-fstack-protector-all) CFLAGS_s390 = -m64 +CFLAGS_x86 = $(CFLAGS_STACKPROTECTOR) +CFLAGS_i386 = $(CFLAGS_STACKPROTECTOR) +CFLAGS_x86_64 = $(CFLAGS_STACKPROTECTOR) CFLAGS ?= -Os -fno-ident -fno-asynchronous-unwind-tables $(CFLAGS_$(ARCH)) LDFLAGS := -s @@ -118,6 +126,10 @@ nolibc-test: nolibc-test.c sysroot/$(ARCH)/include $(QUIET_CC)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ \ -nostdlib -static -Isysroot/$(ARCH)/include $< -lgcc +foo: foo.c sysroot/$(ARCH)/include + $(QUIET_CC)$(CC) $(CFLAGS) $(LDFLAGS) -o $@ \ + -nostdlib -static -Isysroot/$(ARCH)/include $< -lgcc + # qemu user-land test run-user: nolibc-test $(Q)qemu-$(QEMU_ARCH) ./nolibc-test > "$(CURDIR)/run.out" || :
Stack protection is a feature to detect and handle stack buffer overflows at runtime. For this to work the compiler and libc have to collaborate. This patch adds the following parts to nolibc that are required by the compiler: * __stack_chk_guard: random sentinel value * __stack_chk_fail: handler for detected stack smashes In addition an initialization function is added that randomizes the sentinel value. Only support for global guards is implemented. Register guards are useful in multi-threaded context which nolibc does not provide support for. Link: https://lwn.net/Articles/584225/ Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- tools/include/nolibc/Makefile | 4 +-- tools/include/nolibc/arch-i386.h | 8 +++++- tools/include/nolibc/arch-x86_64.h | 5 ++++ tools/include/nolibc/nolibc.h | 1 + tools/include/nolibc/stackprotector.h | 48 +++++++++++++++++++++++++++++++++ tools/testing/selftests/nolibc/Makefile | 12 +++++++++ 6 files changed, 75 insertions(+), 3 deletions(-)