[PATCH v5 4/4] elf: Add glibc.rtld.execstack

[Adhemerval Zanella Netto]

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.