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

[Siddhesh Poyarekar <siddhesh at sourceware dot 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]);
       }
-- 
2.7.4