| Message ID | 20240612211000.987116-1-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | elf: Make non-executable stack disable by default | expand |
On Wed, Jun 12, 2024 at 06:09:38PM -0300, Adhemerval Zanella wrote: > It is past time to not allow executable stacks as default, even if > kernel protection like SELinux already exist (although not widely > deployed). > Should we just change DEFAULT_STACK_PERMS to (PF_R|PF_W)? > For compatibility, a new tunable, glibc.rtld.noexecstack, can be use > to allow programs to run with an executable stack. It has the side > effect of complete disable executable stacks for setuid binaries, > but allowing in first place is *really* a bad idea. > > Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu. > --- > Makeconfig | 1 + > NEWS | 4 ++ > elf/Makefile | 45 ++++++++++++++ > elf/dl-load.c | 8 +++ > elf/dl-support.c | 5 ++ > elf/dl-tunables.list | 6 ++ > elf/rtld.c | 4 ++ > elf/tst-execstack-prog-static.c | 1 + > elf/tst-execstack.c | 106 +++++++++----------------------- > elf/tst-ro-dynamic-mod.map | 3 +- > elf/tst-rtld-list-tunables.exp | 1 + > manual/tunables.texi | 19 ++++++ > nptl/Makefile | 7 +++ > 13 files changed, 133 insertions(+), 77 deletions(-) > create mode 100644 elf/tst-execstack-prog-static.c > > diff --git a/Makeconfig b/Makeconfig > index 29819363da..76f5772272 100644 > --- a/Makeconfig > +++ b/Makeconfig > @@ -356,6 +356,7 @@ endif > # Must be supported by the linker. > no-whole-archive = -Wl,--no-whole-archive > whole-archive = -Wl,--whole-archive > +noexecstack = -Wl,-z,noexecstack > > # Installed name of the startup code. > # The ELF convention is that the startfile is called crt1.o > diff --git a/NEWS b/NEWS > index 20e263f581..be589cb616 100644 > --- a/NEWS > +++ b/NEWS > @@ -36,6 +36,10 @@ Major new features: > * On Linux, update epoll header to include epoll ioctl definitions and > related structure added in Linux kernel 6.9. > > +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable > + stacks from either main program, dependencies, or dynamically loadeed > + libraries. The default is to disable executable stacks. > + > Deprecated and removed features, and other changes affecting compatibility: > > * Architectures which use a 32-bit seconds-since-epoch field in struct > diff --git a/elf/Makefile b/elf/Makefile > index 57b3a19d36..56414fcbba 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -543,6 +543,15 @@ tests-execstack-yes = \ > tst-execstack-needed \ > tst-execstack-prog \ > # tests-execstack-yes > +tests-execstack-static-yes = \ > + tst-execstack-prog-static \ > + # tests-execstack-static-yes > +tests-execstack-special-yes = \ > + $(objpfx)tst-execstack-default.out \ > + $(objpfx)tst-execstack-needed-default.out \ > + $(objpfx)tst-execstack-prog-default.out \ > + $(objpfx)tst-execstack-prog-static-default.out \ > + # tests-execstack-special-yes > endif > ifeq ($(have-depaudit),yes) > tests += \ > @@ -630,6 +639,8 @@ $(objpfx)tst-rtld-does-not-exist.out: tst-rtld-does-not-exist.sh $(objpfx)ld.so > $(evaluate-test) > > tests += $(tests-execstack-$(have-z-execstack)) > +tests-static += $(tests-execstack-static-$(have-z-execstack)) > +tests-special += $(tests-execstack-special-$(have-z-execstack)) > ifeq ($(run-built-tests),yes) > tests-special += \ > $(objpfx)noload-mem.out \ > @@ -1850,11 +1861,45 @@ CPPFLAGS-tst-execstack.c += -DUSE_PTHREADS=0 > LDFLAGS-tst-execstack = -Wl,-z,noexecstack > LDFLAGS-tst-execstack-mod.so = -Wl,-z,execstack > > +$(objpfx)tst-execstack-default.out: $(objpfx)tst-execstack > + $(test-wrapper-env) $(run-program-env) $< > $@; \ > + $(evaluate-test) > + > +tst-execstack-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ > + ALLOW_EXECSTACK=1 > + > $(objpfx)tst-execstack-needed: $(objpfx)tst-execstack-mod.so > LDFLAGS-tst-execstack-needed = -Wl,-z,noexecstack > > +tst-execstack-needed-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ > + ALLOW_EXECSTACK=1 > + > +$(objpfx)tst-execstack-needed-default.out: $(objpfx)tst-execstack-needed > + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ > + grep -q 'error while loading shared libraries:.*executable stack is not allowed$$' $@ \ > + && grep -q '^status: 127$$' $@; \ > + $(evaluate-test) > + > LDFLAGS-tst-execstack-prog = -Wl,-z,execstack > CFLAGS-tst-execstack-prog.c += -Wno-trampolines > +tst-execstack-prog-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 > + > +LDFLAGS-tst-execstack-prog-static = -Wl,-z,execstack > +CFLAGS-tst-execstack-prog-static.c += -Wno-trampolines > +tst-execstack-prog-static-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 > + > +$(objpfx)tst-execstack-prog-default.out: $(objpfx)tst-execstack-prog > + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ > + grep -q 'Fatal glibc error: executable stack is not allowed$$' $@ \ > + && grep -q '^status: 127$$' $@; \ > + $(evaluate-test) > + > +$(objpfx)tst-execstack-prog-static-default.out: $(objpfx)tst-execstack-prog-static > + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ > + grep -q 'Fatal glibc error: executable stack is not allowed$$' $@ \ > + && grep -q '^status: 127$$' $@; \ > + $(evaluate-test) > + > CFLAGS-tst-execstack-mod.c += -Wno-trampolines > endif > > diff --git a/elf/dl-load.c b/elf/dl-load.c > index 8a89b71016..414955694f 100644 > --- a/elf/dl-load.c > +++ b/elf/dl-load.c > @@ -32,6 +32,7 @@ > #include <sys/stat.h> > #include <sys/types.h> > #include <gnu/lib-names.h> > +#include <dl-tunables.h> > > /* Type for the buffer we put the ELF header and hopefully the program > header. This buffer does not really have to be too large. In most > @@ -1297,6 +1298,13 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, > > if (__glibc_unlikely ((stack_flags &~ GL(dl_stack_flags)) & PF_X)) > { > + if (TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) > + { > + errstring = N_("\ > +executable stack is not allowed"); > + goto lose; > + } > + > /* The stack is presently not executable, but this module > requires that it be executable. */ > #if PTHREAD_IN_LIBC > diff --git a/elf/dl-support.c b/elf/dl-support.c > index 451932dd03..084e2579b1 100644 > --- a/elf/dl-support.c > +++ b/elf/dl-support.c > @@ -45,6 +45,7 @@ > #include <dl-find_object.h> > #include <array_length.h> > #include <dl-symbol-redir-ifunc.h> > +#include <dl-tunables.h> > > extern char *__progname; > char **_dl_argv = &__progname; /* This is checked for some error messages. */ > @@ -335,6 +336,10 @@ _dl_non_dynamic_init (void) > break; > } > > + if ((__glibc_unlikely (GL(dl_stack_flags)) & PF_X) > + && TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) > + _dl_fatal_printf ("Fatal glibc error: executable stack is not allowed\n"); > + > call_function_static_weak (_dl_find_object_init); > > /* Setup relro on the binary itself. */ > diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list > index 1186272c81..745c9ce2c1 100644 > --- a/elf/dl-tunables.list > +++ b/elf/dl-tunables.list > @@ -142,6 +142,12 @@ glibc { > maxval: 1 > default: 0 > } > + noexecstack { > + type: INT_32 > + minval: 0 > + maxval: 1 > + default: 1 > + } > } > > mem { > diff --git a/elf/rtld.c b/elf/rtld.c > index e9525ea987..0762d68d68 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -1668,6 +1668,10 @@ dl_main (const ElfW(Phdr) *phdr, > > bool has_interp = rtld_setup_main_map (main_map); > > + if ((__glibc_unlikely (GL(dl_stack_flags)) & PF_X) > + && TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) > + _dl_fatal_printf ("Fatal glibc error: executable stack is not allowed\n"); > + > /* If the current libname is different from the SONAME, add the > latter as well. */ > if (GL(dl_rtld_map).l_info[DT_SONAME] != NULL > diff --git a/elf/tst-execstack-prog-static.c b/elf/tst-execstack-prog-static.c > new file mode 100644 > index 0000000000..180657e5ef > --- /dev/null > +++ b/elf/tst-execstack-prog-static.c > @@ -0,0 +1 @@ > +#include "tst-execstack-prog.c" > diff --git a/elf/tst-execstack.c b/elf/tst-execstack.c > index 560b353918..56fb8bd874 100644 > --- a/elf/tst-execstack.c > +++ b/elf/tst-execstack.c > @@ -8,17 +8,10 @@ > #include <unistd.h> > #include <error.h> > #include <stackinfo.h> > - > -static void > -print_maps (void) > -{ > -#if 0 > - char *cmd = NULL; > - asprintf (&cmd, "cat /proc/%d/maps", getpid ()); > - system (cmd); > - free (cmd); > -#endif > -} print_maps removal belongs to a separate patch. > +#include <stdlib.h> > +#include <support/check.h> > +#include <support/xthread.h> > +#include <support/xdlfcn.h> > > static void deeper (void (*f) (void)); > > @@ -47,7 +40,7 @@ waiter_thread (void *arg) > } > #endif > > -static bool allow_execstack = true; > +static bool kernel_allow_execstack = true; > > > static int > @@ -74,61 +67,46 @@ do_test (void) > { > n = getline (&line, &linelen, fp); > if (n > 0 && line[0] == '0') > - allow_execstack = false; > + kernel_allow_execstack = false; > } > > fclose (fp); > } > } > > - printf ("executable stacks %sallowed\n", allow_execstack ? "" : "not "); > + printf ("kernel allows executable stacks: %s\n", > + kernel_allow_execstack ? "yes" : "not "); > + > + bool glibc_allow_execstack = getenv ("ALLOW_EXECSTACK") != 0; > + printf ("expected allow executable stacks: %s\n", > + glibc_allow_execstack ? "yes" : "not "); > > static void *f; /* Address of this is used in other threads. */ > > #if USE_PTHREADS > /* Create some threads while stacks are nonexecutable. */ > #define N 5 > - pthread_t thr[N]; > > - pthread_barrier_init (&startup_barrier, NULL, N + 1); > - pthread_barrier_init (&go_barrier, NULL, N + 1); > + xpthread_barrier_init (&startup_barrier, NULL, N + 1); > + xpthread_barrier_init (&go_barrier, NULL, N + 1); > > for (int i = 0; i < N; ++i) > - { > - int rc = pthread_create (&thr[i], NULL, &waiter_thread, &f); > - if (rc) > - error (1, rc, "pthread_create"); > - } > + xpthread_create (NULL, &waiter_thread, &f); > > /* Make sure they are all there using their stacks. */ > - pthread_barrier_wait (&startup_barrier); > + xpthread_barrier_wait (&startup_barrier); > puts ("threads waiting"); > #endif > > - print_maps (); > - > #if USE_PTHREADS > void *old_stack_addr, *new_stack_addr; > size_t stack_size; > pthread_t me = pthread_self (); > pthread_attr_t attr; > - int ret = 0; > > - ret = pthread_getattr_np (me, &attr); > - if (ret) > - { > - printf ("before execstack: pthread_getattr_np returned error: %s\n", > - strerror (ret)); > - return 1; > - } > - > - ret = pthread_attr_getstack (&attr, &old_stack_addr, &stack_size); > - if (ret) > - { > - printf ("before execstack: pthread_attr_getstack returned error: %s\n", > - strerror (ret)); > - return 1; > - } > + TEST_VERIFY_EXIT (pthread_getattr_np (me, &attr) == 0); > + TEST_VERIFY_EXIT (pthread_attr_getstack (&attr, &old_stack_addr, > + &stack_size) == 0); > # if _STACK_GROWS_DOWN > old_stack_addr += stack_size; > # else > @@ -146,39 +124,22 @@ do_test (void) > if (h == NULL) > { > printf ("cannot load: %s\n", dlerror ()); > - return allow_execstack; > + return kernel_allow_execstack > + ? (glibc_allow_execstack ? 1 : 0) > + : 0; > } > > - f = dlsym (h, "tryme"); > - if (f == NULL) > - { > - printf ("symbol not found: %s\n", dlerror ()); > - return 1; > - } > + f = xdlsym (h, "tryme"); > > /* Test if that really made our stack executable. > The `tryme' function should crash if not. */ > > (*((void (*) (void)) f)) (); > > - print_maps (); > - > #if USE_PTHREADS > - ret = pthread_getattr_np (me, &attr); > - if (ret) > - { > - printf ("after execstack: pthread_getattr_np returned error: %s\n", > - strerror (ret)); > - return 1; > - } > - > - ret = pthread_attr_getstack (&attr, &new_stack_addr, &stack_size); > - if (ret) > - { > - printf ("after execstack: pthread_attr_getstack returned error: %s\n", > - strerror (ret)); > - return 1; > - } > + TEST_VERIFY_EXIT (pthread_getattr_np (me, &attr) == 0); > + TEST_VERIFY_EXIT (pthread_attr_getstack (&attr, &new_stack_addr, > + &stack_size) == 0); > > # if _STACK_GROWS_DOWN > new_stack_addr += stack_size; > @@ -206,26 +167,19 @@ do_test (void) > /* Test that growing the stack region gets new executable pages too. */ > deeper ((void (*) (void)) f); > > - print_maps (); > - > #if USE_PTHREADS > /* Test that a fresh thread now gets an executable stack. */ > - { > - pthread_t th; > - int rc = pthread_create (&th, NULL, &tryme_thread, f); > - if (rc) > - error (1, rc, "pthread_create"); > - } > + xpthread_create (NULL, &tryme_thread, f); > > puts ("threads go"); > /* The existing threads' stacks should have been changed. > Let them run to test it. */ > - pthread_barrier_wait (&go_barrier); > + xpthread_barrier_wait (&go_barrier); > > - pthread_exit ((void *) (long int) (! allow_execstack)); > + pthread_exit ((void *) (long int) (! kernel_allow_execstack)); > #endif > > - return ! allow_execstack; > + return ! kernel_allow_execstack; > } > > static void > diff --git a/elf/tst-ro-dynamic-mod.map b/elf/tst-ro-dynamic-mod.map > index 2fe4a2998c..b68b036d3a 100644 > --- a/elf/tst-ro-dynamic-mod.map > +++ b/elf/tst-ro-dynamic-mod.map > @@ -4,7 +4,7 @@ SECTIONS > .dynamic : { *(.dynamic) } :text :dynamic > .rodata : { *(.data*) *(.bss*) } :text > /DISCARD/ : { > - *(.note.gnu.property) > + *(.note.gnu.property) *(.note.GNU-stack) > } > .note : { *(.note.*) } :text :note > } > @@ -13,4 +13,5 @@ PHDRS > text PT_LOAD FLAGS(5) FILEHDR PHDRS; > dynamic PT_DYNAMIC FLAGS(4); > note PT_NOTE FLAGS(4); > + gnu_stack PT_GNU_STACK FLAGS(6); > } > diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp > index db0e1c86e9..2ac06e083a 100644 > --- a/elf/tst-rtld-list-tunables.exp > +++ b/elf/tst-rtld-list-tunables.exp > @@ -14,4 +14,5 @@ glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0x[f]+) > glibc.rtld.dynamic_sort: 2 (min: 1, max: 2) > glibc.rtld.enable_secure: 0 (min: 0, max: 1) > glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10) > +glibc.rtld.noexecstack: 1 (min: 0, max: 1) > glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+) > diff --git a/manual/tunables.texi b/manual/tunables.texi > index 8dd02d8149..6fcf0fb451 100644 > --- a/manual/tunables.texi > +++ b/manual/tunables.texi > @@ -356,6 +356,25 @@ tests for @code{AT_SECURE} programs and not meant to be a security feature. > The default value of this tunable is @samp{0}. > @end deftp > > +@deftp Tunable glibc.rtld.noexecstack > +Initially, @theglibc{} will use either the default architecture flags (that might > +contain the executable bit) or the value of @code{PT_GNU_STACK} (if present). > +If any shared library dependency or dynamic object loaded with @code{dlopen} > +or @code{dlmopen} requires an executable stack (either by the default flags > +or @code{PT_GNU_STACK} from the library) @theglibc{} will try to change the > +permission of the stack to enable execution for all running threads. > + > +The @code{glibc.rtld.noexecstack} tunable allows the user to control whether > +to control executable stacks from the main program, dependencies, or from > +dynamically loaded libraries. Setting its value to @code{0} allows executable > +stacks, where @code{1} disables it. The default value is @code{1}. > + > +When executable stacks are not allowed, and if the main program or dependencies > +require an executable stack, the loader will fail with an error message. Trying > +to load a dynamic shared library with @code{dlopen} or @code{dlmopen} will fail, > +with a proper message that can be obtained with @code{dlerror}. > +@end deftp > + > @node Elision Tunables > @section Elision Tunables > @cindex elision tunables > diff --git a/nptl/Makefile b/nptl/Makefile > index b3f8af2e1c..e504d43e22 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -468,6 +468,7 @@ tests-internal += \ > # tests-internal > ifeq ($(have-z-execstack),yes) > tests += tst-execstack-threads > +tests-special += $(objpfx)tst-execstack-threads-default.out > endif > endif > > @@ -675,6 +676,12 @@ $(objpfx)tst-execstack-threads.out: $(objpfx)tst-execstack-threads-mod.so > LDFLAGS-tst-execstack-threads = -Wl,-z,noexecstack > LDFLAGS-tst-execstack-threads-mod.so = -Wl,-z,execstack > CFLAGS-tst-execstack-threads-mod.c += -Wno-trampolines > +tst-execstack-threads-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ > + ALLOW_EXECSTACK=1 > + > +$(objpfx)tst-execstack-threads-default.out: $(objpfx)tst-execstack-threads > + $(test-wrapper-env) $(run-program-env) $< > $@; \ > + $(evaluate-test) > > tst-stackguard1-ARGS = --command "$(host-test-program-cmd) --child" > tst-stackguard1-static-ARGS = --command "$(objpfx)tst-stackguard1-static --child" > -- > 2.43.0 > H.J.
* Adhemerval Zanella: > +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable > + stacks from either main program, dependencies, or dynamically loadeed > + libraries. The default is to disable executable stacks. Typo: “load[e]d” Should we refuse to enable executable stack on dlopen by default? And only enable it if the initial set of objects request executable stack? Thanks, Florian
On 12/06/24 19:50, H.J. Lu wrote: > On Wed, Jun 12, 2024 at 06:09:38PM -0300, Adhemerval Zanella wrote: >> It is past time to not allow executable stacks as default, even if >> kernel protection like SELinux already exist (although not widely >> deployed). >> > > Should we just change DEFAULT_STACK_PERMS to (PF_R|PF_W)? I don't think we can't if we really want to disable executable-stacks by kernel loaded binaries without GNU_PT_STACK. In this case we need to have DEFAULT_STACK_PERMS as the default one that kernel uses. > >> For compatibility, a new tunable, glibc.rtld.noexecstack, can be use >> to allow programs to run with an executable stack. It has the side >> effect of complete disable executable stacks for setuid binaries, >> but allowing in first place is *really* a bad idea. >> >> Checked on x86_64-linux-gnu, i686-linux-gnu, and aarch64-linux-gnu. >> --- >> Makeconfig | 1 + >> NEWS | 4 ++ >> elf/Makefile | 45 ++++++++++++++ >> elf/dl-load.c | 8 +++ >> elf/dl-support.c | 5 ++ >> elf/dl-tunables.list | 6 ++ >> elf/rtld.c | 4 ++ >> elf/tst-execstack-prog-static.c | 1 + >> elf/tst-execstack.c | 106 +++++++++----------------------- >> elf/tst-ro-dynamic-mod.map | 3 +- >> elf/tst-rtld-list-tunables.exp | 1 + >> manual/tunables.texi | 19 ++++++ >> nptl/Makefile | 7 +++ >> 13 files changed, 133 insertions(+), 77 deletions(-) >> create mode 100644 elf/tst-execstack-prog-static.c >> >> diff --git a/Makeconfig b/Makeconfig >> index 29819363da..76f5772272 100644 >> --- a/Makeconfig >> +++ b/Makeconfig >> @@ -356,6 +356,7 @@ endif >> # Must be supported by the linker. >> no-whole-archive = -Wl,--no-whole-archive >> whole-archive = -Wl,--whole-archive >> +noexecstack = -Wl,-z,noexecstack >> >> # Installed name of the startup code. >> # The ELF convention is that the startfile is called crt1.o >> diff --git a/NEWS b/NEWS >> index 20e263f581..be589cb616 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -36,6 +36,10 @@ Major new features: >> * On Linux, update epoll header to include epoll ioctl definitions and >> related structure added in Linux kernel 6.9. >> >> +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable >> + stacks from either main program, dependencies, or dynamically loadeed >> + libraries. The default is to disable executable stacks. >> + >> Deprecated and removed features, and other changes affecting compatibility: >> >> * Architectures which use a 32-bit seconds-since-epoch field in struct >> diff --git a/elf/Makefile b/elf/Makefile >> index 57b3a19d36..56414fcbba 100644 >> --- a/elf/Makefile >> +++ b/elf/Makefile >> @@ -543,6 +543,15 @@ tests-execstack-yes = \ >> tst-execstack-needed \ >> tst-execstack-prog \ >> # tests-execstack-yes >> +tests-execstack-static-yes = \ >> + tst-execstack-prog-static \ >> + # tests-execstack-static-yes >> +tests-execstack-special-yes = \ >> + $(objpfx)tst-execstack-default.out \ >> + $(objpfx)tst-execstack-needed-default.out \ >> + $(objpfx)tst-execstack-prog-default.out \ >> + $(objpfx)tst-execstack-prog-static-default.out \ >> + # tests-execstack-special-yes >> endif >> ifeq ($(have-depaudit),yes) >> tests += \ >> @@ -630,6 +639,8 @@ $(objpfx)tst-rtld-does-not-exist.out: tst-rtld-does-not-exist.sh $(objpfx)ld.so >> $(evaluate-test) >> >> tests += $(tests-execstack-$(have-z-execstack)) >> +tests-static += $(tests-execstack-static-$(have-z-execstack)) >> +tests-special += $(tests-execstack-special-$(have-z-execstack)) >> ifeq ($(run-built-tests),yes) >> tests-special += \ >> $(objpfx)noload-mem.out \ >> @@ -1850,11 +1861,45 @@ CPPFLAGS-tst-execstack.c += -DUSE_PTHREADS=0 >> LDFLAGS-tst-execstack = -Wl,-z,noexecstack >> LDFLAGS-tst-execstack-mod.so = -Wl,-z,execstack >> >> +$(objpfx)tst-execstack-default.out: $(objpfx)tst-execstack >> + $(test-wrapper-env) $(run-program-env) $< > $@; \ >> + $(evaluate-test) >> + >> +tst-execstack-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ >> + ALLOW_EXECSTACK=1 >> + >> $(objpfx)tst-execstack-needed: $(objpfx)tst-execstack-mod.so >> LDFLAGS-tst-execstack-needed = -Wl,-z,noexecstack >> >> +tst-execstack-needed-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ >> + ALLOW_EXECSTACK=1 >> + >> +$(objpfx)tst-execstack-needed-default.out: $(objpfx)tst-execstack-needed >> + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ >> + grep -q 'error while loading shared libraries:.*executable stack is not allowed$$' $@ \ >> + && grep -q '^status: 127$$' $@; \ >> + $(evaluate-test) >> + >> LDFLAGS-tst-execstack-prog = -Wl,-z,execstack >> CFLAGS-tst-execstack-prog.c += -Wno-trampolines >> +tst-execstack-prog-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 >> + >> +LDFLAGS-tst-execstack-prog-static = -Wl,-z,execstack >> +CFLAGS-tst-execstack-prog-static.c += -Wno-trampolines >> +tst-execstack-prog-static-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 >> + >> +$(objpfx)tst-execstack-prog-default.out: $(objpfx)tst-execstack-prog >> + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ >> + grep -q 'Fatal glibc error: executable stack is not allowed$$' $@ \ >> + && grep -q '^status: 127$$' $@; \ >> + $(evaluate-test) >> + >> +$(objpfx)tst-execstack-prog-static-default.out: $(objpfx)tst-execstack-prog-static >> + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ >> + grep -q 'Fatal glibc error: executable stack is not allowed$$' $@ \ >> + && grep -q '^status: 127$$' $@; \ >> + $(evaluate-test) >> + >> CFLAGS-tst-execstack-mod.c += -Wno-trampolines >> endif >> >> diff --git a/elf/dl-load.c b/elf/dl-load.c >> index 8a89b71016..414955694f 100644 >> --- a/elf/dl-load.c >> +++ b/elf/dl-load.c >> @@ -32,6 +32,7 @@ >> #include <sys/stat.h> >> #include <sys/types.h> >> #include <gnu/lib-names.h> >> +#include <dl-tunables.h> >> >> /* Type for the buffer we put the ELF header and hopefully the program >> header. This buffer does not really have to be too large. In most >> @@ -1297,6 +1298,13 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, >> >> if (__glibc_unlikely ((stack_flags &~ GL(dl_stack_flags)) & PF_X)) >> { >> + if (TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) >> + { >> + errstring = N_("\ >> +executable stack is not allowed"); >> + goto lose; >> + } >> + >> /* The stack is presently not executable, but this module >> requires that it be executable. */ >> #if PTHREAD_IN_LIBC >> diff --git a/elf/dl-support.c b/elf/dl-support.c >> index 451932dd03..084e2579b1 100644 >> --- a/elf/dl-support.c >> +++ b/elf/dl-support.c >> @@ -45,6 +45,7 @@ >> #include <dl-find_object.h> >> #include <array_length.h> >> #include <dl-symbol-redir-ifunc.h> >> +#include <dl-tunables.h> >> >> extern char *__progname; >> char **_dl_argv = &__progname; /* This is checked for some error messages. */ >> @@ -335,6 +336,10 @@ _dl_non_dynamic_init (void) >> break; >> } >> >> + if ((__glibc_unlikely (GL(dl_stack_flags)) & PF_X) >> + && TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) >> + _dl_fatal_printf ("Fatal glibc error: executable stack is not allowed\n"); >> + >> call_function_static_weak (_dl_find_object_init); >> >> /* Setup relro on the binary itself. */ >> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list >> index 1186272c81..745c9ce2c1 100644 >> --- a/elf/dl-tunables.list >> +++ b/elf/dl-tunables.list >> @@ -142,6 +142,12 @@ glibc { >> maxval: 1 >> default: 0 >> } >> + noexecstack { >> + type: INT_32 >> + minval: 0 >> + maxval: 1 >> + default: 1 >> + } >> } >> >> mem { >> diff --git a/elf/rtld.c b/elf/rtld.c >> index e9525ea987..0762d68d68 100644 >> --- a/elf/rtld.c >> +++ b/elf/rtld.c >> @@ -1668,6 +1668,10 @@ dl_main (const ElfW(Phdr) *phdr, >> >> bool has_interp = rtld_setup_main_map (main_map); >> >> + if ((__glibc_unlikely (GL(dl_stack_flags)) & PF_X) >> + && TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) >> + _dl_fatal_printf ("Fatal glibc error: executable stack is not allowed\n"); >> + >> /* If the current libname is different from the SONAME, add the >> latter as well. */ >> if (GL(dl_rtld_map).l_info[DT_SONAME] != NULL >> diff --git a/elf/tst-execstack-prog-static.c b/elf/tst-execstack-prog-static.c >> new file mode 100644 >> index 0000000000..180657e5ef >> --- /dev/null >> +++ b/elf/tst-execstack-prog-static.c >> @@ -0,0 +1 @@ >> +#include "tst-execstack-prog.c" >> diff --git a/elf/tst-execstack.c b/elf/tst-execstack.c >> index 560b353918..56fb8bd874 100644 >> --- a/elf/tst-execstack.c >> +++ b/elf/tst-execstack.c >> @@ -8,17 +8,10 @@ >> #include <unistd.h> >> #include <error.h> >> #include <stackinfo.h> >> - >> -static void >> -print_maps (void) >> -{ >> -#if 0 >> - char *cmd = NULL; >> - asprintf (&cmd, "cat /proc/%d/maps", getpid ()); >> - system (cmd); >> - free (cmd); >> -#endif >> -} > > print_maps removal belongs to a separate patch. Do we really need a separate patch for this fix? This snipper is not enabled, and I don't think if really worth the trouble. > >> +#include <stdlib.h> >> +#include <support/check.h> >> +#include <support/xthread.h> >> +#include <support/xdlfcn.h> >> >> static void deeper (void (*f) (void)); >> >> @@ -47,7 +40,7 @@ waiter_thread (void *arg) >> } >> #endif >> >> -static bool allow_execstack = true; >> +static bool kernel_allow_execstack = true; >> >> >> static int >> @@ -74,61 +67,46 @@ do_test (void) >> { >> n = getline (&line, &linelen, fp); >> if (n > 0 && line[0] == '0') >> - allow_execstack = false; >> + kernel_allow_execstack = false; >> } >> >> fclose (fp); >> } >> } >> >> - printf ("executable stacks %sallowed\n", allow_execstack ? "" : "not "); >> + printf ("kernel allows executable stacks: %s\n", >> + kernel_allow_execstack ? "yes" : "not "); >> + >> + bool glibc_allow_execstack = getenv ("ALLOW_EXECSTACK") != 0; >> + printf ("expected allow executable stacks: %s\n", >> + glibc_allow_execstack ? "yes" : "not "); >> >> static void *f; /* Address of this is used in other threads. */ >> >> #if USE_PTHREADS >> /* Create some threads while stacks are nonexecutable. */ >> #define N 5 >> - pthread_t thr[N]; >> >> - pthread_barrier_init (&startup_barrier, NULL, N + 1); >> - pthread_barrier_init (&go_barrier, NULL, N + 1); >> + xpthread_barrier_init (&startup_barrier, NULL, N + 1); >> + xpthread_barrier_init (&go_barrier, NULL, N + 1); >> >> for (int i = 0; i < N; ++i) >> - { >> - int rc = pthread_create (&thr[i], NULL, &waiter_thread, &f); >> - if (rc) >> - error (1, rc, "pthread_create"); >> - } >> + xpthread_create (NULL, &waiter_thread, &f); >> >> /* Make sure they are all there using their stacks. */ >> - pthread_barrier_wait (&startup_barrier); >> + xpthread_barrier_wait (&startup_barrier); >> puts ("threads waiting"); >> #endif >> >> - print_maps (); >> - >> #if USE_PTHREADS >> void *old_stack_addr, *new_stack_addr; >> size_t stack_size; >> pthread_t me = pthread_self (); >> pthread_attr_t attr; >> - int ret = 0; >> >> - ret = pthread_getattr_np (me, &attr); >> - if (ret) >> - { >> - printf ("before execstack: pthread_getattr_np returned error: %s\n", >> - strerror (ret)); >> - return 1; >> - } >> - >> - ret = pthread_attr_getstack (&attr, &old_stack_addr, &stack_size); >> - if (ret) >> - { >> - printf ("before execstack: pthread_attr_getstack returned error: %s\n", >> - strerror (ret)); >> - return 1; >> - } >> + TEST_VERIFY_EXIT (pthread_getattr_np (me, &attr) == 0); >> + TEST_VERIFY_EXIT (pthread_attr_getstack (&attr, &old_stack_addr, >> + &stack_size) == 0); >> # if _STACK_GROWS_DOWN >> old_stack_addr += stack_size; >> # else >> @@ -146,39 +124,22 @@ do_test (void) >> if (h == NULL) >> { >> printf ("cannot load: %s\n", dlerror ()); >> - return allow_execstack; >> + return kernel_allow_execstack >> + ? (glibc_allow_execstack ? 1 : 0) >> + : 0; >> } >> >> - f = dlsym (h, "tryme"); >> - if (f == NULL) >> - { >> - printf ("symbol not found: %s\n", dlerror ()); >> - return 1; >> - } >> + f = xdlsym (h, "tryme"); >> >> /* Test if that really made our stack executable. >> The `tryme' function should crash if not. */ >> >> (*((void (*) (void)) f)) (); >> >> - print_maps (); >> - >> #if USE_PTHREADS >> - ret = pthread_getattr_np (me, &attr); >> - if (ret) >> - { >> - printf ("after execstack: pthread_getattr_np returned error: %s\n", >> - strerror (ret)); >> - return 1; >> - } >> - >> - ret = pthread_attr_getstack (&attr, &new_stack_addr, &stack_size); >> - if (ret) >> - { >> - printf ("after execstack: pthread_attr_getstack returned error: %s\n", >> - strerror (ret)); >> - return 1; >> - } >> + TEST_VERIFY_EXIT (pthread_getattr_np (me, &attr) == 0); >> + TEST_VERIFY_EXIT (pthread_attr_getstack (&attr, &new_stack_addr, >> + &stack_size) == 0); >> >> # if _STACK_GROWS_DOWN >> new_stack_addr += stack_size; >> @@ -206,26 +167,19 @@ do_test (void) >> /* Test that growing the stack region gets new executable pages too. */ >> deeper ((void (*) (void)) f); >> >> - print_maps (); >> - >> #if USE_PTHREADS >> /* Test that a fresh thread now gets an executable stack. */ >> - { >> - pthread_t th; >> - int rc = pthread_create (&th, NULL, &tryme_thread, f); >> - if (rc) >> - error (1, rc, "pthread_create"); >> - } >> + xpthread_create (NULL, &tryme_thread, f); >> >> puts ("threads go"); >> /* The existing threads' stacks should have been changed. >> Let them run to test it. */ >> - pthread_barrier_wait (&go_barrier); >> + xpthread_barrier_wait (&go_barrier); >> >> - pthread_exit ((void *) (long int) (! allow_execstack)); >> + pthread_exit ((void *) (long int) (! kernel_allow_execstack)); >> #endif >> >> - return ! allow_execstack; >> + return ! kernel_allow_execstack; >> } >> >> static void >> diff --git a/elf/tst-ro-dynamic-mod.map b/elf/tst-ro-dynamic-mod.map >> index 2fe4a2998c..b68b036d3a 100644 >> --- a/elf/tst-ro-dynamic-mod.map >> +++ b/elf/tst-ro-dynamic-mod.map >> @@ -4,7 +4,7 @@ SECTIONS >> .dynamic : { *(.dynamic) } :text :dynamic >> .rodata : { *(.data*) *(.bss*) } :text >> /DISCARD/ : { >> - *(.note.gnu.property) >> + *(.note.gnu.property) *(.note.GNU-stack) >> } >> .note : { *(.note.*) } :text :note >> } >> @@ -13,4 +13,5 @@ PHDRS >> text PT_LOAD FLAGS(5) FILEHDR PHDRS; >> dynamic PT_DYNAMIC FLAGS(4); >> note PT_NOTE FLAGS(4); >> + gnu_stack PT_GNU_STACK FLAGS(6); >> } >> diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp >> index db0e1c86e9..2ac06e083a 100644 >> --- a/elf/tst-rtld-list-tunables.exp >> +++ b/elf/tst-rtld-list-tunables.exp >> @@ -14,4 +14,5 @@ glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0x[f]+) >> glibc.rtld.dynamic_sort: 2 (min: 1, max: 2) >> glibc.rtld.enable_secure: 0 (min: 0, max: 1) >> glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10) >> +glibc.rtld.noexecstack: 1 (min: 0, max: 1) >> glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+) >> diff --git a/manual/tunables.texi b/manual/tunables.texi >> index 8dd02d8149..6fcf0fb451 100644 >> --- a/manual/tunables.texi >> +++ b/manual/tunables.texi >> @@ -356,6 +356,25 @@ tests for @code{AT_SECURE} programs and not meant to be a security feature. >> The default value of this tunable is @samp{0}. >> @end deftp >> >> +@deftp Tunable glibc.rtld.noexecstack >> +Initially, @theglibc{} will use either the default architecture flags (that might >> +contain the executable bit) or the value of @code{PT_GNU_STACK} (if present). >> +If any shared library dependency or dynamic object loaded with @code{dlopen} >> +or @code{dlmopen} requires an executable stack (either by the default flags >> +or @code{PT_GNU_STACK} from the library) @theglibc{} will try to change the >> +permission of the stack to enable execution for all running threads. >> + >> +The @code{glibc.rtld.noexecstack} tunable allows the user to control whether >> +to control executable stacks from the main program, dependencies, or from >> +dynamically loaded libraries. Setting its value to @code{0} allows executable >> +stacks, where @code{1} disables it. The default value is @code{1}. >> + >> +When executable stacks are not allowed, and if the main program or dependencies >> +require an executable stack, the loader will fail with an error message. Trying >> +to load a dynamic shared library with @code{dlopen} or @code{dlmopen} will fail, >> +with a proper message that can be obtained with @code{dlerror}. >> +@end deftp >> + >> @node Elision Tunables >> @section Elision Tunables >> @cindex elision tunables >> diff --git a/nptl/Makefile b/nptl/Makefile >> index b3f8af2e1c..e504d43e22 100644 >> --- a/nptl/Makefile >> +++ b/nptl/Makefile >> @@ -468,6 +468,7 @@ tests-internal += \ >> # tests-internal >> ifeq ($(have-z-execstack),yes) >> tests += tst-execstack-threads >> +tests-special += $(objpfx)tst-execstack-threads-default.out >> endif >> endif >> >> @@ -675,6 +676,12 @@ $(objpfx)tst-execstack-threads.out: $(objpfx)tst-execstack-threads-mod.so >> LDFLAGS-tst-execstack-threads = -Wl,-z,noexecstack >> LDFLAGS-tst-execstack-threads-mod.so = -Wl,-z,execstack >> CFLAGS-tst-execstack-threads-mod.c += -Wno-trampolines >> +tst-execstack-threads-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ >> + ALLOW_EXECSTACK=1 >> + >> +$(objpfx)tst-execstack-threads-default.out: $(objpfx)tst-execstack-threads >> + $(test-wrapper-env) $(run-program-env) $< > $@; \ >> + $(evaluate-test) >> >> tst-stackguard1-ARGS = --command "$(host-test-program-cmd) --child" >> tst-stackguard1-static-ARGS = --command "$(objpfx)tst-stackguard1-static --child" >> -- >> 2.43.0 >> > > H.J.
On 13/06/24 02:55, Florian Weimer wrote: > * Adhemerval Zanella: > >> +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable >> + stacks from either main program, dependencies, or dynamically loadeed >> + libraries. The default is to disable executable stacks. > > Typo: “load[e]d” > > Should we refuse to enable executable stack on dlopen by default? That's the idea of this change. > And > only enable it if the initial set of objects request executable stack? No, I think we should only allow it if user explicit enables it with the tunable. Non-executable stack should not be allowed by silent dlopen a bogus shared library.
* Adhemerval Zanella Netto: > On 13/06/24 02:55, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable >>> + stacks from either main program, dependencies, or dynamically loadeed >>> + libraries. The default is to disable executable stacks. >> >> Typo: “load[e]d” >> >> Should we refuse to enable executable stack on dlopen by default? > > That's the idea of this change. > >> And >> only enable it if the initial set of objects request executable stack? > > No, I think we should only allow it if user explicit enables it with the > tunable. Non-executable stack should not be allowed by silent dlopen a > bogus shared library. I think dlopen and initial executable are different scenarios. It's much easier to disable dlopen support by default if we keep the initial execstack support present. Thanks, Florian
On Thu, Jun 13, 2024, at 10:44 AM, Florian Weimer wrote: > * Adhemerval Zanella Netto: >> On 13/06/24 02:55, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable >>>> + stacks from either main program, dependencies, or dynamically loadeed >>>> + libraries. The default is to disable executable stacks. >>> >>> Typo: “load[e]d” >>> >>> Should we refuse to enable executable stack on dlopen by default? >> >> That's the idea of this change. >> >>> And >>> only enable it if the initial set of objects request executable stack? >> >> No, I think we should only allow it if user explicit enables it with the >> tunable. Non-executable stack should not be allowed by silent dlopen a >> bogus shared library. > > I think dlopen and initial executable are different scenarios. It's > much easier to disable dlopen support by default if we keep the initial > execstack support present. What if, instead of a boolean, 'glibc.rtld.execstack' can take these values: - allow: current behavior (governed by .note.GNU-stack) - no-suid: stack is always non-executable in suid binaries, still governed by .note.GNU-stack for others - no-dlopen: .note.GNU-stack is ignored for dlopen() but still honored for initial set of objects - no-suid,no-dlopen / no-dlopen,no-suid: combine effects of no-suid and no-dlopen - forbid: stack is never executable with the default for 2.40 being no-suid,no-dlopen, and we declare our intention to bump it to 'forbid' soon, and to drop support for 'allow' and 'no-suid' without 'no-dlopen' eventually. zw
On 13/06/24 11:58, Zack Weinberg wrote: > On Thu, Jun 13, 2024, at 10:44 AM, Florian Weimer wrote: >> * Adhemerval Zanella Netto: >>> On 13/06/24 02:55, Florian Weimer wrote: >>>> * Adhemerval Zanella: >>>> >>>>> +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable >>>>> + stacks from either main program, dependencies, or dynamically loadeed >>>>> + libraries. The default is to disable executable stacks. >>>> >>>> Typo: “load[e]d” >>>> >>>> Should we refuse to enable executable stack on dlopen by default? >>> >>> That's the idea of this change. >>> >>>> And >>>> only enable it if the initial set of objects request executable stack? >>> >>> No, I think we should only allow it if user explicit enables it with the >>> tunable. Non-executable stack should not be allowed by silent dlopen a >>> bogus shared library. >> >> I think dlopen and initial executable are different scenarios. It's >> much easier to disable dlopen support by default if we keep the initial >> execstack support present. I don't complete agree, in both cases is a permanent transition to a executable stack. I had been almost 20 years since GNU_PT_STACK was added to make nx-stack the default, I *really* think it is time we really enforce this on glibc side. > > What if, instead of a boolean, 'glibc.rtld.execstack' can take these values: > > - allow: current behavior (governed by .note.GNU-stack) > - no-suid: stack is always non-executable in suid binaries, > still governed by .note.GNU-stack for others > - no-dlopen: .note.GNU-stack is ignored for dlopen() but > still honored for initial set of objects > - no-suid,no-dlopen / no-dlopen,no-suid: > combine effects of no-suid and no-dlopen > - forbid: stack is never executable > > with the default for 2.40 being no-suid,no-dlopen, and we declare > our intention to bump it to 'forbid' soon, and to drop support for > 'allow' and 'no-suid' without 'no-dlopen' eventually. The no-suid does not make much sense now that we filter out GLIBC_TUNABLES for setuid binaries, so it would make sense three values: allow, no-dlopen, forbid. But I really do not see any value in allowing dlopen: it means that programs starts with a security premise that can be statically checked, but it can at any moment blow up if you have bad-configured module without nx-stack.
* Adhemerval Zanella Netto: > I don't complete agree, in both cases is a permanent transition to a > executable stack. I had been almost 20 years since GNU_PT_STACK was > added to make nx-stack the default, I *really* think it is time we > really enforce this on glibc side. Ehh, there are still valid use cases for executable stack. I'd feel much more comfortable with getting rid of that nasty dlopen support code. I expect that most of the deliberate uses of executable stack can already be detected when loading the main program. It's also easy to scan all installed shared objects to make sure that they do not require an executable stack. Thanks, Florian
On 14/06/24 07:50, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> I don't complete agree, in both cases is a permanent transition to a >> executable stack. I had been almost 20 years since GNU_PT_STACK was >> added to make nx-stack the default, I *really* think it is time we >> really enforce this on glibc side. > > Ehh, there are still valid use cases for executable stack. I'd feel > much more comfortable with getting rid of that nasty dlopen support > code. I expect that most of the deliberate uses of executable stack can > already be detected when loading the main program. > > It's also easy to scan all installed shared objects to make sure that > they do not require an executable stack. Binutils is now warning of the need of executable stacks [1] and gcc nested function trampolines can now be heap based [2] (besides being a gcc-only extension). You even have sent a patch [3] with the recommendation that "Ensure that all assembler or object input files have the recommended security markup, particularly for non-executable stack.", so it is surprises me the reluctance of making non-executable the default behavior. Besides, if user really wants and requires an executable stack he can just disable noexecstack with the tunable. [1] https://www.redhat.com/en/blog/linkers-warnings-about-executable-stacks-and-segments [2] https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=28d8c680aaea46137170fef2bd1c6a98301518dc [3] https://patchwork.sourceware.org/project/glibc/patch/87tthyfirn.fsf@oldenburg.str.redhat.com/
* Adhemerval Zanella Netto: > On 14/06/24 07:50, Florian Weimer wrote: >> * Adhemerval Zanella Netto: >> >>> I don't complete agree, in both cases is a permanent transition to a >>> executable stack. I had been almost 20 years since GNU_PT_STACK was >>> added to make nx-stack the default, I *really* think it is time we >>> really enforce this on glibc side. >> >> Ehh, there are still valid use cases for executable stack. I'd feel >> much more comfortable with getting rid of that nasty dlopen support >> code. I expect that most of the deliberate uses of executable stack can >> already be detected when loading the main program. >> >> It's also easy to scan all installed shared objects to make sure that >> they do not require an executable stack. > > Binutils is now warning of the need of executable stacks [1] and gcc nested > function trampolines can now be heap based [2] (besides being a gcc-only > extension). You even have sent a patch [3] with the recommendation that > "Ensure that all assembler or object input files have the recommended security > markup, particularly for non-executable stack.", so it is surprises me > the reluctance of making non-executable the default behavior. It is already the default. Just because we can auto-tune away does not change that. If we disable dlopen support by default (using a flag that is protected by RELRO), I don't think there is any additional risk from supporting legacy binaries which require executable stack. There is currently this trend to ban obsolete features and algorithms even if there is working automated negotiation that avoids their use by default. I think this is a mistake. With dynamic linking, we always have stuff in the binary even if its unused (think the gets function), and we should push back against labeling this as a problem. It wouldn't make sense to disable the gets function by default … Thanks, Florian
On 14/06/24 10:10, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 14/06/24 07:50, Florian Weimer wrote: >>> * Adhemerval Zanella Netto: >>> >>>> I don't complete agree, in both cases is a permanent transition to a >>>> executable stack. I had been almost 20 years since GNU_PT_STACK was >>>> added to make nx-stack the default, I *really* think it is time we >>>> really enforce this on glibc side. >>> >>> Ehh, there are still valid use cases for executable stack. I'd feel >>> much more comfortable with getting rid of that nasty dlopen support >>> code. I expect that most of the deliberate uses of executable stack can >>> already be detected when loading the main program. >>> >>> It's also easy to scan all installed shared objects to make sure that >>> they do not require an executable stack. >> >> Binutils is now warning of the need of executable stacks [1] and gcc nested >> function trampolines can now be heap based [2] (besides being a gcc-only >> extension). You even have sent a patch [3] with the recommendation that >> "Ensure that all assembler or object input files have the recommended security >> markup, particularly for non-executable stack.", so it is surprises me >> the reluctance of making non-executable the default behavior. > > It is already the default. Just because we can auto-tune away does not > change that. If we disable dlopen support by default (using a flag that > is protected by RELRO), I don't think there is any additional risk from > supporting legacy binaries which require executable stack. > > There is currently this trend to ban obsolete features and algorithms > even if there is working automated negotiation that avoids their use by > default. I think this is a mistake. With dynamic linking, we always > have stuff in the binary even if its unused (think the gets function), > and we should push back against labeling this as a problem. It wouldn't > make sense to disable the gets function by default … > But the idea is not to ban obsolete features, but rather use better defaults while still allowing users to ask for such features explicitly. If you check StackOverflow, for instance, there are multiple hits of users stumbling on this specific issues is where the binary is instructed to use nx-stack but for no apparent reason, the process starts to use an executable stack. And while I agree that some obsolete features are not troublesome to continue to support, I see no reason to continue defaults to executable stack especially because it has been used extensively as an attack vector. So, it is good to document good practices, but I think for this case we should also enforce and make this an opt-out security feature. I will work on Zack suggestion to layout a plan for the transition.
diff --git a/Makeconfig b/Makeconfig index 29819363da..76f5772272 100644 --- a/Makeconfig +++ b/Makeconfig @@ -356,6 +356,7 @@ endif # Must be supported by the linker. no-whole-archive = -Wl,--no-whole-archive whole-archive = -Wl,--whole-archive +noexecstack = -Wl,-z,noexecstack # Installed name of the startup code. # The ELF convention is that the startfile is called crt1.o diff --git a/NEWS b/NEWS index 20e263f581..be589cb616 100644 --- a/NEWS +++ b/NEWS @@ -36,6 +36,10 @@ Major new features: * On Linux, update epoll header to include epoll ioctl definitions and related structure added in Linux kernel 6.9. +* A new tunable, glibc.rtld.noexecstack, can be used to enable executable + stacks from either main program, dependencies, or dynamically loadeed + libraries. The default is to disable executable stacks. + Deprecated and removed features, and other changes affecting compatibility: * Architectures which use a 32-bit seconds-since-epoch field in struct diff --git a/elf/Makefile b/elf/Makefile index 57b3a19d36..56414fcbba 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -543,6 +543,15 @@ tests-execstack-yes = \ tst-execstack-needed \ tst-execstack-prog \ # tests-execstack-yes +tests-execstack-static-yes = \ + tst-execstack-prog-static \ + # tests-execstack-static-yes +tests-execstack-special-yes = \ + $(objpfx)tst-execstack-default.out \ + $(objpfx)tst-execstack-needed-default.out \ + $(objpfx)tst-execstack-prog-default.out \ + $(objpfx)tst-execstack-prog-static-default.out \ + # tests-execstack-special-yes endif ifeq ($(have-depaudit),yes) tests += \ @@ -630,6 +639,8 @@ $(objpfx)tst-rtld-does-not-exist.out: tst-rtld-does-not-exist.sh $(objpfx)ld.so $(evaluate-test) tests += $(tests-execstack-$(have-z-execstack)) +tests-static += $(tests-execstack-static-$(have-z-execstack)) +tests-special += $(tests-execstack-special-$(have-z-execstack)) ifeq ($(run-built-tests),yes) tests-special += \ $(objpfx)noload-mem.out \ @@ -1850,11 +1861,45 @@ CPPFLAGS-tst-execstack.c += -DUSE_PTHREADS=0 LDFLAGS-tst-execstack = -Wl,-z,noexecstack LDFLAGS-tst-execstack-mod.so = -Wl,-z,execstack +$(objpfx)tst-execstack-default.out: $(objpfx)tst-execstack + $(test-wrapper-env) $(run-program-env) $< > $@; \ + $(evaluate-test) + +tst-execstack-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ + ALLOW_EXECSTACK=1 + $(objpfx)tst-execstack-needed: $(objpfx)tst-execstack-mod.so LDFLAGS-tst-execstack-needed = -Wl,-z,noexecstack +tst-execstack-needed-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ + ALLOW_EXECSTACK=1 + +$(objpfx)tst-execstack-needed-default.out: $(objpfx)tst-execstack-needed + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ + grep -q 'error while loading shared libraries:.*executable stack is not allowed$$' $@ \ + && grep -q '^status: 127$$' $@; \ + $(evaluate-test) + LDFLAGS-tst-execstack-prog = -Wl,-z,execstack CFLAGS-tst-execstack-prog.c += -Wno-trampolines +tst-execstack-prog-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 + +LDFLAGS-tst-execstack-prog-static = -Wl,-z,execstack +CFLAGS-tst-execstack-prog-static.c += -Wno-trampolines +tst-execstack-prog-static-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 + +$(objpfx)tst-execstack-prog-default.out: $(objpfx)tst-execstack-prog + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ + grep -q 'Fatal glibc error: executable stack is not allowed$$' $@ \ + && grep -q '^status: 127$$' $@; \ + $(evaluate-test) + +$(objpfx)tst-execstack-prog-static-default.out: $(objpfx)tst-execstack-prog-static + $(test-wrapper-env) $(run-program-env) $< > $@ 2>&1; echo "status: $$?" >> $@; \ + grep -q 'Fatal glibc error: executable stack is not allowed$$' $@ \ + && grep -q '^status: 127$$' $@; \ + $(evaluate-test) + CFLAGS-tst-execstack-mod.c += -Wno-trampolines endif diff --git a/elf/dl-load.c b/elf/dl-load.c index 8a89b71016..414955694f 100644 --- a/elf/dl-load.c +++ b/elf/dl-load.c @@ -32,6 +32,7 @@ #include <sys/stat.h> #include <sys/types.h> #include <gnu/lib-names.h> +#include <dl-tunables.h> /* Type for the buffer we put the ELF header and hopefully the program header. This buffer does not really have to be too large. In most @@ -1297,6 +1298,13 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, if (__glibc_unlikely ((stack_flags &~ GL(dl_stack_flags)) & PF_X)) { + if (TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) + { + errstring = N_("\ +executable stack is not allowed"); + goto lose; + } + /* The stack is presently not executable, but this module requires that it be executable. */ #if PTHREAD_IN_LIBC diff --git a/elf/dl-support.c b/elf/dl-support.c index 451932dd03..084e2579b1 100644 --- a/elf/dl-support.c +++ b/elf/dl-support.c @@ -45,6 +45,7 @@ #include <dl-find_object.h> #include <array_length.h> #include <dl-symbol-redir-ifunc.h> +#include <dl-tunables.h> extern char *__progname; char **_dl_argv = &__progname; /* This is checked for some error messages. */ @@ -335,6 +336,10 @@ _dl_non_dynamic_init (void) break; } + if ((__glibc_unlikely (GL(dl_stack_flags)) & PF_X) + && TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) + _dl_fatal_printf ("Fatal glibc error: executable stack is not allowed\n"); + call_function_static_weak (_dl_find_object_init); /* Setup relro on the binary itself. */ diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list index 1186272c81..745c9ce2c1 100644 --- a/elf/dl-tunables.list +++ b/elf/dl-tunables.list @@ -142,6 +142,12 @@ glibc { maxval: 1 default: 0 } + noexecstack { + type: INT_32 + minval: 0 + maxval: 1 + default: 1 + } } mem { diff --git a/elf/rtld.c b/elf/rtld.c index e9525ea987..0762d68d68 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -1668,6 +1668,10 @@ dl_main (const ElfW(Phdr) *phdr, bool has_interp = rtld_setup_main_map (main_map); + if ((__glibc_unlikely (GL(dl_stack_flags)) & PF_X) + && TUNABLE_GET (glibc, rtld, noexecstack, int32_t, NULL) == 1) + _dl_fatal_printf ("Fatal glibc error: executable stack is not allowed\n"); + /* If the current libname is different from the SONAME, add the latter as well. */ if (GL(dl_rtld_map).l_info[DT_SONAME] != NULL diff --git a/elf/tst-execstack-prog-static.c b/elf/tst-execstack-prog-static.c new file mode 100644 index 0000000000..180657e5ef --- /dev/null +++ b/elf/tst-execstack-prog-static.c @@ -0,0 +1 @@ +#include "tst-execstack-prog.c" diff --git a/elf/tst-execstack.c b/elf/tst-execstack.c index 560b353918..56fb8bd874 100644 --- a/elf/tst-execstack.c +++ b/elf/tst-execstack.c @@ -8,17 +8,10 @@ #include <unistd.h> #include <error.h> #include <stackinfo.h> - -static void -print_maps (void) -{ -#if 0 - char *cmd = NULL; - asprintf (&cmd, "cat /proc/%d/maps", getpid ()); - system (cmd); - free (cmd); -#endif -} +#include <stdlib.h> +#include <support/check.h> +#include <support/xthread.h> +#include <support/xdlfcn.h> static void deeper (void (*f) (void)); @@ -47,7 +40,7 @@ waiter_thread (void *arg) } #endif -static bool allow_execstack = true; +static bool kernel_allow_execstack = true; static int @@ -74,61 +67,46 @@ do_test (void) { n = getline (&line, &linelen, fp); if (n > 0 && line[0] == '0') - allow_execstack = false; + kernel_allow_execstack = false; } fclose (fp); } } - printf ("executable stacks %sallowed\n", allow_execstack ? "" : "not "); + printf ("kernel allows executable stacks: %s\n", + kernel_allow_execstack ? "yes" : "not "); + + bool glibc_allow_execstack = getenv ("ALLOW_EXECSTACK") != 0; + printf ("expected allow executable stacks: %s\n", + glibc_allow_execstack ? "yes" : "not "); static void *f; /* Address of this is used in other threads. */ #if USE_PTHREADS /* Create some threads while stacks are nonexecutable. */ #define N 5 - pthread_t thr[N]; - pthread_barrier_init (&startup_barrier, NULL, N + 1); - pthread_barrier_init (&go_barrier, NULL, N + 1); + xpthread_barrier_init (&startup_barrier, NULL, N + 1); + xpthread_barrier_init (&go_barrier, NULL, N + 1); for (int i = 0; i < N; ++i) - { - int rc = pthread_create (&thr[i], NULL, &waiter_thread, &f); - if (rc) - error (1, rc, "pthread_create"); - } + xpthread_create (NULL, &waiter_thread, &f); /* Make sure they are all there using their stacks. */ - pthread_barrier_wait (&startup_barrier); + xpthread_barrier_wait (&startup_barrier); puts ("threads waiting"); #endif - print_maps (); - #if USE_PTHREADS void *old_stack_addr, *new_stack_addr; size_t stack_size; pthread_t me = pthread_self (); pthread_attr_t attr; - int ret = 0; - ret = pthread_getattr_np (me, &attr); - if (ret) - { - printf ("before execstack: pthread_getattr_np returned error: %s\n", - strerror (ret)); - return 1; - } - - ret = pthread_attr_getstack (&attr, &old_stack_addr, &stack_size); - if (ret) - { - printf ("before execstack: pthread_attr_getstack returned error: %s\n", - strerror (ret)); - return 1; - } + TEST_VERIFY_EXIT (pthread_getattr_np (me, &attr) == 0); + TEST_VERIFY_EXIT (pthread_attr_getstack (&attr, &old_stack_addr, + &stack_size) == 0); # if _STACK_GROWS_DOWN old_stack_addr += stack_size; # else @@ -146,39 +124,22 @@ do_test (void) if (h == NULL) { printf ("cannot load: %s\n", dlerror ()); - return allow_execstack; + return kernel_allow_execstack + ? (glibc_allow_execstack ? 1 : 0) + : 0; } - f = dlsym (h, "tryme"); - if (f == NULL) - { - printf ("symbol not found: %s\n", dlerror ()); - return 1; - } + f = xdlsym (h, "tryme"); /* Test if that really made our stack executable. The `tryme' function should crash if not. */ (*((void (*) (void)) f)) (); - print_maps (); - #if USE_PTHREADS - ret = pthread_getattr_np (me, &attr); - if (ret) - { - printf ("after execstack: pthread_getattr_np returned error: %s\n", - strerror (ret)); - return 1; - } - - ret = pthread_attr_getstack (&attr, &new_stack_addr, &stack_size); - if (ret) - { - printf ("after execstack: pthread_attr_getstack returned error: %s\n", - strerror (ret)); - return 1; - } + TEST_VERIFY_EXIT (pthread_getattr_np (me, &attr) == 0); + TEST_VERIFY_EXIT (pthread_attr_getstack (&attr, &new_stack_addr, + &stack_size) == 0); # if _STACK_GROWS_DOWN new_stack_addr += stack_size; @@ -206,26 +167,19 @@ do_test (void) /* Test that growing the stack region gets new executable pages too. */ deeper ((void (*) (void)) f); - print_maps (); - #if USE_PTHREADS /* Test that a fresh thread now gets an executable stack. */ - { - pthread_t th; - int rc = pthread_create (&th, NULL, &tryme_thread, f); - if (rc) - error (1, rc, "pthread_create"); - } + xpthread_create (NULL, &tryme_thread, f); puts ("threads go"); /* The existing threads' stacks should have been changed. Let them run to test it. */ - pthread_barrier_wait (&go_barrier); + xpthread_barrier_wait (&go_barrier); - pthread_exit ((void *) (long int) (! allow_execstack)); + pthread_exit ((void *) (long int) (! kernel_allow_execstack)); #endif - return ! allow_execstack; + return ! kernel_allow_execstack; } static void diff --git a/elf/tst-ro-dynamic-mod.map b/elf/tst-ro-dynamic-mod.map index 2fe4a2998c..b68b036d3a 100644 --- a/elf/tst-ro-dynamic-mod.map +++ b/elf/tst-ro-dynamic-mod.map @@ -4,7 +4,7 @@ SECTIONS .dynamic : { *(.dynamic) } :text :dynamic .rodata : { *(.data*) *(.bss*) } :text /DISCARD/ : { - *(.note.gnu.property) + *(.note.gnu.property) *(.note.GNU-stack) } .note : { *(.note.*) } :text :note } @@ -13,4 +13,5 @@ PHDRS text PT_LOAD FLAGS(5) FILEHDR PHDRS; dynamic PT_DYNAMIC FLAGS(4); note PT_NOTE FLAGS(4); + gnu_stack PT_GNU_STACK FLAGS(6); } diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp index db0e1c86e9..2ac06e083a 100644 --- a/elf/tst-rtld-list-tunables.exp +++ b/elf/tst-rtld-list-tunables.exp @@ -14,4 +14,5 @@ glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0x[f]+) glibc.rtld.dynamic_sort: 2 (min: 1, max: 2) glibc.rtld.enable_secure: 0 (min: 0, max: 1) glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10) +glibc.rtld.noexecstack: 1 (min: 0, max: 1) glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+) diff --git a/manual/tunables.texi b/manual/tunables.texi index 8dd02d8149..6fcf0fb451 100644 --- a/manual/tunables.texi +++ b/manual/tunables.texi @@ -356,6 +356,25 @@ tests for @code{AT_SECURE} programs and not meant to be a security feature. The default value of this tunable is @samp{0}. @end deftp +@deftp Tunable glibc.rtld.noexecstack +Initially, @theglibc{} will use either the default architecture flags (that might +contain the executable bit) or the value of @code{PT_GNU_STACK} (if present). +If any shared library dependency or dynamic object loaded with @code{dlopen} +or @code{dlmopen} requires an executable stack (either by the default flags +or @code{PT_GNU_STACK} from the library) @theglibc{} will try to change the +permission of the stack to enable execution for all running threads. + +The @code{glibc.rtld.noexecstack} tunable allows the user to control whether +to control executable stacks from the main program, dependencies, or from +dynamically loaded libraries. Setting its value to @code{0} allows executable +stacks, where @code{1} disables it. The default value is @code{1}. + +When executable stacks are not allowed, and if the main program or dependencies +require an executable stack, the loader will fail with an error message. Trying +to load a dynamic shared library with @code{dlopen} or @code{dlmopen} will fail, +with a proper message that can be obtained with @code{dlerror}. +@end deftp + @node Elision Tunables @section Elision Tunables @cindex elision tunables diff --git a/nptl/Makefile b/nptl/Makefile index b3f8af2e1c..e504d43e22 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -468,6 +468,7 @@ tests-internal += \ # tests-internal ifeq ($(have-z-execstack),yes) tests += tst-execstack-threads +tests-special += $(objpfx)tst-execstack-threads-default.out endif endif @@ -675,6 +676,12 @@ $(objpfx)tst-execstack-threads.out: $(objpfx)tst-execstack-threads-mod.so LDFLAGS-tst-execstack-threads = -Wl,-z,noexecstack LDFLAGS-tst-execstack-threads-mod.so = -Wl,-z,execstack CFLAGS-tst-execstack-threads-mod.c += -Wno-trampolines +tst-execstack-threads-ENV = GLIBC_TUNABLES=glibc.rtld.noexecstack=0 \ + ALLOW_EXECSTACK=1 + +$(objpfx)tst-execstack-threads-default.out: $(objpfx)tst-execstack-threads + $(test-wrapper-env) $(run-program-env) $< > $@; \ + $(evaluate-test) tst-stackguard1-ARGS = --command "$(host-test-program-cmd) --child" tst-stackguard1-static-ARGS = --command "$(objpfx)tst-stackguard1-static --child"