Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date: Thu, 19 Feb 2015 22:19:23 +0000
From: Tim Brown <tmb@...35.com>
To: oss-security@...ts.openwall.com
Subject: Fixing the glibc runtime linker

A while ago, I suggested that I'd been working on glibc with regard to making 
it handle runtime linking more securely. Today, I finally managed to get a 
patch together which I think is worth sharing. I'd therefore like to issue a 
request for comments for the attached patch, and enquire if any distros feel 
like incorporating it? A few points on the rationale...

Why? 

Over the last couple of years I've spent a good deal of time dealing with 
vendors who, for one reason or another have shipped binaries where it is 
possible to inject "untrusted" code into running processes, notably but not 
exclusively via DT_RPATH.

What's the fix?

More often than not, the underlying issue is an empty element within the 
DT_RPATH header or equivalent. Sometimes it's not, but even in those cases, it 
is largely that one or more elements isn't qualifed (i.e. it doesn't start 
with /). The attached patch fixes this, by ignoring any elements of DT_RPATH, 
LD_LIBRARY_PATH that do not start with a /, and/or junking any use of dlopen 
where the filename is likewise unqualified.

Won't this break stuff?

Maybe (certainly it is means a change to glibc behaviour), but more often than 
not, the fact that a given binary currently works in an unsafe way is a bug - 
and an exploitable one at that. Moreoever, Solaris has had a similar sanitity 
check (in their case only for privileged setuid binaries) for a good number of 
years without serious incident. I believe we should be fixing software that 
exhibits the behaviour I've described, but this patch will (I think) kill the 
bug class irrespective of that.

Further thoughts?

The patch attached is the most robust variant I've produced in that it kills 
unqualified linker paths, irrespective of the privilege or otherwise of the 
affected binary. We could kill the checks for non-setuid binaries or we could 
add some additional errors in such cases. I did experiment with only checking 
a subset of cases (namely where LD_LIBRARY_PATH itself is set) if the process 
wasn't privileged, but in the end, I concluded that the loading of any 
unqualified linker path could provide an exploit vector (if the non-setuid 
binary is executed from a privileged process etc) and so erred on the side of 
caution. A useful variant of my patch for auditors is one that logs dangerous 
conditions rather than reject them outright, but I'm unconviced that it is 
helpful for the masses.

Tim
-- 
Tim Brown
<mailto:tmb@...35.com>

Description: Prevent unqualified linker paths.
 A while ago, I suggested that I'd been working on glibc with regard to making it handle runtime linking more securely. Today, I finally managed to get a patch together which I think is worth 
 sharing. I'd therefore like to issue a request for comments for the attached patch, and enquire if any distros feel like incorporating it? A few points on the rationale...
 .
 Why? 
 .
 Over the last couple of years I've spent a good deal of time dealing with vendors who, for one reason or another have shipped binaries where it is possible to inject "untrusted" code into 
 running processes, notably but not exclusively via DT_RPATH.
 .
 What's the fix?
 .
 More often than not, the underlying issue is an empty element within the DT_RPATH header or equivalent. Sometimes it's not, but even in those cases, it is largely that one or more elements isn't 
 qualifed (i.e. it doesn't start with /). The attached patch fixes this, by ignoring any elements of DT_RPATH, LD_LIBRARY_PATH that do not start with a /, and/or junking any use of dlopen where 
 the filename is likewise unqualified.
 .
 Won't this break stuff?
 .
 Maybe (certainly it is means a change to glibc behaviour), but more often than not, the fact that a given binary currently works in an unsafe way is a bug - and an exploitable one at that. 
 Moreoever, Solaris has had a similar sanitity check (in their case only for privileged setuid binaries) for a good number of years without serious incident. I believe we should be fixing software 
 that exhibits the behaviour I've described, but this patch will (I think) kill the bug class irrespective of that.
 .
 Further thoughts?
 .
 The patch attached is the most robust variant I've produced in that it kills unqualified linker paths, irrespective of the privilege or otherwise of the affected binary. We could kill the checks 
 for non-setuid binaries or we could add some additional errors in such cases. I did experiment with only checking a subset of cases (namely where LD_LIBRARY_PATH itself is set) if the process 
 wasn't privileged, but in the end, I concluded that the loading of any unqualified linker path could provide an exploit vector (if the non-setuid binary is executed from a privileged process etc) 
 and so erred on the side of caution. A useful variant of my patch for auditors is one that logs dangerous conditions rather than reject them outright, but I'm unconviced that it is helpful for 
 the masses.
Author: Tim Brown <timb@...-dimension.org.uk>
Origin: other, https://www.nth-dimension.org.uk/downloads.php?id=100
Last-Update: 2015-02-19 

--- glibc-2.19.orig/elf/dl-load.c
+++ glibc-2.19/elf/dl-load.c
@@ -2203,6 +2203,35 @@ open_path (const char *name, size_t name
 	  if (this_dir->status[cnt] == nonexisting)
 	    continue;
 
+          if (INTUSE(__libc_enable_secure))
+            { 
+              if (this_dir->status[cnt] == insecure)
+                continue;
+              else
+                { 
+                  if (*(this_dir->dirname) != '/')
+                    { 
+                      this_dir->status[cnt] = insecure;
+                      continue;
+                    }
+                }
+            }
+          else
+            { 
+              /* We could actually be a bit cleverer here. ATM we just
+                 forbid all unqualified paths even if it's not setuid. */
+              if (this_dir->status[cnt] == insecure)
+                continue;
+              else
+                { 
+                  if (*(this_dir->dirname) != '/')
+                    { 
+                      this_dir->status[cnt] = insecure;
+                      continue;
+                    }
+                }
+            }
+
 	  buflen =
 	    ((char *) __mempcpy (__mempcpy (edp, capstr[cnt].str,
 					    capstr[cnt].len),
@@ -2241,7 +2270,8 @@ open_path (const char *name, size_t name
 	    }
 
 	  /* Remember whether we found any existing directory.  */
-	  here_any |= this_dir->status[cnt] != nonexisting;
+          /* Or if it was insecure. */
+	  here_any |= ((this_dir->status[cnt] != nonexisting) && (this_dir->status[cnt] != insecure));
 
 	  if (fd != -1 && __builtin_expect (secure, 0)
 	      && INTUSE(__libc_enable_secure))
@@ -2532,20 +2562,32 @@ _dl_map_object (struct link_map *loader,
     }
   else
     {
-      /* The path may contain dynamic string tokens.  */
-      realname = (loader
-		  ? expand_dynamic_string_token (loader, name, 0)
-		  : local_strdup (name));
-      if (realname == NULL)
-	fd = -1;
+      if (INTUSE(__libc_enable_secure) && (*name != '/'))
+        fd = -1;
       else
-	{
-	  fd = open_verify (realname, &fb,
-			    loader ?: GL(dl_ns)[nsid]._ns_loaded, 0,
-			    &found_other_class, true);
-	  if (__builtin_expect (fd, 0) == -1)
-	    free (realname);
-	}
+        {
+          /* We could actually be a bit cleverer here. ATM we just
+          forbid all unqualified paths even if it's not setuid. */
+          if (*name != '/')
+            fd = -1;
+          else
+            {
+              /* The path may contain dynamic string tokens.  */
+              realname = (loader
+                          ? expand_dynamic_string_token (loader, name, 0)
+                          : local_strdup (name));
+              if (realname == NULL)
+                fd = -1;
+              else
+                {
+                  fd = open_verify (realname, &fb,
+                                    loader ?: GL(dl_ns)[nsid]._ns_loaded, 0,
+                                    &found_other_class, true);
+                  if (__builtin_expect (fd, 0) == -1)
+                    free (realname);
+                }
+            }
+      }
     }
 
 #ifdef SHARED
--- glibc-2.19.orig/sysdeps/generic/ldsodefs.h
+++ glibc-2.19/sysdeps/generic/ldsodefs.h
@@ -149,7 +149,7 @@ struct r_found_version
 
 /* We want to cache information about the searches for shared objects.  */
 
-enum r_dir_status { unknown, nonexisting, existing };
+enum r_dir_status { unknown, nonexisting, existing, insecure };
 
 struct r_search_path_elem
   {


[ CONTENT OF TYPE application/pgp-signature SKIPPED ]

Powered by blists - more mailing lists

Your e-mail address:

Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.

Powered by Openwall GNU/*/Linux - Powered by OpenVZ