From 36bd371e26c716cbc18e11801b13eff0352d91b0 Mon Sep 17 00:00:00 2001 From: Nick Kralevich Date: Wed, 16 Jan 2013 13:13:22 -0800 Subject: [PATCH] Revert "stack protector: use AT_RANDOM" The AT_RANDOM changes broke setuid / setgid executables such as "ping". When the linker executes a setuid program, it cleans the environment, removing any invalid environment entries, and adding "NULL"s to the end of the environment array for each removed variable. Later on, we try to determine the location of the aux environment variable, and get tripped up by these extra NULLs. Reverting this patch will get setuid executables working again, but getauxval() is still broken for setuid programs because of this bug. This reverts commit e3a49a8661125f24aec8a1453e54b3b78005e21e. Change-Id: I05c58a896b1fe32cfb5d95d43b096045cda0aa4a --- libc/bionic/libc_init_common.c | 28 +++++++++------------------- libc/bionic/libc_init_static.c | 2 +- libc/private/bionic_ssp.h | 28 ++++++++++++++++++++++------ libc/private/bionic_tls.h | 3 +-- linker/linker.cpp | 8 ++++---- tests/stack_protector_test.cpp | 16 +++++++++++++--- 6 files changed, 50 insertions(+), 35 deletions(-) diff --git a/libc/bionic/libc_init_common.c b/libc/bionic/libc_init_common.c index e4ad2ee75..86e1eb574 100644 --- a/libc/bionic/libc_init_common.c +++ b/libc/bionic/libc_init_common.c @@ -53,22 +53,6 @@ unsigned int __page_shift = PAGE_SHIFT; int __system_properties_init(void); -static Elf32_auxv_t* get_aux_from_elfdata(uintptr_t* elf_data) { - int argc = *elf_data; - char** argv = (char**) (elf_data + 1); - char** envp = argv + argc + 1; - - // The auxiliary vector is at the end of the environment block - while(*envp != NULL) { - envp++; - } - /* The end of the environment block is marked by two NULL pointers */ - envp++; - - return (Elf32_auxv_t*) envp; -} - - /* Init TLS for the initial thread. Called by the linker _before_ libc is mapped * in memory. Beware: all writes to libc globals from this function will * apply to linker-private copies and will not be visible from libc later on. @@ -80,7 +64,7 @@ static Elf32_auxv_t* get_aux_from_elfdata(uintptr_t* elf_data) { * This function also stores the elf_data argument in a specific TLS slot to be later * picked up by the libc constructor. */ -void __libc_init_tls(uintptr_t* elf_data) { +void __libc_init_tls(unsigned** elf_data) { unsigned stack_top = (__get_sp() & ~(PAGE_SIZE - 1)) + PAGE_SIZE; unsigned stack_size = 128 * 1024; unsigned stack_bottom = stack_top - stack_size; @@ -93,7 +77,6 @@ void __libc_init_tls(uintptr_t* elf_data) { _init_thread(&thread, gettid(), &thread_attr, (void*) stack_bottom, false); static void* tls_area[BIONIC_TLS_SLOTS]; - __libc_auxv = get_aux_from_elfdata(elf_data); __init_tls(tls_area, &thread); tls_area[TLS_SLOT_BIONIC_PREINIT] = elf_data; } @@ -113,7 +96,14 @@ void __libc_init_common(uintptr_t* elf_data) { __progname = argv[0] ? argv[0] : ""; environ = envp; - __libc_auxv = get_aux_from_elfdata(elf_data); + // The auxiliary vector is at the end of the environment block + while(*envp != NULL) { + envp++; + } + /* The end of the environment block is marked by two NULL pointers */ + envp++; + + __libc_auxv = (Elf32_auxv_t*) envp; __system_properties_init(); // Requires 'environ'. } diff --git a/libc/bionic/libc_init_static.c b/libc/bionic/libc_init_static.c index 1cef63203..24a4397a8 100644 --- a/libc/bionic/libc_init_static.c +++ b/libc/bionic/libc_init_static.c @@ -96,7 +96,7 @@ __noreturn void __libc_init(uintptr_t *elfdata, int argc; char **argv, **envp; - __libc_init_tls(elfdata); + __libc_init_tls(NULL); /* Initialize the C runtime environment */ __libc_init_common(elfdata); diff --git a/libc/private/bionic_ssp.h b/libc/private/bionic_ssp.h index 14ced64ad..697216c18 100644 --- a/libc/private/bionic_ssp.h +++ b/libc/private/bionic_ssp.h @@ -29,8 +29,8 @@ #ifndef _PRIVATE_SSP_H #define _PRIVATE_SSP_H -#include -#include +#include +#include __BEGIN_DECLS @@ -48,11 +48,27 @@ extern void* __stack_chk_guard; extern void __stack_chk_fail(); __inline__ static void* __attribute__((always_inline)) __generate_stack_chk_guard(void) { + union { + uintptr_t value; + char bytes[sizeof(uintptr_t)]; + } u; - void* src = (void*) getauxval(AT_RANDOM); - void* result; - memcpy(&result, src, sizeof(result)); - return result; + /* Try pulling random bytes from /dev/urandom. */ + int fd = TEMP_FAILURE_RETRY(open("/dev/urandom", O_RDONLY)); + if (fd != -1) { + ssize_t byte_count = TEMP_FAILURE_RETRY(read(fd, &u.bytes, sizeof(u))); + close(fd); + if (byte_count == sizeof(u)) { + return (void*) u.value; + } + } + + /* If that failed, switch to 'terminator canary'. */ + u.bytes[0] = 0; + u.bytes[1] = 0; + u.bytes[2] = '\n'; + u.bytes[3] = 255; + return (void*) u.value; } __END_DECLS diff --git a/libc/private/bionic_tls.h b/libc/private/bionic_tls.h index b33e53ec9..f661ccf24 100644 --- a/libc/private/bionic_tls.h +++ b/libc/private/bionic_tls.h @@ -29,7 +29,6 @@ #define _SYS_TLS_H #include -#include __BEGIN_DECLS @@ -135,7 +134,7 @@ extern void* __get_tls( void ); extern void* __get_stack_base(int *p_stack_size); /* Initialize the TLS. */ -extern void __libc_init_tls(uintptr_t* elfdata); +extern void __libc_init_tls(unsigned** elfdata); __END_DECLS diff --git a/linker/linker.cpp b/linker/linker.cpp index 120f9ee41..0a89b7224 100755 --- a/linker/linker.cpp +++ b/linker/linker.cpp @@ -1785,7 +1785,7 @@ static bool soinfo_link_image(soinfo* si) { * fixed it's own GOT. It is safe to make references to externs * and other non-local data at this point. */ -static unsigned __linker_init_post_relocation(uintptr_t* elfdata, unsigned linker_base) +static unsigned __linker_init_post_relocation(unsigned **elfdata, unsigned linker_base) { static soinfo linker_soinfo; @@ -1976,7 +1976,7 @@ static unsigned __linker_init_post_relocation(uintptr_t* elfdata, unsigned linke * Find the value of AT_BASE passed to us by the kernel. This is the load * location of the linker. */ -static unsigned find_linker_base(uintptr_t* elfdata) { +static unsigned find_linker_base(unsigned **elfdata) { int argc = (int) *elfdata; char **argv = (char**) (elfdata + 1); unsigned *vecs = (unsigned*) (argv + argc + 1); @@ -2032,8 +2032,8 @@ get_elf_exec_load_bias(const Elf32_Ehdr* elf) * relocations, any attempt to reference an extern variable, extern * function, or other GOT reference will generate a segfault. */ -extern "C" unsigned __linker_init(uintptr_t* elfdata) { - uintptr_t linker_addr = find_linker_base(elfdata); +extern "C" unsigned __linker_init(unsigned **elfdata) { + unsigned linker_addr = find_linker_base(elfdata); Elf32_Ehdr *elf_hdr = (Elf32_Ehdr *) linker_addr; Elf32_Phdr *phdr = (Elf32_Phdr *)((unsigned char *) linker_addr + elf_hdr->e_phoff); diff --git a/tests/stack_protector_test.cpp b/tests/stack_protector_test.cpp index ca90deac1..9cf3c38ab 100644 --- a/tests/stack_protector_test.cpp +++ b/tests/stack_protector_test.cpp @@ -56,7 +56,13 @@ struct stack_protector_checker { // Duplicate tid. gettid(2) bug? Seeing this would be very upsetting. ASSERT_TRUE(tids.find(tid) == tids.end()); - +#ifdef __GLIBC__ + // glibc uses the same guard for every thread. bionic uses a different guard for each one. +#else + // Duplicate guard. Our bug. Note this is potentially flaky; we _could_ get the + // same guard for two threads, but it should be vanishingly unlikely. + ASSERT_TRUE(guards.find(guard) == guards.end()); +#endif // Uninitialized guard. Our bug. Note this is potentially flaky; we _could_ get // four random zero bytes, but it should be vanishingly unlikely. ASSERT_NE(guard, 0U); @@ -72,7 +78,7 @@ static void* ThreadGuardHelper(void* arg) { return NULL; } -TEST(stack_protector, same_guard_per_thread) { +TEST(stack_protector, guard_per_thread) { stack_protector_checker checker; size_t thread_count = 10; for (size_t i = 0; i < thread_count; ++i) { @@ -84,8 +90,12 @@ TEST(stack_protector, same_guard_per_thread) { } ASSERT_EQ(thread_count, checker.tids.size()); - // bionic x86 and glibc uses the same guard for every thread. + // glibc uses the same guard for every thread. bionic uses a different guard for each one. +#ifdef __BIONIC__ + ASSERT_EQ(thread_count, checker.guards.size()); +#else ASSERT_EQ(1U, checker.guards.size()); +#endif } #endif