[PATCH 0/2] Environment variable security and tunables

classic Classic list List threaded Threaded
12 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 (GLIBC_TUNABLES): Define as
        TUNABLES_VALSTRING_ENVVAR.
        (__tunables_init): Call tunables_delete_env.
        (is_name): Move definition...
        * elf/dl-tunables.h: ... here.
        (TUNABLES_VALSTRING_ENVVAR): New macro.
        (tunables_delete_env): New function.
        [!HAVE_TUNABLES](__tunables_init): Call it.
---
 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..23208b5 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
|

[PING][PATCH 0/2] Environment variable security and tunables

Siddhesh Poyarekar-8
In reply to this post by Siddhesh Poyarekar-9
Ping!

On Sunday 29 January 2017 10:41 PM, Siddhesh Poyarekar wrote:

> 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(-)
>
Reply | Threaded
Open this post in threaded view
|

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

Florian Weimer-5
In reply to this post by Siddhesh Poyarekar-9
On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
> 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.

I expected a patch with a preprocessor condition for unsecvars.h for
glibc 2.25.  Wouldn't this have the same effect than your tunables-based
changes?

Thanks,
Florian
Reply | Threaded
Open this post in threaded view
|

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

Siddhesh Poyarekar-9
On Tuesday 31 January 2017 06:52 PM, Florian Weimer wrote:
> On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
>> 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.
>
> I expected a patch with a preprocessor condition for unsecvars.h for
> glibc 2.25.  Wouldn't this have the same effect than your tunables-based
> changes?

It would, but I chose to limit the change to within the tunables code
base.  I can do it your way if that's your preference.

Siddhesh
Reply | Threaded
Open this post in threaded view
|

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

Florian Weimer-5
On 01/31/2017 02:29 PM, Siddhesh Poyarekar wrote:

> On Tuesday 31 January 2017 06:52 PM, Florian Weimer wrote:
>> On 01/29/2017 06:11 PM, Siddhesh Poyarekar wrote:
>>> 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.
>>
>> I expected a patch with a preprocessor condition for unsecvars.h for
>> glibc 2.25.  Wouldn't this have the same effect than your tunables-based
>> changes?
>
> It would, but I chose to limit the change to within the tunables code
> base.  I can do it your way if that's your preference.

I'd suggest to keep the changes to a non-tunables configuration to a
minimum, and unsecvars.h is better at that goal, I think.

Thanks,
Florian

Reply | Threaded
Open this post in threaded view
|

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

Siddhesh Poyarekar-9
On Tuesday 31 January 2017 08:58 PM, Florian Weimer wrote:
> I'd suggest to keep the changes to a non-tunables configuration to a
> minimum, and unsecvars.h is better at that goal, I think.

Fair enough.  I have to step out now, so I'll post a modified version of
the 2.24 patch for the 2/2 tomorrow.  What do you think about 1/2?

Siddhesh
Reply | Threaded
Open this post in threaded view
|

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

Florian Weimer-5
In reply to this post by Siddhesh Poyarekar-9
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

Reply | Threaded
Open this post in threaded view
|

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

Siddhesh Poyarekar-9
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
Reply | Threaded
Open this post in threaded view
|

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

Carlos O'Donell-6
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.

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

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

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