From patchwork Wed Jan 25 09:25:10 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 19021 Received: (qmail 128389 invoked by alias); 25 Jan 2017 09:25:34 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 128319 invoked by uid 89); 25 Jan 2017 09:25:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_NEUTRAL autolearn=no version=3.3.2 spammy=3208, 3548, discussions, UD:list X-HELO: homiemail-a46.g.dreamhost.com From: Siddhesh Poyarekar To: libc-alpha@sourceware.org Cc: fweimer@redhat.com Subject: [PATCH 1/2] tunables: Fix environment variable processing for setuid binaries Date: Wed, 25 Jan 2017 14:55:10 +0530 Message-Id: <1485336311-2119-2-git-send-email-siddhesh@sourceware.org> In-Reply-To: <1485336311-2119-1-git-send-email-siddhesh@sourceware.org> References: <1485336311-2119-1-git-send-email-siddhesh@sourceware.org> Florian Weimer pointed out that we have three different kinds of environment variables (and hence tunables): 1. Variables that are removed for setxid processes 2. Variables that are ignored in setxid processes but is passed on to child processes 3. Variables that are passed on to child processes all the time Tunables currently only does (2) and (3) when it should be doing (1) for MALLOC_CHECK_. This patch enhances the is_secure flag in tunables to an enum value that can specify which of the above three categories the tunable (and its envvar alias) belongs to. The default is for tunables to be in (1). Hence, all of the malloc tunables barring MALLOC_CHECK_ are explicitly specified to belong to category (2). There were discussions around abolishing category (2) completely but we can do that as a separate exercise in 2.26. Tested on x86_64 to verify that there are no regressions. * elf/dl-tunable-types.h (tunable_seclevel_t): New enum. * elf/dl-tunables.c (tunables_strdup): Remove. (get_next_env): Also return the previous envp. (copy_to_heap): New function. (parse_tunables): Erase tunables of category TUNABLES_SECLEVEL_SXID_ERASE. (maybe_enable_malloc_check): Make MALLOC_CHECK_ TUNABLE_SECLEVEL_NONE if /etc/setuid-debug is accessible. (__tunables_init)[TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring]: Update GLIBC_TUNABLES envvar after parsing. [TUNABLES_FRONTEND != TUNABLES_FRONTEND_valstring]: Erase tunable envvars of category TUNABLES_SECLEVEL_SXID_ERASE. * elf/dl-tunables.h (struct _tunable): Change member is_secure to security_level. * elf/dl-tunables.list: Add security_level annotations for all tunables. * scripts/gen-tunables.awk: Recognize and generate enum values for security_level. --- elf/dl-tunable-types.h | 15 +++++ elf/dl-tunables.c | 161 +++++++++++++++++++++++++++++++++-------------- elf/dl-tunables.h | 15 +++-- elf/dl-tunables.list | 16 ++++- scripts/gen-tunables.awk | 8 +-- 5 files changed, 157 insertions(+), 58 deletions(-) diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h index 5273dab..a986f0b 100644 --- a/elf/dl-tunable-types.h +++ b/elf/dl-tunable-types.h @@ -43,4 +43,19 @@ typedef union const char *strval; } tunable_val_t; +/* Security level for tunables. This decides what to do with individual + tunables for AT_SECURE binaries. */ +typedef enum +{ + /* Erase the tunable for AT_SECURE binaries so that child processes don't + read it. */ + TUNABLE_SECLEVEL_SXID_ERASE = 0, + /* Ignore the tunable for AT_SECURE binaries, but don't erase it, so that + child processes can read it. */ + TUNABLE_SECLEVEL_SXID_IGNORE = 1, + /* Read the tunable. */ + TUNABLE_SECLEVEL_NONE = 2, +} tunable_seclevel_t; + + #endif diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c index cbf4c8e..c3d4c24 100644 --- a/elf/dl-tunables.c +++ b/elf/dl-tunables.c @@ -50,36 +50,13 @@ is_name (const char *orig, const char *envname) return false; } -#if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring -static char * -tunables_strdup (const char *in) -{ - size_t i = 0; - - while (in[i++] != '\0'); - char *out = __sbrk (i); - - /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to - set the thread-local errno since the TCB has not yet been set up. This - needs to be fixed with an __sbrk implementation that does not set - errno. */ - if (out == (void *)-1) - return NULL; - - i--; - - while (i-- > 0) - out[i] = in[i]; - - return out; -} -#endif - static char ** -get_next_env (char **envp, char **name, size_t *namelen, char **val) +get_next_env (char **envp, char **name, size_t *namelen, char **val, + char ***prev_envp) { while (envp != NULL && *envp != NULL) { + char **prev = envp; char *envline = *envp++; int len = 0; @@ -93,6 +70,7 @@ get_next_env (char **envp, char **name, size_t *namelen, char **val) *name = envline; *namelen = len; *val = &envline[len + 1]; + *prev_envp = prev; return envp; } @@ -243,6 +221,45 @@ tunable_initialize (tunable_t *cur, const char *strval) } #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring +# define ALLOC_SIZE 4096 +/* Allocate bytes on heap to store tunable values copied over from the + valstring. We use a hardcoded ALLOC_SIZE to avoid querying the page size, + since it may not be available this early in the startup process. */ +static char * +copy_to_heap (const char *in, size_t len) +{ + static size_t heap_size = 0; + static char *heap = NULL; + char *out; + + if (heap_size < len + 1) + { + size_t ext = ALIGN_UP (len + 1, ALLOC_SIZE); + out = __sbrk (ext); + + /* FIXME: In reality if the allocation fails, __sbrk will crash attempting to + set the thread-local errno since the TCB has not yet been set up. This + needs to be fixed with an __sbrk implementation that does not set + errno. */ + if (out == (void *) -1) + return NULL; + + heap_size += ext; + + if (heap == NULL) + heap = out; + } + + out = heap; + while (len--) + *heap++ = *in++; + *heap++ = '\0'; + + heap_size -= len + 1; + + return out; +} + static void parse_tunables (char *tunestr) { @@ -281,31 +298,42 @@ parse_tunables (char *tunestr) while (p[len] != ':' && p[len] != '\0') len++; - char end = p[len]; - p[len] = '\0'; - /* Add the tunable if it exists. */ for (size_t i = 0; i < sizeof (tunable_list) / sizeof (tunable_t); i++) { tunable_t *cur = &tunable_list[i]; - /* If we are in a secure context (AT_SECURE) then ignore the tunable - unless it is explicitly marked as secure. Tunable values take - precendence over their envvar aliases. */ - if (__libc_enable_secure && !cur->is_secure) - continue; - if (is_name (cur->name, name)) { - tunable_initialize (cur, value); + /* If we are in a secure context (AT_SECURE) then ignore the tunable + unless it is explicitly marked as secure. Tunable values take + precendence over their envvar aliases. */ + if (__libc_enable_secure) + { + if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE) + { + char *q = &p[len]; + while (*q != '\0') + *name++ = *q++; + name[0] = '\0'; + len = 0; + } + + if (cur->security_level != TUNABLE_SECLEVEL_NONE) + continue; + } + + char *val = copy_to_heap (value, len); + if (val != NULL) + tunable_initialize (cur, val); break; } } - if (end == ':') + if (p[len] == '\0') + return; + else p += len + 1; - else - return; } } #endif @@ -320,8 +348,9 @@ static inline void __always_inline maybe_enable_malloc_check (void) { - if (__access_noerrno ("/etc/suid-debug", F_OK) == 0) - tunable_list[TUNABLE_ENUM_NAME(glibc, malloc, check)].is_secure = true; + tunable_id_t id = TUNABLE_ENUM_NAME(glibc, malloc, check); + if (__libc_enable_secure && __access_noerrno ("/etc/suid-debug", F_OK) == 0) + tunable_list[id].security_level = TUNABLE_SECLEVEL_NONE; } /* Initialize the tunables list from the environment. For now we only use the @@ -333,17 +362,23 @@ __tunables_init (char **envp) char *envname = NULL; char *envval = NULL; size_t len = 0; + char **prev_envp = envp; maybe_enable_malloc_check (); - while ((envp = get_next_env (envp, &envname, &len, &envval)) != NULL) + while ((envp = get_next_env (envp, &envname, &len, &envval, + &prev_envp)) != NULL) { #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring if (is_name (GLIBC_TUNABLES, envname)) { - char *val = tunables_strdup (envval); - if (val != NULL) - parse_tunables (val); + size_t i = 0; + while (envname[i++] != '\0'); + char *new_env = copy_to_heap (envname, i); + if (new_env != NULL) + parse_tunables (new_env + len + 1); + /* Put in the updated envval. */ + *prev_envp = new_env; continue; } #endif @@ -354,8 +389,7 @@ __tunables_init (char **envp) /* Skip over tunables that have either been set already or should be skipped. */ - if (cur->strval != NULL || cur->env_alias == NULL - || (__libc_enable_secure && !cur->is_secure)) + if (cur->strval != NULL || cur->env_alias == NULL) continue; const char *name = cur->env_alias; @@ -363,6 +397,39 @@ __tunables_init (char **envp) /* We have a match. Initialize and move on to the next line. */ if (is_name (name, envname)) { + /* For AT_SECURE binaries, we need to check the security settings of + the tunable and decide whether we read the value and also whether + we erase the value so that child processes don't inherit them in + the environment. */ + if (__libc_enable_secure) + { + if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE) + { + /* Erase the environment variable. */ + char **ep = prev_envp; + + while (*ep != NULL) + { + if (is_name (name, *ep)) + { + char **dp = ep; + + do + dp[0] = dp[1]; + while (*dp++); + } + else + ++ep; + } + /* Reset the iterator so that we read the environment again + from the point we erased. */ + envp = prev_envp; + } + + if (cur->security_level != TUNABLE_SECLEVEL_NONE) + continue; + } + tunable_initialize (cur, envval); break; } diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h index e07825c..f33adfb 100644 --- a/elf/dl-tunables.h +++ b/elf/dl-tunables.h @@ -41,11 +41,16 @@ struct _tunable tunable_val_t val; /* The value. */ const char *strval; /* The string containing the value, points into envp. */ - bool is_secure; /* Whether the tunable must be read - even for setuid binaries. Note that - even if the tunable is read, it may - not get used by the target module if - the value is considered unsafe. */ + tunable_seclevel_t security_level; /* Specify the security level for the + tunable with respect to AT_SECURE + programs. See description of + tunable_seclevel_t to see a + description of the values. + + Note that even if the tunable is + read, it may not get used by the + target module if the value is + considered unsafe. */ /* Compatibility elements. */ const char *env_alias; /* The compatibility environment variable name. */ diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list index d8cd912..cb9e8f1 100644 --- a/elf/dl-tunables.list +++ b/elf/dl-tunables.list @@ -21,8 +21,13 @@ # minval: Optional minimum acceptable value # maxval: Optional maximum acceptable value # env_alias: An alias environment variable -# is_secure: Specify whether the environment variable should be read for -# setuid binaries. +# security_level: Specify security level of the tunable. Valid values are: +# +# SXID_ERASE: (default) Don't read for AT_SECURE binaries and +# removed so that child processes can't read it. +# SXID_IGNORE: Don't read for AT_SECURE binaries, but retained for +# non-AT_SECURE subprocesses. +# SXID_NONE: Read all the time. glibc { malloc { @@ -35,34 +40,41 @@ glibc { top_pad { type: SIZE_T env_alias: MALLOC_TOP_PAD_ + security_level: SXID_IGNORE } perturb { type: INT_32 minval: 0 maxval: 0xff env_alias: MALLOC_PERTURB_ + security_level: SXID_IGNORE } mmap_threshold { type: SIZE_T env_alias: MALLOC_MMAP_THRESHOLD_ + security_level: SXID_IGNORE } trim_threshold { type: SIZE_T env_alias: MALLOC_TRIM_THRESHOLD_ + security_level: SXID_IGNORE } mmap_max { type: INT_32 env_alias: MALLOC_MMAP_MAX_ + security_level: SXID_IGNORE } arena_max { type: SIZE_T env_alias: MALLOC_ARENA_MAX minval: 1 + security_level: SXID_IGNORE } arena_test { type: SIZE_T env_alias: MALLOC_ARENA_TEST minval: 1 + security_level: SXID_IGNORE } } } diff --git a/scripts/gen-tunables.awk b/scripts/gen-tunables.awk index b65b5a4..e7bfc22 100644 --- a/scripts/gen-tunables.awk +++ b/scripts/gen-tunables.awk @@ -52,7 +52,7 @@ $1 == "}" { env_alias[top_ns][ns][tunable] = "NULL" } if (!is_secure[top_ns][ns][tunable]) { - is_secure[top_ns][ns][tunable] = "false" + is_secure[top_ns][ns][tunable] = "SXID_ERASE" } tunable = "" @@ -102,8 +102,8 @@ $1 == "}" { else if (attr == "env_alias") { env_alias[top_ns][ns][tunable] = sprintf("\"%s\"", val) } - else if (attr == "is_secure") { - if (val == "true" || val == "false") { + else if (attr == "security_level") { + if (val == "SXID_ERASE" || val == "SXID_IGNORE" || val == "NONE") { is_secure[top_ns][ns][tunable] = val } else { @@ -146,7 +146,7 @@ END { for (n in types[t]) { for (m in types[t][n]) { printf (" {TUNABLE_NAME_S(%s, %s, %s)", t, n, m) - printf (", {TUNABLE_TYPE_%s, %s, %s}, {.numval = 0}, NULL, %s, %s},\n", + printf (", {TUNABLE_TYPE_%s, %s, %s}, {.numval = 0}, NULL, TUNABLE_SECLEVEL_%s, %s},\n", types[t][n][m], minvals[t][n][m], maxvals[t][n][m], is_secure[t][n][m], env_alias[t][n][m]); }