ld.so: Examine GLRO to detect inactive loader [BZ #20204]
Commit Message
GLRO (_rtld_global_ro) is read-only after initialization and can
therefore not be patched at run time, unlike the hook table addresses
and their contents, so this is a desirable hardening feature.
The hooks are only needed if ld.so has not been initialized, and this
happens only after static dlopen (dlmopen uses a single ld.so object
across all namespaces).
2017-12-18 Florian Weimer <fweimer@redhat.com>
[BZ #20204]
ld.so: Harden dl-libc/libdl hooks.
* sysdeps/generic/ldsodefs.h (rtld_active): New function.
* dlfcn/dladdr.c (__dladdr): Call it.
* dlfcn/dladdr1.c (__dladdr1): Likewise.
* dlfcn/dlclose.c (__dlcose): Likewise.
* dlfcn/dlerror.c (__dlerror): Likewise.
* dlfcn/dlinfo.c (__dlinfo): Likewise.
* dlfcn/dlmopen.c (__dlmopen): Likewise.
* dlfcn/dlopen.c (__dlopen): Likewise.
* dlfcn/dlopenold.c (__dlopen_nocheck): Likewise.
* dlfcn/dlsym.c (__dlsym): Likewise.
* dlfcn/dlvsym.c (__dlvsym): Likewise.
* elf/dl-libc.c (__libc_dlopen_mode, __libc_dlsym)
(__libc_dlclose): Likewise.
Comments
On 12/18/2017 04:34 AM, Florian Weimer wrote:
> GLRO (_rtld_global_ro) is read-only after initialization and can
> therefore not be patched at run time, unlike the hook table addresses
> and their contents, so this is a desirable hardening feature.
>
> The hooks are only needed if ld.so has not been initialized, and this
> happens only after static dlopen (dlmopen uses a single ld.so object
> across all namespaces).
High level:
The hardening aspect of this change is valuable enough that it should
go in right away.
Design:
Refactoring the symbol check into a function call is great.
Implementation:
Needs 2 additional comments. See notes below.
---
I looked into trying to write a test case for this, but it's not trivial.
OK to checkin if you add the additional comments.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 2017-12-18 Florian Weimer <fweimer@redhat.com>
>
> [BZ #20204]
> ld.so: Harden dl-libc/libdl hooks.
> * sysdeps/generic/ldsodefs.h (rtld_active): New function.
> * dlfcn/dladdr.c (__dladdr): Call it.
> * dlfcn/dladdr1.c (__dladdr1): Likewise.
> * dlfcn/dlclose.c (__dlcose): Likewise.
> * dlfcn/dlerror.c (__dlerror): Likewise.
> * dlfcn/dlinfo.c (__dlinfo): Likewise.
> * dlfcn/dlmopen.c (__dlmopen): Likewise.
> * dlfcn/dlopen.c (__dlopen): Likewise.
> * dlfcn/dlopenold.c (__dlopen_nocheck): Likewise.
> * dlfcn/dlsym.c (__dlsym): Likewise.
> * dlfcn/dlvsym.c (__dlvsym): Likewise.
> * elf/dl-libc.c (__libc_dlopen_mode, __libc_dlsym)
> (__libc_dlclose): Likewise.
>
> diff --git a/dlfcn/dladdr.c b/dlfcn/dladdr.c
> index 1753434160..1bebd00240 100644
> --- a/dlfcn/dladdr.c
> +++ b/dlfcn/dladdr.c
> @@ -17,6 +17,7 @@
> <http://www.gnu.org/licenses/>. */
>
> #include <dlfcn.h>
> +#include <ldsodefs.h>
OK.
>
> #if !defined SHARED && IS_IN (libdl)
>
> @@ -32,7 +33,7 @@ int
> __dladdr (const void *address, Dl_info *info)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK. Nice change.
> return _dlfcn_hook->dladdr (address, info);
> # endif
> return _dl_addr (address, info, NULL, NULL);
> diff --git a/dlfcn/dladdr1.c b/dlfcn/dladdr1.c
> index a19f9fdea2..901cf43f6b 100644
> --- a/dlfcn/dladdr1.c
> +++ b/dlfcn/dladdr1.c
> @@ -17,6 +17,7 @@
> <http://www.gnu.org/licenses/>. */
>
> #include <dlfcn.h>
> +#include <ldsodefs.h>
OK.
>
> #if !defined SHARED && IS_IN (libdl)
>
> @@ -32,7 +33,7 @@ int
> __dladdr1 (const void *address, Dl_info *info, void **extra, int flags)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dladdr1 (address, info, extra, flags);
> # endif
>
> diff --git a/dlfcn/dlclose.c b/dlfcn/dlclose.c
> index da66e20488..223887d338 100644
> --- a/dlfcn/dlclose.c
> +++ b/dlfcn/dlclose.c
> @@ -39,7 +39,7 @@ int
> __dlclose (void *handle)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlclose (handle);
> # endif
>
> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
> index fb5012ee85..b33c05095a 100644
> --- a/dlfcn/dlerror.c
> +++ b/dlfcn/dlerror.c
> @@ -63,7 +63,7 @@ __dlerror (void)
> struct dl_action_result *result;
>
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlerror ();
> # endif
>
> diff --git a/dlfcn/dlinfo.c b/dlfcn/dlinfo.c
> index a34e947ed3..b011257468 100644
> --- a/dlfcn/dlinfo.c
> +++ b/dlfcn/dlinfo.c
> @@ -111,7 +111,7 @@ int
> __dlinfo (void *handle, int request, void *arg DL_CALLER_DECL)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlinfo (handle, request, arg,
> DL_CALLER);
> # endif
> diff --git a/dlfcn/dlmopen.c b/dlfcn/dlmopen.c
> index 07d59ade30..58f88bb7c6 100644
> --- a/dlfcn/dlmopen.c
> +++ b/dlfcn/dlmopen.c
> @@ -79,7 +79,7 @@ void *
> __dlmopen (Lmid_t nsid, const char *file, int mode DL_CALLER_DECL)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlmopen (nsid, file, mode, RETURN_ADDRESS (0));
> # endif
>
> diff --git a/dlfcn/dlopen.c b/dlfcn/dlopen.c
> index 22120655d2..73651a8430 100644
> --- a/dlfcn/dlopen.c
> +++ b/dlfcn/dlopen.c
> @@ -74,7 +74,7 @@ void *
> __dlopen (const char *file, int mode DL_CALLER_DECL)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlopen (file, mode, DL_CALLER);
> # endif
>
> diff --git a/dlfcn/dlopenold.c b/dlfcn/dlopenold.c
> index a3db500705..d899c4e890 100644
> --- a/dlfcn/dlopenold.c
> +++ b/dlfcn/dlopenold.c
> @@ -70,7 +70,7 @@ __dlopen_nocheck (const char *file, int mode)
> mode |= RTLD_LAZY;
> args.mode = mode;
>
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlopen (file, mode, RETURN_ADDRESS (0));
>
> return _dlerror_run (dlopen_doit, &args) ? NULL : args.new;
> diff --git a/dlfcn/dlsym.c b/dlfcn/dlsym.c
> index 7976c5f75c..19733a0f19 100644
> --- a/dlfcn/dlsym.c
> +++ b/dlfcn/dlsym.c
> @@ -55,7 +55,7 @@ void *
> __dlsym (void *handle, const char *name DL_CALLER_DECL)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlsym (handle, name, DL_CALLER);
> # endif
>
> diff --git a/dlfcn/dlvsym.c b/dlfcn/dlvsym.c
> index 5ed220b77c..ad46b65023 100644
> --- a/dlfcn/dlvsym.c
> +++ b/dlfcn/dlvsym.c
> @@ -58,7 +58,7 @@ __dlvsym (void *handle, const char *name, const char *version_str
> DL_CALLER_DECL)
> {
> # ifdef SHARED
> - if (__glibc_unlikely (_dlfcn_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dlfcn_hook->dlvsym (handle, name, version_str, DL_CALLER);
> # endif
>
> diff --git a/elf/dl-libc.c b/elf/dl-libc.c
> index bd3c18d20f..7d9a8948f3 100644
> --- a/elf/dl-libc.c
> +++ b/elf/dl-libc.c
> @@ -157,7 +157,7 @@ __libc_dlopen_mode (const char *name, int mode)
> args.caller_dlopen = RETURN_ADDRESS (0);
>
> #ifdef SHARED
> - if (__glibc_unlikely (_dl_open_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dl_open_hook->dlopen_mode (name, mode);
> return (dlerror_run (do_dlopen, &args) ? NULL : (void *) args.map);
> #else
> @@ -203,7 +203,7 @@ __libc_dlsym (void *map, const char *name)
> args.name = name;
>
> #ifdef SHARED
> - if (__glibc_unlikely (_dl_open_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dl_open_hook->dlsym (map, name);
> #endif
> return (dlerror_run (do_dlsym, &args) ? NULL
> @@ -215,7 +215,7 @@ int
> __libc_dlclose (void *map)
> {
> #ifdef SHARED
> - if (__glibc_unlikely (_dl_open_hook != NULL))
> + if (!rtld_active ())
OK.
> return _dl_open_hook->dlclose (map);
> #endif
> return dlerror_run (do_dlclose, map);
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 196513851f..61313a72ba 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1144,6 +1144,20 @@ extern void _dl_non_dynamic_init (void)
> extern void _dl_aux_init (ElfW(auxv_t) *av)
> attribute_hidden;
>
> +/* Return true if the ld.so copy in this namespace is actually active
> + and working. If false, the dl_open/dlfcn hooks have to be used to
> + call into the outer dynamic linker (which happens after static
> + dlopen). */
> +#ifdef SHARED
> +static inline bool
> +rtld_active (void)
> +{
> + /* The default-initialized variable does not have a non-zero
> + dl_init_all_dirs member, so this allows us to recognize an
> + initialized and active ld.so copy. */
> + return GLRO(dl_init_all_dirs) != NULL;
We need to add a comment to the definition of GLRO(dl_init_all_dirs) that
it is being used as the initialization marker, then at assignment in rtld.c
we need a similar comment. This ties the definition, assignment, and usage
together in a meaningful way.
> +}
> +#endif
>
> __END_DECLS
>
>
@@ -17,6 +17,7 @@
<http://www.gnu.org/licenses/>. */
#include <dlfcn.h>
+#include <ldsodefs.h>
#if !defined SHARED && IS_IN (libdl)
@@ -32,7 +33,7 @@ int
__dladdr (const void *address, Dl_info *info)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dladdr (address, info);
# endif
return _dl_addr (address, info, NULL, NULL);
@@ -17,6 +17,7 @@
<http://www.gnu.org/licenses/>. */
#include <dlfcn.h>
+#include <ldsodefs.h>
#if !defined SHARED && IS_IN (libdl)
@@ -32,7 +33,7 @@ int
__dladdr1 (const void *address, Dl_info *info, void **extra, int flags)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dladdr1 (address, info, extra, flags);
# endif
@@ -39,7 +39,7 @@ int
__dlclose (void *handle)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlclose (handle);
# endif
@@ -63,7 +63,7 @@ __dlerror (void)
struct dl_action_result *result;
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlerror ();
# endif
@@ -111,7 +111,7 @@ int
__dlinfo (void *handle, int request, void *arg DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlinfo (handle, request, arg,
DL_CALLER);
# endif
@@ -79,7 +79,7 @@ void *
__dlmopen (Lmid_t nsid, const char *file, int mode DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlmopen (nsid, file, mode, RETURN_ADDRESS (0));
# endif
@@ -74,7 +74,7 @@ void *
__dlopen (const char *file, int mode DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlopen (file, mode, DL_CALLER);
# endif
@@ -70,7 +70,7 @@ __dlopen_nocheck (const char *file, int mode)
mode |= RTLD_LAZY;
args.mode = mode;
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlopen (file, mode, RETURN_ADDRESS (0));
return _dlerror_run (dlopen_doit, &args) ? NULL : args.new;
@@ -55,7 +55,7 @@ void *
__dlsym (void *handle, const char *name DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlsym (handle, name, DL_CALLER);
# endif
@@ -58,7 +58,7 @@ __dlvsym (void *handle, const char *name, const char *version_str
DL_CALLER_DECL)
{
# ifdef SHARED
- if (__glibc_unlikely (_dlfcn_hook != NULL))
+ if (!rtld_active ())
return _dlfcn_hook->dlvsym (handle, name, version_str, DL_CALLER);
# endif
@@ -157,7 +157,7 @@ __libc_dlopen_mode (const char *name, int mode)
args.caller_dlopen = RETURN_ADDRESS (0);
#ifdef SHARED
- if (__glibc_unlikely (_dl_open_hook != NULL))
+ if (!rtld_active ())
return _dl_open_hook->dlopen_mode (name, mode);
return (dlerror_run (do_dlopen, &args) ? NULL : (void *) args.map);
#else
@@ -203,7 +203,7 @@ __libc_dlsym (void *map, const char *name)
args.name = name;
#ifdef SHARED
- if (__glibc_unlikely (_dl_open_hook != NULL))
+ if (!rtld_active ())
return _dl_open_hook->dlsym (map, name);
#endif
return (dlerror_run (do_dlsym, &args) ? NULL
@@ -215,7 +215,7 @@ int
__libc_dlclose (void *map)
{
#ifdef SHARED
- if (__glibc_unlikely (_dl_open_hook != NULL))
+ if (!rtld_active ())
return _dl_open_hook->dlclose (map);
#endif
return dlerror_run (do_dlclose, map);
@@ -1144,6 +1144,20 @@ extern void _dl_non_dynamic_init (void)
extern void _dl_aux_init (ElfW(auxv_t) *av)
attribute_hidden;
+/* Return true if the ld.so copy in this namespace is actually active
+ and working. If false, the dl_open/dlfcn hooks have to be used to
+ call into the outer dynamic linker (which happens after static
+ dlopen). */
+#ifdef SHARED
+static inline bool
+rtld_active (void)
+{
+ /* The default-initialized variable does not have a non-zero
+ dl_init_all_dirs member, so this allows us to recognize an
+ initialized and active ld.so copy. */
+ return GLRO(dl_init_all_dirs) != NULL;
+}
+#endif
__END_DECLS