diff mbox

Generate additional secret keys for the heap protector

Message ID bd0185d8-1172-f9b8-eef1-1d01a71be23f@redhat.com
State New
Headers show

Commit Message

Florian Weimer Oct. 28, 2016, 3:39 p.m. UTC
On 10/28/2016 04:46 PM, Carlos O'Donell wrote:
> Florian,

>

> Awesome patch. This is looking really good and the expansion of entropy

> into the keys using sha256 looks great.

>

> OK to checkin if you:

>

> * Fix spelling.

> * Remove _dl_setup_pointer_guard linux+generic implementation as unused.

> * Remove _dl_setup_stack_chk_guard linux+generic implementation as unused


New version attached.  I'm waiting until the libcrypt fix is checked in 
(I'll try to test on SPARC and commit if successful).  And it only makes 
sense to have this change if we end up with the heap protector, so I'll 
wait for that, too.

> Non-blocking questions:

>

> Does the sha256 code add about 10kb to the loader's size? A fair trade-off

> for being able to correctly expand the randomness we have into the size we

> need. I also think the startup cost has to be paid, particularly for the

> benefit of hardening the malloc headers against any many of the previously

> used overflow attacks.


Before:

-rwxrwxr-x. 1 fweimer fweimer 1231576 Oct 28 17:17 elf/ld.so
    text    data     bss     dec     hex filename
  141373    5092     424  146889   23dc9 elf/ld.so

After:

-rwxrwxr-x. 1 fweimer fweimer 1245552 Oct 28 17:15 elf/ld.so
    text    data     bss     dec     hex filename
  143629    5092     424  149145   24699 elf/ld.so

So it's a 2256 bytes increase in text size.  The disk size increase is 
mostly debugging information (it's down to 4096 bytes without debugging 
information).

I have not tried to measure the cycle impact.

Thanks,
Florian

Comments

Carlos O'Donell Oct. 28, 2016, 4:48 p.m. UTC | #1
On 10/28/2016 11:39 AM, Florian Weimer wrote:
> On 10/28/2016 04:46 PM, Carlos O'Donell wrote:

>> Florian,

>>

>> Awesome patch. This is looking really good and the expansion of entropy

>> into the keys using sha256 looks great.

>>

>> OK to checkin if you:

>>

>> * Fix spelling.

>> * Remove _dl_setup_pointer_guard linux+generic implementation as unused.

>> * Remove _dl_setup_stack_chk_guard linux+generic implementation as unused

> 

> New version attached. I'm waiting until the libcrypt fix is checked

> in (I'll try to test on SPARC and commit if successful). And it only

> makes sense to have this change if we end up with the heap protector,

> so I'll wait for that, too.


This version looks good to me.

-- 
Cheers,
Carlos.
diff mbox

Patch

Generate additional secret keys for the heap protector

2016-10-28  Florian Weimer  <fweimer@redhat.com>

	* elf/dl-keysetup.h: New file.
	* elf/dl-keysetup.c: Likewise.
	* elf/Makefile (dl-routines): Add dl-keysetup.
	* csu/libc-start.c (LIBC_START_MAIN): Call __compute_keys to
	obtain key material.
	* elf/rtld.c (security_init): Likewise.
	* crypt/sha256.c: Use relative #include for sha256-block.c
	* sysdeps/generic/dl-osinfo.h (_dl_setup_stack_chk_guard)
	(_dl_setup_pointer_guard): Remove.
	* sysdeps/unix/sysv/linux/dl-osinfo.h (_dl_setup_stack_chk_guard)
	(_dl_setup_pointer_guard): Likewise.

diff --git a/crypt/sha256.c b/crypt/sha256.c
index b5497d9..a6ab2e1 100644
--- a/crypt/sha256.c
+++ b/crypt/sha256.c
@@ -211,4 +211,4 @@  __sha256_process_bytes (const void *buffer, size_t len, struct sha256_ctx *ctx)
     }
 }
 
-#include <sha256-block.c>
+#include "sha256-block.c"
diff --git a/csu/libc-start.c b/csu/libc-start.c
index 99c040a..333a4cc 100644
--- a/csu/libc-start.c
+++ b/csu/libc-start.c
@@ -21,6 +21,7 @@ 
 #include <unistd.h>
 #include <ldsodefs.h>
 #include <exit-thread.h>
+#include <elf/dl-keysetup.h>
 
 extern void __libc_init_first (int argc, char **argv, char **envp);
 
@@ -192,21 +193,21 @@  LIBC_START_MAIN (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL),
      we need to setup errno.  */
   __pthread_initialize_minimal ();
 
+  struct key_setup keys;
+  __compute_keys (_dl_random, &keys);
+
   /* Set up the stack checker's canary.  */
-  uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
 # ifdef THREAD_SET_STACK_GUARD
-  THREAD_SET_STACK_GUARD (stack_chk_guard);
+  THREAD_SET_STACK_GUARD (keys.stack);
 # else
-  __stack_chk_guard = stack_chk_guard;
+  __stack_chk_guard = keys.stack;
 # endif
 
   /* Set up the pointer guard value.  */
-  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 (keys.pointer);
 # else
-  __pointer_chk_guard_local = pointer_chk_guard;
+  __pointer_chk_guard_local = keys.pointer;
 # endif
 
 #endif
diff --git a/elf/Makefile b/elf/Makefile
index 82c7e05..c0a8e2e 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -31,7 +31,8 @@  routines	= $(all-dl-routines) dl-support dl-iteratephdr \
 dl-routines	= $(addprefix dl-,load lookup object reloc deps hwcaps \
 				  runtime error init fini debug misc \
 				  version profile conflict tls origin scope \
-				  execstack caller open close trampoline)
+				  execstack caller open close trampoline \
+				  keysetup)
 ifeq (yes,$(use-ldconfig))
 dl-routines += dl-cache
 endif
diff --git a/elf/dl-keysetup.c b/elf/dl-keysetup.c
new file mode 100644
index 0000000..45bd4de
--- /dev/null
+++ b/elf/dl-keysetup.c
@@ -0,0 +1,77 @@ 
+/* Compute secret keys used for protection heuristics.
+   Copyright (C) 2016 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; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#include <dl-keysetup.h>
+#include <string.h>
+
+enum { at_random_size = 16 };
+
+#if __WORDSIZE == 64
+enum { sha256_output_size = 32 };
+static void compute_sha256_of_random (const void *random, void *result);
+#endif
+
+void
+__compute_keys (const void *random, struct key_setup *result)
+{
+#if __WORDSIZE == 32
+  _Static_assert (sizeof (*result) == at_random_size,
+                  "no key expansion required");
+  memcpy (random, result, sizeof (result));
+#else
+  /* We use SHA-256 to expand the 16 bytes of randomness into 32
+     bytes, so that it is hard to guess the remaining keys once a
+     subset of them is known.  */
+  _Static_assert (sizeof (*result) == sha256_output_size,
+                  "SHA-256 provides required size");
+  compute_sha256_of_random (random, result);
+#endif
+
+  /* Prevent leakage of the stack canary through a read buffer
+     overflow of a NUL-terminated string.  */
+  *(char *) &result->stack = '\0';
+
+  /* Clear the lowest three bits in the heap header guard value, so
+     that the flag bits remain unchanged.  */
+  result->heap_header <<= 3;
+}
+
+#if __WORDSIZE == 64
+
+#pragma GCC visibility push (hidden)
+
+/* Avoid symbol collisions with libcrypt.  */
+#define __sha256_process_block __dl_sha256_process_block
+#define __sha256_init_ctx __dl_sha256_init_ctx
+#define __sha256_process_bytes __dl_sha256_process_bytes
+#define __sha256_finish_ctx __dl_sha256_finish_ctx
+
+#include "../crypt/sha256.h"
+#include "../crypt/sha256.c"
+
+#pragma GCC visibility pop
+
+static void
+compute_sha256_of_random (const void *random, void *result)
+{
+  struct sha256_ctx ctx;
+  __sha256_init_ctx (&ctx);
+  __sha256_process_bytes (random, at_random_size, &ctx);
+  __sha256_finish_ctx (&ctx, result);
+}
+#endif
diff --git a/elf/dl-keysetup.h b/elf/dl-keysetup.h
new file mode 100644
index 0000000..6197b1d
--- /dev/null
+++ b/elf/dl-keysetup.h
@@ -0,0 +1,45 @@ 
+/* Compute secret keys used for protection heuristics.
+   Copyright (C) 2016 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; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef KEY_SETUP_H
+#define KEY_SETUP_H
+
+#include <stdint.h>
+
+/* The set of protection keys used by glibc.  */
+struct key_setup
+{
+  /* Canary for the stack-smashing protector.  */
+  uintptr_t stack;
+
+  /* Pointer guard, protecting selected function pointers.  */
+  uintptr_t pointer;
+
+  /* Heap guard, protecting the malloc chunk header.  */
+  uintptr_t heap_header;
+
+  /* Heap guard part two, protecting the previous chunk size field.  */
+  uintptr_t heap_footer;
+};
+
+/* Derive the keys in *RESULT from RANDOM, which comes from the
+   auxiliary vector and points to 16 bytes of randomness.  */
+void __compute_keys (const void *random, struct key_setup *result)
+  attribute_hidden;
+
+#endif /* KEY_SETUP_H */
diff --git a/elf/rtld.c b/elf/rtld.c
index 647661c..de965da 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -41,6 +41,7 @@ 
 #include <tls.h>
 #include <stap-probe.h>
 #include <stackinfo.h>
+#include <dl-keysetup.h>
 
 #include <assert.h>
 
@@ -699,21 +700,21 @@  rtld_lock_default_unlock_recursive (void *lock)
 static void
 security_init (void)
 {
+  struct key_setup keys;
+  __compute_keys (_dl_random, &keys);
+
   /* Set up the stack checker's canary.  */
-  uintptr_t stack_chk_guard = _dl_setup_stack_chk_guard (_dl_random);
 #ifdef THREAD_SET_STACK_GUARD
-  THREAD_SET_STACK_GUARD (stack_chk_guard);
+  THREAD_SET_STACK_GUARD (keys.stack);
 #else
-  __stack_chk_guard = stack_chk_guard;
+  __stack_chk_guard = keys.stack;
 #endif
 
   /* Set up the pointer guard as well, if necessary.  */
-  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 (keys.pointer);
 #endif
-  __pointer_chk_guard_local = pointer_chk_guard;
+  __pointer_chk_guard_local = keys.pointer;
 
   /* We do not need the _dl_random value anymore.  The less
      information we leave behind, the better, so clear the
diff --git a/sysdeps/generic/dl-osinfo.h b/sysdeps/generic/dl-osinfo.h
index 2489e7b..47464c2 100644
--- a/sysdeps/generic/dl-osinfo.h
+++ b/sysdeps/generic/dl-osinfo.h
@@ -16,44 +16,4 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <endian.h>
-#include <stdint.h>
-
-static inline uintptr_t __attribute__ ((always_inline))
-_dl_setup_stack_chk_guard (void *dl_random)
-{
-  union
-  {
-    uintptr_t num;
-    unsigned char bytes[sizeof (uintptr_t)];
-  } ret = { 0 };
-
-  if (dl_random == NULL)
-    {
-      ret.bytes[sizeof (ret) - 1] = 255;
-      ret.bytes[sizeof (ret) - 2] = '\n';
-    }
-  else
-    {
-      memcpy (ret.bytes, dl_random, sizeof (ret));
-#if BYTE_ORDER == LITTLE_ENDIAN
-      ret.num &= ~(uintptr_t) 0xff;
-#elif BYTE_ORDER == BIG_ENDIAN
-      ret.num &= ~((uintptr_t) 0xff << (8 * (sizeof (ret) - 1)));
-#else
-# error "BYTE_ORDER unknown"
-#endif
-    }
-  return ret.num;
-}
-
-static inline uintptr_t __attribute__ ((always_inline))
-_dl_setup_pointer_guard (void *dl_random, uintptr_t stack_chk_guard)
-{
-  uintptr_t ret;
-  if (dl_random == NULL)
-    ret = stack_chk_guard;
-  else
-    memcpy (&ret, (char *) dl_random + sizeof (ret), sizeof (ret));
-  return ret;
-}
+/* No operating-system specific code in the default version.  */
diff --git a/sysdeps/unix/sysv/linux/dl-osinfo.h b/sysdeps/unix/sysv/linux/dl-osinfo.h
index ac72c92..408cf5a 100644
--- a/sysdeps/unix/sysv/linux/dl-osinfo.h
+++ b/sysdeps/unix/sysv/linux/dl-osinfo.h
@@ -46,34 +46,3 @@ 
     else if (__LINUX_KERNEL_VERSION > 0)				      \
       FATAL ("FATAL: cannot determine kernel version\n");		      \
   } while (0)
-
-static inline uintptr_t __attribute__ ((always_inline))
-_dl_setup_stack_chk_guard (void *dl_random)
-{
-  union
-  {
-    uintptr_t num;
-    unsigned char bytes[sizeof (uintptr_t)];
-  } ret;
-
-  /* We need in the moment only 8 bytes on 32-bit platforms and 16
-     bytes on 64-bit platforms.  Therefore we can use the data
-     directly and not use the kernel-provided data to seed a PRNG.  */
-  memcpy (ret.bytes, dl_random, sizeof (ret));
-#if BYTE_ORDER == LITTLE_ENDIAN
-  ret.num &= ~(uintptr_t) 0xff;
-#elif BYTE_ORDER == BIG_ENDIAN
-  ret.num &= ~((uintptr_t) 0xff << (8 * (sizeof (ret) - 1)));
-#else
-# error "BYTE_ORDER unknown"
-#endif
-  return ret.num;
-}
-
-static inline uintptr_t __attribute__ ((always_inline))
-_dl_setup_pointer_guard (void *dl_random, uintptr_t stack_chk_guard)
-{
-  uintptr_t ret;
-  memcpy (&ret, (char *) dl_random + sizeof (ret), sizeof (ret));
-  return ret;
-}