| Message ID | 20231017130526.2216827-13-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve loader environment variable handling | expand |
On 2023-10-17 09:05, Adhemerval Zanella wrote: > Loader does not ignore LD_PROFILE in secure-execution mode (different > than man-page states [1]), rather it uses a different path > (/var/profile) and ignore LD_PROFILE_OUTPUT. > > Allowing secure-execution profiling is already a non good security > boundary, since it enables different code paths and extra OS access by > the process. But by ignoring LD_PROFILE_OUTPUT, the resulting profile > file might also be acceded in a racy manner since the file name does not > use any process-specific information (such as pid, timing, etc.). > > Another side-effect is it forces lazy binding even on libraries that > might be with DF_BIND_NOW. > > [1] https://man7.org/linux/man-pages/man8/ld.so.8.html > --- I tend to agree. Carlos, Florian, is profiling of setuid binaries something that needs to be supported as compatibility behaviour? I'm inclined to agree with Adhemerval and just rip it out. Thanks, Sid > elf/Makefile | 3 +++ > elf/rtld.c | 8 +++----- > elf/tst-env-setuid.c | 12 +++++++++++- > 3 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/elf/Makefile b/elf/Makefile > index f1cd6e13fa..608bef46f5 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -3021,3 +3021,6 @@ $(objpfx)tst-dlclose-lazy.out: \ > $(objpfx)tst-dlclose-lazy-mod1.so $(objpfx)tst-dlclose-lazy-mod2.so > > tst-env-setuid-ARGS = -- $(host-test-program-cmd) > + > +# Reuse a module with a SONAME, to specific as the LD_PROFILE. > +$(objpfx)tst-env-setuid: $(objpfx)tst-sonamemove-runmod2.so > diff --git a/elf/rtld.c b/elf/rtld.c > index 51b6d9f326..a09cf2a9df 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -361,6 +361,7 @@ struct rtld_global_ro _rtld_global_ro attribute_relro = > ._dl_fpu_control = _FPU_DEFAULT, > ._dl_pagesize = EXEC_PAGESIZE, > ._dl_inhibit_cache = 0, > + ._dl_profile_output = "/var/tmp", > > /* Function pointers. */ > ._dl_debug_printf = _dl_debug_printf, > @@ -2534,10 +2535,6 @@ process_envvars (struct dl_main_state *state) > char *envline; > char *debug_output = NULL; > > - /* This is the default place for profiling data file. */ > - GLRO(dl_profile_output) > - = &"/var/tmp\0/var/profile"[__libc_enable_secure ? 9 : 0]; > - > while ((envline = _dl_next_ld_env_entry (&runp)) != NULL) > { > size_t len = 0; > @@ -2586,7 +2583,8 @@ process_envvars (struct dl_main_state *state) > } > > /* Which shared object shall be profiled. */ > - if (memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0') > + if (!__libc_enable_secure > + && memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0') > GLRO(dl_profile) = &envline[8]; > break; > > diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c > index ba295a6a14..76b8e1fb45 100644 > --- a/elf/tst-env-setuid.c > +++ b/elf/tst-env-setuid.c > @@ -34,6 +34,9 @@ static char SETGID_CHILD[] = "setgid-child"; > > #define FILTERED_VALUE "some-filtered-value" > #define UNFILTERED_VALUE "some-unfiltered-value" > +/* It assumes no other programs is being profile with a library with same > + SONAME using the default folder. */ > +#define PROFILE_LIB "tst-sonamemove-runmod2.so" > > struct envvar_t > { > @@ -50,7 +53,7 @@ static const struct envvar_t filtered_envvars[] = > { "LD_HWCAP_MASK", FILTERED_VALUE }, > { "LD_LIBRARY_PATH", FILTERED_VALUE }, > { "LD_PRELOAD", FILTERED_VALUE }, > - { "LD_PROFILE", FILTERED_VALUE }, > + { "LD_PROFILE", "tst-sonamemove-runmod2.so" }, > { "MALLOC_ARENA_MAX", FILTERED_VALUE }, > { "MALLOC_PERTURB_", FILTERED_VALUE }, > { "MALLOC_TRACE", FILTERED_VALUE }, > @@ -87,6 +90,13 @@ test_child (void) > ret |= !(env != NULL && strcmp (env, e->value) == 0); > } > > + /* Also check if no profile file was created. */ > + { > + char *profilepath = xasprintf ("/var/tmp/%s.profile", PROFILE_LIB); > + ret |= !access (profilepath, R_OK); > + free (profilepath); > + } > + > return ret; > } >
* Siddhesh Poyarekar: > On 2023-10-17 09:05, Adhemerval Zanella wrote: >> Loader does not ignore LD_PROFILE in secure-execution mode (different >> than man-page states [1]), rather it uses a different path >> (/var/profile) and ignore LD_PROFILE_OUTPUT. >> Allowing secure-execution profiling is already a non good security >> boundary, since it enables different code paths and extra OS access by >> the process. But by ignoring LD_PROFILE_OUTPUT, the resulting profile >> file might also be acceded in a racy manner since the file name does not >> use any process-specific information (such as pid, timing, etc.). >> Another side-effect is it forces lazy binding even on libraries that >> might be with DF_BIND_NOW. >> [1] https://man7.org/linux/man-pages/man8/ld.so.8.html >> --- > > I tend to agree. Carlos, Florian, is profiling of setuid binaries > something that needs to be supported as compatibility behaviour? I'm > inclined to agree with Adhemerval and just rip it out. It's not something we need to support. LD_PROFILE does not seem to be regularly used for profiling anyway. Thanks, Florian
diff --git a/elf/Makefile b/elf/Makefile index f1cd6e13fa..608bef46f5 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -3021,3 +3021,6 @@ $(objpfx)tst-dlclose-lazy.out: \ $(objpfx)tst-dlclose-lazy-mod1.so $(objpfx)tst-dlclose-lazy-mod2.so tst-env-setuid-ARGS = -- $(host-test-program-cmd) + +# Reuse a module with a SONAME, to specific as the LD_PROFILE. +$(objpfx)tst-env-setuid: $(objpfx)tst-sonamemove-runmod2.so diff --git a/elf/rtld.c b/elf/rtld.c index 51b6d9f326..a09cf2a9df 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -361,6 +361,7 @@ struct rtld_global_ro _rtld_global_ro attribute_relro = ._dl_fpu_control = _FPU_DEFAULT, ._dl_pagesize = EXEC_PAGESIZE, ._dl_inhibit_cache = 0, + ._dl_profile_output = "/var/tmp", /* Function pointers. */ ._dl_debug_printf = _dl_debug_printf, @@ -2534,10 +2535,6 @@ process_envvars (struct dl_main_state *state) char *envline; char *debug_output = NULL; - /* This is the default place for profiling data file. */ - GLRO(dl_profile_output) - = &"/var/tmp\0/var/profile"[__libc_enable_secure ? 9 : 0]; - while ((envline = _dl_next_ld_env_entry (&runp)) != NULL) { size_t len = 0; @@ -2586,7 +2583,8 @@ process_envvars (struct dl_main_state *state) } /* Which shared object shall be profiled. */ - if (memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0') + if (!__libc_enable_secure + && memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0') GLRO(dl_profile) = &envline[8]; break; diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c index ba295a6a14..76b8e1fb45 100644 --- a/elf/tst-env-setuid.c +++ b/elf/tst-env-setuid.c @@ -34,6 +34,9 @@ static char SETGID_CHILD[] = "setgid-child"; #define FILTERED_VALUE "some-filtered-value" #define UNFILTERED_VALUE "some-unfiltered-value" +/* It assumes no other programs is being profile with a library with same + SONAME using the default folder. */ +#define PROFILE_LIB "tst-sonamemove-runmod2.so" struct envvar_t { @@ -50,7 +53,7 @@ static const struct envvar_t filtered_envvars[] = { "LD_HWCAP_MASK", FILTERED_VALUE }, { "LD_LIBRARY_PATH", FILTERED_VALUE }, { "LD_PRELOAD", FILTERED_VALUE }, - { "LD_PROFILE", FILTERED_VALUE }, + { "LD_PROFILE", "tst-sonamemove-runmod2.so" }, { "MALLOC_ARENA_MAX", FILTERED_VALUE }, { "MALLOC_PERTURB_", FILTERED_VALUE }, { "MALLOC_TRACE", FILTERED_VALUE }, @@ -87,6 +90,13 @@ test_child (void) ret |= !(env != NULL && strcmp (env, e->value) == 0); } + /* Also check if no profile file was created. */ + { + char *profilepath = xasprintf ("/var/tmp/%s.profile", PROFILE_LIB); + ret |= !access (profilepath, R_OK); + free (profilepath); + } + return ret; }