[PATCH v5 2/4] elf: Do not change stack permission on dlopen/dlmopen
Adhemerval Zanella Netto
adhemerval.zanella@linaro.org
Mon Dec 2 20:35:37 GMT 2024
On 29/11/24 16:51, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> If some shared library loaded with dlopen/dlmopen requires an executable
>> stack, either implicitly because of a missing GNU_STACK ELF header
>> (where the ABI default flags implies in the executable bit) or explicitly
>> because of the executable bit from GNU_STACK; the loader will try to set
>> the both the main thread and all thread stacks (from the pthread cache)
>> as executable.
>>
>> Besides the issue where any __nptl_change_stack_perm failure does not
>> undo the previous executable transition (meaning that if the library
>> fails to load, there can be thread stacks with executable stacks), this
>> behavior was used on recent CVE [1] as a vector for RCE.
>
>> [1] https://www.qualys.com/2023/07/19/cve-2023-38408/rce-openssh-forwarded-ssh-agent.txt
>
> It's no longer recent, I would say.
>
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index e986d7faab..f525eec662 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -1282,12 +1282,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))
>> {
>> /* The stack is presently not executable, but this module
>> - requires that it be executable. */
>> -#if PTHREAD_IN_LIBC
>> - errval = _dl_make_stacks_executable (stack_endp);
>> -#else
>> - errval = (*GL(dl_make_stack_executable_hook)) (stack_endp);
>> -#endif
>> + requires that it be executable. Only tries to change the
>> + stack protection during process startup. */
>> + if ((mode & __RTLD_DLOPEN) == 0)
>> + errval = _dl_make_stack_executable (stack_endp);
>> + else
>> + errval = EINVAL;
>> +
>> if (errval)
>> {
>> errstring = N_("\
>
> The specified error message is the same for the initial vs dlopen
> failure case, but I think that's okay because the full dlopen failure
> message will look different because it includes the DSO name.
>
I can change the error message if it would preferable, but I agree that it
should give is enough information to diagnosticate the failure.
>> diff --git a/elf/tst-execstack.c b/elf/tst-execstack.c
>> index 560b353918..cd758c089e 100644
>> --- a/elf/tst-execstack.c
>> +++ b/elf/tst-execstack.c
>> @@ -9,6 +9,11 @@
>> #include <error.h>
>> #include <stackinfo.h>
>>
>> +#include <stdlib.h>
>> +#include <support/check.h>
>> +#include <support/xthread.h>
>> +#include <support/xdlfcn.h>
>
> These cleanups seem mostly unrelated?
Right, I will split the patch with one to move the test to libsupport and
another one with the required changes for the patch itself.
>
> And why doesn't the dlopen call fail?
>
It does, the tests changes the expected results as:
185 /* Loading this module should force stacks to become executable. */
186 #if USE_PTHREADS
187 const char *soname = "tst-execstack-threads-mod.so";
188 #else
189 const char *soname = "tst-execstack-mod.so";
190 #endif
191 void *h = dlopen (soname, RTLD_LAZY);
192 #if !DEFAULT_RWX_STACK
193 TEST_VERIFY_EXIT (h == NULL);
194 #else
195 TEST_VERIFY_EXIT (h != NULL);
[...]
244 return ! allow_execstack;
So for DEFAULT_RWX_STACK (the default), the test expects that dlopen fails.
>
>> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
>> index 91447a5e77..b897da7e7b 100644
>> --- a/sysdeps/generic/ldsodefs.h
>> +++ b/sysdeps/generic/ldsodefs.h
>> @@ -413,13 +413,6 @@ struct rtld_global
>> #endif
>> #include <dl-procruntime.c>
>>
>> -#if !PTHREAD_IN_LIBC
>> - /* If loading a shared object requires that we make the stack executable
>> - when it was not, we do it by calling this function.
>> - It returns an errno code or zero on success. */
>> - EXTERN int (*_dl_make_stack_executable_hook) (void **);
>> -#endif
>> -
>
> Huh. Hurd doesn't even use that, presumably because stacks are always
> executable?
No idea of default Hurd ABI, but I would guess it is default to executable since
the glibc build sets GNU_STACK to RWE.
>
>> diff --git a/sysdeps/unix/sysv/linux/dl-execstack.c b/sysdeps/unix/sysv/linux/dl-execstack.c
>> index b986898598..68db6737f0 100644
>> --- a/sysdeps/unix/sysv/linux/dl-execstack.c
>> +++ b/sysdeps/unix/sysv/linux/dl-execstack.c
>
>> -static int
>> -make_main_stack_executable (void **stack_endp)
>> +int
>> +_dl_make_stack_executable (void **stack_endp)
>> {
>> /* This gives us the highest/lowest page that needs to be changed. */
>> uintptr_t page = ((uintptr_t) *stack_endp
>> @@ -52,57 +43,3 @@ make_main_stack_executable (void **stack_endp)
>>
>> return 0;
>> }
>
> Okay. We only need this for the main thread because pthread_create
> creates new stacks directly with the right permissions.
Yes, using GL(dl_stack_flags) that should be set only during loading
(maybe move it to GLRO, not sure if we can).
>
>
>> diff --git a/sysdeps/unix/sysv/linux/mips/Makefile b/sysdeps/unix/sysv/linux/mips/Makefile
>> index d5725c69d8..05ec9150b2 100644
>> --- a/sysdeps/unix/sysv/linux/mips/Makefile
>> +++ b/sysdeps/unix/sysv/linux/mips/Makefile
>> @@ -61,6 +61,7 @@ ifeq ($(subdir),elf)
>> # this test is expected to fail.
>> ifneq ($(mips-has-gnustack),yes)
>> test-xfail-check-execstack = yes
>> +CFLAGS-tst-execstack.c += -DDEFAULT_RWX_STACK=1
>> endif
>> endif
>
> Is the xfail still needed?
Without --enable-kernel=4.8, the mips seems to require an executable stack for
some fp emulation:
sysdeps/unix/sysv/linux/mips/configure.ac
138 # Check if we are supposed to run on kernels older than 4.8.0. If so,
139 # force executable stack to avoid potential runtime problems with fpu
140 # emulation.
And kernel seems to still enforce it some, even on some recent kernels (6.3.0-2-5kc-malta):
$ readelf -lW elf/tst-execstack | grep GNU_STACK
GNU_STACK 0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x000000 RW 0x10
$ gdb --args ./elf/tst-execstack --direct
[...]
(gdb) b __start
(gdb) r
[...]
Breakpoint 2, __start () at ../sysdeps/mips/start.
S:81
81 SETUP_GPX64($25,$0)
(gdb) info proc
process 792
$ cat /proc/792/maps | grep "\[stack\]"
fffffdc000-ffffffd000 rwxp 00000000 00:00 0 [stack]
I think once we move to a minimal kernel of at least 4.8 for mips we can clean this up.
>
>>
>> @@ -68,6 +69,12 @@ ifeq ($(subdir),stdlib)
>> gen-as-const-headers += ucontext_i.sym
>> endif
>>
>> +ifeq ($(subdir),nptl)
>> +ifeq ($(mips-force-execstack),yes)
>> +CFLAGS-tst-execstack-threads.c += -DDEFAULT_RWX_STACK=1
>> +endif
>> +endif
>> +
>> ifeq ($(mips-force-execstack),yes)
>> CFLAGS-.o += -Wa,-execstack
>> CFLAGS-.os += -Wa,-execstack
>
> Why is setting DEFAULT_RWX_STACK only needed on MIPS, but not on i386?
> Does it reflect the combined kernel/toolchain default?
Yes, and DEFAULT_RWX_STACK is only needed on mips because during
_dl_map_object_from_fd mips without $(mips-force-execstack) will have
GL(dl_stack_flags) equal to PF_X|PF_W|PF_R (due the -Wa,-execstack);
different than i686 (where with a recent toolchain by default it will
be just PF_W|PF_R).
More information about the Libc-alpha
mailing list