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] |
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] |