Re: [PATCH] elf: Check for empty tokens before dynamic string token expansion [BZ #22625]

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Sun, Dec 17, 2017 at 10:26:01PM +0100, Aurelien Jarno wrote:
> On 2017-12-17 23:07, Dmitry V. Levin wrote:
[...]
> > >  	}
> > > +      else
> > > +	{
> > > +	  to_free = cp = expand_dynamic_string_token (l, cp, 1);
> > > +
> > > +	  /* expand_dynamic_string_token can return NULL in case of memory
> > > +	     allocation failure.  */
> > > +	  if (cp == NULL)
> > > +	    continue;
> > > +
> > > +	  /* Recompute the length after dynamic string token expansion
> > > +	     and ignore empty paths.  */
> > > +	  len = strlen (cp);
> > > +	  if (len == 0)
> > > +	    {
> > > +	      free (to_free);
> > > +	      continue;
> > > +	    }
> > > +	}
> > 
> > The alternative (that I was testing in my patch) to len == 0 check is to
> > change expand_dynamic_string_token to return NULL if _dl_dst_substitute
> > returned an empty string, e.g.
> >
> > -  return _dl_dst_substitute (l, s, result, is_path);
> > +  char *retval = _dl_dst_substitute (l, s, result, is_path);
> > +
> > +  /* If substitution of dynamic string tokens resulted to an empty string,
> > +     return NULL as in case of insufficient memory.  */
> > +  if (__glibc_unlikely (*retval == '\0'))
> > +    {
> > +      free (result);
> > +      return NULL;
> > +    }
> > +
> > +  return retval;
> > 
> > Both users of expand_dynamic_string_token, fillin_rpath and _dl_map_object,
> > would be happy.
> 
> I thought about that, but refrained as much as possible to change any
> interface, as it might trigger more bugs, even if it looks ok.

I agree in general, but in this particular case I've been playing
with this code for some time and think the change would be safe.

> The current bug actually show that a simple change might have big
> consequences. Overall it seems the RPATH handling code became very
> complex over the time, and would clearly benefit from a rewrite. I
> would not be really surprised if other similar bugs are found.

There are definitely more bugs in this code.  For example, _dl_init_paths
calls _dl_dst_substitute twice on $LD_LIBRARY_PATH: the first time
_dl_dst_substitute is called directly, the second time the result is
passed to fillin_rpath which calls expand_dynamic_string_token which in
turn calls _dl_dst_substitute.  As $LD_LIBRARY_PATH is filtered in case
of __libc_enable_secure, this doesn't look security sensitive, it just
does a wrong thing:

$ mkdir -p /tmp/'$ORIGIN' && cd /tmp/'$ORIGIN' && echo 'int main(){}' |gcc -xc - && strace -qq -E LD_LIBRARY_PATH='$ORIGIN' -e /open ./a.out
open("/tmp//tmp/$ORIGIN/tls/x86_64/libc.so.6", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/tmp//tmp/$ORIGIN/tls/libc.so.6", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/tmp//tmp/$ORIGIN/x86_64/libc.so.6", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/tmp//tmp/$ORIGIN/libc.so.6", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
open("/lib64/libc.so.6", O_RDONLY|O_CLOEXEC) = 3


-- 
ldv

Attachment: signature.asc
Description: PGP signature