Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Sat, 23 Mar 2013 02:12:34 +0400
From: "Dmitry V. Levin" <ldv@...linux.org>
To: owl-dev@...ts.openwall.com
Subject: Re: glibc

On Mon, Nov 05, 2012 at 01:23:58AM +0400, Dmitry V. Levin wrote:
> On Sun, Nov 04, 2012 at 11:21:42PM +0400, Vasily Kulikov wrote:
> > On Wed, Oct 31, 2012 at 22:55 +0400, Dmitry V. Levin wrote:
> > > The first one already contains references to getenv, and it goes to libc.a
> > > anyway, so this is not an issue.
> > > 
> > > The second one comes from elf/rtld.c that had no references to
> > > getenv before.
> > 
> > Sure.
> > 
> > > The following patch fixes the linking problem by replacing __strncmp_ia32
> > > with strncmp and removing __GI_strncmp in rtld-strncmp-c.os:
> > 
> > This works, thanks!  I see you've added it to your git tree only now,
> > did it work before or you just haven't tested the tree on i686?
> 
> There is no i686 packages in Sisyphus so I do not usually build glibc for
> i686 and I haven't tested my 2.16+ tree on i686 before.

For the record: the right fix for this issue is to avoid introducing
getenv to rtld, for two reasons:
- getenv uses strncmp, and strncmp is not actually implemented in rtld on
  x86-64 because rtld is not used to call strncmp directly or indirectly,
  see also http://sourceware.org/bugzilla/show_bug.cgi?id=11120
- locale-related environment sanitation could be implemented in a more
  reliable way without relying on getenv.

I've applied the following change to my 2.16+ tree:

diff --git a/elf/dl-environ.c b/elf/dl-environ.c
index cd56998..97bd6bc 100644
--- a/elf/dl-environ.c
+++ b/elf/dl-environ.c
@@ -83,3 +83,33 @@ unsetenv (const char *name)
 
   return 0;
 }
+
+void
+unset_unsafe_env (const char *name)
+{
+  char **ep;
+
+  ep = __environ;
+  while (*ep != NULL)
+    {
+      size_t cnt = 0;
+
+      while ((*ep)[cnt] == name[cnt] && name[cnt] != '\0')
+	++cnt;
+
+      if (name[cnt] == '\0' && (*ep)[cnt] == '=' &&
+	  ((*ep)[++cnt] == '.' || strchr((*ep)+cnt, '/')))
+	{
+	  /* Found it.  Remove this pointer by moving later ones to
+	     the front.  */
+	  char **dp = ep;
+
+	  do
+	    dp[0] = dp[1];
+	  while (*dp++);
+	  /* Continue the loop in case NAME appears again.  */
+	}
+      else
+	++ep;
+    }
+}
diff --git a/elf/rtld.c b/elf/rtld.c
index 58f5c1a..a5440ea 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -2780,16 +2780,11 @@ process_envvars (enum mode *modep)
 	    }
 	}
 
-      /* This loop is buggy: it will only check the first occurrence of each
-	 variable (but will correctly remove all in case of a match).  This
-	 may be a problem if the list is later re-ordered or accessed by an
-	 application with something other than the glibc getenv().  */
       for (nextp = restricted_envvars; *nextp != '\0';
 	   nextp = (char *) rawmemchr (nextp, '\0') + 1)
 	{
-	  const char *value = getenv (nextp);
-	  if (value && (value[0] == '.' || strchr(value, '/')))
-	    unsetenv (nextp);
+	  extern void unset_unsafe_env (const char *) __THROW;
+	  unset_unsafe_env (nextp);
 	}
 
       if (__access ("/etc/suid-debug", F_OK) != 0)


-- 
ldv

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.