Re: [REVIEW][PATCH] exec: Don't exec files the userns root can not read.

[Amir Goldstein]

On Wed, Oct 19, 2016 at 12:15 AM, Eric W. Biederman
<ebiederm@xxxxxxxxxxxx> wrote:
>
> When the user namespace support was merged the need to prevent
> ptracing an executable that is not readable was overlooked.
>
> Correct this oversight by not letting exec succeed if during exec an
> executable is not readable and the current user namespace capabilities
> do not apply to the executable's file.
>
> While it happens that distros install some files setuid and
> non-readable I have not found any executable files just installed
> non-readalbe.  Executables that are setuid to a user not mapped in a
> user namespace are worthless, so I don't expect this to introduce
> any problems in practice.
>
> There may be a way to allow this execution to happen by setting
> mm->user_ns to a more privileged user namespace and watching out for
> the possibility of using dynamic linkers or other shared libraries
> that the kernel loads into the mm to bypass the read-only
> restriction.  But the analysis is more difficult and it would
> require more code churn so I don't think the effort is worth it.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Jann Horn <jann@xxxxxxxxx>
> Fixes: 9e4a36ece652 ("userns: Fail exec for suid and sgid binaries with ids outside our user namespace.")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> ---
>
> Tossing this out for review in case I missed something silly but this
> patch seems pretty trivial.
>
>  arch/x86/ia32/ia32_aout.c |  4 +++-
>  fs/binfmt_aout.c          |  4 +++-
>  fs/binfmt_elf.c           |  4 +++-
>  fs/binfmt_elf_fdpic.c     |  4 +++-
>  fs/binfmt_flat.c          |  4 +++-
>  fs/exec.c                 | 19 ++++++++++++++++---
>  include/linux/binfmts.h   |  6 +++++-
>  7 files changed, 36 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c
> index cb26f18d43af..7ad20dedd929 100644
> --- a/arch/x86/ia32/ia32_aout.c
> +++ b/arch/x86/ia32/ia32_aout.c
> @@ -294,7 +294,9 @@ static int load_aout_binary(struct linux_binprm *bprm)
>         set_personality(PER_LINUX);
>         set_personality_ia32(false);
>
> -       setup_new_exec(bprm);
> +       retval = setup_new_exec(bprm);
> +       if (retval)
> +               return retval;
>
>         regs->cs = __USER32_CS;
>         regs->r8 = regs->r9 = regs->r10 = regs->r11 = regs->r12 =
> diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c
> index ae1b5404fced..b7b8aa03ccd0 100644
> --- a/fs/binfmt_aout.c
> +++ b/fs/binfmt_aout.c
> @@ -242,7 +242,9 @@ static int load_aout_binary(struct linux_binprm * bprm)
>  #else
>         set_personality(PER_LINUX);
>  #endif
> -       setup_new_exec(bprm);
> +       retval = setup_new_exec(bprm);
> +       if (retval)
> +               return retval;
>
>         current->mm->end_code = ex.a_text +
>                 (current->mm->start_code = N_TXTADDR(ex));
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 2472af2798c7..423fece0b8c4 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -852,7 +852,9 @@ static int load_elf_binary(struct linux_binprm *bprm)
>         if (!(current->personality & ADDR_NO_RANDOMIZE) && randomize_va_space)
>                 current->flags |= PF_RANDOMIZE;
>
> -       setup_new_exec(bprm);
> +       retval = setup_new_exec(bprm);
> +       if (retval)
> +               goto out_free_dentry;
>         install_exec_creds(bprm);
>
>         /* Do this so that we can load the interpreter, if need be.  We will
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index 464a972e88c1..d3099caff96d 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -352,7 +352,9 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
>         if (elf_read_implies_exec(&exec_params.hdr, executable_stack))
>                 current->personality |= READ_IMPLIES_EXEC;
>
> -       setup_new_exec(bprm);
> +       retval = setup_new_exec(bprm);
> +       if (retval)
> +               goto error;
>
>         set_binfmt(&elf_fdpic_format);
>
> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> index 9b2917a30294..25ca68940ad4 100644
> --- a/fs/binfmt_flat.c
> +++ b/fs/binfmt_flat.c
> @@ -524,7 +524,9 @@ static int load_flat_file(struct linux_binprm *bprm,
>
>                 /* OK, This is the point of no return */
>                 set_personality(PER_LINUX_32BIT);
> -               setup_new_exec(bprm);
> +               ret = setup_new_exec(bprm);
> +               if (ret)
> +                       goto err;
>         }
>
>         /*
> diff --git a/fs/exec.c b/fs/exec.c
> index 6fcfb3f7b137..f724ed94ba7a 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1270,12 +1270,21 @@ EXPORT_SYMBOL(flush_old_exec);
>
>  void would_dump(struct linux_binprm *bprm, struct file *file)
>  {
> -       if (inode_permission(file_inode(file), MAY_READ) < 0)
> +       struct inode *inode = file_inode(file);
> +       if (inode_permission(inode, MAY_READ) < 0) {
> +               struct user_namespace *user_ns = current->mm->user_ns;
>                 bprm->interp_flags |= BINPRM_FLAGS_ENFORCE_NONDUMP;
> +
> +               /* May the user_ns root read the executable? */
> +               if (!kuid_has_mapping(user_ns, inode->i_uid) ||
> +                   !kgid_has_mapping(user_ns, inode->i_gid)) {
> +                       bprm->interp_flags |= BINPRM_FLAGS_EXEC_INACCESSIBLE;
> +               }

This feels like it should belong inside
inode_permission(file_inode(file), MAY_EXEC)
which hopefully should be checked long before getting here??

> +       }
>  }
>  EXPORT_SYMBOL(would_dump);
>
> -void setup_new_exec(struct linux_binprm * bprm)
> +int setup_new_exec(struct linux_binprm * bprm)
>  {
>         arch_pick_mmap_layout(current->mm);
>
> @@ -1296,12 +1305,15 @@ void setup_new_exec(struct linux_binprm * bprm)
>          */
>         current->mm->task_size = TASK_SIZE;
>
> +       would_dump(bprm, bprm->file);
> +       if (bprm->interp_flags & BINPRM_FLAGS_EXEC_INACCESSIBLE)
> +               return -EPERM;
> +
>         /* install the new credentials */
>         if (!uid_eq(bprm->cred->uid, current_euid()) ||
>             !gid_eq(bprm->cred->gid, current_egid())) {
>                 current->pdeath_signal = 0;
>         } else {
> -               would_dump(bprm, bprm->file);
>                 if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP)
>                         set_dumpable(current->mm, suid_dumpable);
>         }
> @@ -1311,6 +1323,7 @@ void setup_new_exec(struct linux_binprm * bprm)
>         current->self_exec_id++;
>         flush_signal_handlers(current, 0);
>         do_close_on_exec(current->files);
> +       return 0;
>  }
>  EXPORT_SYMBOL(setup_new_exec);
>
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 1303b570b18c..8e5fb9eca2ee 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -57,6 +57,10 @@ struct linux_binprm {
>  #define BINPRM_FLAGS_PATH_INACCESSIBLE_BIT 2
>  #define BINPRM_FLAGS_PATH_INACCESSIBLE (1 << BINPRM_FLAGS_PATH_INACCESSIBLE_BIT)
>
> +/* executable is inaccessible for performing exec */
> +#define BINPRM_FLAGS_EXEC_INACCESSIBLE_BIT 3
> +#define BINPRM_FLAGS_EXEC_INACCESSIBLE (1 << BINPRM_FLAGS_EXEC_INACCESSIBLE_BIT)
> +
>  /* Function parameter for binfmt->coredump */
>  struct coredump_params {
>         const siginfo_t *siginfo;
> @@ -100,7 +104,7 @@ extern int prepare_binprm(struct linux_binprm *);
>  extern int __must_check remove_arg_zero(struct linux_binprm *);
>  extern int search_binary_handler(struct linux_binprm *);
>  extern int flush_old_exec(struct linux_binprm * bprm);
> -extern void setup_new_exec(struct linux_binprm * bprm);
> +extern int setup_new_exec(struct linux_binprm * bprm);
>  extern void would_dump(struct linux_binprm *, struct file *);
>
>  extern int suid_dumpable;
> --
> 2.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html