[v2] elf: Check for empty tokens before dynamic string token expansion [BZ #22625]
Commit Message
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:
@@ -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;
}
}
}