elf: Remove ad-hoc restrictions on dlopen callers [BZ #22787]

Message ID 20180205140121.440E240137CD1@oldenburg.str.redhat.com
State Committed
Headers

Commit Message

Florian Weimer Feb. 5, 2018, 2:01 p.m. UTC
  This looks like a post-exploitation hardening measure: If an attacker is
able to redirect execution flow, they could use that to load a DSO which
contains additional code (or perhaps make the stack executable).

However, the checks are not in the correct place to be effective: If
they are performed before the critical operation, an attacker with
sufficient control over execution flow could simply jump directly to
the code which performs the operation, bypassing the check.  The check
would have to be executed unconditionally after the operation and
terminate the process in case a caller violation was detected.

Furthermore, in _dl_check_caller, there was a fallback reading global
writable data (GL(dl_rtld_map).l_map_start and
GL(dl_rtld_map).l_text_end), which could conceivably be targeted by an
attacker to disable the check, too.

Other critical functions (such as system) remain completely
unprotected, so the value of these additional checks does not appear
that large.  Therefore this commit removes this functionality.

2018-02-05  Florian Weimer  <fweimer@redhat.com>

	[BZ #22787]
	* include/caller.h: Remove file.
	* elf/dl-caller.c: Likewise.
	* elf/Makefile (dl-routines): Remove dl-caller.
	(shared-only-routines): Do not add dl-caller.
	* elf/dl-load.c (_dl_map_object_from_fd): Do not call
	__check_caller.
	* elf/dl-open.c (struct dl_open_args): Remove caller_dl_open
	member.
	(dl_open_worker): Do not call __check_caller.
	(_dl_open): Do not set caller_dl_open member.
	* elf/rtld.c (_rtld_global_ro): Do not initialize
	_dl_check_caller member.
	* sysdeps/generic/ldsodefs.h (rtld_global): Remove
	_dl_check_caller member.
	(_dl_check_caller): Remove declaration.
	* sysdeps/unix/sysv/linux/dl-execstack.c
	(_dl_make_stack_executable): Do not call __check_caller.
  

Comments

Florian Weimer Feb. 20, 2018, 2 p.m. UTC | #1
On 02/05/2018 03:01 PM, Florian Weimer wrote:
> This looks like a post-exploitation hardening measure: If an attacker is
> able to redirect execution flow, they could use that to load a DSO which
> contains additional code (or perhaps make the stack executable).
> 
> However, the checks are not in the correct place to be effective: If
> they are performed before the critical operation, an attacker with
> sufficient control over execution flow could simply jump directly to
> the code which performs the operation, bypassing the check.  The check
> would have to be executed unconditionally after the operation and
> terminate the process in case a caller violation was detected.
> 
> Furthermore, in _dl_check_caller, there was a fallback reading global
> writable data (GL(dl_rtld_map).l_map_start and
> GL(dl_rtld_map).l_text_end), which could conceivably be targeted by an
> attacker to disable the check, too.
> 
> Other critical functions (such as system) remain completely
> unprotected, so the value of these additional checks does not appear
> that large.  Therefore this commit removes this functionality.

Ping?

<https://sourceware.org/ml/libc-alpha/2018-02/msg00123.html>

Thanks,
Florian
  
Andreas Schwab Feb. 20, 2018, 2:17 p.m. UTC | #2
On Feb 05 2018, fweimer@redhat.com (Florian Weimer) wrote:

> 	[BZ #22787]
> 	* include/caller.h: Remove file.
> 	* elf/dl-caller.c: Likewise.
> 	* elf/Makefile (dl-routines): Remove dl-caller.
> 	(shared-only-routines): Do not add dl-caller.
> 	* elf/dl-load.c (_dl_map_object_from_fd): Do not call
> 	__check_caller.
> 	* elf/dl-open.c (struct dl_open_args): Remove caller_dl_open
> 	member.
> 	(dl_open_worker): Do not call __check_caller.
> 	(_dl_open): Do not set caller_dl_open member.
> 	* elf/rtld.c (_rtld_global_ro): Do not initialize
> 	_dl_check_caller member.
> 	* sysdeps/generic/ldsodefs.h (rtld_global): Remove
> 	_dl_check_caller member.
> 	(_dl_check_caller): Remove declaration.
> 	* sysdeps/unix/sysv/linux/dl-execstack.c
> 	(_dl_make_stack_executable): Do not call __check_caller.

Ok.

Andreas.
  

Patch

diff --git a/elf/Makefile b/elf/Makefile
index 2a432d8bee..9bdb9220c7 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -32,7 +32,7 @@  routines	= $(all-dl-routines) dl-support dl-iteratephdr \
 dl-routines	= $(addprefix dl-,load lookup object reloc deps hwcaps \
 				  runtime init fini debug misc \
 				  version profile tls origin scope \
-				  execstack caller open close trampoline \
+				  execstack open close trampoline \
 				  exception sort-maps)
 ifeq (yes,$(use-ldconfig))
 dl-routines += dl-cache
@@ -54,7 +54,6 @@  all-dl-routines = $(dl-routines) $(sysdep-dl-routines)
 # But they are absent from the shared libc, because that code is in ld.so.
 elide-routines.os = $(all-dl-routines) dl-support enbl-secure dl-origin \
 		    dl-sysdep dl-exception dl-reloc-static-pie
-shared-only-routines += dl-caller
 
 # ld.so uses those routines, plus some special stuff for being the program
 # interpreter and operating independent of libc.
diff --git a/elf/dl-caller.c b/elf/dl-caller.c
deleted file mode 100644
index 81a77af4eb..0000000000
--- a/elf/dl-caller.c
+++ /dev/null
@@ -1,86 +0,0 @@ 
-/* Check whether caller comes from the right place.
-   Copyright (C) 2004-2018 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#include <assert.h>
-#include <ldsodefs.h>
-#include <stddef.h>
-#include <caller.h>
-#include <gnu/lib-names.h>
-
-
-int
-attribute_hidden
-_dl_check_caller (const void *caller, enum allowmask mask)
-{
-  static const char expected1[] = LIBC_SO;
-  static const char expected2[] = LIBDL_SO;
-#ifdef LIBPTHREAD_SO
-  static const char expected3[] = LIBPTHREAD_SO;
-#endif
-  static const char expected4[] = LD_SO;
-
-  for (Lmid_t ns = 0; ns < GL(dl_nns); ++ns)
-    for (struct link_map *l = GL(dl_ns)[ns]._ns_loaded; l != NULL;
-	 l = l->l_next)
-      if (caller >= (const void *) l->l_map_start
-	  && caller < (const void *) l->l_text_end)
-	{
-	  /* The address falls into this DSO's address range.  Check the
-	     name.  */
-	  if ((mask & allow_libc) && strcmp (expected1, l->l_name) == 0)
-	    return 0;
-	  if ((mask & allow_libdl) && strcmp (expected2, l->l_name) == 0)
-	    return 0;
-#ifdef LIBPTHREAD_SO
-	  if ((mask & allow_libpthread) && strcmp (expected3, l->l_name) == 0)
-	    return 0;
-#endif
-	  if ((mask & allow_ldso) && strcmp (expected4, l->l_name) == 0)
-	    return 0;
-
-	  struct libname_list *runp = l->l_libname;
-
-	  while (runp != NULL)
-	    {
-	      if ((mask & allow_libc) && strcmp (expected1, runp->name) == 0)
-		return 0;
-	      if ((mask & allow_libdl) && strcmp (expected2, runp->name) == 0)
-		return 0;
-#ifdef LIBPTHREAD_SO
-	      if ((mask & allow_libpthread)
-		  && strcmp (expected3, runp->name) == 0)
-		return 0;
-#endif
-	      if ((mask & allow_ldso) && strcmp (expected4, runp->name) == 0)
-		return 0;
-
-	      runp = runp->next;
-	    }
-
-	  break;
-	}
-
-  /* Maybe the dynamic linker is not yet on the list.  */
-  if ((mask & allow_ldso) != 0
-      && caller >= (const void *) GL(dl_rtld_map).l_map_start
-      && caller < (const void *) GL(dl_rtld_map).l_text_end)
-    return 0;
-
-  /* No valid caller.  */
-  return 1;
-}
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 7554a99b5a..a5e3a25462 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -33,7 +33,6 @@ 
 #include "dynamic-link.h"
 #include <abi-tag.h>
 #include <stackinfo.h>
-#include <caller.h>
 #include <sysdep.h>
 #include <stap-probe.h>
 #include <libc-pointer-arith.h>
@@ -1183,12 +1182,6 @@  _dl_map_object_from_fd (const char *name, const char *origname, int fd,
 
   if (__glibc_unlikely ((stack_flags &~ GL(dl_stack_flags)) & PF_X))
     {
-      if (__glibc_unlikely (__check_caller (RETURN_ADDRESS (0), allow_ldso) != 0))
-	{
-	  errstring = N_("invalid caller");
-	  goto call_lose;
-	}
-
       /* The stack is presently not executable, but this module
 	 requires that it be executable.  We must change the
 	 protection of the variable which contains the flags used in
diff --git a/elf/dl-open.c b/elf/dl-open.c
index 0e37dd74b6..9dde4acfbc 100644
--- a/elf/dl-open.c
+++ b/elf/dl-open.c
@@ -28,7 +28,6 @@ 
 #include <sys/param.h>
 #include <libc-lock.h>
 #include <ldsodefs.h>
-#include <caller.h>
 #include <sysdep-cancel.h>
 #include <tls.h>
 #include <stap-probe.h>
@@ -47,8 +46,6 @@  struct dl_open_args
   int mode;
   /* This is the caller of the dlopen() function.  */
   const void *caller_dlopen;
-  /* This is the caller of _dl_open().  */
-  const void *caller_dl_open;
   struct link_map *map;
   /* Namespace ID.  */
   Lmid_t nsid;
@@ -187,11 +184,6 @@  dl_open_worker (void *a)
   int mode = args->mode;
   struct link_map *call_map = NULL;
 
-  /* Check whether _dl_open() has been called from a valid DSO.  */
-  if (__check_caller (args->caller_dl_open,
-		      allow_libc|allow_libdl|allow_ldso) != 0)
-    _dl_signal_error (0, "dlopen", NULL, N_("invalid caller"));
-
   /* Determine the caller's map if necessary.  This is needed in case
      we have a DST, when we don't know the namespace ID we have to put
      the new object in, or when the file name has no path in which
@@ -583,7 +575,6 @@  no more namespaces available for dlmopen()"));
   args.file = file;
   args.mode = mode;
   args.caller_dlopen = caller_dlopen;
-  args.caller_dl_open = RETURN_ADDRESS (0);
   args.map = NULL;
   args.nsid = nsid;
   args.argc = argc;
diff --git a/elf/rtld.c b/elf/rtld.c
index 453f56eb15..f8d9597cdd 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -278,7 +278,6 @@  struct rtld_global_ro _rtld_global_ro attribute_relro =
     ._dl_debug_printf = _dl_debug_printf,
     ._dl_mcount = _dl_mcount,
     ._dl_lookup_symbol_x = _dl_lookup_symbol_x,
-    ._dl_check_caller = _dl_check_caller,
     ._dl_open = _dl_open,
     ._dl_close = _dl_close,
     ._dl_tls_get_addr_soft = _dl_tls_get_addr_soft,
diff --git a/include/caller.h b/include/caller.h
deleted file mode 100644
index 3a4a4b7de8..0000000000
--- a/include/caller.h
+++ /dev/null
@@ -1,31 +0,0 @@ 
-/* Copyright (C) 2004-2018 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <http://www.gnu.org/licenses/>.  */
-
-#ifndef _CALLER_H
-#define _CALLER_H 1
-
-#include <ldsodefs.h>
-
-/* _dl_check_caller only works in DSOs.  */
-#ifdef SHARED
-# define __check_caller(caller, mask) \
-  GLRO(dl_check_caller) (caller, mask)
-#else
-# define __check_caller(caller, mask) (0)
-#endif
-
-#endif /* caller.h */
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 0ea27865c0..5e1b24ecb5 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -596,7 +596,6 @@  struct rtld_global_ro
 				   const ElfW(Sym) **, struct r_scope_elem *[],
 				   const struct r_found_version *, int, int,
 				   struct link_map *);
-  int (*_dl_check_caller) (const void *, enum allowmask);
   void *(*_dl_open) (const char *file, int mode, const void *caller_dlopen,
 		     Lmid_t nsid, int argc, char *argv[], char *env[]);
   void (*_dl_close) (void *map);
@@ -1102,10 +1101,6 @@  extern size_t _dl_dst_count (const char *name) attribute_hidden;
 extern char *_dl_dst_substitute (struct link_map *l, const char *name,
 				 char *result) attribute_hidden;
 
-/* Check validity of the caller.  */
-extern int _dl_check_caller (const void *caller, enum allowmask mask)
-     attribute_hidden;
-
 /* Open the shared object NAME, relocate it, and run its initializer if it
    hasn't already been run.  MODE is as for `dlopen' (see <dlfcn.h>).  If
    the object is already opened, returns its existing map.  */
diff --git a/sysdeps/unix/sysv/linux/dl-execstack.c b/sysdeps/unix/sysv/linux/dl-execstack.c
index c36d809d02..8ea69bdde3 100644
--- a/sysdeps/unix/sysv/linux/dl-execstack.c
+++ b/sysdeps/unix/sysv/linux/dl-execstack.c
@@ -22,7 +22,6 @@ 
 #include <libintl.h>
 #include <stdbool.h>
 #include <stackinfo.h>
-#include <caller.h>
 #include <sysdep.h>
 
 
@@ -37,12 +36,6 @@  _dl_make_stack_executable (void **stack_endp)
 		    & -(intptr_t) GLRO(dl_pagesize));
   int result = 0;
 
-  /* Challenge the caller.  */
-  if (__builtin_expect (__check_caller (RETURN_ADDRESS (0),
-					allow_ldso|allow_libpthread) != 0, 0)
-      || __builtin_expect (*stack_endp != __libc_stack_end, 0))
-    return EPERM;
-
   if (__builtin_expect (__mprotect ((void *) page, GLRO(dl_pagesize),
 				    __stack_prot) == 0, 1))
     goto return_success;