tunables: Fix environment variable processing for setuid binaries (bz #21073)

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.

	[BZ #21073]
	* 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.
	(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/tst-env-setuid.c: New test case.
	* elf/tst-env-setuid-tunables: new test case.
	* elf/Makefile (tests-static): Add them.

7 files changed, 482 insertions, 31 deletions

diff --git a/elf/Makefile b/elf/Makefile
index c7a29693bd..61abeb59ee 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -149,7 +149,7 @@ tests-static = tst-tls1-static tst-tls2-static tst-stackguard1-static \
 	       tst-leaks1-static tst-array1-static tst-array5-static \
 	       tst-ptrguard1-static tst-dl-iter-static \
 	       tst-tlsalign-static tst-tlsalign-extern-static \
-	       tst-linkall-static
+	       tst-linkall-static tst-env-setuid tst-env-setuid-tunables
 ifeq (yes,$(build-shared))
 tests-static += tst-tls9-static
 tst-tls9-static-ENV = \
@@ -1397,3 +1397,7 @@ $(objpfx)tst-nodelete-dlclose-plugin.so: $(objpfx)tst-nodelete-dlclose-dso.so
 $(objpfx)tst-nodelete-dlclose: $(objpfx)tst-nodelete-dlclose-dso.so
 $(objpfx)tst-nodelete-dlclose.out: $(objpfx)tst-nodelete-dlclose-dso.so \
 				   $(objpfx)tst-nodelete-dlclose-plugin.so
+
+tst-env-setuid-ENV = MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096
+tst-env-setuid-tunables-ENV = \
+	GLIBC_TUNABLES=glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096
diff --git a/elf/dl-tunable-types.h b/elf/dl-tunable-types.h
index 5273dab48f..a986f0b593 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 cbf4c8e8f2..a8d53d6a31 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -76,10 +76,12 @@ tunables_strdup (const char *in)
 #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 +95,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,8 +246,13 @@ tunable_initialize (tunable_t *cur, const char *strval)
 }
 
 #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
+/* Parse the tunable string TUNESTR and adjust it to drop any tunables that may
+   be unsafe for AT_SECURE processes so that it can be used as the new
+   environment variable value for GLIBC_TUNABLES.  VALSTRING is the original
+   environment variable string which we use to make NULL terminated values so
+   that we don't have to allocate memory again for it.  */
 static void
-parse_tunables (char *tunestr)
+parse_tunables (char *tunestr, char *valstring)
 {
   if (tunestr == NULL || *tunestr == '\0')
     return;
@@ -275,37 +283,65 @@ parse_tunables (char *tunestr)
 
       p += len + 1;
 
-      char *value = p;
+      /* Take the value from the valstring since we need to NULL terminate it.  */
+      char *value = &valstring[p - tunestr];
       len = 0;
 
       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))
 	    {
+	      /* 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)
+		    {
+		      if (p[len] == '\0')
+			{
+			  /* Last tunable in the valstring.  Null-terminate and
+			     return.  */
+			  *name = '\0';
+			  return;
+			}
+		      else
+			{
+			  /* Remove the current tunable from the string.  We do
+			     this by overwriting the string starting from NAME
+			     (which is where the current tunable begins) with
+			     the remainder of the string.  We then have P point
+			     to NAME so that we continue in the correct
+			     position in the valstring.  */
+			  char *q = &p[len + 1];
+			  p = name;
+			  while (*q != '\0')
+			    *name++ = *q++;
+			  name[0] = '\0';
+			  len = 0;
+			}
+		    }
+
+		  if (cur->security_level != TUNABLE_SECLEVEL_NONE)
+		    break;
+		}
+
+	      value[len] = '\0';
 	      tunable_initialize (cur, value);
 	      break;
 	    }
 	}
 
-      if (end == ':')
-	p += len + 1;
-      else
+      if (p[len] == '\0')
 	return;
+      else
+	p += len + 1;
     }
 }
 #endif
@@ -320,8 +356,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 +370,21 @@ __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);
+	  char *new_env = tunables_strdup (envname);
+	  if (new_env != NULL)
+	    parse_tunables (new_env + len + 1, envval);
+	  /* Put in the updated envval.  */
+	  *prev_envp = new_env;
 	  continue;
 	}
 #endif
@@ -354,8 +395,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 +403,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 e07825c443..f33adfb359 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 d8cd912559..cb9e8f173b 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/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
new file mode 100644
index 0000000000..a563f69972
--- /dev/null
+++ b/elf/tst-env-setuid-tunables.c
@@ -0,0 +1,60 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Verify that tunables correctly filter out unsafe tunables like
+   glibc.malloc.check and glibc.malloc.mmap_threshold but also retain
+   glibc.malloc.mmap_threshold in an unprivileged child.  */
+
+#define test_parent test_parent_tunables
+#define test_child test_child_tunables
+
+static int test_child_tunables (void);
+static int test_parent_tunables (void);
+
+#include "tst-env-setuid.c"
+
+#define CHILD_VALSTRING_VALUE "glibc.malloc.mmap_threshold=4096"
+#define PARENT_VALSTRING_VALUE \
+  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096"
+
+static int
+test_child_tunables (void)
+{
+  const char *val = getenv ("GLIBC_TUNABLES");
+
+  if (val != NULL && strcmp (val, CHILD_VALSTRING_VALUE) == 0)
+    return 0;
+
+  if (val != NULL)
+    printf ("Unexpected GLIBC_TUNABLES VALUE %s\n", val);
+
+  return 1;
+}
+
+static int
+test_parent_tunables (void)
+{
+  const char *val = getenv ("GLIBC_TUNABLES");
+
+  if (val != NULL && strcmp (val, PARENT_VALSTRING_VALUE) == 0)
+    return 0;
+
+  if (val != NULL)
+    printf ("Unexpected GLIBC_TUNABLES VALUE %s\n", val);
+
+  return 1;
+}
diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
new file mode 100644
index 0000000000..85d423d799
--- /dev/null
+++ b/elf/tst-env-setuid.c
@@ -0,0 +1,282 @@
+/* Copyright (C) 2012-2017 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/* Verify that tunables correctly filter out unsafe environment variables like
+   MALLOC_CHECK_ and MALLOC_MMAP_THRESHOLD_ but also retain
+   MALLOC_MMAP_THRESHOLD_ in an unprivileged child.  */
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <unistd.h>
+
+#include <support/support.h>
+#include <support/test-driver.h>
+
+static char SETGID_CHILD[] = "setgid-child";
+#define CHILD_STATUS 42
+
+/* Return a GID which is not our current GID, but is present in the
+   supplementary group list.  */
+static gid_t
+choose_gid (void)
+{
+  const int count = 64;
+  gid_t groups[count];
+  int ret = getgroups (count, groups);
+  if (ret < 0)
+    {
+      printf ("getgroups: %m\n");
+      exit (1);
+    }
+  gid_t current = getgid ();
+  for (int i = 0; i < ret; ++i)
+    {
+      if (groups[i] != current)
+	return groups[i];
+    }
+  return 0;
+}
+
+/* Spawn and execute a program and verify that it returns the CHILD_STATUS.  */
+static pid_t
+do_execve (char **args)
+{
+  pid_t kid = vfork ();
+
+  if (kid < 0)
+    {
+      printf ("vfork: %m\n");
+      return -1;
+    }
+
+  if (kid == 0)
+    {
+      /* Child process.  */
+      execve (args[0], args, environ);
+      _exit (-errno);
+    }
+
+  if (kid < 0)
+    return 1;
+
+  int status;
+
+  if (waitpid (kid, &status, 0) < 0)
+    {
+      printf ("waitpid: %m\n");
+      return 1;
+    }
+
+  if (!WIFEXITED (status) || WEXITSTATUS (status) != CHILD_STATUS)
+    {
+      printf ("Unexpected exit status %d from child process\n",
+	      status);
+      return 1;
+    }
+  return 0;
+}
+
+/* Copies the executable into a restricted directory, so that we can
+   safely make it SGID with the TARGET group ID.  Then runs the
+   executable.  */
+static int
+run_executable_sgid (gid_t target)
+{
+  char *dirname = xasprintf ("%s/tst-tunables-setuid.%jd",
+			     test_dir, (intmax_t) getpid ());
+  char *execname = xasprintf ("%s/bin", dirname);
+  int infd = -1;
+  int outfd = -1;
+  int ret = 0;
+  if (mkdir (dirname, 0700) < 0)
+    {
+      printf ("mkdir: %m\n");
+      goto err;
+    }
+  infd = open ("/proc/self/exe", O_RDONLY);
+  if (infd < 0)
+    {
+      printf ("open (/proc/self/exe): %m\n");
+      goto err;
+    }
+  outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700);
+  if (outfd < 0)
+    {
+      printf ("open (%s): %m\n", execname);
+      goto err;
+    }
+  char buf[4096];
+  for (;;)
+    {
+      ssize_t rdcount = read (infd, buf, sizeof (buf));
+      if (rdcount < 0)
+	{
+	  printf ("read: %m\n");
+	  goto err;
+	}
+      if (rdcount == 0)
+	break;
+      char *p = buf;
+      char *end = buf + rdcount;
+      while (p != end)
+	{
+	  ssize_t wrcount = write (outfd, buf, end - p);
+	  if (wrcount == 0)
+	    errno = ENOSPC;
+	  if (wrcount <= 0)
+	    {
+	      printf ("write: %m\n");
+	      goto err;
+	    }
+	  p += wrcount;
+	}
+    }
+  if (fchown (outfd, getuid (), target) < 0)
+    {
+      printf ("fchown (%s): %m\n", execname);
+      goto err;
+    }
+  if (fchmod (outfd, 02750) < 0)
+    {
+      printf ("fchmod (%s): %m\n", execname);
+      goto err;
+    }
+  if (close (outfd) < 0)
+    {
+      printf ("close (outfd): %m\n");
+      goto err;
+    }
+  if (close (infd) < 0)
+    {
+      printf ("close (infd): %m\n");
+      goto err;
+    }
+
+  char *args[] = {execname, SETGID_CHILD, NULL};
+
+  ret = do_execve (args);
+
+err:
+  if (outfd >= 0)
+    close (outfd);
+  if (infd >= 0)
+    close (infd);
+  if (execname)
+    {
+      unlink (execname);
+      free (execname);
+    }
+  if (dirname)
+    {
+      rmdir (dirname);
+      free (dirname);
+    }
+  return ret;
+}
+
+#ifndef test_child
+static int
+test_child (void)
+{
+  if (getenv ("MALLOC_CHECK_") != NULL)
+    {
+      printf ("MALLOC_CHECK_ is still set\n");
+      return 1;
+    }
+
+  if (getenv ("MALLOC_MMAP_THRESHOLD_") == NULL)
+    {
+      printf ("MALLOC_MMAP_THRESHOLD_ lost\n");
+      return 1;
+    }
+
+  return 0;
+}
+#endif
+
+#ifndef test_parent
+static int
+test_parent (void)
+{
+  if (getenv ("MALLOC_CHECK_") == NULL)
+    {
+      printf ("MALLOC_CHECK_ lost\n");
+      return 1;
+    }
+
+  if (getenv ("MALLOC_MMAP_THRESHOLD_") == NULL)
+    {
+      printf ("MALLOC_MMAP_THRESHOLD_ lost\n");
+      return 1;
+    }
+
+  return 0;
+}
+#endif
+
+static int
+do_test_prep (int argc, char **argv)
+{
+  /* Setgid child process.  */
+  if (argc == 2 && strcmp (argv[1], SETGID_CHILD) == 0)
+    {
+      if (getgid () == getegid ())
+	{
+	  /* This can happen if the file system is mounted nosuid.  */
+	  fprintf (stderr, "SGID failed: GID and EGID match (%jd)\n",
+		   (intmax_t) getgid ());
+	  exit (EXIT_UNSUPPORTED);
+	}
+
+      int ret = test_child ();
+
+      if (ret != 0)
+	exit (1);
+
+      exit (CHILD_STATUS);
+    }
+  else
+    {
+      if (test_parent () != 0)
+	exit (1);
+
+      /* Try running a setgid program.  */
+      gid_t target = choose_gid ();
+      if (target == 0)
+	{
+	  fprintf (stderr,
+		   "Could not find a suitable GID for user %jd, skipping test\n",
+		   (intmax_t) getuid ());
+	  exit (0);
+	}
+
+      if (run_executable_sgid (target) == 0)
+	exit (0);
+    }
+
+  /* Something went wrong and our argv was corrupted.  */
+  _exit (1);
+}
+
+#define TEST_FUNCTION_ARGV do_test_prep
+#include <support/test-driver.c>