diff mbox

Ignore LD_POINTER_GUARD for set-user-ID/set-group-ID binaries.

Message ID 561D0E20.3000409@redhat.com
State New
Headers show

Commit Message

Florian Weimer Oct. 13, 2015, 1:58 p.m. UTC 
On 10/11/2015 02:10 AM, Mike Frysinger wrote:
> On 09 Oct 2015 21:59, Carlos O'Donell wrote:
>> On 10/08/2015 04:44 AM, Florian Weimer wrote:
>>> On 09/05/2015 06:39 PM, Hector Marco-Gisbert wrote:
>>>> A weakness in the dynamic loader have been found, Glibc prior to
>>>> 2.22.90 are affected. The issue is that the LD_POINTER_GUARD in the
>>>> environment is not sanitized allowing local attackers easily to bypass
>>>> the pointer guarding protection on set-user-ID and set-group-ID
>>>> programs. 
>>>>
>>>> Details of the weakness:
>>>> http://hmarco.org/bugs/glibc_ptr_mangle_weakness.html
>>>>
>>>> This patch prevents to disable the pointer guarding protection for
>>>> set-user-ID/set-group-ID programs.
>>>>
>>>> For example, executing "LD_POINTER_GUARD=0 /bin/ping" does not disable
>>>> the pointer guarding protection unless it is directly executed by root
>>>> (rUID==eUID).
>>>
>>> Does anyone actually use LD_POINTER_GUARD for debugging?  Maybe we can
>>> simply retire the environment variable instead.
>>
>> I vote we remove it. It has long since passed the point of usefullness.
>> With a proper tunables infrastructure we would have added it in one release
>> while we tested things, and then removed it one or two releases later.
> 
> sounds fine to me.  punt it and be done.

Great, then let's do it.

(I have an idea how we can preserve backwards compatibility even if we
have some form of libio vtable hardening, so we don't have to keep
around LD_POINTER_GUARD for the sake of old applications.)

Florian

Comments

Mike Frysinger Oct. 15, 2015, 3:53 a.m. UTC | #1 
On 13 Oct 2015 15:58, Florian Weimer wrote:
> Always enable pointer guard [BZ #18928]
> 
> Honoring the LD_POINTER_GUARD environment variable in AT_SECURE mode
> has security implications. This commit enables pointer guard
> unconditionally, and the environment variable is now ignored.

lgtm

cc-ing Michael Kerrisk for man-page upate to ldso(8)
-mike
Mike Frysinger Oct. 15, 2015, 3:05 p.m. UTC | #2 
btw, you going to cherry pick to 2.22 ?
-mike
Mike Frysinger Oct. 19, 2015, 3:33 p.m. UTC | #3 
On 15 Oct 2015 11:05, Mike Frysinger wrote:
> btw, you going to cherry pick to 2.22 ?

i've pushed this to the 2.22 branch
-mike
Mike Frysinger Oct. 19, 2015, 9:05 p.m. UTC | #4 
On 19 Oct 2015 11:33, Mike Frysinger wrote:
> On 15 Oct 2015 11:05, Mike Frysinger wrote:
> > btw, you going to cherry pick to 2.22 ?
> 
> i've pushed this to the 2.22 branch

i've also pushed this related commit per-request:

commit 5fb7924cb6cf606ce865122e5bbac9df934db14e
Author: Florian Weimer <fweimer@redhat.com>
Date:   Tue Oct 6 13:12:36 2015 +0200

    Harden tls_dtor_list with pointer mangling [BZ #19018]

    (cherry picked from commit f586e1328681b400078c995a0bb6ad301ef73549)
-mike
diff mbox

Patch

Always enable pointer guard [BZ #18928]

Honoring the LD_POINTER_GUARD environment variable in AT_SECURE mode
has security implications. This commit enables pointer guard
unconditionally, and the environment variable is now ignored.

2015-10-13  Florian Weimer  <fweimer@redhat.com>

	[BZ #18928]
	* sysdeps/generic/ldsodefs.h (struct rtld_global_ro): Remove
	_dl_pointer_guard member.
	* elf/rtld.c (_rtld_global_ro): Remove _dl_pointer_guard
	initializer.
	(security_init): Always set up pointer guard.
	(process_envvars): Do not process LD_POINTER_GUARD.

diff --git a/NEWS b/NEWS
index 478ed2d..0d1cc3b 100644
--- a/NEWS
+++ b/NEWS
@@ -16,10 +16,13 @@  Version 2.23
   18265, 18370, 18421, 18480, 18525, 18595, 18589, 18610, 18618, 18647,
   18661, 18674, 18675, 18681, 18724, 18757, 18778, 18781, 18787, 18789,
   18790, 18795, 18796, 18803, 18820, 18823, 18824, 18825, 18857, 18863,
-  18870, 18872, 18873, 18875, 18887, 18921, 18951, 18952, 18956, 18961,
-  18966, 18967, 18969, 18970, 18977, 18980, 18981, 18985, 19003, 19012,
-  19016, 19018, 19032, 19046, 19049, 19050, 19059, 19071, 19076, 19077,
-  19078, 19079, 19085, 19086, 19088, 19094, 19095.
+  18870, 18872, 18873, 18875, 18887, 18921, 18928, 18951, 18952, 18956,
+  18961, 18966, 18967, 18969, 18970, 18977, 18980, 18981, 18985, 19003,
+  19012, 19016, 19018, 19032, 19046, 19049, 19050, 19059, 19071, 19076,
+  19077, 19078, 19079, 19085, 19086, 19088, 19094, 19095.
+
+* The LD_POINTER_GUARD environment variable can no longer be used to disable
+  the pointer guard feature.  It is always enabled.
 
 * The obsolete header <regexp.h> has been removed.  Programs that require
   this header must be updated to use <regex.h> instead.
diff --git a/elf/rtld.c b/elf/rtld.c
index 1474c72..52160df 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -162,7 +162,6 @@  struct rtld_global_ro _rtld_global_ro attribute_relro =
     ._dl_hwcap_mask = HWCAP_IMPORTANT,
     ._dl_lazy = 1,
     ._dl_fpu_control = _FPU_DEFAULT,
-    ._dl_pointer_guard = 1,
     ._dl_pagesize = EXEC_PAGESIZE,
     ._dl_inhibit_cache = 0,
 
@@ -709,15 +708,12 @@  security_init (void)
 #endif
 
   /* Set up the pointer guard as well, if necessary.  */
-  if (GLRO(dl_pointer_guard))
-    {
-      uintptr_t pointer_chk_guard = _dl_setup_pointer_guard (_dl_random,
-							     stack_chk_guard);
+  uintptr_t pointer_chk_guard
+    = _dl_setup_pointer_guard (_dl_random, stack_chk_guard);
 #ifdef THREAD_SET_POINTER_GUARD
-      THREAD_SET_POINTER_GUARD (pointer_chk_guard);
+  THREAD_SET_POINTER_GUARD (pointer_chk_guard);
 #endif
-      __pointer_chk_guard_local = pointer_chk_guard;
-    }
+  __pointer_chk_guard_local = pointer_chk_guard;
 
   /* We do not need the _dl_random value anymore.  The less
      information we leave behind, the better, so clear the
@@ -2471,9 +2467,6 @@  process_envvars (enum mode *modep)
 	      GLRO(dl_use_load_bias) = envline[14] == '1' ? -1 : 0;
 	      break;
 	    }
-
-	  if (memcmp (envline, "POINTER_GUARD", 13) == 0)
-	    GLRO(dl_pointer_guard) = envline[14] != '0';
 	  break;
 
 	case 14:
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index 7f7ff72..0625826 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -592,9 +592,6 @@  struct rtld_global_ro
   /* List of auditing interfaces.  */
   struct audit_ifaces *_dl_audit;
   unsigned int _dl_naudit;
-
-  /* 0 if internal pointer values should not be guarded, 1 if they should.  */
-  EXTERN int _dl_pointer_guard;
 };
 # define __rtld_global_attribute__
 # if IS_IN (rtld)