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

[Adhemerval Zanella Netto]

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?