This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Tue, Dec 19, 2017 at 06:19:06PM +0100, Aurelien Jarno wrote:
> On 2017-12-19 14:16, Florian Weimer wrote:
> > On 12/18/2017 09:42 PM, Aurelien Jarno wrote:
> > > -  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;
> > 
> > I'm not really happy with this.  OOM and a zero-string expansion are very
> > different things.  We seem to have other abuses in the existing code, but I
> > don't think we should add further instances.
> 
> That's something changed from the first version. I can rollback that
> change, and add back the check or empty string in fillin_rpath.

That was my idea to move the check into expand_dynamic_string_token,
feel free to rollback if you like.

> > Furthermore, I'm not sure if the fix is robust enough.  There is another bug
> > in the $ORIGIN DST component skipping:  $ORIGIN eats the trailing colon.
> > This is because this code:
> > 
> >       else
> > 	{
> > 	  *wp++ = *name++;
> > 	  if (is_path && *name == ':')
> > 
> > happens when name is at the ':' after $ORIGIN processing, so only the next
> > ':' after that (or the end of the string) terminates this component, and the
> > call to the is_trusted_path_normalize covers two components mushed together
> > instead of one.  So the check likely fails, and both path elements are
> > skipped.  In some cases, this results in an empty string where it would not
> > usually be non-empty.
> 
> While this is indeed a problem in _dl_dst_substitute, it's not an issue
> in this case as expand_dynamic_string_token and thus _dl_dst_substitute
> do not get called from fillin_rpath with a colon. Indeed since commit
> b75891075bec (which introduced this bug), expand_dynamic_string_token
> is called after the split of the path by colons instead of before.

Yes.  In fact, the check is now redundant and could be omitted if desired:

--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -326,16 +326,8 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result,
 	  *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;
+	      /* check_for_trusted == false */
+	      last_elem = wp;
 	    }
 	}
     }


-- 
ldv

Attachment: signature.asc
Description: PGP signature


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]