From: Amir Goldstein <amir73il@gmail.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Jann Horn <jann@thejh.net>, Michal Hocko <mhocko@kernel.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Linux Containers <containers@lists.linux-foundation.org>,
Oleg Nesterov <oleg@redhat.com>,
Andy Lutomirski <luto@amacapital.net>,
linux-mm@kvack.org, linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [REVIEW][PATCH] exec: Don't exec files the userns root can not read.
Date: Wed, 19 Oct 2016 09:13:01 +0300 [thread overview]
Message-ID: <CAOQ4uxjyZF346vq-Oi=HwB=jj6ePycHBnEfvVPet9KqPxL9mgg@mail.gmail.com> (raw)
In-Reply-To: <87k2d5nytz.fsf_-_@xmission.com>
On Wed, Oct 19, 2016 at 12:15 AM, Eric W. Biederman
<ebiederm@xmission.com> 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@vger.kernel.org
> Reported-by: Jann Horn <jann@thejh.net>
> Fixes: 9e4a36ece652 ("userns: Fail exec for suid and sgid binaries with ids outside our user namespace.")
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
>
> 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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2016-10-19 6:13 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-17 16:39 [REVIEW][PATCH] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access Eric W. Biederman
2016-10-17 17:25 ` Jann Horn
2016-10-17 17:33 ` Eric W. Biederman
2016-10-18 13:50 ` Michal Hocko
2016-10-18 13:57 ` Jann Horn
2016-10-18 14:56 ` Eric W. Biederman
2016-10-18 15:05 ` Jann Horn
2016-10-18 15:35 ` Eric W. Biederman
2016-10-18 19:12 ` Jann Horn
2016-10-18 21:07 ` Eric W. Biederman
2016-10-18 21:15 ` [REVIEW][PATCH] exec: Don't exec files the userns root can not read Eric W. Biederman
2016-10-19 6:13 ` Amir Goldstein [this message]
2016-10-19 13:33 ` Eric W. Biederman
2016-10-19 17:04 ` Eric W. Biederman
2016-10-19 15:30 ` Andy Lutomirski
2016-10-19 16:52 ` Eric W. Biederman
2016-10-19 17:29 ` Jann Horn
2016-10-19 17:32 ` Andy Lutomirski
2016-10-19 17:55 ` Eric W. Biederman
2016-10-19 18:38 ` Andy Lutomirski
2016-10-19 21:26 ` Eric W. Biederman
2016-10-19 23:17 ` Andy Lutomirski
2016-11-17 17:02 ` [REVIEW][PATCH 0/3] Fixing ptrace vs exec vs userns interactions Eric W. Biederman
2016-11-17 17:05 ` [REVIEW][PATCH 1/3] ptrace: Capture the ptracer's creds not PT_PTRACE_CAP Eric W. Biederman
2016-11-17 23:14 ` Kees Cook
2016-11-18 18:56 ` Eric W. Biederman
2016-11-17 23:27 ` Andy Lutomirski
2016-11-17 23:44 ` Eric W. Biederman
2016-11-17 17:08 ` [REVIEW][PATCH 2/3] exec: Don't allow ptracing an exec of an unreadable file Eric W. Biederman
2016-11-17 20:47 ` Willy Tarreau
2016-11-17 21:07 ` Kees Cook
2016-11-17 21:32 ` Willy Tarreau
2016-11-17 21:51 ` Eric W. Biederman
2016-11-17 22:50 ` [REVIEW][PATCH 2/3] ptrace: Don't allow accessing an undumpable mm Eric W. Biederman
2016-11-17 23:17 ` Kees Cook
2016-11-17 23:28 ` [REVIEW][PATCH 2/3] exec: Don't allow ptracing an exec of an unreadable file Andy Lutomirski
2016-11-17 23:29 ` Andy Lutomirski
2016-11-17 23:55 ` Eric W. Biederman
2016-11-18 0:10 ` Andy Lutomirski
2016-11-18 0:35 ` Eric W. Biederman
2016-11-17 17:10 ` [REVIEW][PATCH 3/3] exec: Ensure mm->user_ns contains the execed files Eric W. Biederman
2016-11-19 7:17 ` [REVIEW][PATCH 0/3] Fixing ptrace vs exec vs userns interactions Willy Tarreau
2016-11-19 9:28 ` Willy Tarreau
2016-11-19 9:33 ` Willy Tarreau
2016-11-19 18:44 ` Eric W. Biederman
2016-11-19 18:35 ` Eric W. Biederman
2016-11-19 18:37 ` Eric W. Biederman
2016-10-19 18:36 ` [REVIEW][PATCH] exec: Don't exec files the userns root can not read Andy Lutomirski
2016-10-18 18:06 ` [REVIEW][PATCH] mm: Add a user_ns owner to mm_struct and fix ptrace_may_access Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAOQ4uxjyZF346vq-Oi=HwB=jj6ePycHBnEfvVPet9KqPxL9mgg@mail.gmail.com' \
--to=amir73il@gmail.com \
--cc=containers@lists.linux-foundation.org \
--cc=ebiederm@xmission.com \
--cc=jann@thejh.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=luto@amacapital.net \
--cc=mhocko@kernel.org \
--cc=oleg@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox