diff mbox

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

Message ID 1485709870-25804-3-git-send-email-siddhesh@sourceware.org
State New
Headers show

Commit Message

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

Comments

Florian Weimer Jan. 31, 2017, 1:22 p.m. UTC | #1 
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
Siddhesh Poyarekar Jan. 31, 2017, 1:29 p.m. UTC | #2 
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
Florian Weimer Jan. 31, 2017, 3:28 p.m. UTC | #3 
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
Siddhesh Poyarekar Jan. 31, 2017, 3:42 p.m. UTC | #4 
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
diff mbox

Patch

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