diff mbox

[1/2] tunables: Fix environment variable processing for setuid binaries

Message ID 1485709870-25804-2-git-send-email-siddhesh@sourceware.org
State New, archived
Headers

Commit Message

Siddhesh Poyarekar Jan. 29, 2017, 5:11 p.m. UTC 
  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(-)

Comments

Florian Weimer Jan. 31, 2017, 4:51 p.m. UTC | #1 
On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
>  #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);

Is this really necessary?  That means that the tunable allocations are 
fairly substantial.

> +	      /* 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;
> +		}

This does not quite work because the next read position is not updated, 
although the tunable definition in the input string has been moved.  As 
a result, a subsequent tunable is not recognized and applied or filtered.

Thanks,
Florian
Siddhesh Poyarekar Feb. 1, 2017, 4:53 a.m. UTC | #2 
On Tuesday 31 January 2017 10:21 PM, Florian Weimer wrote:
> On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
>>  #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);
> 
> Is this really necessary?  That means that the tunable allocations are
> fairly substantial.

It is just a minor optimization - instead of allocating for every copy,
we allocate 4K at a time and then copy as we see fit.  We will end up
copying a total length < 2xlength of the tunable string so in most cases
this should only be a single 4K allocation.

>> +          /* 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;
>> +        }
> 
> This does not quite work because the next read position is not updated,
> although the tunable definition in the input string has been moved.  As
> a result, a subsequent tunable is not recognized and applied or filtered.

Ugh right, I'll post an updated patch.

Siddhesh
Carlos O'Donell Feb. 1, 2017, 5:01 a.m. UTC | #3 
On 01/31/2017 11:53 PM, Siddhesh Poyarekar wrote:
> On Tuesday 31 January 2017 10:21 PM, Florian Weimer wrote:
>> This does not quite work because the next read position is not updated,
>> although the tunable definition in the input string has been moved.  As
>> a result, a subsequent tunable is not recognized and applied or filtered.
> 
> Ugh right, I'll post an updated patch.

We really need a GLIBC_PRIVATE get/set API for regression testing all of
these changes. Work for 2.26 I guess.
Siddhesh Poyarekar Feb. 1, 2017, 11:40 a.m. UTC | #4 
On Wednesday 01 February 2017 10:31 AM, Carlos O'Donell wrote:
> We really need a GLIBC_PRIVATE get/set API for regression testing all of
> these changes. Work for 2.26 I guess.

I have added a couple of test cases now that should at least do some
rudimentary verification of the routines that remove tunables form the
valstring and environment.  We may still need something that exercises
the tunable_list to see if the level 2 tunables are correctly ignored in
setuid binaries but still passed on to the child.  That or come to a
consensus on whether to mark them as level 3 variables and simply drop
them form the environment/valstring.

Siddhesh
diff mbox

Patch

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]);
       }