Patchwork Never leave $ORIGIN unexpanded

login
register
mail settings
Submitter Dmitry Levin
Date Dec. 29, 2015, 5:42 p.m.
Message ID <20151229174253.GC26641@altlinux.org>
Download mbox | patch
Permalink /patch/10160/
State New
Headers show

Comments

Dmitry Levin - Dec. 29, 2015, 5:42 p.m.
2011-05-13  Andreas Schwab  <schwab@redhat.com>

	* elf/dl-load.c (is_dst): Remove parameter secure, all callers
	changed.  Move check for valid use of $ORIGIN ...
	(_dl_dst_substitute): ... here.  Reset check_for_trusted when a
	path element is skipped.

---
 This change started its life as commit 207e77fd3f0a94acdf0557608dd4f10ce0e0f22f,
 it's in wide use, it was rebased and reviewed several times.

 elf/dl-load.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)
Mike Frysinger - Dec. 29, 2015, 10:31 p.m.
On 29 Dec 2015 20:42, Dmitry V. Levin wrote:
>  This change started its life as commit 207e77fd3f0a94acdf0557608dd4f10ce0e0f22f,
>  it's in wide use, it was rebased and reviewed several times.

where ?  links to discussions would be helpful, as would a more verbose
explanation.
-mike
Dmitry Levin - Dec. 29, 2015, 11:55 p.m.
On Tue, Dec 29, 2015 at 05:31:08PM -0500, Mike Frysinger wrote:
> On 29 Dec 2015 20:42, Dmitry V. Levin wrote:
> >  This change started its life as commit 207e77fd3f0a94acdf0557608dd4f10ce0e0f22f,
> >  it's in wide use, it was rebased and reviewed several times.
> 
> where ?

It's in Fedora since glibc-2.13.90-12 (13.05.2011).
I reviewed it twice at least.

> links to discussions would be helpful,

It's a follow-up to the series of commits made to fix
https://sourceware.org/bugzilla/show_bug.cgi?id=12393

I have no idea why it remained in fedora branch and hasn't been merged
to master.

> as would a more verbose explanation.

The idea is, as the subject says, never to leave $ORIGIN unexpanded:
if a privileged executable's rpath element contains $ORIGIN in a position
that is not allowed for expansion in privileged executables, this rpath
element shouldn't be left as is, it should be discarded.
Dmitry Levin - Dec. 18, 2017, 4:21 a.m.
On Wed, Dec 30, 2015 at 02:55:26AM +0300, Dmitry V. Levin wrote:
> On Tue, Dec 29, 2015 at 05:31:08PM -0500, Mike Frysinger wrote:
> > On 29 Dec 2015 20:42, Dmitry V. Levin wrote:
> > >  This change started its life as commit 207e77fd3f0a94acdf0557608dd4f10ce0e0f22f,
> > >  it's in wide use, it was rebased and reviewed several times.
> > 
> > where ?
> 
> It's in Fedora since glibc-2.13.90-12 (13.05.2011).
> I reviewed it twice at least.
> 
> > links to discussions would be helpful,
> 
> It's a follow-up to the series of commits made to fix
> https://sourceware.org/bugzilla/show_bug.cgi?id=12393
> 
> I have no idea why it remained in fedora branch and hasn't been merged
> to master.
> 
> > as would a more verbose explanation.
> 
> The idea is, as the subject says, never to leave $ORIGIN unexpanded:
> if a privileged executable's rpath element contains $ORIGIN in a position
> that is not allowed for expansion in privileged executables, this rpath
> element shouldn't be left as is, it should be discarded.

So the question is, whether we consider the current ld.so behaviour safe or not:

$ rm -rf '$ORIGIN' && mkdir -m0700 '$ORIGIN' &&
  ln -snf /dev/null '$ORIGIN/libc.so.6' &&
  echo 'int main(){}' |gcc -xc - -Wl,-rpath,'./$ORIGIN' &&
  chgrp -h another_group a.out && chmod 02710 a.out && ./a.out
./a.out: error while loading shared libraries: ./$ORIGIN/libc.so.6: file too short

If we agree that it's unsafe, than the fix is ready to be applied.

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 6fb615e..0938b70 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -197,43 +197,36 @@  is_trusted_path_normalize (const char *path, size_t len)
 
 
 static size_t
-is_dst (const char *start, const char *name, const char *str,
-	int is_path, int secure)
+is_dst (const char *start, const char *name, const char *str, int is_path)
 {
   size_t len;
   bool is_curly = false;
 
   if (name[0] == '{')
     {
       is_curly = true;
       ++name;
     }
 
   len = 0;
   while (name[len] == str[len] && name[len] != '\0')
     ++len;
 
   if (is_curly)
     {
       if (name[len] != '}')
 	return 0;
 
       /* Point again at the beginning of the name.  */
       --name;
       /* Skip over closing curly brace and adjust for the --name.  */
       len += 2;
     }
   else if (name[len] != '\0' && name[len] != '/'
 	   && (!is_path || name[len] != ':'))
     return 0;
 
-  if (__glibc_unlikely (secure)
-      && ((name[len] != '\0' && name[len] != '/'
-	   && (!is_path || name[len] != ':'))
-	  || (name != start + 1 && (!is_path || name[-2] != ':'))))
-    return 0;
-
   return len;
 }
 
 
@@ -241,26 +234,23 @@  size_t
 _dl_dst_count (const char *name, int is_path)
 {
   const char *const start = name;
   size_t cnt = 0;
 
   do
     {
       size_t len;
 
-      /* $ORIGIN is not expanded for SUID/GUID programs (except if it
-	 is $ORIGIN alone) and it must always appear first in path.  */
       ++name;
-      if ((len = is_dst (start, name, "ORIGIN", is_path,
-			 __libc_enable_secure)) != 0
-	  || (len = is_dst (start, name, "PLATFORM", is_path, 0)) != 0
-	  || (len = is_dst (start, name, "LIB", is_path, 0)) != 0)
+      if ((len = is_dst (start, name, "ORIGIN", is_path)) != 0
+	  || (len = is_dst (start, name, "PLATFORM", is_path)) != 0
+	  || (len = is_dst (start, name, "LIB", is_path)) != 0)
 	++cnt;
 
       name = strchr (name + len, '$');
     }
   while (name != NULL);
 
   return cnt;
 }
 
 
@@ -268,92 +258,101 @@  char *
 _dl_dst_substitute (struct link_map *l, const char *name, char *result,
 		    int is_path)
 {
   const char *const start = name;
 
   /* Now fill the result path.  While copying over the string we keep
      track of the start of the last path element.  When we come across
      a DST we copy over the value or (if the value is not available)
      leave the entire path element out.  */
   char *wp = result;
   char *last_elem = result;
   bool check_for_trusted = false;
 
   do
     {
       if (__glibc_unlikely (*name == '$'))
 	{
 	  const char *repl = NULL;
 	  size_t len;
 
 	  ++name;
-	  if ((len = is_dst (start, name, "ORIGIN", is_path,
-			     __libc_enable_secure)) != 0)
+	  if ((len = is_dst (start, name, "ORIGIN", is_path)) != 0)
 	    {
-	      repl = l->l_origin;
+	      /* For SUID/GUID programs $ORIGIN must always appear
+		 first in a path element.  */
+	      if (__glibc_unlikely (__libc_enable_secure)
+		  && ((name[len] != '\0' && name[len] != '/'
+		       && (!is_path || name[len] != ':'))
+		      || (name != start + 1 && (!is_path || name[-2] != ':'))))
+		repl = (const char *) -1;
+	      else
+		repl = l->l_origin;
+
 	      check_for_trusted = (__libc_enable_secure
 				   && l->l_type == lt_executable);
 	    }
-	  else if ((len = is_dst (start, name, "PLATFORM", is_path, 0)) != 0)
+	  else if ((len = is_dst (start, name, "PLATFORM", is_path)) != 0)
 	    repl = GLRO(dl_platform);
-	  else if ((len = is_dst (start, name, "LIB", is_path, 0)) != 0)
+	  else if ((len = is_dst (start, name, "LIB", is_path)) != 0)
 	    repl = DL_DST_LIB;
 
 	  if (repl != NULL && repl != (const char *) -1)
 	    {
 	      wp = __stpcpy (wp, repl);
 	      name += len;
 	    }
 	  else if (len > 1)
 	    {
 	      /* We cannot use this path element, the value of the
 		 replacement is unknown.  */
 	      wp = last_elem;
 	      name += len;
 	      while (*name != '\0' && (!is_path || *name != ':'))
 		++name;
 	      /* Also skip following colon if this is the first rpath
 		 element, but keep an empty element at the end.  */
 	      if (wp == result && is_path && *name == ':' && name[1] != '\0')
 		++name;
+	      check_for_trusted = false;
 	    }
 	  else
 	    /* No DST we recognize.  */
 	    *wp++ = '$';
 	}
       else
 	{
 	  *wp++ = *name++;
 	  if (is_path && *name == ':')
 	    {
 	      /* In SUID/SGID programs, after $ORIGIN expansion the
 		 normalized path must be rooted in one of the trusted
 		 directories.  */
 	      if (__glibc_unlikely (check_for_trusted)
 		  && !is_trusted_path_normalize (last_elem, wp - last_elem))
 		wp = last_elem;
 	      else
 		last_elem = wp;
 
 	      check_for_trusted = false;
 	    }
 	}
     }
   while (*name != '\0');
 
   /* In SUID/SGID programs, after $ORIGIN expansion the normalized
      path must be rooted in one of the trusted directories.  */
   if (__glibc_unlikely (check_for_trusted)
       && !is_trusted_path_normalize (last_elem, wp - last_elem))
     wp = last_elem;
 
   *wp = '\0';
 
   return result;
 }
 
 
 /* Return copy of argument with all recognized dynamic string tokens
    ($ORIGIN and $PLATFORM for now) replaced.  On some platforms it
    might not be possible to determine the path from which the object
    belonging to the map is loaded.  In this case the path element
    containing $ORIGIN is left out.  */