Re: [PATCH][v3] Add dynamic linker support for $EXEC_ORIGIN.

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/23/2013 02:46 PM, Brooks Moses wrote:
> Updated to include Mike Frysinger's comments, as below.  Mike suggests 
> that this still needs signoff from someone more familiar with the ld.so 
> code; any volunteers?

I've finally worked the patcwork queue down to this path :-)

You're going to end up with at least one more iteration, because I'd
like to see documentation of $EXEC_ORIGIN in the glibc manual. Despite
Mike's claims that we shouldn't make you write a whole new chapter for
$ORIGN/$LIB/$PLATFORM/magic I *do* think I can hold you accountable for
creating an empty section with a couple of sentences along with a detailed
description of only $EXEC_ORIGIN. To be clear I'm not asking you to document
anything but the new substitution you're adding.

Please create a new chapter called "Dynamic Library Loading" to the already
existing and empty libdl.texi, add a section called "Runtime Paths", and
under that explain $EXEC_ORIGIN. Feel free to leave everything else blank
or fill it in as you wish with some minimal text.

My opinion here is that any text is better than no text, and your description
here will be the canonical definition of $EXEC_ORIGIN.
 
> ---
> The $ORIGIN rpath token expands to the directory containing the true
> copy of the application executable -- which is to say, if the executable
> is invoked through a symlink, that symlink will be resolved in the
> $ORIGIN substitution.
> 
> In some cases, such as symlink farms backed by cloud storage filesystems
> where the resolved path encodes effectively-arbitrary storage data, it
> is much more useful to have a rpath token that points to the
> non-expanded, as-called version of the executable path.  This patch adds
> the $EXEC_ORIGIN token for this purpose, resolving to the executable
> path as passed to execve().

Why is this what you want? What is wrong with the path encoding effectively
arbitrary storage data? I still don't see what problem you're trying to
solve by using $EXEC_ORIGIN. Can you walk me through an example execution
sequence that shows how you would use this?

I promise you I'm not trying to block this patch for no reason, but
I have an 8 month old, and I don't get much sleep, so many explanations
need to be dumbed down for me or explained twice.

Roland has also expressed his worry that we don't really understand the
problem that this is meant to solve, and I am in agreement for now.

> One potential security risk of such a token expansion is that, although
> copying a setuid/setgid executable (to change its $ORIGIN) loses the
> setuid/setgid bit, creating a symlink does not -- and so a user could
> inject arbitrary code into a setuid/setgid executable that references
> $EXEC_ORIGIN by creating an appropriate symlink.  In order to slam the
> door shut on this security risk, this patch simply prohibits the use of
> $EXEC_ORIGIN in setuid/setgid binaries.

OK.
 
I assume the testsuite run was free of regressions for at least x86-64.

> ---
> 2013-12-08  Brooks Moses  <bmoses@google.com>
> 

Needs bug [BZ #xxxx] for user visible feature.

Needs documentation for user visible feature (as discussed above).

I'll review the rest though.

> 	* elf/dl-support.c (_dl_exec_origin_path): New global variable.
> 	* elf/dl-dst.h (DL_DST_REQUIRED): Include _dl_exec_origin_path
> 	in computing dst_len.
> 	* elf/dl-load.c (_dl_dst_count): Also handle "EXEC_ORIGIN"
> 	token.
> 	(_dl_dst_substitute): Likewise.
> 	* elf/rtld.c (get_at_execfn): New static function.
> 	(get_directory): New static function.
> 	(set_exec_origin_path): New static function.
> 	(dl_main): Call set_exec_origin_path.
> 	* sysdeps/generic/ldsodefs.h (rtld_global_ro): Add
> 	_dl_exec_origin_path.
> 
>  elf/dl-dst.h               |  9 ++++--
>  elf/dl-load.c              | 10 ++++++-
>  elf/dl-support.c           |  3 ++
>  elf/rtld.c                 | 72 ++++++++++++++++++++++++++++++++++++++++++++++
>  sysdeps/generic/ldsodefs.h |  3 ++
>  5 files changed, 94 insertions(+), 3 deletions(-)
> 
> diff --git a/elf/dl-dst.h b/elf/dl-dst.h
> index 3ed95d0..13cd0e7 100644
> --- a/elf/dl-dst.h
> +++ b/elf/dl-dst.h
> @@ -65,8 +65,13 @@
>  	else								      \
>  	  dst_len = (l)->l_origin == (char *) -1			      \
>  	    ? 0 : strlen ((l)->l_origin);				      \
> -	dst_len = MAX (MAX (dst_len, GLRO(dl_platformlen)),		      \
> -		       strlen (DL_DST_LIB));				      \
> +									      \
> +	const char *exec_origin = GLRO(dl_exec_origin_path);		      \
> +	size_t exec_origin_len =					      \
> +	  (exec_origin == NULL) ? 0 : strlen (exec_origin);		      \
> +									      \
> +	dst_len = MAX (MAX (MAX (dst_len, GLRO(dl_platformlen)),	      \
> +			    strlen (DL_DST_LIB)), exec_origin_len);	      \

OK.

>  	if (dst_len > 4)						      \
>  	  __len += __cnt * (dst_len - 4);				      \
>        }									      \
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index d3e1cf8..04956dc 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -306,7 +306,8 @@ _dl_dst_count (const char *name, int is_path)
>        if ((len = is_dst (start, name, "ORIGIN", is_path,
>  			 INTUSE(__libc_enable_secure))) != 0
>  	  || (len = is_dst (start, name, "PLATFORM", is_path, 0)) != 0
> -	  || (len = is_dst (start, name, "LIB", is_path, 0)) != 0)
> +	  || (len = is_dst (start, name, "LIB", is_path, 0)) != 0
> +	  || (len = is_dst (start, name, "EXEC_ORIGIN", is_path, 0)) != 0)

OK.

>  	++cnt;
>  
>        name = strchr (name + len, '$');
> @@ -350,6 +351,13 @@ _dl_dst_substitute (struct link_map *l, const char *name, char *result,
>  	    repl = GLRO(dl_platform);
>  	  else if ((len = is_dst (start, name, "LIB", is_path, 0)) != 0)
>  	    repl = DL_DST_LIB;
> +	  else if ((len = is_dst (start, name, "EXEC_ORIGIN", is_path, 0)) != 0)
> +	    {
> +	      if (INTUSE(__libc_enable_secure) != 0)
> +		_dl_fatal_printf ("$EXEC_ORIGIN rpath entry not allowed in setuid/setgid executables.\n");

OK. I like that as a new feature you are choosing to explicitly disallow this.
You could for example try to argue that it fall back to $ORIGIN in a secure
context, but this is a stronger statement, and simple rules for security
issues is the best way forward.

> +
> +	      repl = GLRO(dl_exec_origin_path);
> +	    }
>  
>  	  if (repl != NULL && repl != (const char *) -1)
>  	    {
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index 2023bd0..d2d427a 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -67,6 +67,9 @@ void *__libc_stack_end;
>  /* Path where the binary is found.  */
>  const char *_dl_origin_path;
>  
> +/* Directory where the AT_EXECFN is found.  */
> +const char *_dl_exec_origin_path;
> +

OK.

>  /* Nonzero if runtime lookup should not update the .got/.plt.  */
>  int _dl_bind_not;
>  
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 3d207a3..0b3dd1f 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -51,6 +51,9 @@ extern __typeof (__mempcpy) __mempcpy attribute_hidden;
>  extern __typeof (_exit) exit_internal asm ("_exit") attribute_hidden;
>  #define _exit exit_internal
>  
> +/* Given file path, return absolute directory path.  */
> +static char * get_directory (const char *file_path);

OK.

> +
>  /* Helper function to handle errors while resolving symbols.  */
>  static void print_unresolved (int errcode, const char *objname,
>  			      const char *errsting);
> @@ -73,6 +76,9 @@ enum mode { normal, list, verify, trace };
>     all the entries.  */
>  static void process_envvars (enum mode *modep);
>  
> +/* Set GLRO(dl_exec_origin_path).  */
> +static void set_exec_origin_path (const char *exe_path);
> +

OK.

>  #ifdef DL_ARGV_NOT_RELRO
>  int _dl_argc attribute_hidden;
>  char **_dl_argv = NULL;
> @@ -1032,6 +1038,8 @@ of this helper program; chances are you did not intend to run this program.\n\
>  			in LIST\n\
>    --audit LIST          use objects named in LIST as auditors\n");
>  
> +      set_exec_origin_path (INTUSE(_dl_argv)[1]);

OK.

> +
>        ++_dl_skip_args;
>        --_dl_argc;
>        ++INTUSE(_dl_argv);
> @@ -1126,6 +1134,8 @@ of this helper program; chances are you did not intend to run this program.\n\
>      }
>    else
>      {
> +      set_exec_origin_path ((const char *) getauxval (AT_EXECFN));

OK.

> +
>        /* Create a link_map for the executable itself.
>  	 This will be what dlopen on "" returns.  */
>        main_map = _dl_new_object ((char *) "", "", lt_executable, NULL,
> @@ -2807,3 +2817,65 @@ print_statistics (hp_timing_t *rtld_total_timep)
>      }
>  #endif
>  }
> +
> +/* Given file path, return an absolute directory path.
> +   Examples: in: "/foo/bar/a.out", out: "/foo/bar/";
> +   in: "./a.out", out: "/dot/resolved/to/full/path/./".  */
> +static char *
> +get_directory (const char *file_path)

Shouldn't this go into dl-misc.c as an internal_function?

Perhaps called _dl_dirname_abs()? Since it's like dirname, but
with certain resolution semantics.

> +{
> +  assert (file_path != NULL);
> +
> +  /* Find the end of the directory substring in file_path.  */
> +  size_t path_len = strlen (file_path);
> +  while (path_len > 0 && file_path[path_len - 1] != '/')
> +    --path_len;
> +

OK.

> +  /* Allocate space and set the path prefix according to whether or not
> +     this is an absolute path.  */
> +  char *dest;
> +  char *full_dir_path;
> +  if (file_path[0] == '/')
> +    {
> +      full_dir_path = malloc (path_len + 1);
> +      assert (full_dir_path != NULL);
> +      dest = full_dir_path;
> +    }
> +  else
> +    {
> +      /* For a relative path, we need to include space for the largest
> +	 possible current path, a joining '/', the relevant part of
> +	 file_path, and a trailing '\0'.  */
> +      full_dir_path = malloc (PATH_MAX + path_len + 2);
> +      assert (full_dir_path != NULL);
> +
> +      char *status = __getcwd (full_dir_path, PATH_MAX);
> +      assert (status != NULL);
> +
> +      char *dest = __rawmemchr (full_dir_path, '\0');
> +      if (dest[-1] != '/')
> +	*dest++ = '/';
> +    }
> +
> +  if (path_len > 0)
> +    dest = __mempcpy (dest, file_path, path_len);
> +  *dest = '\0';
> +
> +  /* Confirm that the constructed path is valid.  */
> +  struct stat64 st;
> +  assert (__xstat64 (_STAT_VER, full_dir_path, &st) == 0);

OK.

> +
> +  return full_dir_path;
> +}
> +
> +/* Set GLRO(dl_exec_origin_path).  */
> +static void
> +set_exec_origin_path (const char *exe_path)
> +{
> +  assert (GLRO(dl_exec_origin_path) == NULL);
> +
> +  if (GLRO(dl_origin_path) != NULL)
> +    GLRO(dl_exec_origin_path) = strdup (GLRO(dl_origin_path));

This needs a comment explaining why you prefer LD_ORIGIN_PATH over
the path you are about to set here.

You also need to document that LD_ORIGIN_PATH is used in preference
of the invoked with path e.g. the original symlink or argv[1].

> +  else if (exe_path != NULL)
> +    GLRO(dl_exec_origin_path) = get_directory (exe_path);

OK.

> +}
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 146aca4..561a763 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -518,6 +518,9 @@ struct rtld_global_ro
>    /* Location of the binary.  */
>    EXTERN const char *_dl_origin_path;
>  
> +  /* Directory where the AT_EXECFN is found.  */
> +  EXTERN const char *_dl_exec_origin_path;

OK.

> +
>    /* -1 if the dynamic linker should honor library load bias,
>       0 if not, -2 use the default (honor biases for normal
>       binaries, don't honor for PIEs).  */
> 

Cheers,
Carlos.