| Message ID | 20241128173851.1920696-5-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve executable stack handling | expand |
* Adhemerval Zanella: > diff --git a/elf/Makefile b/elf/Makefile > index bdd169f4e8..6dd15c63d9 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -556,6 +556,13 @@ tests-execstack-yes = \ > tests-execstack-static-yes = \ > tst-execstack-prog-static > # tests-execstack-static-yes > +ifeq (yes,$(run-built-tests)) > +tests-execstack-special-yes = \ > + $(objpfx)tst-execstack-needed-noexecstack.out \ > + $(objpfx)tst-execstack-prog-noexecstack.out \ > + $(objpfx)tst-execstack-prog-static-noexecstack.out \ > + # tests-execstack-special-yes > +endif # $(run-built-tests) > endif > ifeq ($(have-depaudit),yes) > tests += \ > @@ -651,6 +658,7 @@ $(objpfx)tst-rtld-dash-dash.out: tst-rtld-dash-dash.sh $(objpfx)ld.so > > tests += $(tests-execstack-$(have-z-execstack)) > tests-static+= $(tests-execstack-static-$(have-z-execstack)) > +tests-special += $(tests-execstack-special-$(have-z-execstack)) Should this be guarded by $(run-built-tests)? > diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp > index db0e1c86e9..9f5990f340 100644 > --- a/elf/tst-rtld-list-tunables.exp > +++ b/elf/tst-rtld-list-tunables.exp > @@ -13,5 +13,6 @@ glibc.malloc.top_pad: 0x20000 (min: 0x0, max: 0x[f]+) > 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.execstack: 1 (min: 0, max: 1) > glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10) > glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+) > diff --git a/manual/tunables.texi b/manual/tunables.texi > index 0b1b2898c0..c3e894f4fe 100644 > --- a/manual/tunables.texi > +++ b/manual/tunables.texi > @@ -355,6 +355,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.execstack > +@theglibc{} will use either the default architecture ABI flags (that might > +contain the executable bit) or the value of @code{PT_GNU_STACK} (if present) > +to define whether to mark the stack non-executable, and if the program or > +any shared library dependency require an executable stack the loader will > +change the main stack permission if kernel starts with a non executable stack. > + > +The @code{glibc.rtld.execstack} tunable allows the user to control how > +to proceed regarding the stack execution bit. Setting its value to @code{0} > +disables executable stacks, where @code{1} enables it. The default value > +is @code{1}. This is ambiguous because 1 enables automatic handling. Executable stacks will still be disabled in most cases. > +When executable stacks are not allowed, and if the main program requires an > +executable stack, the loader will fail with an error message. > +@strong{NB:} Trying to load a dynamic shared library with @code{dlopen} or > +@code{dlmopen} that requires an executable stack will always fail if the > +default flags does not contain the executable bit. > +@end deftp This paragraph seems to have some sort of formatting glitch. Could we get a couple more modes? fully disabled, load/dlopen will fail fully disabled, but load/dlopen will proceed (useful with setarch -X) auto-enable, dlopen might fail (the default) auto-enable, dlopen will silently proceed instead of failing always enabled (useful for legacy applications that need dlopen) Thanks, Florian
On 29/11/24 17:01, Florian Weimer wrote: > * Adhemerval Zanella: > >> diff --git a/elf/Makefile b/elf/Makefile >> index bdd169f4e8..6dd15c63d9 100644 >> --- a/elf/Makefile >> +++ b/elf/Makefile >> @@ -556,6 +556,13 @@ tests-execstack-yes = \ >> tests-execstack-static-yes = \ >> tst-execstack-prog-static >> # tests-execstack-static-yes >> +ifeq (yes,$(run-built-tests)) >> +tests-execstack-special-yes = \ >> + $(objpfx)tst-execstack-needed-noexecstack.out \ >> + $(objpfx)tst-execstack-prog-noexecstack.out \ >> + $(objpfx)tst-execstack-prog-static-noexecstack.out \ >> + # tests-execstack-special-yes >> +endif # $(run-built-tests) >> endif >> ifeq ($(have-depaudit),yes) >> tests += \ >> @@ -651,6 +658,7 @@ $(objpfx)tst-rtld-dash-dash.out: tst-rtld-dash-dash.sh $(objpfx)ld.so >> >> tests += $(tests-execstack-$(have-z-execstack)) >> tests-static+= $(tests-execstack-static-$(have-z-execstack)) >> +tests-special += $(tests-execstack-special-$(have-z-execstack)) > > Should this be guarded by $(run-built-tests)? I don't think it is necessary, since tests-execstack-special-yes will only be non-empty if $(run-built-tests) is 'yes' (in fact I fixed it from v4, where I saw the tests were failing on Hurd due the missing $(run-built-tests) check). > >> diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp >> index db0e1c86e9..9f5990f340 100644 >> --- a/elf/tst-rtld-list-tunables.exp >> +++ b/elf/tst-rtld-list-tunables.exp >> @@ -13,5 +13,6 @@ glibc.malloc.top_pad: 0x20000 (min: 0x0, max: 0x[f]+) >> 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.execstack: 1 (min: 0, max: 1) >> glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10) >> glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+) >> diff --git a/manual/tunables.texi b/manual/tunables.texi >> index 0b1b2898c0..c3e894f4fe 100644 >> --- a/manual/tunables.texi >> +++ b/manual/tunables.texi >> @@ -355,6 +355,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.execstack >> +@theglibc{} will use either the default architecture ABI flags (that might >> +contain the executable bit) or the value of @code{PT_GNU_STACK} (if present) >> +to define whether to mark the stack non-executable, and if the program or >> +any shared library dependency require an executable stack the loader will >> +change the main stack permission if kernel starts with a non executable stack. >> + >> +The @code{glibc.rtld.execstack} tunable allows the user to control how >> +to proceed regarding the stack execution bit. Setting its value to @code{0} >> +disables executable stacks, where @code{1} enables it. The default value >> +is @code{1}. > > This is ambiguous because 1 enables automatic handling. Executable > stacks will still be disabled in most cases. I think we can phrase this better: The @code{glibc.rtld.execstack} tunable allows the user to control how to proceed regarding the stack execution support. Setting its value to @code{0} disables executable stacks, even if the program or direct dependencies require it. The value @code{1} will follow the program and dependencies requirement, where glibc might turn the initial and subsequent thread stack executable, if required. The default value is @code{1}. > >> +When executable stacks are not allowed, and if the main program requires an >> +executable stack, the loader will fail with an error message. >> +@strong{NB:} Trying to load a dynamic shared library with @code{dlopen} or >> +@code{dlmopen} that requires an executable stack will always fail if the >> +default flags does not contain the executable bit. >> +@end deftp > > This paragraph seems to have some sort of formatting glitch. Hum, which kind of glitch? On libc.pdf I see that 'NB:' is correctly rendered on same line of the 'When executable stack...'. > > Could we get a couple more modes? > > fully disabled, load/dlopen will fail > fully disabled, but load/dlopen will proceed (useful with setarch -X) > auto-enable, dlopen might fail (the default) > auto-enable, dlopen will silently proceed instead of failing > always enabled (useful for legacy applications that need dlopen) I am not sure if all these modes do make sense: 1. 'fully disabled, but load/dlopen will proceed (useful with setarch -X)' - means that either we will need to reinstate the code to make the thread executable again, or loading a module that requires executable stack without actually making all the thread stack executable. 2. 'auto-enable, dlopen will silently proceed instead of failing' - same for 1, it will put the program in an inconsistent wrt stack execution requirements. 3. 'always enabled (useful for legacy applications that need dlopen)' - same for 1 wrt reinstate code. I really don't think it would be good to way to just override the security hardening this patchset it aiming to fix with a tunable. I think if a program or any of its dependencies do require executable stack, it should be well defined during *program loading*, which can be easily verifiable without the need to run the program.
On 03/12/24 14:25, Adhemerval Zanella Netto wrote: > > > On 29/11/24 17:01, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> diff --git a/elf/Makefile b/elf/Makefile >>> index bdd169f4e8..6dd15c63d9 100644 >>> --- a/elf/Makefile >>> +++ b/elf/Makefile >>> @@ -556,6 +556,13 @@ tests-execstack-yes = \ >>> tests-execstack-static-yes = \ >>> tst-execstack-prog-static >>> # tests-execstack-static-yes >>> +ifeq (yes,$(run-built-tests)) >>> +tests-execstack-special-yes = \ >>> + $(objpfx)tst-execstack-needed-noexecstack.out \ >>> + $(objpfx)tst-execstack-prog-noexecstack.out \ >>> + $(objpfx)tst-execstack-prog-static-noexecstack.out \ >>> + # tests-execstack-special-yes >>> +endif # $(run-built-tests) >>> endif >>> ifeq ($(have-depaudit),yes) >>> tests += \ >>> @@ -651,6 +658,7 @@ $(objpfx)tst-rtld-dash-dash.out: tst-rtld-dash-dash.sh $(objpfx)ld.so >>> >>> tests += $(tests-execstack-$(have-z-execstack)) >>> tests-static+= $(tests-execstack-static-$(have-z-execstack)) >>> +tests-special += $(tests-execstack-special-$(have-z-execstack)) >> >> Should this be guarded by $(run-built-tests)? > > I don't think it is necessary, since tests-execstack-special-yes will only be non-empty > if $(run-built-tests) is 'yes' (in fact I fixed it from v4, where I saw the tests were > failing on Hurd due the missing $(run-built-tests) check). > >> >>> diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp >>> index db0e1c86e9..9f5990f340 100644 >>> --- a/elf/tst-rtld-list-tunables.exp >>> +++ b/elf/tst-rtld-list-tunables.exp >>> @@ -13,5 +13,6 @@ glibc.malloc.top_pad: 0x20000 (min: 0x0, max: 0x[f]+) >>> 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.execstack: 1 (min: 0, max: 1) >>> glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10) >>> glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+) >>> diff --git a/manual/tunables.texi b/manual/tunables.texi >>> index 0b1b2898c0..c3e894f4fe 100644 >>> --- a/manual/tunables.texi >>> +++ b/manual/tunables.texi >>> @@ -355,6 +355,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.execstack >>> +@theglibc{} will use either the default architecture ABI flags (that might >>> +contain the executable bit) or the value of @code{PT_GNU_STACK} (if present) >>> +to define whether to mark the stack non-executable, and if the program or >>> +any shared library dependency require an executable stack the loader will >>> +change the main stack permission if kernel starts with a non executable stack. >>> + >>> +The @code{glibc.rtld.execstack} tunable allows the user to control how >>> +to proceed regarding the stack execution bit. Setting its value to @code{0} >>> +disables executable stacks, where @code{1} enables it. The default value >>> +is @code{1}. >> >> This is ambiguous because 1 enables automatic handling. Executable >> stacks will still be disabled in most cases. > > I think we can phrase this better: > > The @code{glibc.rtld.execstack} tunable allows the user to control how > to proceed regarding the stack execution support. Setting its value to @code{0} > disables executable stacks, even if the program or direct dependencies require > it. The value @code{1} will follow the program and dependencies requirement, > where glibc might turn the initial and subsequent thread stack executable, > if required. The default value is @code{1}. > >> >>> +When executable stacks are not allowed, and if the main program requires an >>> +executable stack, the loader will fail with an error message. >>> +@strong{NB:} Trying to load a dynamic shared library with @code{dlopen} or >>> +@code{dlmopen} that requires an executable stack will always fail if the >>> +default flags does not contain the executable bit. >>> +@end deftp >> >> This paragraph seems to have some sort of formatting glitch. > > Hum, which kind of glitch? On libc.pdf I see that 'NB:' is correctly rendered > on same line of the 'When executable stack...'. > >> >> Could we get a couple more modes? >> >> fully disabled, load/dlopen will fail >> fully disabled, but load/dlopen will proceed (useful with setarch -X) >> auto-enable, dlopen might fail (the default) >> auto-enable, dlopen will silently proceed instead of failing >> always enabled (useful for legacy applications that need dlopen) > > I am not sure if all these modes do make sense: > > 1. 'fully disabled, but load/dlopen will proceed (useful with setarch -X)' - > means that either we will need to reinstate the code to make the thread > executable again, or loading a module that requires executable stack > without actually making all the thread stack executable. > > 2. 'auto-enable, dlopen will silently proceed instead of failing' - > same for 1, it will put the program in an inconsistent wrt stack > execution requirements. > > 3. 'always enabled (useful for legacy applications that need dlopen)' - > same for 1 wrt reinstate code. > > I really don't think it would be good to way to just override the security > hardening this patchset it aiming to fix with a tunable. I think > if a program or any of its dependencies do require executable stack, > it should be well defined during *program loading*, which can be easily > verifiable without the need to run the program. > > Florian, do you think this patch still required additional work?
diff --git a/NEWS b/NEWS index 8cb5597631..c2e8bf4b34 100644 --- a/NEWS +++ b/NEWS @@ -38,6 +38,11 @@ Major new features: liable to change. Features from C2Y are also enabled by _GNU_SOURCE, or by compiling with "gcc -std=gnu2y". +* A new tunable, glibc.rtld.execstack, can be used to control whether + executable stacks are allowed from main program, either implicitly due + a mising GNU_STACK ELF header or explicit explicitly because of the + executable bit in GNU_STACK. The default is to allow executable stacks. + Deprecated and removed features, and other changes affecting compatibility: * The big-endian ARC port (arceb-linux-gnu) has been removed. diff --git a/elf/Makefile b/elf/Makefile index bdd169f4e8..6dd15c63d9 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -556,6 +556,13 @@ tests-execstack-yes = \ tests-execstack-static-yes = \ tst-execstack-prog-static # tests-execstack-static-yes +ifeq (yes,$(run-built-tests)) +tests-execstack-special-yes = \ + $(objpfx)tst-execstack-needed-noexecstack.out \ + $(objpfx)tst-execstack-prog-noexecstack.out \ + $(objpfx)tst-execstack-prog-static-noexecstack.out \ + # tests-execstack-special-yes +endif # $(run-built-tests) endif ifeq ($(have-depaudit),yes) tests += \ @@ -651,6 +658,7 @@ $(objpfx)tst-rtld-dash-dash.out: tst-rtld-dash-dash.sh $(objpfx)ld.so 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)tst-ldconfig-X.out \ @@ -1923,6 +1931,42 @@ CFLAGS-tst-execstack-mod.c += -Wno-trampolines LDFLAGS-tst-execstack-prog-static = -Wl,-z,execstack CFLAGS-tst-execstack-prog-static.c += -Wno-trampolines + +ifeq (yes,$(build-hardcoded-path-in-tests)) +tst-execstack-prog-noexecstack-msg = "Fatal glibc error: executable stack is not allowed$$" +else +tst-execstack-prog-noexecstack-msg = "error while loading shared libraries:.*cannot enable executable stack as shared object requires:" +endif + +$(objpfx)tst-execstack-prog-noexecstack.out: $(objpfx)tst-execstack-prog + $(test-program-cmd-before-env) \ + $(run-program-env) \ + GLIBC_TUNABLES=glibc.rtld.execstack=0 \ + $(test-program-cmd-after-env) $< \ + > $@ 2>&1; echo "status: $$?" >> $@; \ + grep -q $(tst-execstack-prog-noexecstack-msg) $@ \ + && grep -q '^status: 127$$' $@; \ + $(evaluate-test) + +$(objpfx)tst-execstack-needed-noexecstack.out: $(objpfx)tst-execstack-needed + $(test-program-cmd-before-env) \ + $(run-program-env) \ + GLIBC_TUNABLES=glibc.rtld.execstack=0 \ + $(test-program-cmd-after-env) $< \ + > $@ 2>&1; echo "status: $$?" >> $@; \ + grep -q 'error while loading shared libraries:.*cannot enable executable stack as shared object requires:' $@ \ + && grep -q '^status: 127$$' $@; \ + $(evaluate-test) + +$(objpfx)tst-execstack-prog-static-noexecstack.out: $(objpfx)tst-execstack-prog-static + $(test-program-cmd-before-env) \ + $(run-program-env) \ + GLIBC_TUNABLES=glibc.rtld.execstack=0 \ + $< \ + > $@ 2>&1; echo "status: $$?" >> $@; \ + grep -q 'Fatal glibc error: executable stack is not allowed$$' $@ \ + && grep -q '^status: 127$$' $@; \ + $(evaluate-test) endif LDFLAGS-tst-array2 = -Wl,--no-as-needed diff --git a/elf/dl-load.c b/elf/dl-load.c index f525eec662..0e402968ac 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 @@ -1284,7 +1285,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd, /* The stack is presently not executable, but this module requires that it be executable. Only tries to change the stack protection during process startup. */ - if ((mode & __RTLD_DLOPEN) == 0) + if ((mode & __RTLD_DLOPEN) == 0 + && TUNABLE_GET (glibc, rtld, execstack, int32_t, NULL) == 1) errval = _dl_make_stack_executable (stack_endp); else errval = EINVAL; diff --git a/elf/dl-support.c b/elf/dl-support.c index fe1f8c8f6a..73fcd33892 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. */ @@ -331,6 +332,10 @@ _dl_non_dynamic_init (void) break; } + if ((__glibc_unlikely (GL(dl_stack_flags)) & PF_X) + && TUNABLE_GET (glibc, rtld, execstack, int32_t, NULL) == 0) + _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 40ac5b3776..8e656296bb 100644 --- a/elf/dl-tunables.list +++ b/elf/dl-tunables.list @@ -135,6 +135,12 @@ glibc { maxval: 1 default: 0 } + execstack { + type: INT_32 + minval: 0 + maxval: 1 + default: 1 + } } mem { diff --git a/elf/rtld.c b/elf/rtld.c index 3b232f8525..baa39b3351 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -1644,6 +1644,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, execstack, int32_t, NULL) == 0) + _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-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp index db0e1c86e9..9f5990f340 100644 --- a/elf/tst-rtld-list-tunables.exp +++ b/elf/tst-rtld-list-tunables.exp @@ -13,5 +13,6 @@ glibc.malloc.top_pad: 0x20000 (min: 0x0, max: 0x[f]+) 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.execstack: 1 (min: 0, max: 1) glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10) glibc.rtld.optional_static_tls: 0x200 (min: 0x0, max: 0x[f]+) diff --git a/manual/tunables.texi b/manual/tunables.texi index 0b1b2898c0..c3e894f4fe 100644 --- a/manual/tunables.texi +++ b/manual/tunables.texi @@ -355,6 +355,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.execstack +@theglibc{} will use either the default architecture ABI flags (that might +contain the executable bit) or the value of @code{PT_GNU_STACK} (if present) +to define whether to mark the stack non-executable, and if the program or +any shared library dependency require an executable stack the loader will +change the main stack permission if kernel starts with a non executable stack. + +The @code{glibc.rtld.execstack} tunable allows the user to control how +to proceed regarding the stack execution bit. Setting its value to @code{0} +disables executable stacks, where @code{1} enables it. The default value +is @code{1}. + +When executable stacks are not allowed, and if the main program requires an +executable stack, the loader will fail with an error message. +@strong{NB:} Trying to load a dynamic shared library with @code{dlopen} or +@code{dlmopen} that requires an executable stack will always fail if the +default flags does not contain the executable bit. +@end deftp + @node Elision Tunables @section Elision Tunables @cindex elision tunables