[PATCH] Deny preload of files on NOEXEC mounts

[Adhemerval Zanella]

On 19/07/2021 16:33, Jordan R Abrahams via Libc-alpha wrote:
> I'm from Google's ChromeOS Toolchain team, and I have a security
> hardening patch to port upstream. I've confirmed with our
> security team that this patch can be public, and I'm looking for
> review and feedback.
> 
> I'm a new contributor, but Google already has a CLA on file for
> the FSF, so it should not be an issue. If I'm missing any
> contribution info, please do let me know.
> 
> Best regards,
> ~Jordan R Abrahams
> 
> -- >8 --
> This commit hardens a security flaw in dl-load.c.
> 
> At present, one could dynamically load any shared object files even if
> they resided on a NOEXEC mount partition. This introduces an exploit
> where an attacker may get around this NOEXEC requirement by preloading
> a malicious shared library.
> 
> For example, from shell this can look like:
> 
>   $ LD_PRELOAD='/tmp/malicious_lib.so' id
>   Hello world from malicious_lib!
> 
> Which will work even if /tmp/ is a noexec mount.
> 
> A proof of concept of this exploit can be found here at
> https://bugs.chromium.org/p/chromium/issues/detail?id=1182687
> The bug thread is restricted, but we are willing to add
> interested parties with the approval of our security team.
> 
> This patch hardens against the exploit by checking the file descriptor
> if the file lies in a NOEXEC mount partition via an fstatvfs call.
> The error string is set, and we jump to the `lose` cleanup code.
> 
> With this patch, the above example instead becomes:
> 
>   $ LD_PRELOAD='/tmp/malicious_lib.so' id
>   /usr/bin/id: error while loading shared libraries: /tmp/malicious_lib.so: file not located on exec mount
>   # <... normal stdout for id ...>
> 
> Signed-off-by: Jordan R Abrahams <ajordanr@google.com>
Don't noexec already prevents that? An example below with the glibc
2.33 on 5.11.0-25.

$ cat test.c 
int main (int argc, char *argv[])
{
  return 0;
}
$ cat libpreload.c 
#include <stdio.h>

static void
__attribute__ ((constructor))
lib_init (void)
{
  printf ("%s\n", __func__);
}
$ gcc -Wall test.c -o test
$ gcc -Wall -shared libpreload.c -o libpreload.so
$ LD_PRELOAD=./libpreload.so ./test 
lib_init
$ dd if=/dev/zero of=loopbackfile.img bs=1M count=1
$ sudo losetup -fP loopbackfile.img 
$ losetup -a | grep loopbackfile.img
/dev/loop16: []: (/tmp/test/loopbackfile.img)
$ mkfs.ext4 loopbackfile.img 
$ mkdir loopfs
$ sudo mount -o loop,noexec /dev/loop16 loopfs
$ mount | grep loopfs
/dev/loop16 on /tmp/test/loopfs type ext4 (rw,noexec,relatime)
$ mv libpreload.so loopfs/
$ LD_PRELOAD=loopfs/libpreload.so ./test 
ERROR: ld.so: object 'loopfs/libpreload.so' from LD_PRELOAD cannot be preloaded (failed to map segment from shared object): ignored.

I am not sure if this is an Ubuntu hardnening, if this is only enabled
on recent kernels, or only enabled for some filesystems.  It seems that
with CONFIG_SECURITY the code on security/security.c does filter out
the PROT_EXEC on mmap_prot() (but you will need to check how all the
pieces does work).

This kind of hardening does sound it would be better implemented by the
kernel itself, although I don't have a strong opinion if the idea is
to hardened against possible kernel version or FS that does not fully 
support it.

Some comments below regarding the code. Did you actually ran the make
check? I am asking because first the code does not build (there is
a wrong label usage) and you would see a lot of linknamespace issues
(I explained better below).

> ---
>  elf/dl-load.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 650e4edc35..81ca08c38f 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -29,6 +29,7 @@
>  #include <sys/mman.h>
>  #include <sys/param.h>
>  #include <sys/stat.h>
> +#include <sys/statvfs.h>
>  #include <sys/types.h>
>  #include <gnu/lib-names.h>
>  
> @@ -1572,6 +1573,20 @@ print_search_path (struct r_search_path_elem **list,
>      _dl_debug_printf_c ("\t\t(%s)\n", what);
>  }
>  

> +/* Check if a the passed in file descriptor points to file on an executable mount.  */
> +static int
> +check_exec (int fd)
> +{
> +    struct statvfs buf;
> +    int stated = fstatvfs (fd, &buf);
> +    if (stated == 0)
> +      {
> +	return !(buf.f_flag & ST_NOEXEC);
> +      }
> +    /* Could not fstat the file.  */
> +    return false;
> +}
> +


Wrong indentation and besides using fstatvfs that creates a linknamespace
polution, it might defeats the hardening (since one can interpose fstatvfs).
You will need to add a hidden_def (__fstatvfs) and use __fstatvfs() instead.

>  /* Open a file and verify it is an ELF file for this architecture.  We
>     ignore only ELF files for other architectures.  Non-ELF files and
>     ELF files with different header information cause fatal errors since
> @@ -1650,8 +1665,8 @@ open_verify (const char *name, int fd,
>  #endif
>  
>    if (fd == -1)
> -    /* Open the file.  We always open files read-only.  */
> -    fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC);
> +      /* Open the file.  We always open files read-only.  */
> +      fd = __open64_nocancel (name, O_RDONLY | O_CLOEXEC);
>  
>    if (fd != -1)
>      {

Superfluous change here.

> @@ -1667,6 +1682,14 @@ open_verify (const char *name, int fd,
>        __set_errno (0);
>        fbp->len = 0;
>        assert (sizeof (fbp->buf) > sizeof (ElfW(Ehdr)));
> +
> +      /* Before we read in the file, check if the file is in an exec mount */
> +      if (__glibc_unlikely (!check_exec(fd)))

Space before '(': "check_exext (fd)".

> +	{
> +	  errstring = N_("file not located on exec mount");
> +	  goto call_lose;
> +	}
> +

This should 'lose' (there is no label call_lose).

>        /* Read in the header.  */
>        do
>  	{
>