From be5755969d70668bbab0e0c0ed75ebd867189723 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Thu, 16 Dec 2010 19:52:02 +0100 Subject: [PATCH] linker: Remove unsecure env. variable for setuid programs. This removes several unsecure environment variables from the environment block when the program being loaded is setuid. The list of env. variables is the same than what GLibc uses at this point. Change-Id: I456d3ea0880fe0d4de0d3c5dd51871dd36e87fd6 --- linker/Android.mk | 3 +- linker/linker.c | 72 ++++++++++------- linker/linker_environ.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++++ linker/linker_environ.h | 54 +++++++++++++ 4 files changed, 304 insertions(+), 29 deletions(-) create mode 100644 linker/linker_environ.c create mode 100644 linker/linker_environ.h diff --git a/linker/Android.mk b/linker/Android.mk index 27a667736..da311cd23 100644 --- a/linker/Android.mk +++ b/linker/Android.mk @@ -4,6 +4,7 @@ include $(CLEAR_VARS) LOCAL_SRC_FILES:= \ arch/$(TARGET_ARCH)/begin.S \ linker.c \ + linker_environ.c \ linker_format.c \ rt.c \ dlfcn.c \ @@ -93,6 +94,6 @@ $(linked_module): $(TARGET_CRTBEGIN_STATIC_O) $(all_objects) $(all_libraries) $( # just for this module $(LOCAL_BUILT_MODULE): TARGET_CRTBEGIN_STATIC_O := # This line is not strictly necessary because the dynamic linker is built -# as a static executable, but it won't hurt if in the future we start +# as a static executable, but it won't hurt if in the future we start # building the linker as a dynamic one. $(LOCAL_BUILT_MODULE): TARGET_CRTBEGIN_DYNAMIC_O := diff --git a/linker/linker.c b/linker/linker.c index 42a5205b3..c4f54f73a 100644 --- a/linker/linker.c +++ b/linker/linker.c @@ -48,6 +48,7 @@ #include "linker.h" #include "linker_debug.h" +#include "linker_environ.h" #include "linker_format.h" #include "ba.h" @@ -123,6 +124,9 @@ static soinfo *preloads[LDPRELOAD_MAX + 1]; int debug_verbosity; static int pid; +/* This boolean is set if the program being loaded is setuid */ +static int program_is_setuid; + #if STATS struct _link_stats linker_stats; #endif @@ -286,7 +290,7 @@ static soinfo *alloc_info(const char *name) /* Make sure we get a clean block of soinfo */ memset(si, 0, sizeof(soinfo)); - strcpy((char*) si->name, name); + strlcpy((char*) si->name, name, sizeof(si->name)); sonext->next = si; si->ba_index = -1; /* by default, prelinked */ si->next = NULL; @@ -314,7 +318,7 @@ static void free_info(soinfo *si) return; } - /* prev will never be NULL, because the first entry in solist is + /* prev will never be NULL, because the first entry in solist is always the static libdl_info. */ prev->next = si->next; @@ -1232,8 +1236,8 @@ soinfo *find_library(const char *name) return init_library(si); } -/* TODO: - * notify gdb of unload +/* TODO: + * notify gdb of unload * for non-prelinked libraries, find a way to decrement libbase */ static void call_destructors(soinfo *si); @@ -1765,14 +1769,14 @@ static int link_image(soinfo *si, unsigned wr_offset) } #endif if (phdr->p_type == PT_LOAD) { - /* For the executable, we use the si->size field only in - dl_unwind_find_exidx(), so the meaning of si->size - is not the size of the executable; it is the last + /* For the executable, we use the si->size field only in + dl_unwind_find_exidx(), so the meaning of si->size + is not the size of the executable; it is the last virtual address of the loadable part of the executable; since si->base == 0 for an executable, we use the - range [0, si->size) to determine whether a PC value + range [0, si->size) to determine whether a PC value falls within the executable section. Of course, if - a value is below phdr->p_vaddr, it's not in the + a value is below phdr->p_vaddr, it's not in the executable section, but a) we shouldn't be asking for such a value anyway, and b) if we have to provide an EXIDX for such a value, then the executable's @@ -1927,7 +1931,7 @@ static int link_image(soinfo *si, unsigned wr_offset) } } - DEBUG("%5d si->base = 0x%08x, si->strtab = %p, si->symtab = %p\n", + DEBUG("%5d si->base = 0x%08x, si->strtab = %p, si->symtab = %p\n", pid, si->base, si->strtab, si->symtab); if((si->strtab == 0) || (si->symtab == 0)) { @@ -2033,7 +2037,7 @@ static int link_image(soinfo *si, unsigned wr_offset) ftp://ftp.freebsd.org/pub/FreeBSD/CERT/advisories/FreeBSD-SA-02:23.stdio.asc */ - if (getuid() != geteuid() || getgid() != getegid()) + if (program_is_setuid) nullify_closed_stdio (); call_constructors(si); notify_gdb_of_load(si); @@ -2045,7 +2049,7 @@ fail: return -1; } -static void parse_library_path(char *path, char *delim) +static void parse_library_path(const char *path, char *delim) { size_t len; char *ldpaths_bufp = ldpaths_buf; @@ -2068,7 +2072,7 @@ static void parse_library_path(char *path, char *delim) } } -static void parse_preloads(char *path, char *delim) +static void parse_preloads(const char *path, char *delim) { size_t len; char *ldpreloads_bufp = ldpreloads_buf; @@ -2110,8 +2114,8 @@ unsigned __linker_init(unsigned **elfdata) unsigned *vecs = (unsigned*) (argv + argc + 1); soinfo *si; struct link_map * map; - char *ldpath_env = NULL; - char *ldpreload_env = NULL; + const char *ldpath_env = NULL; + const char *ldpreload_env = NULL; /* Setup a temporary TLS area that is used to get a working * errno for system calls. @@ -2135,20 +2139,32 @@ unsigned __linker_init(unsigned **elfdata) */ __tls_area[TLS_SLOT_BIONIC_PREINIT] = elfdata; + /* Are we setuid? */ + program_is_setuid = (getuid() != geteuid()) || (getgid() != getegid()); + + /* Initialize environment functions, and get to the ELF aux vectors table */ + vecs = linker_env_init(vecs); + + /* Sanitize environment if we're loading a setuid program */ + if (program_is_setuid) + linker_env_secure(); + debugger_init(); - /* skip past the environment */ - while(vecs[0] != 0) { - if(!strncmp((char*) vecs[0], "DEBUG=", 6)) { - debug_verbosity = atoi(((char*) vecs[0]) + 6); - } else if(!strncmp((char*) vecs[0], "LD_LIBRARY_PATH=", 16)) { - ldpath_env = (char*) vecs[0] + 16; - } else if(!strncmp((char*) vecs[0], "LD_PRELOAD=", 11)) { - ldpreload_env = (char*) vecs[0] + 11; + /* Get a few environment variables */ + { + const char* env; + env = linker_env_get("DEBUG"); /* XXX: TODO: Change to LD_DEBUG */ + if (env) + debug_verbosity = atoi(env); + + /* Normally, these are cleaned by linker_env_secure, but the test + * against program_is_setuid doesn't cost us anything */ + if (!program_is_setuid) { + ldpath_env = linker_env_get("LD_LIBRARY_PATH"); + ldpreload_env = linker_env_get("LD_PRELOAD"); } - vecs++; } - vecs++; INFO("[ android linker & debugger ]\n"); DEBUG("%5d elfdata @ 0x%08x\n", pid, (unsigned)elfdata); @@ -2176,7 +2192,7 @@ unsigned __linker_init(unsigned **elfdata) * is. Don't use alloc_info(), because the linker shouldn't * be on the soinfo list. */ - strcpy((char*) linker_soinfo.name, "/system/bin/linker"); + strlcpy((char*) linker_soinfo.name, "/system/bin/linker", sizeof linker_soinfo.name); linker_soinfo.flags = 0; linker_soinfo.base = 0; // This is the important part; must be zero. insert_soinfo_into_debug_map(&linker_soinfo); @@ -2206,10 +2222,10 @@ unsigned __linker_init(unsigned **elfdata) si->refcount = 1; /* Use LD_LIBRARY_PATH if we aren't setuid/setgid */ - if (ldpath_env && getuid() == geteuid() && getgid() == getegid()) + if (ldpath_env) parse_library_path(ldpath_env, ":"); - if (ldpreload_env && getuid() == geteuid() && getgid() == getegid()) { + if (ldpreload_env) { parse_preloads(ldpreload_env, " :"); } diff --git a/linker/linker_environ.c b/linker/linker_environ.c new file mode 100644 index 000000000..6c5b57185 --- /dev/null +++ b/linker/linker_environ.c @@ -0,0 +1,204 @@ +/* + * Copyright (C) 2010 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ +#include "linker_environ.h" +#include + +static char** _envp; + +/* Returns 1 if 'str' points to a valid environment variable definition. + * For now, we check that: + * - It is smaller than MAX_ENV_LEN (to detect non-zero terminated strings) + * - It contains at least one equal sign that is not the first character + */ +static int +_is_valid_definition(const char* str) +{ + int pos = 0; + int first_equal_pos = -1; + + /* According to its sources, the kernel uses 32*PAGE_SIZE by default + * as the maximum size for an env. variable definition. + */ + const int MAX_ENV_LEN = 32*4096; + + if (str == NULL) + return 0; + + /* Parse the string, looking for the first '=' there, and its size */ + do { + if (str[pos] == '\0') + break; + if (str[pos] == '=' && first_equal_pos < 0) + first_equal_pos = pos; + pos++; + } while (pos < MAX_ENV_LEN); + + if (pos >= MAX_ENV_LEN) /* Too large */ + return 0; + + if (first_equal_pos < 1) /* No equal sign, or it is the first character */ + return 0; + + return 1; +} + +unsigned* +linker_env_init(unsigned* vecs) +{ + /* Store environment pointer - can't be NULL */ + _envp = (char**) vecs; + + /* Skip over all definitions */ + while (vecs[0] != 0) + vecs++; + /* The end of the environment block is marked by two NULL pointers */ + vecs++; + + /* As a sanity check, we're going to remove all invalid variable + * definitions from the environment array. + */ + { + char** readp = _envp; + char** writep = _envp; + for ( ; readp[0] != NULL; readp++ ) { + if (!_is_valid_definition(readp[0])) + continue; + writep[0] = readp[0]; + writep++; + } + writep[0] = NULL; + } + + /* Return the address of the aux vectors table */ + return vecs; +} + +/* Check if the environment variable definition at 'envstr' + * starts with '=', and if so return the address of the + * first character after the equal sign. Otherwise return NULL. + */ +static char* +env_match(char* envstr, const char* name) +{ + size_t cnt = 0; + + while (envstr[cnt] == name[cnt] && name[cnt] != '\0') + cnt++; + + if (name[cnt] != '\0' && envstr[cnt] == '=') + return envstr + cnt + 1; + + return NULL; +} + +#define MAX_ENV_LEN (16*4096) + +const char* +linker_env_get(const char* name) +{ + char** readp = _envp; + + if (name == NULL || name[0] == '\0') + return NULL; + + for ( ; readp[0] != NULL; readp++ ) { + char* val = env_match(readp[0], name); + if (val != NULL) { + /* Return NULL for empty strings, or if it is too large */ + if (val[0] == '\0') + val = NULL; + return val; + } + } + return NULL; +} + + +void +linker_env_unset(const char* name) +{ + char** readp = _envp; + char** writep = readp; + + if (name == NULL || name[0] == '\0') + return; + + for ( ; readp[0] != NULL; readp++ ) { + if (env_match(readp[0], name)) + continue; + writep[0] = readp[0]; + writep++; + } + /* end list with a NULL */ + writep[0] = NULL; +} + + + +/* Remove unsafe environment variables. This should be used when + * running setuid programs. */ +void +linker_env_secure(void) +{ + /* The same list than GLibc at this point */ + static const char* const unsec_vars[] = { + "GCONV_PATH", + "GETCONF_DIR", + "HOSTALIASES", + "LD_AUDIT", + "LD_DEBUG", + "LD_DEBUG_OUTPUT", + "LD_DYNAMIC_WEAK", + "LD_LIBRARY_PATH", + "LD_ORIGIN_PATH", + "LD_PRELOAD", + "LD_PROFILE", + "LD_SHOW_AUXV", + "LD_USE_LOAD_BIAS", + "LOCALDOMAIN", + "LOCPATH", + "MALLOC_TRACE", + "MALLOC_CHECK_", + "NIS_PATH", + "NLSPATH", + "RESOLV_HOST_CONF", + "RES_OPTIONS", + "TMPDIR", + "TZDIR", + "LD_AOUT_LIBRARY_PATH", + "LD_AOUT_PRELOAD", + }; + + const char* const* cp = unsec_vars; + const char* const* endp = cp + sizeof(unsec_vars)/sizeof(unsec_vars[0]); + + while (cp < endp) { + linker_env_unset(*cp); + cp++; + } +} diff --git a/linker/linker_environ.h b/linker/linker_environ.h new file mode 100644 index 000000000..98ad1de27 --- /dev/null +++ b/linker/linker_environ.h @@ -0,0 +1,54 @@ +/* + * Copyright (C) 2010 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ +#ifndef LINKER_ENVIRON_H +#define LINKER_ENVIRON_H + +/* Call this function before anything else. 'vecs' must be the pointer + * to the environment block in the ELF data block. The function returns + * the start of the aux vectors after the env block. + */ +extern unsigned* linker_env_init(unsigned* vecs); + +/* Unset a given environment variable. In case the variable is defined + * multiple times, unset all instances. This modifies the environment + * block, so any pointer returned by linker_env_get() after this call + * might become invalid */ +extern void linker_env_unset(const char* name); + + +/* Returns the value of environment variable 'name' if defined and not + * empty, or NULL otherwise. Note that the returned pointer may become + * invalid if linker_env_unset() or linker_env_secure() are called + * after this function. */ +extern const char* linker_env_get(const char* name); + +/* Remove unsecure environment variables. This should be used when + * running setuid programs. */ +extern void linker_env_secure(void); + +#endif /* LINKER_ENVIRON_H */ -- 2.11.0