Ping^2? Re: [PATCH][v3] Add dynamic linker support for $EXEC_ORIGIN.

[Brooks Moses <bmoses at google dot com>]

Ping^2?

On Sat, Dec 28, 2013 at 10:50 AM, Brooks Moses <bmoses@google.com> wrote:
>
> Andreas, Carlos, Roland -
>
> As best I can tell from git history, you three are the people most
> familiar with the ld.so code.  Any chance one of you could review this
> before the code freeze?  If so, I'd appreciate it, and if not, I'll
> hold off on re-pinging until after the release.
>
> Thanks,
> - Brooks
>
> On Mon, Dec 23, 2013 at 11:46 AM, Brooks Moses <bmoses@google.com> 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?
> >
> > On Sun, Dec 22, 2013 at 11:51 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> >> On Thursday 12 December 2013 17:13:39 Brooks Moses wrote:
> >>> --- a/elf/dl-dst.h
> >>> +++ b/elf/dl-dst.h
> >>>
> >>> +     char *exec_origin = GLRO(dl_exec_origin_path);                        \
> >>
> >> shouldn't that be const ?
> >
> > Yup, it should.  Fixed.
> >
> >>> --- a/elf/rtld.c
> >>> +++ b/elf/rtld.c
> >>>
> >>> +static void set_exec_origin_path(const char *exe_path);
> >>
> >> space before the (
> >
> > Also fixed.
> >
> >> otherwise, looks OK, but probably should get sign off from someone more
> >> familiar with the ldso code
> >
> > Thanks!
> > - Brooks
> >
> > ---
> > 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().
> >
> > 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.
> >
> > ---
> > 2013-12-08  Brooks Moses  <bmoses@google.com>
> >
> >         * 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);           \
> >         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)
> >         ++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");
> > +
> > +             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;
> > +
> >  /* 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);
> > +
> >  /* 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);
> > +
> >  #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]);
> > +
> >        ++_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));
> > +
> >        /* 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)
> > +{
> > +  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;
> > +
> > +  /* 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);
> > +
> > +  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));
> > +  else if (exe_path != NULL)
> > +    GLRO(dl_exec_origin_path) = get_directory (exe_path);
> > +}
> > 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;
> > +
> >    /* -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).  */
> > --
> > 1.8.5.1
> >