Re: [PATCH] elf: remove redundant __libc_enable_secure check from fillin_rpath

[Aurelien Jarno <aurelien at aurel32 dot net>]

On 2017-12-19 01:41, Dmitry V. Levin wrote:
> There are just two users of fillin_rpath: one is decompose_rpath that
> sets check_trusted argument to 0, another one is _dl_init_paths that
> sets check_trusted argument to __libc_enable_secure and invokes
> fillin_rpath only when LD_LIBRARY_PATH is non-empty.
> 
> Starting with commit
> glibc-2.25.90-512-gf6110a8fee2ca36f8e2d2abecf3cba9fa7b8ea7d,
> LD_LIBRARY_PATH is ignored for __libc_enable_secure executables,
> so check_trusted argument of fillin_rpath is always zero.
> 
> * elf/dl-load.c (is_trusted_path): Remove.
> (fillin_rpath): Remove check_trusted argument and its use,
> all callers changed.
> ---
>  ChangeLog     |  6 ++++++
>  elf/dl-load.c | 33 +++------------------------------
>  2 files changed, 9 insertions(+), 30 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index e7d97dc..2964464 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -117,24 +117,6 @@ static const size_t system_dirs_len[] =
>  #define nsystem_dirs_len array_length (system_dirs_len)
>  
>  static bool
> -is_trusted_path (const char *path, size_t len)
> -{
> -  const char *trun = system_dirs;
> -
> -  for (size_t idx = 0; idx < nsystem_dirs_len; ++idx)
> -    {
> -      if (len == system_dirs_len[idx] && memcmp (trun, path, len) == 0)
> -	/* Found it.  */
> -	return true;
> -
> -      trun += system_dirs_len[idx] + 1;
> -    }
> -
> -  return false;
> -}
> -
> -
> -static bool
>  is_trusted_path_normalize (const char *path, size_t len)
>  {
>    if (len == 0)
> @@ -428,8 +410,7 @@ static size_t max_dirnamelen;
>  
>  static struct r_search_path_elem **
>  fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
> -	      int check_trusted, const char *what, const char *where,
> -	      struct link_map *l)
> +	      const char *what, const char *where, struct link_map *l)
>  {
>    char *cp;
>    size_t nelems = 0;
> @@ -459,13 +440,6 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
>        if (len > 0 && cp[len - 1] != '/')
>  	cp[len++] = '/';
>  
> -      /* Make sure we don't use untrusted directories if we run SUID.  */
> -      if (__glibc_unlikely (check_trusted) && !is_trusted_path (cp, len))
> -	{
> -	  free (to_free);
> -	  continue;
> -	}
> -
>        /* See if this directory is already known.  */
>        for (dirp = GL(dl_all_dirs); dirp != NULL; dirp = dirp->next)
>  	if (dirp->dirnamelen == len && memcmp (cp, dirp->dirname, len) == 0)
> @@ -614,7 +588,7 @@ decompose_rpath (struct r_search_path_struct *sps,
>        _dl_signal_error (ENOMEM, NULL, NULL, errstring);
>      }
>  
> -  fillin_rpath (copy, result, ":", 0, what, where, l);
> +  fillin_rpath (copy, result, ":", what, where, l);
>  
>    /* Free the copied RPATH string.  `fillin_rpath' make own copies if
>       necessary.  */
> @@ -791,8 +765,7 @@ _dl_init_paths (const char *llp)
>  	}
>  
>        (void) fillin_rpath (llp_tmp, env_path_list.dirs, ":;",
> -			   __libc_enable_secure, "LD_LIBRARY_PATH",
> -			   NULL, l);
> +			   "LD_LIBRARY_PATH", NULL, l);
>  
>        if (env_path_list.dirs[0] == NULL)
>  	{

This assumes that __libc_enable_secure is never set when the dynamic
linker is invoked directly with --library-path. That seems a valid
assumption, so that patch looks all good to me.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Attachment: signature.asc
Description: PGP signature