Re: [PATCH] elf: Fix LD_AUDIT for modules with invalid version (BZ#24122)

[Carlos O'Donell <carlos@redhat.com>]

On 1/23/19 9:07 AM, Adhemerval Zanella wrote:

Thanks for fixing this!

I'd like to see v2 please

- Only output message when LD_DEBUG=all, since la_version returning 0
  or > LAV_CURRENT is not an error, but simply an API mandated situation
  where we ignore the module. I don't see that as an error.

  The Solaris docs say:
  "If the audit library return is zero, or a version that is greater 
   than the rtld-audit interface the runtime linker supports, the audit
   library is discarded."

  Just discarded. It is up to the auditor to decide how it exposes to
  the user that it is running and operating as expected. We can provide
  feedback via LD_DEBUG=all.

- Optional: use -Wl,z,lazy for the test, or remove the comment about
  the PLT call.

- Suggested comment in tst-audit13mod1.c

> The error handling patch for invalid audit modules version access
> invalid memory:
> 
> elf/rtld.c:
> 
> 1454               unsigned int (*laversion) (unsigned int);
> 1455               unsigned int lav;
> 1456               if  (err_str == NULL
> 1457                    && (laversion = largs.result) != NULL
> 1458                    && (lav = laversion (LAV_CURRENT)) > 0
> 1459                    && lav <= LAV_CURRENT)
> 1460                 {
> [...]
> 1526               else
> 1527                 {
> 1528                   /* We cannot use the DSO, it does not have the
> 1529                      appropriate interfaces or it expects something
> 1530                      more recent.  */
> 1531 #ifndef NDEBUG
> 1532                   Lmid_t ns = dlmargs.map->l_ns;
> 1533 #endif
> 1534                   _dl_close (dlmargs.map);
> 1535
> 1536                   /* Make sure the namespace has been cleared entirely.  */
> 1537                   assert (GL(dl_ns)[ns]._ns_loaded == NULL);
> 1538                   assert (GL(dl_ns)[ns]._ns_nloaded == 0);
> 1539
> 1540                   GL(dl_tls_max_dtv_idx) = tls_idx;
> 1541                   goto not_loaded;
> 1542                 }
> 
> 1431           const char *err_str = NULL;
> 1432           bool malloced;
> 1433           (void) _dl_catch_error (&objname, &err_str, &malloced, dlmopen_doit,
> 1434                                   &dlmargs);
> 1435           if (__glibc_unlikely (err_str != NULL))
> 1436             {
> 1437             not_loaded:
> 1438               _dl_error_printf ("\
> 1439 ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
> 1440                                 name, err_str);
> 1441               if (malloced)
> 1442                 free ((char *) err_str);
> 1443             }
> 
> On failure the err_str will be NULL and _dl_debug_vdprintf does not handle
> it properly:
> 
> elf/dl-misc.c:
> 200             case 's':
> 201               /* Get the string argument.  */
> 202               iov[niov].iov_base = va_arg (arg, char *);
> 203               iov[niov].iov_len = strlen (iov[niov].iov_base);
> 204               if (prec != -1)
> 205                 iov[niov].iov_len = MIN ((size_t) prec, iov[niov].iov_len);
> 206               ++niov;
> 207               break;
> 
> This patch fixes the issues and improves the error message.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu
> 
> 	[BZ #24122]
> 	* elf/Makefile (tests): Add tst-audit13.
> 	(modules-names): Add tst-audit13mod1.
> 	(tst-audit13.out, tst-audit13-ENV): New rule.
> 	* elf/rtld.c (dl_main): Handle invalid audit module version.
> 	* elf/tst-audit13.c: New file.
> 	* elf/tst-audit13mod1.c: Likewise.
> ---
>  ChangeLog             | 10 +++++
>  elf/Makefile          |  8 +++-
>  elf/rtld.c            | 19 ++++++---
>  elf/tst-audit13.c     | 28 +++++++++++++
>  elf/tst-audit13mod1.c | 91 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 149 insertions(+), 7 deletions(-)
>  create mode 100644 elf/tst-audit13.c
>  create mode 100644 elf/tst-audit13mod1.c
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 9cf5cd8dfd..f71ed7cfff 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -187,7 +187,7 @@ tests += restest1 preloadtest loadfail multiload origtest resolvfail \
>  	 tst-nodelete2 tst-audit11 tst-audit12 tst-dlsym-error tst-noload \
>  	 tst-latepthread tst-tls-manydynamic tst-nodelete-dlclose \
>  	 tst-debug1 tst-main1 tst-absolute-sym tst-absolute-zero tst-big-note \
> -	 tst-unwind-ctor tst-unwind-main
> +	 tst-unwind-ctor tst-unwind-main tst-audit13
>  #	 reldep9
>  tests-internal += loadtest unload unload2 circleload1 \
>  	 neededtest neededtest2 neededtest3 neededtest4 \
> @@ -275,7 +275,8 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>  		tst-latepthreadmod $(tst-tls-many-dynamic-modules) \
>  		tst-nodelete-dlclose-dso tst-nodelete-dlclose-plugin \
>  		tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
> -		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib
> +		tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
> +		tst-audit13mod1
>  # Most modules build with _ISOMAC defined, but those filtered out
>  # depend on internal headers.
>  modules-names-tests = $(filter-out ifuncmod% tst-libc_dlvsym-dso tst-tlsmod%,\
> @@ -1382,6 +1383,9 @@ tst-audit12-ENV = LD_AUDIT=$(objpfx)tst-auditmod12.so
>  $(objpfx)tst-audit12mod1.so: $(objpfx)tst-audit12mod2.so
>  LDFLAGS-tst-audit12mod2.so = -Wl,--version-script=tst-audit12mod2.map
>  
> +$(objpfx)tst-audit13.out: $(objpfx)tst-audit13mod1.so
> +tst-audit13-ENV = LD_AUDIT=$(objpfx)tst-audit13mod1.so

OK.

> +
>  # Override -z defs, so that we can reference an undefined symbol.
>  # Force lazy binding for the same reason.
>  LDFLAGS-tst-latepthreadmod.so = \
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 5d97f41b7b..ad62c58e17 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1453,10 +1453,12 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>  
>  	      unsigned int (*laversion) (unsigned int);
>  	      unsigned int lav;
> -	      if  (err_str == NULL
> -		   && (laversion = largs.result) != NULL
> -		   && (lav = laversion (LAV_CURRENT)) > 0
> -		   && lav <= LAV_CURRENT)
> +	      if (err_str != NULL)
> +		goto not_loaded;

OK. This means we had a dlopen failure of the module, which is something we
*should* raise an error about.

> +
> +	      if ((laversion = largs.result) != NULL
> +		  && (lav = laversion (LAV_CURRENT)) > 0
> +		  && lav <= LAV_CURRENT)

OK, this means the DSO is loaded, and we run la_version symbol and check
for the validity of the results. If lav == 0 or lav > LAV_CURRENT then we
ignore the module.

>  		{
>  		  /* Allocate structure for the callback function pointers.
>  		     This call can never fail.  */
> @@ -1538,7 +1540,14 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>  		  assert (GL(dl_ns)[ns]._ns_nloaded == 0);
>  
>  		  GL(dl_tls_max_dtv_idx) = tls_idx;
> -		  goto not_loaded;

OK, this is the "ignore the module case."

> +		  _dl_error_printf ("\
> +ERROR: ld.so: object '%s' cannot be loaded as audit interface: ", name);
> +		  if (laversion == NULL)
> +		    _dl_error_printf ("la_version function not found.\n");
> +		  else
> +		     _dl_error_printf (
> +"invalid version '%u' (expected minimum of '%u'); ignored.\n",
> +				       lav, LAV_CURRENT);

If this message is output unconditionally, then I think this is wrong.

We must only output a message in the event of LD_DEBUG=all.

It is not "wrong" to return 0 or larger than the audit version, it is
simply part of the API, and is not an error that should generate a
message the user would see. The module is silently dropped. I think this
is important for modules which dynamically disable themselves. We don't
want those module to start printing errors at load time because of them.
Consider a module that only audits when the sysadmin sets a system
parameter, and otherwise disables itself by returning 0. This should not
be an error if LD_AUDIT remains set (as it should be to keep the module
checking for the admin parameter).

During LD_DEBUG=all you definitely want to see these audit issues
as a developer of the application, but not as a user running the
application.

>  		}
>  	    }
>  	}
> diff --git a/elf/tst-audit13.c b/elf/tst-audit13.c
> new file mode 100644
> index 0000000000..6f587baf58
> --- /dev/null
> +++ b/elf/tst-audit13.c
> @@ -0,0 +1,28 @@
> +/* Check for invalid audit version (BZ#24122).
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +
> +static int
> +do_test (void)
> +{
> +  puts ("plt call");
> +  return 0;

OK. Note that there might not be a PLT call here if the distro was
defaulting to -Wl,-z,now. I don't think it matters in this case though.
If you really want that PLT call you must use cflags -Wl,-z,lazy when
you build the test.


> +}
> +
> +#include <support/test-driver.c>
> diff --git a/elf/tst-audit13mod1.c b/elf/tst-audit13mod1.c
> new file mode 100644
> index 0000000000..96f1adef7a
> --- /dev/null
> +++ b/elf/tst-audit13mod1.c
> @@ -0,0 +1,91 @@
> +/* Check for invalid audit version (BZ#24122).
> +   Copyright (C) 2019 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <link.h>
> +#include <stdlib.h>
> +
> +unsigned int
> +la_version (unsigned int version)
> +{
> +  /* Invalid version, object should be discarded by the loader.  */

Suggest:

/* The audit specification says that a version of 0 or a version
   greater than any version supported by the dynamic loader shall
   cause the module to be ignored.  */

Basically "return 0;" is the way to dynamically disable the auditor.

> +  return 0;
> +}
> +
> +void
> +la_activity (uintptr_t *cookie, unsigned int flag)
> +{
> +  exit (EXIT_FAILURE);
> +}
> +
> +char *
> +la_objsearch (const char *name, uintptr_t *cookie, unsigned int flag)
> +{
> +  exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +la_objopen (struct link_map *map, Lmid_t lmid, uintptr_t * cookie)
> +{
> +  exit (EXIT_FAILURE);
> +}
> +
> +void
> +la_preinit (uintptr_t * cookie)
> +{
> +  exit (EXIT_FAILURE);
> +}

OK. Perfect. I like the exit (EXIT_FAILURE); :-)
> +
> +uintptr_t
> +#if __ELF_NATIVE_CLASS == 32
> +la_symbind32 (Elf32_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> +              uintptr_t *defcook, unsigned int *flags, const char *symname)
> +#else
> +la_symbind64 (Elf64_Sym *sym, unsigned int ndx, uintptr_t *refcook,
> +              uintptr_t *defcook, unsigned int *flags, const char *symname)
> +#endif
> +{
> +  exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +la_objclose (uintptr_t * cookie)
> +{
> +  exit (EXIT_FAILURE);
> +}
> +
> +#include <tst-audit.h>
> +#if (!defined (pltenter) || !defined (pltexit) || !defined (La_regs) \
> +     || !defined (La_retval) || !defined (int_retval))
> +# error "architecture specific code needed in sysdeps/CPU/tst-audit.h"
> +#endif
> +
> +ElfW(Addr)
> +pltenter (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
> +          uintptr_t *defcook, La_regs *regs, unsigned int *flags,
> +          const char *symname, long int *framesizep)
> +{ 
> +  exit (EXIT_FAILURE);
> +}
> +
> +unsigned int
> +pltexit (ElfW(Sym) *sym, unsigned int ndx, uintptr_t *refcook,
> +         uintptr_t *defcook, const La_regs *inregs, La_retval *outregs,
> +         const char *symname)
> +{
> +  exit (EXIT_FAILURE);
> +}
> 

OK.

-- 
Cheers,
Carlos.