[PATCH 0/2] Environment variable security and tunables

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] Environment variable security and tunables

Siddhesh Poyarekar-9
Hi,

Here's a patchset that fixes environment variable processing for AT_SECURE
processes.  The second patch removes GLIBC_TUNABLES from AT_SECURE processes
even when tunables are not built, to avoid passing on the variable (and hence
unsafe tunables) to child processes who may end up loading a glibc with
tunables enabled.

I will follow up with a patch for 2.24 to add GLIBC_TUNABLES to
unsecure-envvars.

Siddhesh

Siddhesh Poyarekar (2):
  tunables: Fix environment variable processing for setuid binaries
  Erase GLIBC_TUNABLES for setxid processes when tunables is disabled

 elf/dl-tunable-types.h   |  15 +++++
 elf/dl-tunables.c        | 165 +++++++++++++++++++++++++++++------------------
 elf/dl-tunables.h        |  64 ++++++++++++++++--
 elf/dl-tunables.list     |  16 ++++-
 scripts/gen-tunables.awk |   8 +--
 5 files changed, 191 insertions(+), 77 deletions(-)

--
2.7.4

Reply | Threaded
Open this post in threaded view
|

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

Siddhesh Poyarekar-9
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

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Erase GLIBC_TUNABLES for setxid processes when tunables is disabled

Siddhesh Poyarekar-9
In reply to this post by Siddhesh Poyarekar-9
A setxid program that uses a glibc with tunables disabled may pass on
GLIBC_TUNABLES as is to its child processes.  If the child process
ends up using a different glibc that has tunables enabled, it will end
up getting access to unsafe tunables.  To fix this, remove
GLIBC_TUNABLES from the environment for setxid process.

In addition to this, I'll post a patch for earlier releases (2.24 and
older) to add GLIBC_TUNABLES to unsecure_envvars (is unsecure even a
word?) so that they too don't end up passing on unsafe tunables.
---
 elf/dl-tunables.c | 40 +++++-----------------------------------
 elf/dl-tunables.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 37 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index c3d4c24..02aa864 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -31,25 +31,9 @@
 #include "dl-tunables.h"
 
 #if TUNABLES_FRONTEND == TUNABLES_FRONTEND_valstring
-# define GLIBC_TUNABLES "GLIBC_TUNABLES"
+# define GLIBC_TUNABLES TUNABLES_VALSTRING_ENVVAR
 #endif
 
-/* Compare environment or tunable names, bounded by the name hardcoded in
-   glibc.  */
-static bool
-is_name (const char *orig, const char *envname)
-{
-  for (;*orig != '\0' && *envname != '\0'; envname++, orig++)
-    if (*orig != *envname)
-      break;
-
-  /* The ENVNAME is immediately followed by a value.  */
-  if (*orig == '\0' && *envname == '=')
-    return true;
-  else
-    return false;
-}
-
 static char **
 get_next_env (char **envp, char **name, size_t *namelen, char **val,
       char ***prev_envp)
@@ -405,24 +389,10 @@ __tunables_init (char **envp)
  {
   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.  */
+      /* Erase the environment variable and then reset the
+ iterator so that we read the environment again from
+ the point we erased.  */
+      tunables_delete_env (name, prev_envp)
       envp = prev_envp;
     }
 
diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h
index f33adfb..589e22f 100644
--- a/elf/dl-tunables.h
+++ b/elf/dl-tunables.h
@@ -21,12 +21,57 @@
 #ifndef _TUNABLES_H_
 #define _TUNABLES_H_
 
+#define TUNABLES_VALSTRING_ENVVAR "GLIBC_TUNABLES"
+
+/* Compare environment or tunable names, bounded by the name hardcoded in
+   glibc.  */
+static inline bool
+__always_inline
+is_name (const char *orig, const char *envname)
+{
+  for (;*orig != '\0' && *envname != '\0'; envname++, orig++)
+    if (*orig != *envname)
+      break;
+
+  /* The ENVNAME is immediately followed by a value.  */
+  if (*orig == '\0' && *envname == '=')
+    return true;
+  else
+    return false;
+}
+
+static inline void
+__always_inline
+tunables_delete_env (const char *name, char **envp)
+{
+  if (envp == NULL)
+    return;
+
+  while (*envp != NULL)
+    {
+      if (is_name (name, *envp))
+ {
+  char **dp = envp;
+
+  do
+    dp[0] = dp[1];
+  while (*dp++);
+ }
+      else
+ ++envp;
+    }
+}
+
 #if !HAVE_TUNABLES
 static inline void
 __always_inline
-__tunables_init (char **unused __attribute__ ((unused)))
+__tunables_init (char **envp)
 {
-  /* This is optimized out if tunables are not enabled.  */
+  /* If tunables is not enabled, we want to make sure that we don't pass on
+     insecure tunables to child processes of setxid processes, so just drop
+     GLIBC_TUNABLES from the environment.  */
+  if (__libc_enable_secure)
+    tunables_delete_env (TUNABLES_VALSTRING_ENVVAR, envp);
 }
 #else
 
--
2.7.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] Environment variable security and tunables

Siddhesh Poyarekar-8
In reply to this post by Siddhesh Poyarekar-9
Please ignore this, I botched up testing this so I'm repeating the test
and will post an updated patchset.

Siddhesh

On Wednesday 25 January 2017 02:55 PM, Siddhesh Poyarekar wrote:

> Hi,
>
> Here's a patchset that fixes environment variable processing for AT_SECURE
> processes.  The second patch removes GLIBC_TUNABLES from AT_SECURE processes
> even when tunables are not built, to avoid passing on the variable (and hence
> unsafe tunables) to child processes who may end up loading a glibc with
> tunables enabled.
>
> I will follow up with a patch for 2.24 to add GLIBC_TUNABLES to
> unsecure-envvars.
>
> Siddhesh
>
> Siddhesh Poyarekar (2):
>   tunables: Fix environment variable processing for setuid binaries
>   Erase GLIBC_TUNABLES for setxid processes when tunables is disabled
>
>  elf/dl-tunable-types.h   |  15 +++++
>  elf/dl-tunables.c        | 165 +++++++++++++++++++++++++++++------------------
>  elf/dl-tunables.h        |  64 ++++++++++++++++--
>  elf/dl-tunables.list     |  16 ++++-
>  scripts/gen-tunables.awk |   8 +--
>  5 files changed, 191 insertions(+), 77 deletions(-)
>