From e7f7785449a1f459a4a3ca92f82f56fb054dd2b9 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 12 Mar 2020 10:17:17 -0500 Subject: [PATCH 01/24] binfmt: Move install_exec_creds after setup_new_exec to match binfmt_elf In 2016 Linus moved install_exec_creds immediately after setup_new_exec, in binfmt_elf as a cleanup and as part of closing a potential information leak. Perform the same cleanup for the other binary formats. Different binary formats doing the same things the same way makes exec easier to reason about and easier to maintain. Greg Ungerer reports: > I tested the the whole series on non-MMU m68k and non-MMU arm > (exercising binfmt_flat) and it all tested out with no problems, > so for the binfmt_flat changes: Tested-by: Greg Ungerer Ref: 9f834ec18def ("binfmt_elf: switch to new creds when switching to new mm") Reviewed-by: Kees Cook Reviewed-by: Greg Ungerer Signed-off-by: "Eric W. Biederman" --- arch/x86/ia32/ia32_aout.c | 3 +-- fs/binfmt_aout.c | 2 +- fs/binfmt_elf_fdpic.c | 2 +- fs/binfmt_flat.c | 3 +-- 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c index 9bb71abd66bd..37b36a8ce5fa 100644 --- a/arch/x86/ia32/ia32_aout.c +++ b/arch/x86/ia32/ia32_aout.c @@ -140,6 +140,7 @@ static int load_aout_binary(struct linux_binprm *bprm) set_personality_ia32(false); setup_new_exec(bprm); + install_exec_creds(bprm); regs->cs = __USER32_CS; regs->r8 = regs->r9 = regs->r10 = regs->r11 = regs->r12 = @@ -156,8 +157,6 @@ static int load_aout_binary(struct linux_binprm *bprm) if (retval < 0) return retval; - install_exec_creds(bprm); - if (N_MAGIC(ex) == OMAGIC) { unsigned long text_addr, map_size; diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c index 8e8346a81723..ace587b66904 100644 --- a/fs/binfmt_aout.c +++ b/fs/binfmt_aout.c @@ -162,6 +162,7 @@ static int load_aout_binary(struct linux_binprm * bprm) set_personality(PER_LINUX); #endif setup_new_exec(bprm); + install_exec_creds(bprm); current->mm->end_code = ex.a_text + (current->mm->start_code = N_TXTADDR(ex)); @@ -174,7 +175,6 @@ static int load_aout_binary(struct linux_binprm * bprm) if (retval < 0) return retval; - install_exec_creds(bprm); if (N_MAGIC(ex) == OMAGIC) { unsigned long text_addr, map_size; diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 240f66663543..6c94c6d53d97 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -353,6 +353,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) current->personality |= READ_IMPLIES_EXEC; setup_new_exec(bprm); + install_exec_creds(bprm); set_binfmt(&elf_fdpic_format); @@ -434,7 +435,6 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) current->mm->start_stack = current->mm->start_brk + stack_size; #endif - install_exec_creds(bprm); if (create_elf_fdpic_tables(bprm, current->mm, &exec_params, &interp_params) < 0) goto error; diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 831a2b25ba79..1a1d1fcb893f 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -541,6 +541,7 @@ 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); + install_exec_creds(bprm); } /* @@ -963,8 +964,6 @@ static int load_flat_binary(struct linux_binprm *bprm) } } - install_exec_creds(bprm); - set_binfmt(&flat_format); #ifdef CONFIG_MMU From 89826cce37542f7950e8f4b9258284805e98430c Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 2 Apr 2020 18:04:54 -0500 Subject: [PATCH 02/24] exec: Make unlocking exec_update_mutex explict With install_exec_creds updated to follow immediately after setup_new_exec, the failure of unshare_sighand is the only code path where exec_update_mutex is held but not explicitly unlocked. Update that code path to explicitly unlock exec_update_mutex. Remove the unlocking of exec_update_mutex from free_bprm. Reviewed-by: Kees Cook Reviewed-by: Greg Ungerer Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 6 +++--- include/linux/binfmts.h | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 06b4c550af5d..6bd82a007bfc 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1344,7 +1344,7 @@ int flush_old_exec(struct linux_binprm * bprm) */ retval = unshare_sighand(me); if (retval) - goto out; + goto out_unlock; set_fs(USER_DS); me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | @@ -1361,6 +1361,8 @@ int flush_old_exec(struct linux_binprm * bprm) do_close_on_exec(me->files); return 0; +out_unlock: + mutex_unlock(&me->signal->exec_update_mutex); out: return retval; } @@ -1477,8 +1479,6 @@ static void free_bprm(struct linux_binprm *bprm) { free_arg_pages(bprm); if (bprm->cred) { - if (bprm->called_exec_mmap) - mutex_unlock(¤t->signal->exec_update_mutex); mutex_unlock(¤t->signal->cred_guard_mutex); abort_creds(bprm->cred); } diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index a345d9fed3d8..6f564b9ad882 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -47,8 +47,7 @@ struct linux_binprm { secureexec:1, /* * Set by flush_old_exec, when exec_mmap has been called. - * This is past the point of no return, when the - * exec_update_mutex has been taken. + * This is past the point of no return. */ called_exec_mmap:1; #ifdef __alpha__ From 1507b7a30ad284a2a136ee79c214c0e86c62da64 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 2 Apr 2020 18:17:50 -0500 Subject: [PATCH 03/24] exec: Rename the flag called_exec_mmap point_of_no_return Update the comments and make the code easier to understand by renaming this flag. Reviewed-by: Kees Cook Reviewed-by: Greg Ungerer Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 12 ++++++------ include/linux/binfmts.h | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 6bd82a007bfc..71de9f57ae09 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1326,12 +1326,12 @@ int flush_old_exec(struct linux_binprm * bprm) goto out; /* - * After setting bprm->called_exec_mmap (to mark that current is - * using the prepared mm now), we have nothing left of the original - * process. If anything from here on returns an error, the check - * in search_binary_handler() will SEGV current. + * With the new mm installed it is completely impossible to + * fail and return to the original process. If anything from + * here on returns an error, the check in + * search_binary_handler() will SEGV current. */ - bprm->called_exec_mmap = 1; + bprm->point_of_no_return = true; bprm->mm = NULL; #ifdef CONFIG_POSIX_TIMERS @@ -1720,7 +1720,7 @@ int search_binary_handler(struct linux_binprm *bprm) read_lock(&binfmt_lock); put_binfmt(fmt); - if (retval < 0 && bprm->called_exec_mmap) { + if (retval < 0 && bprm->point_of_no_return) { /* we got to flush_old_exec() and failed after it */ read_unlock(&binfmt_lock); force_sigsegv(SIGSEGV); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 6f564b9ad882..8f479dad7931 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -46,10 +46,10 @@ struct linux_binprm { */ secureexec:1, /* - * Set by flush_old_exec, when exec_mmap has been called. - * This is past the point of no return. + * Set when errors can no longer be returned to the + * original userspace. */ - called_exec_mmap:1; + point_of_no_return:1; #ifdef __alpha__ unsigned int taso:1; #endif From 96ecee29b0b560662ec082ee9b6f2049f2a79090 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sun, 3 May 2020 06:48:17 -0500 Subject: [PATCH 04/24] exec: Merge install_exec_creds into setup_new_exec The two functions are now always called one right after the other so merge them together to make future maintenance easier. Reviewed-by: Kees Cook Reviewed-by: Greg Ungerer Signed-off-by: "Eric W. Biederman" --- arch/x86/ia32/ia32_aout.c | 1 - fs/binfmt_aout.c | 1 - fs/binfmt_elf.c | 1 - fs/binfmt_elf_fdpic.c | 1 - fs/binfmt_flat.c | 1 - fs/exec.c | 56 ++++++++++++++++++--------------------- include/linux/binfmts.h | 1 - kernel/events/core.c | 2 +- 8 files changed, 27 insertions(+), 37 deletions(-) diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c index 37b36a8ce5fa..8255fdc3a027 100644 --- a/arch/x86/ia32/ia32_aout.c +++ b/arch/x86/ia32/ia32_aout.c @@ -140,7 +140,6 @@ static int load_aout_binary(struct linux_binprm *bprm) set_personality_ia32(false); setup_new_exec(bprm); - install_exec_creds(bprm); 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 ace587b66904..c8ba28f285e5 100644 --- a/fs/binfmt_aout.c +++ b/fs/binfmt_aout.c @@ -162,7 +162,6 @@ static int load_aout_binary(struct linux_binprm * bprm) set_personality(PER_LINUX); #endif setup_new_exec(bprm); - install_exec_creds(bprm); 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 13f25e241ac4..e6b586623035 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -858,7 +858,6 @@ out_free_interp: current->flags |= PF_RANDOMIZE; setup_new_exec(bprm); - install_exec_creds(bprm); /* Do this so that we can load the interpreter, if need be. We will change some of these later */ diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 6c94c6d53d97..9a1aa61b4cc3 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -353,7 +353,6 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) current->personality |= READ_IMPLIES_EXEC; setup_new_exec(bprm); - install_exec_creds(bprm); set_binfmt(&elf_fdpic_format); diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 1a1d1fcb893f..252878969582 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -541,7 +541,6 @@ 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); - install_exec_creds(bprm); } /* diff --git a/fs/exec.c b/fs/exec.c index 71de9f57ae09..93e40f865523 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1443,6 +1443,31 @@ void setup_new_exec(struct linux_binprm * bprm) group */ WRITE_ONCE(current->self_exec_id, current->self_exec_id + 1); flush_signal_handlers(current, 0); + + /* + * install the new credentials for this executable + */ + security_bprm_committing_creds(bprm); + + commit_creds(bprm->cred); + bprm->cred = NULL; + + /* + * Disable monitoring for regular users + * when executing setuid binaries. Must + * wait until new credentials are committed + * by commit_creds() above + */ + if (get_dumpable(current->mm) != SUID_DUMP_USER) + perf_event_exit_task(current); + /* + * cred_guard_mutex must be held at least to this point to prevent + * ptrace_attach() from altering our determination of the task's + * credentials; any time after this it may be unlocked. + */ + security_bprm_committed_creds(bprm); + mutex_unlock(¤t->signal->exec_update_mutex); + mutex_unlock(¤t->signal->cred_guard_mutex); } EXPORT_SYMBOL(setup_new_exec); @@ -1458,7 +1483,7 @@ EXPORT_SYMBOL(finalize_exec); /* * Prepare credentials and lock ->cred_guard_mutex. - * install_exec_creds() commits the new creds and drops the lock. + * setup_new_exec() commits the new creds and drops the lock. * Or, if exec fails before, free_bprm() should release ->cred and * and unlock. */ @@ -1504,35 +1529,6 @@ int bprm_change_interp(const char *interp, struct linux_binprm *bprm) } EXPORT_SYMBOL(bprm_change_interp); -/* - * install the new credentials for this executable - */ -void install_exec_creds(struct linux_binprm *bprm) -{ - security_bprm_committing_creds(bprm); - - commit_creds(bprm->cred); - bprm->cred = NULL; - - /* - * Disable monitoring for regular users - * when executing setuid binaries. Must - * wait until new credentials are committed - * by commit_creds() above - */ - if (get_dumpable(current->mm) != SUID_DUMP_USER) - perf_event_exit_task(current); - /* - * cred_guard_mutex must be held at least to this point to prevent - * ptrace_attach() from altering our determination of the task's - * credentials; any time after this it may be unlocked. - */ - security_bprm_committed_creds(bprm); - mutex_unlock(¤t->signal->exec_update_mutex); - mutex_unlock(¤t->signal->cred_guard_mutex); -} -EXPORT_SYMBOL(install_exec_creds); - /* * determine how safe it is to execute the proposed program * - the caller must hold ->cred_guard_mutex to protect against diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 8f479dad7931..2a8fddf3574a 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -145,7 +145,6 @@ extern int transfer_args_to_stack(struct linux_binprm *bprm, extern int bprm_change_interp(const char *interp, struct linux_binprm *bprm); extern int copy_strings_kernel(int argc, const char *const *argv, struct linux_binprm *bprm); -extern void install_exec_creds(struct linux_binprm *bprm); extern void set_binfmt(struct linux_binfmt *new); extern ssize_t read_code(struct file *, unsigned long, loff_t, size_t); diff --git a/kernel/events/core.c b/kernel/events/core.c index 633b4ae72ed5..169449b5e56b 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -12217,7 +12217,7 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn) * When a child task exits, feed back event values to parent events. * * Can be called with exec_update_mutex held when called from - * install_exec_creds(). + * setup_new_exec(). */ void perf_event_exit_task(struct task_struct *child) { From 7d503feba0c88586b4c9f1212e9cc582c9370fa7 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 2 Apr 2020 18:35:14 -0500 Subject: [PATCH 05/24] exec: In setup_new_exec cache current in the local variable me At least gcc 8.3 when generating code for x86_64 has a hard time consolidating multiple calls to current aka get_current(), and winds up unnecessarily rereading %gs:current_task several times in setup_new_exec. Caching the value of current in the local variable of me generates slightly better and shorter assembly. Reviewed-by: Kees Cook Reviewed-by: Greg Ungerer Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 93e40f865523..8c3abafb9bb1 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1391,6 +1391,7 @@ EXPORT_SYMBOL(would_dump); void setup_new_exec(struct linux_binprm * bprm) { + struct task_struct *me = current; /* * Once here, prepare_binrpm() will not be called any more, so * the final state of setuid/setgid/fscaps can be merged into the @@ -1400,7 +1401,7 @@ void setup_new_exec(struct linux_binprm * bprm) if (bprm->secureexec) { /* Make sure parent cannot signal privileged process. */ - current->pdeath_signal = 0; + me->pdeath_signal = 0; /* * For secureexec, reset the stack limit to sane default to @@ -1413,9 +1414,9 @@ void setup_new_exec(struct linux_binprm * bprm) bprm->rlim_stack.rlim_cur = _STK_LIM; } - arch_pick_mmap_layout(current->mm, &bprm->rlim_stack); + arch_pick_mmap_layout(me->mm, &bprm->rlim_stack); - current->sas_ss_sp = current->sas_ss_size = 0; + me->sas_ss_sp = me->sas_ss_size = 0; /* * Figure out dumpability. Note that this checking only of current @@ -1431,18 +1432,18 @@ void setup_new_exec(struct linux_binprm * bprm) arch_setup_new_exec(); perf_event_exec(); - __set_task_comm(current, kbasename(bprm->filename), true); + __set_task_comm(me, kbasename(bprm->filename), true); /* Set the new mm task size. We have to do that late because it may * depend on TIF_32BIT which is only updated in flush_thread() on * some architectures like powerpc */ - current->mm->task_size = TASK_SIZE; + me->mm->task_size = TASK_SIZE; /* An exec changes our domain. We are no longer part of the thread group */ - WRITE_ONCE(current->self_exec_id, current->self_exec_id + 1); - flush_signal_handlers(current, 0); + WRITE_ONCE(me->self_exec_id, me->self_exec_id + 1); + flush_signal_handlers(me, 0); /* * install the new credentials for this executable @@ -1458,16 +1459,16 @@ void setup_new_exec(struct linux_binprm * bprm) * wait until new credentials are committed * by commit_creds() above */ - if (get_dumpable(current->mm) != SUID_DUMP_USER) - perf_event_exit_task(current); + if (get_dumpable(me->mm) != SUID_DUMP_USER) + perf_event_exit_task(me); /* * cred_guard_mutex must be held at least to this point to prevent * ptrace_attach() from altering our determination of the task's * credentials; any time after this it may be unlocked. */ security_bprm_committed_creds(bprm); - mutex_unlock(¤t->signal->exec_update_mutex); - mutex_unlock(¤t->signal->cred_guard_mutex); + mutex_unlock(&me->signal->exec_update_mutex); + mutex_unlock(&me->signal->cred_guard_mutex); } EXPORT_SYMBOL(setup_new_exec); From df9e4d2c4a53503a97fc08eeebdc04e3c11b4618 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sun, 3 May 2020 07:15:28 -0500 Subject: [PATCH 06/24] exec: Move most of setup_new_exec into flush_old_exec The current idiom for the callers is: flush_old_exec(bprm); set_personality(...); setup_new_exec(bprm); In 2010 Linus split flush_old_exec into flush_old_exec and setup_new_exec. With the intention that setup_new_exec be what is called after the processes new personality is set. Move the code that doesn't depend upon the personality from setup_new_exec into flush_old_exec. This is to facilitate future changes by having as much code together in one function as possible. To see why it is safe to move this code please note that effectively this change moves the personality setting in the binfmt and the following three lines of code after everything except unlocking the mutexes: arch_pick_mmap_layout arch_setup_new_exec mm->task_size = TASK_SIZE The function arch_pick_mmap_layout at most sets: mm->get_unmapped_area mm->mmap_base mm->mmap_legacy_base mm->mmap_compat_base mm->mmap_compat_legacy_base which nothing in flush_old_exec or setup_new_exec depends on. The function arch_setup_new_exec only sets architecture specific state and the rest of the functions only deal in state that applies to all architectures. The last line just sets mm->task_size and again nothing in flush_old_exec or setup_new_exec depend on task_size. Ref: 221af7f87b97 ("Split 'flush_old_exec' into two functions") Reviewed-by: Kees Cook Reviewed-by: Greg Ungerer Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 133 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 68 insertions(+), 65 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 8c3abafb9bb1..0eff20558735 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1359,6 +1359,73 @@ int flush_old_exec(struct linux_binprm * bprm) * undergoing exec(2). */ do_close_on_exec(me->files); + + /* + * Once here, prepare_binrpm() will not be called any more, so + * the final state of setuid/setgid/fscaps can be merged into the + * secureexec flag. + */ + bprm->secureexec |= bprm->cap_elevated; + + if (bprm->secureexec) { + /* Make sure parent cannot signal privileged process. */ + me->pdeath_signal = 0; + + /* + * For secureexec, reset the stack limit to sane default to + * avoid bad behavior from the prior rlimits. This has to + * happen before arch_pick_mmap_layout(), which examines + * RLIMIT_STACK, but after the point of no return to avoid + * needing to clean up the change on failure. + */ + if (bprm->rlim_stack.rlim_cur > _STK_LIM) + bprm->rlim_stack.rlim_cur = _STK_LIM; + } + + me->sas_ss_sp = me->sas_ss_size = 0; + + /* + * Figure out dumpability. Note that this checking only of current + * is wrong, but userspace depends on it. This should be testing + * bprm->secureexec instead. + */ + if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || + !(uid_eq(current_euid(), current_uid()) && + gid_eq(current_egid(), current_gid()))) + set_dumpable(current->mm, suid_dumpable); + else + set_dumpable(current->mm, SUID_DUMP_USER); + + perf_event_exec(); + __set_task_comm(me, kbasename(bprm->filename), true); + + /* An exec changes our domain. We are no longer part of the thread + group */ + WRITE_ONCE(me->self_exec_id, me->self_exec_id + 1); + flush_signal_handlers(me, 0); + + /* + * install the new credentials for this executable + */ + security_bprm_committing_creds(bprm); + + commit_creds(bprm->cred); + bprm->cred = NULL; + + /* + * Disable monitoring for regular users + * when executing setuid binaries. Must + * wait until new credentials are committed + * by commit_creds() above + */ + if (get_dumpable(me->mm) != SUID_DUMP_USER) + perf_event_exit_task(me); + /* + * cred_guard_mutex must be held at least to this point to prevent + * ptrace_attach() from altering our determination of the task's + * credentials; any time after this it may be unlocked. + */ + security_bprm_committed_creds(bprm); return 0; out_unlock: @@ -1391,82 +1458,18 @@ EXPORT_SYMBOL(would_dump); void setup_new_exec(struct linux_binprm * bprm) { + /* Setup things that can depend upon the personality */ struct task_struct *me = current; - /* - * Once here, prepare_binrpm() will not be called any more, so - * the final state of setuid/setgid/fscaps can be merged into the - * secureexec flag. - */ - bprm->secureexec |= bprm->cap_elevated; - - if (bprm->secureexec) { - /* Make sure parent cannot signal privileged process. */ - me->pdeath_signal = 0; - - /* - * For secureexec, reset the stack limit to sane default to - * avoid bad behavior from the prior rlimits. This has to - * happen before arch_pick_mmap_layout(), which examines - * RLIMIT_STACK, but after the point of no return to avoid - * needing to clean up the change on failure. - */ - if (bprm->rlim_stack.rlim_cur > _STK_LIM) - bprm->rlim_stack.rlim_cur = _STK_LIM; - } arch_pick_mmap_layout(me->mm, &bprm->rlim_stack); - me->sas_ss_sp = me->sas_ss_size = 0; - - /* - * Figure out dumpability. Note that this checking only of current - * is wrong, but userspace depends on it. This should be testing - * bprm->secureexec instead. - */ - if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || - !(uid_eq(current_euid(), current_uid()) && - gid_eq(current_egid(), current_gid()))) - set_dumpable(current->mm, suid_dumpable); - else - set_dumpable(current->mm, SUID_DUMP_USER); - arch_setup_new_exec(); - perf_event_exec(); - __set_task_comm(me, kbasename(bprm->filename), true); /* Set the new mm task size. We have to do that late because it may * depend on TIF_32BIT which is only updated in flush_thread() on * some architectures like powerpc */ me->mm->task_size = TASK_SIZE; - - /* An exec changes our domain. We are no longer part of the thread - group */ - WRITE_ONCE(me->self_exec_id, me->self_exec_id + 1); - flush_signal_handlers(me, 0); - - /* - * install the new credentials for this executable - */ - security_bprm_committing_creds(bprm); - - commit_creds(bprm->cred); - bprm->cred = NULL; - - /* - * Disable monitoring for regular users - * when executing setuid binaries. Must - * wait until new credentials are committed - * by commit_creds() above - */ - if (get_dumpable(me->mm) != SUID_DUMP_USER) - perf_event_exit_task(me); - /* - * cred_guard_mutex must be held at least to this point to prevent - * ptrace_attach() from altering our determination of the task's - * credentials; any time after this it may be unlocked. - */ - security_bprm_committed_creds(bprm); mutex_unlock(&me->signal->exec_update_mutex); mutex_unlock(&me->signal->cred_guard_mutex); } From 2388777a0a5957a10b3d78677216530a9b3bd09f Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sun, 3 May 2020 07:54:10 -0500 Subject: [PATCH 07/24] exec: Rename flush_old_exec begin_new_exec There is and has been for a very long time been a lot more going on in flush_old_exec than just flushing the old state. After the movement of code from setup_new_exec there is a whole lot more going on than just flushing the old executables state. Rename flush_old_exec to begin_new_exec to more accurately reflect what this function does. Reviewed-by: Kees Cook Reviewed-by: Greg Ungerer Signed-off-by: "Eric W. Biederman" --- Documentation/trace/ftrace.rst | 2 +- arch/x86/ia32/ia32_aout.c | 2 +- fs/binfmt_aout.c | 2 +- fs/binfmt_elf.c | 2 +- fs/binfmt_elf_fdpic.c | 2 +- fs/binfmt_flat.c | 2 +- fs/exec.c | 4 ++-- include/linux/binfmts.h | 2 +- 8 files changed, 9 insertions(+), 9 deletions(-) diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst index 3b5614b1d1a5..430a16283103 100644 --- a/Documentation/trace/ftrace.rst +++ b/Documentation/trace/ftrace.rst @@ -1524,7 +1524,7 @@ display-graph option:: => remove_vma => exit_mmap => mmput - => flush_old_exec + => begin_new_exec => load_elf_binary => search_binary_handler => __do_execve_file.isra.32 diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c index 8255fdc3a027..385d3d172ee1 100644 --- a/arch/x86/ia32/ia32_aout.c +++ b/arch/x86/ia32/ia32_aout.c @@ -131,7 +131,7 @@ static int load_aout_binary(struct linux_binprm *bprm) return -ENOMEM; /* Flush all traces of the currently running executable */ - retval = flush_old_exec(bprm); + retval = begin_new_exec(bprm); if (retval) return retval; diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c index c8ba28f285e5..3e84e9bb9084 100644 --- a/fs/binfmt_aout.c +++ b/fs/binfmt_aout.c @@ -151,7 +151,7 @@ static int load_aout_binary(struct linux_binprm * bprm) return -ENOMEM; /* Flush all traces of the currently running executable */ - retval = flush_old_exec(bprm); + retval = begin_new_exec(bprm); if (retval) return retval; diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index e6b586623035..396d5c2e6b5e 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -844,7 +844,7 @@ out_free_interp: goto out_free_dentry; /* Flush all traces of the currently running executable */ - retval = flush_old_exec(bprm); + retval = begin_new_exec(bprm); if (retval) goto out_free_dentry; diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 9a1aa61b4cc3..896e3ca9bf85 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -338,7 +338,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) interp_params.flags |= ELF_FDPIC_FLAG_CONSTDISP; /* flush all traces of the currently running executable */ - retval = flush_old_exec(bprm); + retval = begin_new_exec(bprm); if (retval) goto error; diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 252878969582..9b82bc111d0a 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -534,7 +534,7 @@ static int load_flat_file(struct linux_binprm *bprm, /* Flush all traces of the currently running executable */ if (id == 0) { - ret = flush_old_exec(bprm); + ret = begin_new_exec(bprm); if (ret) goto err; diff --git a/fs/exec.c b/fs/exec.c index 0eff20558735..3cc40048cc65 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1298,7 +1298,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) * signal (via de_thread() or coredump), or will have SEGV raised * (after exec_mmap()) by search_binary_handlers (see below). */ -int flush_old_exec(struct linux_binprm * bprm) +int begin_new_exec(struct linux_binprm * bprm) { struct task_struct *me = current; int retval; @@ -1433,7 +1433,7 @@ out_unlock: out: return retval; } -EXPORT_SYMBOL(flush_old_exec); +EXPORT_SYMBOL(begin_new_exec); void would_dump(struct linux_binprm *bprm, struct file *file) { diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 2a8fddf3574a..1b48e2154766 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -125,7 +125,7 @@ extern void unregister_binfmt(struct linux_binfmt *); 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 int begin_new_exec(struct linux_binprm * bprm); extern void setup_new_exec(struct linux_binprm * bprm); extern void finalize_exec(struct linux_binprm *bprm); extern void would_dump(struct linux_binprm *, struct file *); From 7a60ef4803d5442804d75095627e81602ff23331 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sun, 8 Mar 2020 12:04:44 -0500 Subject: [PATCH 08/24] exec: Move the comment from above de_thread to above unshare_sighand The comment describes work that now happens in unshare_sighand so move the comment where it makes sense. Link: https://lkml.kernel.org/r/87mu6i6zcs.fsf_-_@x220.int.ebiederm.org Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 3cc40048cc65..d4387bc92292 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1093,12 +1093,6 @@ static int exec_mmap(struct mm_struct *mm) return 0; } -/* - * This function makes sure the current process has its own signal table, - * so that flush_signal_handlers can later reset the handlers without - * disturbing other processes. (Other processes might share the signal - * table via the CLONE_SIGHAND option to clone().) - */ static int de_thread(struct task_struct *tsk) { struct signal_struct *sig = tsk->signal; @@ -1240,6 +1234,12 @@ killed: } +/* + * This function makes sure the current process has its own signal table, + * so that flush_signal_handlers can later reset the handlers without + * disturbing other processes. (Other processes might share the signal + * table via the CLONE_SIGHAND option to clone().) + */ static int unshare_sighand(struct task_struct *me) { struct sighand_struct *oldsighand = me->sighand; From 13c432b51449dcdcfa0350fb87250b1dbd1fbd12 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 19 Mar 2020 17:16:12 -0500 Subject: [PATCH 09/24] exec: Fix spelling of search_binary_handler in a comment Link: https://lkml.kernel.org/r/87h7wq6zc1.fsf_-_@x220.int.ebiederm.org Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index d4387bc92292..82106241ed53 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1296,7 +1296,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) * Calling this is the point of no return. None of the failures will be * seen by userspace since either the process is already taking a fatal * signal (via de_thread() or coredump), or will have SEGV raised - * (after exec_mmap()) by search_binary_handlers (see below). + * (after exec_mmap()) by search_binary_handler (see below). */ int begin_new_exec(struct linux_binprm * bprm) { From a28bf136e651e17d7e2c753aa140ce3cc1df36a0 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 30 Mar 2020 16:33:39 -0500 Subject: [PATCH 10/24] exec: Run sync_mm_rss before taking exec_update_mutex Like exec_mm_release sync_mm_rss is about flushing out the state of the old_mm, which does not need to happen under exec_update_mutex. Make this explicit by moving sync_mm_rss outside of exec_update_mutex. Reviewed-by: Kees Cook Link: https://lkml.kernel.org/r/875zd66za3.fsf_-_@x220.int.ebiederm.org Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index 82106241ed53..ecee0ebebf85 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1051,13 +1051,14 @@ static int exec_mmap(struct mm_struct *mm) tsk = current; old_mm = current->mm; exec_mm_release(tsk, old_mm); + if (old_mm) + sync_mm_rss(old_mm); ret = mutex_lock_killable(&tsk->signal->exec_update_mutex); if (ret) return ret; if (old_mm) { - sync_mm_rss(old_mm); /* * Make sure that if there is a core dump in progress * for the old mm, we get out and die instead of going From 8890b29341f30f4a364b2eb6046bb1ac1478f955 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sat, 4 Apr 2020 09:42:56 -0500 Subject: [PATCH 11/24] exec: Move handling of the point of no return to the top level Move the handing of the point of no return from search_binary_handler into __do_execve_file so that it is easier to find, and to keep things robust in the face of change. Make it clear that an existing fatal signal will take precedence over a forced SIGSEGV by not forcing SIGSEGV if a fatal signal is already pending. This does not change the behavior but it saves a reader of the code the tedium of reading and understanding force_sig and the signal delivery code. Update the comment in begin_new_exec about where SIGSEGV is forced. Keep point_of_no_return from being a mystery by documenting what the code is doing where it forces SIGSEGV if the code is past the point of no return. Reviewed-by: Kees Cook Link: https://lkml.kernel.org/r/87y2q25knl.fsf_-_@x220.int.ebiederm.org Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index ecee0ebebf85..fa265ea322b7 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1329,8 +1329,8 @@ int begin_new_exec(struct linux_binprm * bprm) /* * With the new mm installed it is completely impossible to * fail and return to the original process. If anything from - * here on returns an error, the check in - * search_binary_handler() will SEGV current. + * here on returns an error, the check in __do_execve_file() + * will SEGV current. */ bprm->point_of_no_return = true; bprm->mm = NULL; @@ -1721,13 +1721,8 @@ int search_binary_handler(struct linux_binprm *bprm) read_lock(&binfmt_lock); put_binfmt(fmt); - if (retval < 0 && bprm->point_of_no_return) { - /* we got to flush_old_exec() and failed after it */ - read_unlock(&binfmt_lock); - force_sigsegv(SIGSEGV); - return retval; - } - if (retval != -ENOEXEC || !bprm->file) { + if (bprm->point_of_no_return || !bprm->file || + (retval != -ENOEXEC)) { read_unlock(&binfmt_lock); return retval; } @@ -1898,6 +1893,14 @@ static int __do_execve_file(int fd, struct filename *filename, return retval; out: + /* + * If past the point of no return ensure the the code never + * returns to the userspace process. Use an existing fatal + * signal if present otherwise terminate the process with + * SIGSEGV. + */ + if (bprm->point_of_no_return && !fatal_signal_pending(current)) + force_sigsegv(SIGSEGV); if (bprm->mm) { acct_arg_size(bprm, 0); mmput(bprm->mm); From 6834e0bb41bbe144d42b8cd843f3ee30bfe2c825 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sat, 4 Apr 2020 12:01:37 -0500 Subject: [PATCH 12/24] exec: Set the point of no return sooner Make the code more robust by marking the point of no return sooner. This ensures that future code changes don't need to worry about how they return errors if they are past this point. This results in no actual change in behavior as __do_execve_file does not force SIGSEGV when there is a pending fatal signal pending past the point of no return. Further the only error returns from de_thread and exec_mmap that can occur result in fatal signals being pending. Reviewed-by: Kees Cook Link: https://lkml.kernel.org/r/87sgga5klu.fsf_-_@x220.int.ebiederm.org Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index fa265ea322b7..9aa08ce2ffcc 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1304,6 +1304,11 @@ int begin_new_exec(struct linux_binprm * bprm) struct task_struct *me = current; int retval; + /* + * Ensure all future errors are fatal. + */ + bprm->point_of_no_return = true; + /* * Make this the only thread in the thread group. */ @@ -1326,13 +1331,6 @@ int begin_new_exec(struct linux_binprm * bprm) if (retval) goto out; - /* - * With the new mm installed it is completely impossible to - * fail and return to the original process. If anything from - * here on returns an error, the check in __do_execve_file() - * will SEGV current. - */ - bprm->point_of_no_return = true; bprm->mm = NULL; #ifdef CONFIG_POSIX_TIMERS From 87b047d2be417b271d80f5e490a825c6fd53ecad Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 16 Mar 2020 12:21:12 -0500 Subject: [PATCH 13/24] exec: Teach prepare_exec_creds how exec treats uids & gids It is almost possible to use the result of prepare_exec_creds with no modifications during exec. Update prepare_exec_creds to initialize the suid and the fsuid to the euid, and the sgid and the fsgid to the egid. This is all that is needed to handle the common case of exec when nothing special like a setuid exec is happening. That this preserves the existing behavior of exec can be verified by examing bprm_fill_uid and cap_bprm_set_creds. This change makes it clear that the later parts of exec that update bprm->cred are just need to handle special cases such as setuid exec and change of domains. Link: https://lkml.kernel.org/r/871rng22dm.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- kernel/cred.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/cred.c b/kernel/cred.c index 71a792616917..421b1149c651 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -315,6 +315,9 @@ struct cred *prepare_exec_creds(void) new->process_keyring = NULL; #endif + new->suid = new->fsuid = new->euid; + new->sgid = new->fsgid = new->egid; + return new; } From b8bff599261c930630385ee21d3f98e7ce7d4843 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sun, 22 Mar 2020 15:46:24 -0500 Subject: [PATCH 14/24] exec: Factor security_bprm_creds_for_exec out of security_bprm_set_creds Today security_bprm_set_creds has several implementations: apparmor_bprm_set_creds, cap_bprm_set_creds, selinux_bprm_set_creds, smack_bprm_set_creds, and tomoyo_bprm_set_creds. Except for cap_bprm_set_creds they all test bprm->called_set_creds and return immediately if it is true. The function cap_bprm_set_creds ignores bprm->calld_sed_creds entirely. Create a new LSM hook security_bprm_creds_for_exec that is called just before prepare_binprm in __do_execve_file, resulting in a LSM hook that is called exactly once for the entire of exec. Modify the bits of security_bprm_set_creds that only want to be called once per exec into security_bprm_creds_for_exec, leaving only cap_bprm_set_creds behind. Remove bprm->called_set_creds all of it's former users have been moved to security_bprm_creds_for_exec. Add or upate comments a appropriate to bring them up to date and to reflect this change. Link: https://lkml.kernel.org/r/87v9kszrzh.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Acked-by: Casey Schaufler # For the LSM and Smack bits Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 6 +++- include/linux/binfmts.h | 18 +++-------- include/linux/lsm_hook_defs.h | 1 + include/linux/lsm_hooks.h | 50 +++++++++++++++++------------- include/linux/security.h | 6 ++++ security/apparmor/domain.c | 7 ++--- security/apparmor/include/domain.h | 2 +- security/apparmor/lsm.c | 2 +- security/security.c | 5 +++ security/selinux/hooks.c | 8 ++--- security/smack/smack_lsm.c | 9 ++---- security/tomoyo/tomoyo.c | 12 ++----- 12 files changed, 63 insertions(+), 63 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 14b786158aa9..9e70da47f8d9 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1640,7 +1640,6 @@ int prepare_binprm(struct linux_binprm *bprm) retval = security_bprm_set_creds(bprm); if (retval) return retval; - bprm->called_set_creds = 1; memset(bprm->buf, 0, BINPRM_BUF_SIZE); return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos); @@ -1855,6 +1854,11 @@ static int __do_execve_file(int fd, struct filename *filename, if (retval < 0) goto out; + /* Set the unchanging part of bprm->cred */ + retval = security_bprm_creds_for_exec(bprm); + if (retval) + goto out; + retval = prepare_binprm(bprm); if (retval < 0) goto out; diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 1b48e2154766..d1217fcdedea 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -27,22 +27,14 @@ struct linux_binprm { unsigned long argmin; /* rlimit marker for copy_strings() */ unsigned int /* - * True after the bprm_set_creds hook has been called once - * (multiple calls can be made via prepare_binprm() for - * binfmt_script/misc). - */ - called_set_creds:1, - /* - * True if most recent call to the commoncaps bprm_set_creds - * hook (due to multiple prepare_binprm() calls from the - * binfmt_script/misc handlers) resulted in elevated - * privileges. + * True if most recent call to cap_bprm_set_creds + * resulted in elevated privileges. */ cap_elevated:1, /* - * Set by bprm_set_creds hook to indicate a privilege-gaining - * exec has happened. Used to sanitize execution environment - * and to set AT_SECURE auxv for glibc. + * Set by bprm_creds_for_exec hook to indicate a + * privilege-gaining exec has happened. Used to set + * AT_SECURE auxv for glibc. */ secureexec:1, /* diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 9cd4455528e5..aab0695f41df 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -49,6 +49,7 @@ LSM_HOOK(int, 0, syslog, int type) LSM_HOOK(int, 0, settime, const struct timespec64 *ts, const struct timezone *tz) LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages) +LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm) LSM_HOOK(int, 0, bprm_set_creds, struct linux_binprm *bprm) LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 988ca0df7824..c719af37df20 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -34,40 +34,46 @@ * * Security hooks for program execution operations. * + * @bprm_creds_for_exec: + * If the setup in prepare_exec_creds did not setup @bprm->cred->security + * properly for executing @bprm->file, update the LSM's portion of + * @bprm->cred->security to be what commit_creds needs to install for the + * new program. This hook may also optionally check permissions + * (e.g. for transitions between security domains). + * The hook must set @bprm->secureexec to 1 if AT_SECURE should be set to + * request libc enable secure mode. + * @bprm contains the linux_binprm structure. + * Return 0 if the hook is successful and permission is granted. * @bprm_set_creds: - * Save security information in the bprm->security field, typically based - * on information about the bprm->file, for later use by the apply_creds - * hook. This hook may also optionally check permissions (e.g. for + * Assuming that the relevant bits of @bprm->cred->security have been + * previously set, examine @bprm->file and regenerate them. This is + * so that the credentials derived from the interpreter the code is + * actually going to run are used rather than credentials derived + * from a script. This done because the interpreter binary needs to + * reopen script, and may end up opening something completely different. + * This hook may also optionally check permissions (e.g. for * transitions between security domains). - * This hook may be called multiple times during a single execve, e.g. for - * interpreters. The hook can tell whether it has already been called by - * checking to see if @bprm->security is non-NULL. If so, then the hook - * may decide either to retain the security information saved earlier or - * to replace it. The hook must set @bprm->secureexec to 1 if a "secure - * exec" has happened as a result of this hook call. The flag is used to - * indicate the need for a sanitized execution environment, and is also - * passed in the ELF auxiliary table on the initial stack to indicate - * whether libc should enable secure mode. + * The hook must set @bprm->cap_elevated to 1 if AT_SECURE should be set to + * request libc enable secure mode. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. * @bprm_check_security: * This hook mediates the point when a search for a binary handler will - * begin. It allows a check the @bprm->security value which is set in the - * preceding set_creds call. The primary difference from set_creds is - * that the argv list and envp list are reliably available in @bprm. This - * hook may be called multiple times during a single execve; and in each - * pass set_creds is called first. + * begin. It allows a check against the @bprm->cred->security value + * which was set in the preceding creds_for_exec call. The argv list and + * envp list are reliably available in @bprm. This hook may be called + * multiple times during a single execve. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. * @bprm_committing_creds: * Prepare to install the new security attributes of a process being * transformed by an execve operation, based on the old credentials * pointed to by @current->cred and the information set in @bprm->cred by - * the bprm_set_creds hook. @bprm points to the linux_binprm structure. - * This hook is a good place to perform state changes on the process such - * as closing open file descriptors to which access will no longer be - * granted when the attributes are changed. This is called immediately - * before commit_creds(). + * the bprm_creds_for_exec hook. @bprm points to the linux_binprm + * structure. This hook is a good place to perform state changes on the + * process such as closing open file descriptors to which access will no + * longer be granted when the attributes are changed. This is called + * immediately before commit_creds(). * @bprm_committed_creds: * Tidy up after the installation of the new security attributes of a * process being transformed by an execve operation. The new credentials diff --git a/include/linux/security.h b/include/linux/security.h index a8d9310472df..1bd7a6582775 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -276,6 +276,7 @@ int security_quota_on(struct dentry *dentry); int security_syslog(int type); int security_settime64(const struct timespec64 *ts, const struct timezone *tz); int security_vm_enough_memory_mm(struct mm_struct *mm, long pages); +int security_bprm_creds_for_exec(struct linux_binprm *bprm); int security_bprm_set_creds(struct linux_binprm *bprm); int security_bprm_check(struct linux_binprm *bprm); void security_bprm_committing_creds(struct linux_binprm *bprm); @@ -569,6 +570,11 @@ static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages)); } +static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm) +{ + return 0; +} + static inline int security_bprm_set_creds(struct linux_binprm *bprm) { return cap_bprm_set_creds(bprm); diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 6ceb74e0f789..0b870a647488 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -854,14 +854,14 @@ static struct aa_label *handle_onexec(struct aa_label *label, } /** - * apparmor_bprm_set_creds - set the new creds on the bprm struct + * apparmor_bprm_creds_for_exec - Update the new creds on the bprm struct * @bprm: binprm for the exec (NOT NULL) * * Returns: %0 or error on failure * * TODO: once the other paths are done see if we can't refactor into a fn */ -int apparmor_bprm_set_creds(struct linux_binprm *bprm) +int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm) { struct aa_task_ctx *ctx; struct aa_label *label, *new = NULL; @@ -875,9 +875,6 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) file_inode(bprm->file)->i_mode }; - if (bprm->called_set_creds) - return 0; - ctx = task_ctx(current); AA_BUG(!cred_label(bprm->cred)); AA_BUG(!ctx); diff --git a/security/apparmor/include/domain.h b/security/apparmor/include/domain.h index 21b875fe2d37..d14928fe1c6f 100644 --- a/security/apparmor/include/domain.h +++ b/security/apparmor/include/domain.h @@ -30,7 +30,7 @@ struct aa_domain { struct aa_label *x_table_lookup(struct aa_profile *profile, u32 xindex, const char **name); -int apparmor_bprm_set_creds(struct linux_binprm *bprm); +int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm); void aa_free_domain_entries(struct aa_domain *domain); int aa_change_hat(const char *hats[], int count, u64 token, int flags); diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index b621ad74f54a..3623ab08279d 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1232,7 +1232,7 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(cred_prepare, apparmor_cred_prepare), LSM_HOOK_INIT(cred_transfer, apparmor_cred_transfer), - LSM_HOOK_INIT(bprm_set_creds, apparmor_bprm_set_creds), + LSM_HOOK_INIT(bprm_creds_for_exec, apparmor_bprm_creds_for_exec), LSM_HOOK_INIT(bprm_committing_creds, apparmor_bprm_committing_creds), LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds), diff --git a/security/security.c b/security/security.c index 7fed24b9d57e..4ee76a729f73 100644 --- a/security/security.c +++ b/security/security.c @@ -823,6 +823,11 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) return __vm_enough_memory(mm, pages, cap_sys_admin); } +int security_bprm_creds_for_exec(struct linux_binprm *bprm) +{ + return call_int_hook(bprm_creds_for_exec, 0, bprm); +} + int security_bprm_set_creds(struct linux_binprm *bprm) { return call_int_hook(bprm_set_creds, 0, bprm); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 0b4e32161b77..718345dd76bb 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2286,7 +2286,7 @@ static int check_nnp_nosuid(const struct linux_binprm *bprm, return -EACCES; } -static int selinux_bprm_set_creds(struct linux_binprm *bprm) +static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm) { const struct task_security_struct *old_tsec; struct task_security_struct *new_tsec; @@ -2297,8 +2297,6 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) /* SELinux context only depends on initial program or script and not * the script interpreter */ - if (bprm->called_set_creds) - return 0; old_tsec = selinux_cred(current_cred()); new_tsec = selinux_cred(bprm->cred); @@ -6385,7 +6383,7 @@ static int selinux_setprocattr(const char *name, void *value, size_t size) /* Permission checking based on the specified context is performed during the actual operation (execve, open/mkdir/...), when we know the full context of the - operation. See selinux_bprm_set_creds for the execve + operation. See selinux_bprm_creds_for_exec for the execve checks and may_create for the file creation checks. The operation will then fail if the context is not permitted. */ tsec = selinux_cred(new); @@ -6914,7 +6912,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(netlink_send, selinux_netlink_send), - LSM_HOOK_INIT(bprm_set_creds, selinux_bprm_set_creds), + LSM_HOOK_INIT(bprm_creds_for_exec, selinux_bprm_creds_for_exec), LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds), LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds), diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 8c61d175e195..0ac8f4518d07 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -891,12 +891,12 @@ static int smack_sb_statfs(struct dentry *dentry) */ /** - * smack_bprm_set_creds - set creds for exec + * smack_bprm_creds_for_exec - Update bprm->cred if needed for exec * @bprm: the exec information * * Returns 0 if it gets a blob, -EPERM if exec forbidden and -ENOMEM otherwise */ -static int smack_bprm_set_creds(struct linux_binprm *bprm) +static int smack_bprm_creds_for_exec(struct linux_binprm *bprm) { struct inode *inode = file_inode(bprm->file); struct task_smack *bsp = smack_cred(bprm->cred); @@ -904,9 +904,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm) struct superblock_smack *sbsp; int rc; - if (bprm->called_set_creds) - return 0; - isp = smack_inode(inode); if (isp->smk_task == NULL || isp->smk_task == bsp->smk_task) return 0; @@ -4598,7 +4595,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(sb_statfs, smack_sb_statfs), LSM_HOOK_INIT(sb_set_mnt_opts, smack_set_mnt_opts), - LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds), + LSM_HOOK_INIT(bprm_creds_for_exec, smack_bprm_creds_for_exec), LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security), LSM_HOOK_INIT(inode_init_security, smack_inode_init_security), diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 716c92ec941a..f9adddc42ac8 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -63,20 +63,14 @@ static void tomoyo_bprm_committed_creds(struct linux_binprm *bprm) #ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER /** - * tomoyo_bprm_set_creds - Target for security_bprm_set_creds(). + * tomoyo_bprm_for_exec - Target for security_bprm_creds_for_exec(). * * @bprm: Pointer to "struct linux_binprm". * * Returns 0. */ -static int tomoyo_bprm_set_creds(struct linux_binprm *bprm) +static int tomoyo_bprm_creds_for_exec(struct linux_binprm *bprm) { - /* - * Do only if this function is called for the first time of an execve - * operation. - */ - if (bprm->called_set_creds) - return 0; /* * Load policy if /sbin/tomoyo-init exists and /sbin/init is requested * for the first time. @@ -539,7 +533,7 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(task_alloc, tomoyo_task_alloc), LSM_HOOK_INIT(task_free, tomoyo_task_free), #ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER - LSM_HOOK_INIT(bprm_set_creds, tomoyo_bprm_set_creds), + LSM_HOOK_INIT(bprm_creds_for_exec, tomoyo_bprm_creds_for_exec), #endif LSM_HOOK_INIT(bprm_check_security, tomoyo_bprm_check_security), LSM_HOOK_INIT(file_fcntl, tomoyo_file_fcntl), From 112b7147592e8f46bd1da4f961773e6d974f38a8 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 14 May 2020 12:53:44 -0500 Subject: [PATCH 15/24] exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds Rename bprm->cap_elevated to bprm->active_secureexec and initialize it in prepare_binprm instead of in cap_bprm_set_creds. Initializing bprm->active_secureexec in prepare_binprm allows multiple implementations of security_bprm_repopulate_creds to play nicely with each other. Rename security_bprm_set_creds to security_bprm_reopulate_creds to emphasize that this path recomputes part of bprm->cred. This recomputation avoids the time of check vs time of use problems that are inherent in unix #! interpreters. In short two renames and a move in the location of initializing bprm->active_secureexec. Link: https://lkml.kernel.org/r/87o8qkzrxp.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 8 ++++---- include/linux/binfmts.h | 4 ++-- include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 4 ++-- include/linux/security.h | 8 ++++---- security/commoncap.c | 9 ++++----- security/security.c | 4 ++-- 7 files changed, 19 insertions(+), 20 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 9e70da47f8d9..8e3b93d51d31 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1366,7 +1366,7 @@ int begin_new_exec(struct linux_binprm * bprm) * the final state of setuid/setgid/fscaps can be merged into the * secureexec flag. */ - bprm->secureexec |= bprm->cap_elevated; + bprm->secureexec |= bprm->active_secureexec; if (bprm->secureexec) { /* Make sure parent cannot signal privileged process. */ @@ -1634,10 +1634,10 @@ int prepare_binprm(struct linux_binprm *bprm) int retval; loff_t pos = 0; + /* Recompute parts of bprm->cred based on bprm->file */ + bprm->active_secureexec = 0; bprm_fill_uid(bprm); - - /* fill in binprm security blob */ - retval = security_bprm_set_creds(bprm); + retval = security_bprm_repopulate_creds(bprm); if (retval) return retval; diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index d1217fcdedea..8605ab4a0f89 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -27,10 +27,10 @@ struct linux_binprm { unsigned long argmin; /* rlimit marker for copy_strings() */ unsigned int /* - * True if most recent call to cap_bprm_set_creds + * True if most recent call to security_bprm_set_creds * resulted in elevated privileges. */ - cap_elevated:1, + active_secureexec:1, /* * Set by bprm_creds_for_exec hook to indicate a * privilege-gaining exec has happened. Used to set diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index aab0695f41df..1e295ba12c0d 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -50,7 +50,7 @@ LSM_HOOK(int, 0, settime, const struct timespec64 *ts, const struct timezone *tz) LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages) LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm) -LSM_HOOK(int, 0, bprm_set_creds, struct linux_binprm *bprm) +LSM_HOOK(int, 0, bprm_repopulate_creds, struct linux_binprm *bprm) LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index c719af37df20..d618ecc4d660 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -44,7 +44,7 @@ * request libc enable secure mode. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. - * @bprm_set_creds: + * @bprm_repopulate_creds: * Assuming that the relevant bits of @bprm->cred->security have been * previously set, examine @bprm->file and regenerate them. This is * so that the credentials derived from the interpreter the code is @@ -53,7 +53,7 @@ * reopen script, and may end up opening something completely different. * This hook may also optionally check permissions (e.g. for * transitions between security domains). - * The hook must set @bprm->cap_elevated to 1 if AT_SECURE should be set to + * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to * request libc enable secure mode. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. diff --git a/include/linux/security.h b/include/linux/security.h index 1bd7a6582775..6dcec9375e8f 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -140,7 +140,7 @@ extern int cap_capset(struct cred *new, const struct cred *old, const kernel_cap_t *effective, const kernel_cap_t *inheritable, const kernel_cap_t *permitted); -extern int cap_bprm_set_creds(struct linux_binprm *bprm); +extern int cap_bprm_repopulate_creds(struct linux_binprm *bprm); extern int cap_inode_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags); extern int cap_inode_removexattr(struct dentry *dentry, const char *name); @@ -277,7 +277,7 @@ int security_syslog(int type); int security_settime64(const struct timespec64 *ts, const struct timezone *tz); int security_vm_enough_memory_mm(struct mm_struct *mm, long pages); int security_bprm_creds_for_exec(struct linux_binprm *bprm); -int security_bprm_set_creds(struct linux_binprm *bprm); +int security_bprm_repopulate_creds(struct linux_binprm *bprm); int security_bprm_check(struct linux_binprm *bprm); void security_bprm_committing_creds(struct linux_binprm *bprm); void security_bprm_committed_creds(struct linux_binprm *bprm); @@ -575,9 +575,9 @@ static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm) return 0; } -static inline int security_bprm_set_creds(struct linux_binprm *bprm) +static inline int security_bprm_repopulate_creds(struct linux_binprm *bprm) { - return cap_bprm_set_creds(bprm); + return cap_bprm_repopulate_creds(bprm); } static inline int security_bprm_check(struct linux_binprm *bprm) diff --git a/security/commoncap.c b/security/commoncap.c index f4ee0ae106b2..045b5b80ea40 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -797,14 +797,14 @@ static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old, } /** - * cap_bprm_set_creds - Set up the proposed credentials for execve(). + * cap_bprm_repopulate_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds * * Set up the proposed credentials for a new execution context being * constructed by execve(). The proposed creds in @bprm->cred is altered, * which won't take effect immediately. Returns 0 if successful, -ve on error. */ -int cap_bprm_set_creds(struct linux_binprm *bprm) +int cap_bprm_repopulate_creds(struct linux_binprm *bprm) { const struct cred *old = current_cred(); struct cred *new = bprm->cred; @@ -884,12 +884,11 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) return -EPERM; /* Check for privilege-elevated exec. */ - bprm->cap_elevated = 0; if (is_setid || (!__is_real(root_uid, new) && (effective || __cap_grew(permitted, ambient, new)))) - bprm->cap_elevated = 1; + bprm->active_secureexec = 1; return 0; } @@ -1346,7 +1345,7 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(ptrace_traceme, cap_ptrace_traceme), LSM_HOOK_INIT(capget, cap_capget), LSM_HOOK_INIT(capset, cap_capset), - LSM_HOOK_INIT(bprm_set_creds, cap_bprm_set_creds), + LSM_HOOK_INIT(bprm_repopulate_creds, cap_bprm_repopulate_creds), LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv), LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv), LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity), diff --git a/security/security.c b/security/security.c index 4ee76a729f73..b890b7e2a765 100644 --- a/security/security.c +++ b/security/security.c @@ -828,9 +828,9 @@ int security_bprm_creds_for_exec(struct linux_binprm *bprm) return call_int_hook(bprm_creds_for_exec, 0, bprm); } -int security_bprm_set_creds(struct linux_binprm *bprm) +int security_bprm_repopulate_creds(struct linux_binprm *bprm) { - return call_int_hook(bprm_set_creds, 0, bprm); + return call_int_hook(bprm_repopulate_creds, 0, bprm); } int security_bprm_check(struct linux_binprm *bprm) From a16b3357b2b8e910bb614254d8a7e84d2bd59b4c Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Sat, 16 May 2020 06:02:54 -0500 Subject: [PATCH 16/24] exec: Allow load_misc_binary to call prepare_binprm unconditionally Add a flag preserve_creds that binfmt_misc can set to prevent credentials from being updated. This allows binfmt_misc to always call prepare_binprm. Allowing the credential computation logic to be consolidated. Not replacing the credentials with the interpreters credentials is safe because because an open file descriptor to the executable is passed to the interpreter. As the interpreter does not need to reopen the executable it is guaranteed to see the same file that exec sees. Ref: c407c033de84 ("[PATCH] binfmt_misc: improve calculation of interpreter's credentials") Link: https://lkml.kernel.org/r/87imgszrwo.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/binfmt_misc.c | 15 +++------------ fs/exec.c | 19 ++++++++++++------- include/linux/binfmts.h | 2 ++ 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index cdb45829354d..264829745d6f 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -218,19 +218,10 @@ static int load_misc_binary(struct linux_binprm *bprm) goto error; bprm->file = interp_file; - if (fmt->flags & MISC_FMT_CREDENTIALS) { - loff_t pos = 0; - - /* - * No need to call prepare_binprm(), it's already been - * done. bprm->buf is stale, update from interp_file. - */ - memset(bprm->buf, 0, BINPRM_BUF_SIZE); - retval = kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, - &pos); - } else - retval = prepare_binprm(bprm); + if (fmt->flags & MISC_FMT_CREDENTIALS) + bprm->preserve_creds = 1; + retval = prepare_binprm(bprm); if (retval < 0) goto error; diff --git a/fs/exec.c b/fs/exec.c index 8e3b93d51d31..028e0e323af5 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1631,15 +1631,20 @@ static void bprm_fill_uid(struct linux_binprm *bprm) */ int prepare_binprm(struct linux_binprm *bprm) { - int retval; loff_t pos = 0; - /* Recompute parts of bprm->cred based on bprm->file */ - bprm->active_secureexec = 0; - bprm_fill_uid(bprm); - retval = security_bprm_repopulate_creds(bprm); - if (retval) - return retval; + /* Can the interpreter get to the executable without races? */ + if (!bprm->preserve_creds) { + int retval; + + /* Recompute parts of bprm->cred based on bprm->file */ + bprm->active_secureexec = 0; + bprm_fill_uid(bprm); + retval = security_bprm_repopulate_creds(bprm); + if (retval) + return retval; + } + bprm->preserve_creds = 0; memset(bprm->buf, 0, BINPRM_BUF_SIZE); return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 8605ab4a0f89..dbb5614d62a2 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -26,6 +26,8 @@ struct linux_binprm { unsigned long p; /* current top of mem */ unsigned long argmin; /* rlimit marker for copy_strings() */ unsigned int + /* It is safe to use the creds of a script (see binfmt_misc) */ + preserve_creds:1, /* * True if most recent call to security_bprm_set_creds * resulted in elevated privileges. From 8b72ca9004ed35104deb80b07990da5503bc5252 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Wed, 13 May 2020 22:25:20 -0500 Subject: [PATCH 17/24] exec: Move the call of prepare_binprm into search_binary_handler The code in prepare_binary_handler needs to be run every time search_binary_handler is called so move the call into search_binary_handler itself to make the code simpler and easier to understand. Link: https://lkml.kernel.org/r/87d070zrvx.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Reviewed-by: Kees Cook Reviewed-by: James Morris Signed-off-by: "Eric W. Biederman" --- arch/alpha/kernel/binfmt_loader.c | 3 --- fs/binfmt_em86.c | 4 ---- fs/binfmt_misc.c | 4 ---- fs/binfmt_script.c | 3 --- fs/exec.c | 12 +++++------- include/linux/binfmts.h | 1 - 6 files changed, 5 insertions(+), 22 deletions(-) diff --git a/arch/alpha/kernel/binfmt_loader.c b/arch/alpha/kernel/binfmt_loader.c index a8d0d6e06526..d712ba51d15a 100644 --- a/arch/alpha/kernel/binfmt_loader.c +++ b/arch/alpha/kernel/binfmt_loader.c @@ -35,9 +35,6 @@ static int load_binary(struct linux_binprm *bprm) bprm->file = file; bprm->loader = loader; - retval = prepare_binprm(bprm); - if (retval < 0) - return retval; return search_binary_handler(bprm); } diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c index 466497860c62..cedde2341ade 100644 --- a/fs/binfmt_em86.c +++ b/fs/binfmt_em86.c @@ -91,10 +91,6 @@ static int load_em86(struct linux_binprm *bprm) bprm->file = file; - retval = prepare_binprm(bprm); - if (retval < 0) - return retval; - return search_binary_handler(bprm); } diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index 264829745d6f..50a73afdf9b7 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -221,10 +221,6 @@ static int load_misc_binary(struct linux_binprm *bprm) if (fmt->flags & MISC_FMT_CREDENTIALS) bprm->preserve_creds = 1; - retval = prepare_binprm(bprm); - if (retval < 0) - goto error; - retval = search_binary_handler(bprm); if (retval < 0) goto error; diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c index e9e6a6f4a35f..8d718d8fd0fe 100644 --- a/fs/binfmt_script.c +++ b/fs/binfmt_script.c @@ -143,9 +143,6 @@ static int load_script(struct linux_binprm *bprm) return PTR_ERR(file); bprm->file = file; - retval = prepare_binprm(bprm); - if (retval < 0) - return retval; return search_binary_handler(bprm); } diff --git a/fs/exec.c b/fs/exec.c index 028e0e323af5..5fc458460e44 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1629,7 +1629,7 @@ static void bprm_fill_uid(struct linux_binprm *bprm) * * This may be called multiple times for binary chains (scripts for example). */ -int prepare_binprm(struct linux_binprm *bprm) +static int prepare_binprm(struct linux_binprm *bprm) { loff_t pos = 0; @@ -1650,8 +1650,6 @@ int prepare_binprm(struct linux_binprm *bprm) return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos); } -EXPORT_SYMBOL(prepare_binprm); - /* * Arguments are '\0' separated strings found at the location bprm->p * points to; chop off the first by relocating brpm->p to right after @@ -1707,6 +1705,10 @@ int search_binary_handler(struct linux_binprm *bprm) if (bprm->recursion_depth > 5) return -ELOOP; + retval = prepare_binprm(bprm); + if (retval < 0) + return retval; + retval = security_bprm_check(bprm); if (retval) return retval; @@ -1864,10 +1866,6 @@ static int __do_execve_file(int fd, struct filename *filename, if (retval) goto out; - retval = prepare_binprm(bprm); - if (retval < 0) - goto out; - retval = copy_strings_kernel(1, &bprm->filename, bprm); if (retval < 0) goto out; diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index dbb5614d62a2..8c7779d6bf19 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -116,7 +116,6 @@ static inline void insert_binfmt(struct linux_binfmt *fmt) extern void unregister_binfmt(struct linux_binfmt *); -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 begin_new_exec(struct linux_binprm * bprm); From ccbb18b67323b5c70b5f07177381d14cd002621f Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 18 May 2020 10:11:10 -0500 Subject: [PATCH 18/24] exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC The return code -ENOEXEC serves to tell search_binary_handler that it should continue searching for the binfmt to handle a given file. This makes return -ENOEXEC with a bprm->buf that is needed to continue the search problematic. The current binfmt_script manages to escape problems as it closes and clears bprm->file before return -ENOEXEC with bprm->buf modified. This prevents search_binary_handler from looping as it explicitly handles a NULL bprm->file. I plan on moving all of the bprm->file managment into fs/exec.c and out of the binary handlers so this will become a problem. Move closing bprm->file and the test for BINPRM_PATH_INACCESSIBLE down below the last return of -ENOEXEC. Introduce i_sep and i_end to track the end of the first argument and the end of the parameters respectively. Using those, constification of all char * pointers, and the helpers next_terminator and next_non_spacetab guarantee the parameter parsing will not modify bprm->buf. Only modify bprm->buf to terminate the strings i_arg and i_name with '\0' for passing to copy_strings_kernel. When replacing loops with next_non_spacetab and next_terminator care has been take that the logic of the parsing code (short of replacing characters by '\0') remains the same. Link: https://lkml.kernel.org/r/874ksczru6.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/binfmt_script.c | 80 ++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c index 8d718d8fd0fe..85e0ef86eb11 100644 --- a/fs/binfmt_script.c +++ b/fs/binfmt_script.c @@ -16,14 +16,14 @@ #include static inline bool spacetab(char c) { return c == ' ' || c == '\t'; } -static inline char *next_non_spacetab(char *first, const char *last) +static inline const char *next_non_spacetab(const char *first, const char *last) { for (; first <= last; first++) if (!spacetab(*first)) return first; return NULL; } -static inline char *next_terminator(char *first, const char *last) +static inline const char *next_terminator(const char *first, const char *last) { for (; first <= last; first++) if (spacetab(*first) || !*first) @@ -33,8 +33,7 @@ static inline char *next_terminator(char *first, const char *last) static int load_script(struct linux_binprm *bprm) { - const char *i_arg, *i_name; - char *cp, *buf_end; + const char *i_name, *i_sep, *i_arg, *i_end, *buf_end; struct file *file; int retval; @@ -42,20 +41,6 @@ static int load_script(struct linux_binprm *bprm) if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!')) return -ENOEXEC; - /* - * If the script filename will be inaccessible after exec, typically - * because it is a "/dev/fd//.." path against an O_CLOEXEC fd, give - * up now (on the assumption that the interpreter will want to load - * this file). - */ - if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE) - return -ENOENT; - - /* Release since we are not mapping a binary into memory. */ - allow_write_access(bprm->file); - fput(bprm->file); - bprm->file = NULL; - /* * This section handles parsing the #! line into separate * interpreter path and argument strings. We must be careful @@ -71,39 +56,48 @@ static int load_script(struct linux_binprm *bprm) * parse them on its own. */ buf_end = bprm->buf + sizeof(bprm->buf) - 1; - cp = strnchr(bprm->buf, sizeof(bprm->buf), '\n'); - if (!cp) { - cp = next_non_spacetab(bprm->buf + 2, buf_end); - if (!cp) + i_end = strnchr(bprm->buf, sizeof(bprm->buf), '\n'); + if (!i_end) { + i_end = next_non_spacetab(bprm->buf + 2, buf_end); + if (!i_end) return -ENOEXEC; /* Entire buf is spaces/tabs */ /* * If there is no later space/tab/NUL we must assume the * interpreter path is truncated. */ - if (!next_terminator(cp, buf_end)) + if (!next_terminator(i_end, buf_end)) return -ENOEXEC; - cp = buf_end; + i_end = buf_end; } - /* NUL-terminate the buffer and any trailing spaces/tabs. */ - *cp = '\0'; - while (cp > bprm->buf) { - cp--; - if ((*cp == ' ') || (*cp == '\t')) - *cp = '\0'; - else - break; - } - for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++); - if (*cp == '\0') + /* Trim any trailing spaces/tabs from i_end */ + while (spacetab(i_end[-1])) + i_end--; + + /* Skip over leading spaces/tabs */ + i_name = next_non_spacetab(bprm->buf+2, i_end); + if (!i_name || (i_name == i_end)) return -ENOEXEC; /* No interpreter name found */ - i_name = cp; + + /* Is there an optional argument? */ i_arg = NULL; - for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++) - /* nothing */ ; - while ((*cp == ' ') || (*cp == '\t')) - *cp++ = '\0'; - if (*cp) - i_arg = cp; + i_sep = next_terminator(i_name, i_end); + if (i_sep && (*i_sep != '\0')) + i_arg = next_non_spacetab(i_sep, i_end); + + /* + * If the script filename will be inaccessible after exec, typically + * because it is a "/dev/fd//.." path against an O_CLOEXEC fd, give + * up now (on the assumption that the interpreter will want to load + * this file). + */ + if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE) + return -ENOENT; + + /* Release since we are not mapping a binary into memory. */ + allow_write_access(bprm->file); + fput(bprm->file); + bprm->file = NULL; + /* * OK, we've parsed out the interpreter name and * (optional) argument. @@ -121,7 +115,9 @@ static int load_script(struct linux_binprm *bprm) if (retval < 0) return retval; bprm->argc++; + *((char *)i_end) = '\0'; if (i_arg) { + *((char *)i_sep) = '\0'; retval = copy_strings_kernel(1, &i_arg, bprm); if (retval < 0) return retval; From b8a61c9e7b4a0fec493d191429e9653d66a79ccc Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Thu, 14 May 2020 15:17:40 -0500 Subject: [PATCH 19/24] exec: Generic execfd support Most of the support for passing the file descriptor of an executable to an interpreter already lives in the generic code and in binfmt_elf. Rework the fields in binfmt_elf that deal with executable file descriptor passing to make executable file descriptor passing a first class concept. Move the fd_install from binfmt_misc into begin_new_exec after the new creds have been installed. This means that accessing the file through /proc//fd/N is able to see the creds for the new executable before allowing access to the new executables files. Performing the install of the executables file descriptor after the point of no return also means that nothing special needs to be done on error. The exiting of the process will close all of it's open files. Move the would_dump from binfmt_misc into begin_new_exec right after would_dump is called on the bprm->file. This makes it obvious this case exists and that no nesting of bprm->file is currently supported. In binfmt_misc the movement of fd_install into generic code means that it's special error exit path is no longer needed. Link: https://lkml.kernel.org/r/87y2poyd91.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/binfmt_elf.c | 4 ++-- fs/binfmt_elf_fdpic.c | 4 ++-- fs/binfmt_misc.c | 40 ++++++++-------------------------------- fs/exec.c | 15 +++++++++++++++ include/linux/binfmts.h | 10 +++++----- 5 files changed, 32 insertions(+), 41 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 396d5c2e6b5e..441c85f04dfd 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -273,8 +273,8 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, NEW_AUX_ENT(AT_BASE_PLATFORM, (elf_addr_t)(unsigned long)u_base_platform); } - if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) { - NEW_AUX_ENT(AT_EXECFD, bprm->interp_data); + if (bprm->have_execfd) { + NEW_AUX_ENT(AT_EXECFD, bprm->execfd); } #undef NEW_AUX_ENT /* AT_NULL is zero; clear the rest too */ diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 896e3ca9bf85..2d5e9eb12075 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -628,10 +628,10 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, (elf_addr_t) (unsigned long) u_base_platform); } - if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) { + if (bprm->have_execfd) { nr = 0; csp -= 2 * sizeof(unsigned long); - NEW_AUX_ENT(AT_EXECFD, bprm->interp_data); + NEW_AUX_ENT(AT_EXECFD, bprm->execfd); } nr = 0; diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index 50a73afdf9b7..ad2866f28f0c 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -134,7 +134,6 @@ static int load_misc_binary(struct linux_binprm *bprm) Node *fmt; struct file *interp_file = NULL; int retval; - int fd_binary = -1; retval = -ENOEXEC; if (!enabled) @@ -161,29 +160,12 @@ static int load_misc_binary(struct linux_binprm *bprm) } if (fmt->flags & MISC_FMT_OPEN_BINARY) { - - /* if the binary should be opened on behalf of the - * interpreter than keep it open and assign descriptor - * to it - */ - fd_binary = get_unused_fd_flags(0); - if (fd_binary < 0) { - retval = fd_binary; - goto ret; - } - fd_install(fd_binary, bprm->file); - - /* if the binary is not readable than enforce mm->dumpable=0 - regardless of the interpreter's permissions */ - would_dump(bprm, bprm->file); + /* Pass the open binary to the interpreter */ + bprm->have_execfd = 1; + bprm->executable = bprm->file; allow_write_access(bprm->file); bprm->file = NULL; - - /* mark the bprm that fd should be passed to interp */ - bprm->interp_flags |= BINPRM_FLAGS_EXECFD; - bprm->interp_data = fd_binary; - } else { allow_write_access(bprm->file); fput(bprm->file); @@ -192,19 +174,19 @@ static int load_misc_binary(struct linux_binprm *bprm) /* make argv[1] be the path to the binary */ retval = copy_strings_kernel(1, &bprm->interp, bprm); if (retval < 0) - goto error; + goto ret; bprm->argc++; /* add the interp as argv[0] */ retval = copy_strings_kernel(1, &fmt->interpreter, bprm); if (retval < 0) - goto error; + goto ret; bprm->argc++; /* Update interp in case binfmt_script needs it. */ retval = bprm_change_interp(fmt->interpreter, bprm); if (retval < 0) - goto error; + goto ret; if (fmt->flags & MISC_FMT_OPEN_FILE) { interp_file = file_clone_open(fmt->interp_file); @@ -215,7 +197,7 @@ static int load_misc_binary(struct linux_binprm *bprm) } retval = PTR_ERR(interp_file); if (IS_ERR(interp_file)) - goto error; + goto ret; bprm->file = interp_file; if (fmt->flags & MISC_FMT_CREDENTIALS) @@ -223,17 +205,11 @@ static int load_misc_binary(struct linux_binprm *bprm) retval = search_binary_handler(bprm); if (retval < 0) - goto error; + goto ret; ret: dput(fmt->dentry); return retval; -error: - if (fd_binary > 0) - ksys_close(fd_binary); - bprm->interp_flags = 0; - bprm->interp_data = 0; - goto ret; } /* Command parsers */ diff --git a/fs/exec.c b/fs/exec.c index 5fc458460e44..117ad8fc012b 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1323,7 +1323,10 @@ int begin_new_exec(struct linux_binprm * bprm) */ set_mm_exe_file(bprm->mm, bprm->file); + /* If the binary is not readable then enforce mm->dumpable=0 */ would_dump(bprm, bprm->file); + if (bprm->have_execfd) + would_dump(bprm, bprm->executable); /* * Release all of the old mmap stuff @@ -1427,6 +1430,16 @@ int begin_new_exec(struct linux_binprm * bprm) * credentials; any time after this it may be unlocked. */ security_bprm_committed_creds(bprm); + + /* Pass the opened binary to the interpreter. */ + if (bprm->have_execfd) { + retval = get_unused_fd_flags(0); + if (retval < 0) + goto out_unlock; + fd_install(retval, bprm->executable); + bprm->executable = NULL; + bprm->execfd = retval; + } return 0; out_unlock: @@ -1516,6 +1529,8 @@ static void free_bprm(struct linux_binprm *bprm) allow_write_access(bprm->file); fput(bprm->file); } + if (bprm->executable) + fput(bprm->executable); /* If a binfmt changed the interp, free it. */ if (bprm->interp != bprm->filename) kfree(bprm->interp); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 8c7779d6bf19..653508b25815 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -26,6 +26,9 @@ struct linux_binprm { unsigned long p; /* current top of mem */ unsigned long argmin; /* rlimit marker for copy_strings() */ unsigned int + /* Should an execfd be passed to userspace? */ + have_execfd:1, + /* It is safe to use the creds of a script (see binfmt_misc) */ preserve_creds:1, /* @@ -48,6 +51,7 @@ struct linux_binprm { unsigned int taso:1; #endif unsigned int recursion_depth; /* only for search_binary_handler() */ + struct file * executable; /* Executable to pass to the interpreter */ struct file * file; struct cred *cred; /* new credentials */ int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */ @@ -58,7 +62,7 @@ struct linux_binprm { of the time same as filename, but could be different for binfmt_{misc,script} */ unsigned interp_flags; - unsigned interp_data; + int execfd; /* File descriptor of the executable */ unsigned long loader, exec; struct rlimit rlim_stack; /* Saved RLIMIT_STACK used during exec. */ @@ -69,10 +73,6 @@ struct linux_binprm { #define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0 #define BINPRM_FLAGS_ENFORCE_NONDUMP (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT) -/* fd of the binary should be passed to the interpreter */ -#define BINPRM_FLAGS_EXECFD_BIT 1 -#define BINPRM_FLAGS_EXECFD (1 << BINPRM_FLAGS_EXECFD_BIT) - /* filename of the binary will be inaccessible after exec */ #define BINPRM_FLAGS_PATH_INACCESSIBLE_BIT 2 #define BINPRM_FLAGS_PATH_INACCESSIBLE (1 << BINPRM_FLAGS_PATH_INACCESSIBLE_BIT) From bc2bf338d54b7aadaed49bb45b9e10d4592b2a46 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Mon, 18 May 2020 18:43:20 -0500 Subject: [PATCH 20/24] exec: Remove recursion from search_binary_handler Recursion in kernel code is generally a bad idea as it can overflow the kernel stack. Recursion in exec also hides that the code is looping and that the loop changes bprm->file. Instead of recursing in search_binary_handler have the methods that would recurse set bprm->interpreter and return 0. Modify exec_binprm to loop when bprm->interpreter is set. Consolidate all of the reassignments of bprm->file in that loop to make it clear what is going on. The structure of the new loop in exec_binprm is that all errors return immediately, while successful completion (ret == 0 && !bprm->interpreter) just breaks out of the loop and runs what exec_bprm has always run upon successful completion. Fail if the an interpreter is being call after execfd has been set. The code has never properly handled an interpreter being called with execfd being set and with reassignments of bprm->file and the assignment of bprm->executable in generic code it has finally become possible to test and fail when if this problematic condition happens. With the reassignments of bprm->file and the assignment of bprm->executable moved into the generic code add a test to see if bprm->executable is being reassigned. In search_binary_handler remove the test for !bprm->file. With all reassignments of bprm->file moved to exec_binprm bprm->file can never be NULL in search_binary_handler. Link: https://lkml.kernel.org/r/87sgfwyd84.fsf_-_@x220.int.ebiederm.org Acked-by: Linus Torvalds Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- arch/alpha/kernel/binfmt_loader.c | 8 ++--- fs/binfmt_em86.c | 9 ++---- fs/binfmt_misc.c | 18 ++--------- fs/binfmt_script.c | 9 ++---- fs/exec.c | 51 ++++++++++++++++++++----------- include/linux/binfmts.h | 3 +- 6 files changed, 43 insertions(+), 55 deletions(-) diff --git a/arch/alpha/kernel/binfmt_loader.c b/arch/alpha/kernel/binfmt_loader.c index d712ba51d15a..e4be7a543ecf 100644 --- a/arch/alpha/kernel/binfmt_loader.c +++ b/arch/alpha/kernel/binfmt_loader.c @@ -19,10 +19,6 @@ static int load_binary(struct linux_binprm *bprm) if (bprm->loader) return -ENOEXEC; - allow_write_access(bprm->file); - fput(bprm->file); - bprm->file = NULL; - loader = bprm->vma->vm_end - sizeof(void *); file = open_exec("/sbin/loader"); @@ -33,9 +29,9 @@ static int load_binary(struct linux_binprm *bprm) /* Remember if the application is TASO. */ bprm->taso = eh->ah.entry < 0x100000000UL; - bprm->file = file; + bprm->interpreter = file; bprm->loader = loader; - return search_binary_handler(bprm); + return 0; } static struct linux_binfmt loader_format = { diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c index cedde2341ade..995883693cb2 100644 --- a/fs/binfmt_em86.c +++ b/fs/binfmt_em86.c @@ -48,10 +48,6 @@ static int load_em86(struct linux_binprm *bprm) if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE) return -ENOENT; - allow_write_access(bprm->file); - fput(bprm->file); - bprm->file = NULL; - /* Unlike in the script case, we don't have to do any hairy * parsing to find our interpreter... it's hardcoded! */ @@ -89,9 +85,8 @@ static int load_em86(struct linux_binprm *bprm) if (IS_ERR(file)) return PTR_ERR(file); - bprm->file = file; - - return search_binary_handler(bprm); + bprm->interpreter = file; + return 0; } static struct linux_binfmt em86_format = { diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index ad2866f28f0c..53968ea07b57 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -159,18 +159,9 @@ static int load_misc_binary(struct linux_binprm *bprm) goto ret; } - if (fmt->flags & MISC_FMT_OPEN_BINARY) { - /* Pass the open binary to the interpreter */ + if (fmt->flags & MISC_FMT_OPEN_BINARY) bprm->have_execfd = 1; - bprm->executable = bprm->file; - allow_write_access(bprm->file); - bprm->file = NULL; - } else { - allow_write_access(bprm->file); - fput(bprm->file); - bprm->file = NULL; - } /* make argv[1] be the path to the binary */ retval = copy_strings_kernel(1, &bprm->interp, bprm); if (retval < 0) @@ -199,14 +190,11 @@ static int load_misc_binary(struct linux_binprm *bprm) if (IS_ERR(interp_file)) goto ret; - bprm->file = interp_file; + bprm->interpreter = interp_file; if (fmt->flags & MISC_FMT_CREDENTIALS) bprm->preserve_creds = 1; - retval = search_binary_handler(bprm); - if (retval < 0) - goto ret; - + retval = 0; ret: dput(fmt->dentry); return retval; diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c index 85e0ef86eb11..0e8b953d12cf 100644 --- a/fs/binfmt_script.c +++ b/fs/binfmt_script.c @@ -93,11 +93,6 @@ static int load_script(struct linux_binprm *bprm) if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE) return -ENOENT; - /* Release since we are not mapping a binary into memory. */ - allow_write_access(bprm->file); - fput(bprm->file); - bprm->file = NULL; - /* * OK, we've parsed out the interpreter name and * (optional) argument. @@ -138,8 +133,8 @@ static int load_script(struct linux_binprm *bprm) if (IS_ERR(file)) return PTR_ERR(file); - bprm->file = file; - return search_binary_handler(bprm); + bprm->interpreter = file; + return 0; } static struct linux_binfmt script_format = { diff --git a/fs/exec.c b/fs/exec.c index 117ad8fc012b..c3c879a55d65 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1710,16 +1710,12 @@ EXPORT_SYMBOL(remove_arg_zero); /* * cycle the list of binary formats handler, until one recognizes the image */ -int search_binary_handler(struct linux_binprm *bprm) +static int search_binary_handler(struct linux_binprm *bprm) { bool need_retry = IS_ENABLED(CONFIG_MODULES); struct linux_binfmt *fmt; int retval; - /* This allows 4 levels of binfmt rewrites before failing hard. */ - if (bprm->recursion_depth > 5) - return -ELOOP; - retval = prepare_binprm(bprm); if (retval < 0) return retval; @@ -1736,14 +1732,11 @@ int search_binary_handler(struct linux_binprm *bprm) continue; read_unlock(&binfmt_lock); - bprm->recursion_depth++; retval = fmt->load_binary(bprm); - bprm->recursion_depth--; read_lock(&binfmt_lock); put_binfmt(fmt); - if (bprm->point_of_no_return || !bprm->file || - (retval != -ENOEXEC)) { + if (bprm->point_of_no_return || (retval != -ENOEXEC)) { read_unlock(&binfmt_lock); return retval; } @@ -1762,12 +1755,11 @@ int search_binary_handler(struct linux_binprm *bprm) return retval; } -EXPORT_SYMBOL(search_binary_handler); static int exec_binprm(struct linux_binprm *bprm) { pid_t old_pid, old_vpid; - int ret; + int ret, depth; /* Need to fetch pid before load_binary changes it */ old_pid = current->pid; @@ -1775,15 +1767,38 @@ static int exec_binprm(struct linux_binprm *bprm) old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent)); rcu_read_unlock(); - ret = search_binary_handler(bprm); - if (ret >= 0) { - audit_bprm(bprm); - trace_sched_process_exec(current, old_pid, bprm); - ptrace_event(PTRACE_EVENT_EXEC, old_vpid); - proc_exec_connector(current); + /* This allows 4 levels of binfmt rewrites before failing hard. */ + for (depth = 0;; depth++) { + struct file *exec; + if (depth > 5) + return -ELOOP; + + ret = search_binary_handler(bprm); + if (ret < 0) + return ret; + if (!bprm->interpreter) + break; + + exec = bprm->file; + bprm->file = bprm->interpreter; + bprm->interpreter = NULL; + + allow_write_access(exec); + if (unlikely(bprm->have_execfd)) { + if (bprm->executable) { + fput(exec); + return -ENOEXEC; + } + bprm->executable = exec; + } else + fput(exec); } - return ret; + audit_bprm(bprm); + trace_sched_process_exec(current, old_pid, bprm); + ptrace_event(PTRACE_EVENT_EXEC, old_vpid); + proc_exec_connector(current); + return 0; } /* diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 653508b25815..7fc05929c967 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -50,8 +50,8 @@ struct linux_binprm { #ifdef __alpha__ unsigned int taso:1; #endif - unsigned int recursion_depth; /* only for search_binary_handler() */ struct file * executable; /* Executable to pass to the interpreter */ + struct file * interpreter; struct file * file; struct cred *cred; /* new credentials */ int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */ @@ -117,7 +117,6 @@ static inline void insert_binfmt(struct linux_binfmt *fmt) extern void unregister_binfmt(struct linux_binfmt *); extern int __must_check remove_arg_zero(struct linux_binprm *); -extern int search_binary_handler(struct linux_binprm *); extern int begin_new_exec(struct linux_binprm * bprm); extern void setup_new_exec(struct linux_binprm * bprm); extern void finalize_exec(struct linux_binprm *bprm); From b081320f0693cce0394f7c8bad9fba0b25982186 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 20 May 2020 02:05:56 -0700 Subject: [PATCH 21/24] selftests/exec: Add binfmt_script regression test While working on commit b5372fe5dc84 ("exec: load_script: Do not exec truncated interpreter path"), I wrote a series of test scripts to verify corner cases. However, soon after, commit 6eb3c3d0a52d ("exec: increase BINPRM_BUF_SIZE to 256") landed, resulting in the tests needing to be refactored for the larger BINPRM_BUF_SIZE, which got lost on my TODO list. During the recent exec refactoring work[1], the need for these tests resurfaced, so I've finished them up for addition to the kernel selftests. [1] https://lore.kernel.org/lkml/202005191144.E3112135@keescook/ Link: https://lkml.kernel.org/r/202005200204.D07DF079@keescook Signed-off-by: Kees Cook Signed-off-by: Eric W. Biederman --- tools/testing/selftests/exec/Makefile | 1 + tools/testing/selftests/exec/binfmt_script | 171 +++++++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100755 tools/testing/selftests/exec/binfmt_script diff --git a/tools/testing/selftests/exec/Makefile b/tools/testing/selftests/exec/Makefile index 33339e31e365..7f4527f897c4 100644 --- a/tools/testing/selftests/exec/Makefile +++ b/tools/testing/selftests/exec/Makefile @@ -3,6 +3,7 @@ CFLAGS = -Wall CFLAGS += -Wno-nonnull CFLAGS += -D_GNU_SOURCE +TEST_PROGS := binfmt_script TEST_GEN_PROGS := execveat TEST_GEN_FILES := execveat.symlink execveat.denatured script subdir # Makefile is a run-time dependency, since it's accessed by the execveat test diff --git a/tools/testing/selftests/exec/binfmt_script b/tools/testing/selftests/exec/binfmt_script new file mode 100755 index 000000000000..05f94a741c7a --- /dev/null +++ b/tools/testing/selftests/exec/binfmt_script @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0 +# +# Test that truncation of bprm->buf doesn't cause unexpected execs paths, along +# with various other pathological cases. +import os, subprocess + +# Relevant commits +# +# b5372fe5dc84 ("exec: load_script: Do not exec truncated interpreter path") +# 6eb3c3d0a52d ("exec: increase BINPRM_BUF_SIZE to 256") + +# BINPRM_BUF_SIZE +SIZE=256 + +NAME_MAX=int(subprocess.check_output(["getconf", "NAME_MAX", "."])) + +test_num=0 + +code='''#!/usr/bin/perl +print "Executed interpreter! Args:\n"; +print "0 : '$0'\n"; +$counter = 1; +foreach my $a (@ARGV) { + print "$counter : '$a'\n"; + $counter++; +} +''' + +## +# test - produce a binfmt_script hashbang line for testing +# +# @size: bytes for bprm->buf line, including hashbang but not newline +# @good: whether this script is expected to execute correctly +# @hashbang: the special 2 bytes for running binfmt_script +# @leading: any leading whitespace before the executable path +# @root: start of executable pathname +# @target: end of executable pathname +# @arg: bytes following the executable pathname +# @fill: character to fill between @root and @target to reach @size bytes +# @newline: character to use as newline, not counted towards @size +# ... +def test(name, size, good=True, leading="", root="./", target="/perl", + fill="A", arg="", newline="\n", hashbang="#!"): + global test_num, tests, NAME_MAX + test_num += 1 + if test_num > tests: + raise ValueError("more binfmt_script tests than expected! (want %d, expected %d)" + % (test_num, tests)) + + middle = "" + remaining = size - len(hashbang) - len(leading) - len(root) - len(target) - len(arg) + # The middle of the pathname must not exceed NAME_MAX + while remaining >= NAME_MAX: + middle += fill * (NAME_MAX - 1) + middle += '/' + remaining -= NAME_MAX + middle += fill * remaining + + dirpath = root + middle + binary = dirpath + target + if len(target): + os.makedirs(dirpath, mode=0o755, exist_ok=True) + open(binary, "w").write(code) + os.chmod(binary, 0o755) + + buf=hashbang + leading + root + middle + target + arg + newline + if len(newline) > 0: + buf += 'echo this is not really perl\n' + + script = "binfmt_script-%s" % (name) + open(script, "w").write(buf) + os.chmod(script, 0o755) + + proc = subprocess.Popen(["./%s" % (script)], shell=True, + stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + stdout = proc.communicate()[0] + + if proc.returncode == 0 and b'Executed interpreter' in stdout: + if good: + print("ok %d - binfmt_script %s (successful good exec)" + % (test_num, name)) + else: + print("not ok %d - binfmt_script %s succeeded when it should have failed" + % (test_num, name)) + else: + if good: + print("not ok %d - binfmt_script %s failed when it should have succeeded (rc:%d)" + % (test_num, name, proc.returncode)) + else: + print("ok %d - binfmt_script %s (correctly failed bad exec)" + % (test_num, name)) + + # Clean up crazy binaries + os.unlink(script) + if len(target): + elements = binary.split('/') + os.unlink(binary) + elements.pop() + while len(elements) > 1: + os.rmdir("/".join(elements)) + elements.pop() + +tests=27 +print("TAP version 1.3") +print("1..%d" % (tests)) + +### FAIL (8 tests) + +# Entire path is well past the BINFMT_BUF_SIZE. +test(name="too-big", size=SIZE+80, good=False) +# Path is right at max size, making it impossible to tell if it was truncated. +test(name="exact", size=SIZE, good=False) +# Same as above, but with leading whitespace. +test(name="exact-space", size=SIZE, good=False, leading=" ") +# Huge buffer of only whitespace. +test(name="whitespace-too-big", size=SIZE+71, good=False, root="", + fill=" ", target="") +# A good path, but it gets truncated due to leading whitespace. +test(name="truncated", size=SIZE+17, good=False, leading=" " * 19) +# Entirely empty except for #! +test(name="empty", size=2, good=False, root="", + fill="", target="", newline="") +# Within size, but entirely spaces +test(name="spaces", size=SIZE-1, good=False, root="", fill=" ", + target="", newline="") +# Newline before binary. +test(name="newline-prefix", size=SIZE-1, good=False, leading="\n", + root="", fill=" ", target="") + +### ok (19 tests) + +# The original test case that was broken by commit: +# 8099b047ecc4 ("exec: load_script: don't blindly truncate shebang string") +test(name="test.pl", size=439, leading=" ", + root="./nix/store/bwav8kz8b3y471wjsybgzw84mrh4js9-perl-5.28.1/bin", + arg=" -I/nix/store/x6yyav38jgr924nkna62q3pkp0dgmzlx-perl5.28.1-File-Slurp-9999.25/lib/perl5/site_perl -I/nix/store/ha8v67sl8dac92r9z07vzr4gv1y9nwqz-perl5.28.1-Net-DBus-1.1.0/lib/perl5/site_perl -I/nix/store/dcrkvnjmwh69ljsvpbdjjdnqgwx90a9d-perl5.28.1-XML-Parser-2.44/lib/perl5/site_perl -I/nix/store/rmji88k2zz7h4zg97385bygcydrf2q8h-perl5.28.1-XML-Twig-3.52/lib/perl5/site_perl") +# One byte under size, leaving newline visible. +test(name="one-under", size=SIZE-1) +# Two bytes under size, leaving newline visible. +test(name="two-under", size=SIZE-2) +# Exact size, but trailing whitespace visible instead of newline +test(name="exact-trunc-whitespace", size=SIZE, arg=" ") +# Exact size, but trailing space and first arg char visible instead of newline. +test(name="exact-trunc-arg", size=SIZE, arg=" f") +# One bute under, with confirmed non-truncated arg since newline now visible. +test(name="one-under-full-arg", size=SIZE-1, arg=" f") +# Short read buffer by one byte. +test(name="one-under-no-nl", size=SIZE-1, newline="") +# Short read buffer by half buffer size. +test(name="half-under-no-nl", size=int(SIZE/2), newline="") +# One byte under with whitespace arg. leaving wenline visible. +test(name="one-under-trunc-arg", size=SIZE-1, arg=" ") +# One byte under with whitespace leading. leaving wenline visible. +test(name="one-under-leading", size=SIZE-1, leading=" ") +# One byte under with whitespace leading and as arg. leaving newline visible. +test(name="one-under-leading-trunc-arg", size=SIZE-1, leading=" ", arg=" ") +# Same as above, but with 2 bytes under +test(name="two-under-no-nl", size=SIZE-2, newline="") +test(name="two-under-trunc-arg", size=SIZE-2, arg=" ") +test(name="two-under-leading", size=SIZE-2, leading=" ") +test(name="two-under-leading-trunc-arg", size=SIZE-2, leading=" ", arg=" ") +# Same as above, but with buffer half filled +test(name="two-under-no-nl", size=int(SIZE/2), newline="") +test(name="two-under-trunc-arg", size=int(SIZE/2), arg=" ") +test(name="two-under-leading", size=int(SIZE/2), leading=" ") +test(name="two-under-lead-trunc-arg", size=int(SIZE/2), leading=" ", arg=" ") + +if test_num != tests: + raise ValueError("fewer binfmt_script tests than expected! (ran %d, expected %d" + % (test_num, tests)) From 011593480d2931392bc167f7a20c82a6c34114fd Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Wed, 27 May 2020 15:49:01 +0200 Subject: [PATCH 22/24] binfmt_elf_fdpic: fix execfd build regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The change to bprm->have_execfd was incomplete, leading to a build failure: fs/binfmt_elf_fdpic.c: In function 'create_elf_fdpic_tables': fs/binfmt_elf_fdpic.c:591:27: error: 'BINPRM_FLAGS_EXECFD' undeclared Change the last user of BINPRM_FLAGS_EXECFD in a corresponding way. Reported-by: Valdis KlÄ“tnieks Fixes: b8a61c9e7b4a ("exec: Generic execfd support") Signed-off-by: Arnd Bergmann Signed-off-by: Eric W. Biederman --- fs/binfmt_elf_fdpic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 2d5e9eb12075..89da6c85fe4a 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -588,7 +588,7 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, nitems = 1 + DLINFO_ITEMS + (k_platform ? 1 : 0) + (k_base_platform ? 1 : 0) + AT_VECTOR_SIZE_ARCH; - if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) + if (bprm->have_execfd) nitems++; csp = sp; From a7868323c2638a7c6c5b30b37831b73cbdf0dc15 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Fri, 29 May 2020 08:24:10 -0500 Subject: [PATCH 23/24] exec: Add a per bprm->file version of per_clear There is a small bug in the code that recomputes parts of bprm->cred for every bprm->file. The code never recomputes the part of clear_dangerous_personality_flags it is responsible for. Which means that in practice if someone creates a sgid script the interpreter will not be able to use any of: READ_IMPLIES_EXEC ADDR_NO_RANDOMIZE ADDR_COMPAT_LAYOUT MMAP_PAGE_ZERO. This accentially clearing of personality flags probably does not matter in practice because no one has complained but it does make the code more difficult to understand. Further remaining bug compatible prevents the recomputation from being removed and replaced by simply computing bprm->cred once from the final bprm->file. Making this change removes the last behavior difference between computing bprm->creds from the final file and recomputing bprm->cred several times. Which allows this behavior change to be justified for it's own reasons, and for any but hunts looking into why the behavior changed to wind up here instead of in the code that will follow that computes bprm->cred from the final bprm->file. This small logic bug appears to have existed since the code started clearing dangerous personality bits. History Tree: git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git Fixes: 1bb0fa189c6a ("[PATCH] NX: clean up legacy binary support") Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/exec.c | 6 ++++-- include/linux/binfmts.h | 5 +++++ include/linux/lsm_hooks.h | 2 ++ security/commoncap.c | 2 +- 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index c3c879a55d65..0f793536e393 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1354,6 +1354,7 @@ int begin_new_exec(struct linux_binprm * bprm) me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_NO_SETAFFINITY); flush_thread(); + bprm->per_clear |= bprm->pf_per_clear; me->personality &= ~bprm->per_clear; /* @@ -1628,12 +1629,12 @@ static void bprm_fill_uid(struct linux_binprm *bprm) return; if (mode & S_ISUID) { - bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->pf_per_clear |= PER_CLEAR_ON_SETID; bprm->cred->euid = uid; } if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { - bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->pf_per_clear |= PER_CLEAR_ON_SETID; bprm->cred->egid = gid; } } @@ -1654,6 +1655,7 @@ static int prepare_binprm(struct linux_binprm *bprm) /* Recompute parts of bprm->cred based on bprm->file */ bprm->active_secureexec = 0; + bprm->pf_per_clear = 0; bprm_fill_uid(bprm); retval = security_bprm_repopulate_creds(bprm); if (retval) diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 7fc05929c967..50025ead0b72 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -55,6 +55,11 @@ struct linux_binprm { struct file * file; struct cred *cred; /* new credentials */ int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */ + /* + * bits to clear in current->personality + * recalculated for each bprm->file. + */ + unsigned int pf_per_clear; unsigned int per_clear; /* bits to clear in current->personality */ int argc, envc; const char * filename; /* Name of binary as seen by procps */ diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index d618ecc4d660..f68076d440f3 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -55,6 +55,8 @@ * transitions between security domains). * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to * request libc enable secure mode. + * The hook must add to @bprm->pf_per_clear any personality flags that + * should be cleared from current->personality. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. * @bprm_check_security: diff --git a/security/commoncap.c b/security/commoncap.c index 77b04cb6feac..6de72d22dc6c 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -826,7 +826,7 @@ int cap_bprm_repopulate_creds(struct linux_binprm *bprm) /* if we have fs caps, clear dangerous personality flags */ if (__cap_gained(permitted, new, old)) - bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->pf_per_clear |= PER_CLEAR_ON_SETID; /* Don't let someone trace a set[ug]id/setpcap binary with the revised * credentials unless they have the appropriate permit. From 56305aa9b6fab91a5555a45796b79c1b0a6353d1 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Fri, 29 May 2020 22:00:54 -0500 Subject: [PATCH 24/24] exec: Compute file based creds only once Move the computation of creds from prepare_binfmt into begin_new_exec so that the creds need only be computed once. This is just code reorganization no semantic changes of any kind are made. Moving the computation is safe. I have looked through the kernel and verified none of the binfmts look at bprm->cred directly, and that there are no helpers that look at bprm->cred indirectly. Which means that it is not a problem to compute the bprm->cred later in the execution flow as it is not used until it becomes current->cred. A new function bprm_creds_from_file is added to contain the work that needs to be done. bprm_creds_from_file first computes which file bprm->executable or most likely bprm->file that the bprm->creds will be computed from. The funciton bprm_fill_uid is updated to receive the file instead of accessing bprm->file. The now unnecessary work needed to reset the bprm->cred->euid, and bprm->cred->egid is removed from brpm_fill_uid. A small comment to document that bprm_fill_uid now only deals with the work to handle suid and sgid files. The default case is already heandled by prepare_exec_creds. The function security_bprm_repopulate_creds is renamed security_bprm_creds_from_file and now is explicitly passed the file from which to compute the creds. The documentation of the bprm_creds_from_file security hook is updated to explain when the hook is called and what it needs to do. The file is passed from cap_bprm_creds_from_file into get_file_caps so that the caps are computed for the appropriate file. The now unnecessary work in cap_bprm_creds_from_file to reset the ambient capabilites has been removed. A small comment to document that the work of cap_bprm_creds_from_file is to read capabilities from the files secureity attribute and derive capabilities from the fact the user had uid 0 has been added. Reviewed-by: Kees Cook Signed-off-by: "Eric W. Biederman" --- fs/binfmt_misc.c | 2 +- fs/exec.c | 63 +++++++++++++++-------------------- include/linux/binfmts.h | 14 ++------ include/linux/lsm_hook_defs.h | 2 +- include/linux/lsm_hooks.h | 22 ++++++------ include/linux/security.h | 9 ++--- security/commoncap.c | 24 +++++++------ security/security.c | 4 +-- 8 files changed, 61 insertions(+), 79 deletions(-) diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c index 53968ea07b57..bc5506619b7e 100644 --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -192,7 +192,7 @@ static int load_misc_binary(struct linux_binprm *bprm) bprm->interpreter = interp_file; if (fmt->flags & MISC_FMT_CREDENTIALS) - bprm->preserve_creds = 1; + bprm->execfd_creds = 1; retval = 0; ret: diff --git a/fs/exec.c b/fs/exec.c index 0f793536e393..e8599236290d 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -72,6 +72,8 @@ #include +static int bprm_creds_from_file(struct linux_binprm *bprm); + int suid_dumpable = 0; static LIST_HEAD(formats); @@ -1304,6 +1306,11 @@ int begin_new_exec(struct linux_binprm * bprm) struct task_struct *me = current; int retval; + /* Once we are committed compute the creds */ + retval = bprm_creds_from_file(bprm); + if (retval) + return retval; + /* * Ensure all future errors are fatal. */ @@ -1354,7 +1361,6 @@ int begin_new_exec(struct linux_binprm * bprm) me->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE | PF_NO_SETAFFINITY); flush_thread(); - bprm->per_clear |= bprm->pf_per_clear; me->personality &= ~bprm->per_clear; /* @@ -1365,13 +1371,6 @@ int begin_new_exec(struct linux_binprm * bprm) */ do_close_on_exec(me->files); - /* - * Once here, prepare_binrpm() will not be called any more, so - * the final state of setuid/setgid/fscaps can be merged into the - * secureexec flag. - */ - bprm->secureexec |= bprm->active_secureexec; - if (bprm->secureexec) { /* Make sure parent cannot signal privileged process. */ me->pdeath_signal = 0; @@ -1587,29 +1586,21 @@ static void check_unsafe_exec(struct linux_binprm *bprm) spin_unlock(&p->fs->lock); } -static void bprm_fill_uid(struct linux_binprm *bprm) +static void bprm_fill_uid(struct linux_binprm *bprm, struct file *file) { + /* Handle suid and sgid on files */ struct inode *inode; unsigned int mode; kuid_t uid; kgid_t gid; - /* - * Since this can be called multiple times (via prepare_binprm), - * we must clear any previous work done when setting set[ug]id - * bits from any earlier bprm->file uses (for example when run - * first for a setuid script then again for its interpreter). - */ - bprm->cred->euid = current_euid(); - bprm->cred->egid = current_egid(); - - if (!mnt_may_suid(bprm->file->f_path.mnt)) + if (!mnt_may_suid(file->f_path.mnt)) return; if (task_no_new_privs(current)) return; - inode = bprm->file->f_path.dentry->d_inode; + inode = file->f_path.dentry->d_inode; mode = READ_ONCE(inode->i_mode); if (!(mode & (S_ISUID|S_ISGID))) return; @@ -1629,19 +1620,31 @@ static void bprm_fill_uid(struct linux_binprm *bprm) return; if (mode & S_ISUID) { - bprm->pf_per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear |= PER_CLEAR_ON_SETID; bprm->cred->euid = uid; } if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { - bprm->pf_per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear |= PER_CLEAR_ON_SETID; bprm->cred->egid = gid; } } +/* + * Compute brpm->cred based upon the final binary. + */ +static int bprm_creds_from_file(struct linux_binprm *bprm) +{ + /* Compute creds based on which file? */ + struct file *file = bprm->execfd_creds ? bprm->executable : bprm->file; + + bprm_fill_uid(bprm, file); + return security_bprm_creds_from_file(bprm, file); +} + /* * Fill the binprm structure from the inode. - * Check permissions, then read the first BINPRM_BUF_SIZE bytes + * Read the first BINPRM_BUF_SIZE bytes * * This may be called multiple times for binary chains (scripts for example). */ @@ -1649,20 +1652,6 @@ static int prepare_binprm(struct linux_binprm *bprm) { loff_t pos = 0; - /* Can the interpreter get to the executable without races? */ - if (!bprm->preserve_creds) { - int retval; - - /* Recompute parts of bprm->cred based on bprm->file */ - bprm->active_secureexec = 0; - bprm->pf_per_clear = 0; - bprm_fill_uid(bprm); - retval = security_bprm_repopulate_creds(bprm); - if (retval) - return retval; - } - bprm->preserve_creds = 0; - memset(bprm->buf, 0, BINPRM_BUF_SIZE); return kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE, &pos); } diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 50025ead0b72..aece1b340e7d 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -29,13 +29,8 @@ struct linux_binprm { /* Should an execfd be passed to userspace? */ have_execfd:1, - /* It is safe to use the creds of a script (see binfmt_misc) */ - preserve_creds:1, - /* - * True if most recent call to security_bprm_set_creds - * resulted in elevated privileges. - */ - active_secureexec:1, + /* Use the creds of a script (see binfmt_misc) */ + execfd_creds:1, /* * Set by bprm_creds_for_exec hook to indicate a * privilege-gaining exec has happened. Used to set @@ -55,11 +50,6 @@ struct linux_binprm { struct file * file; struct cred *cred; /* new credentials */ int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */ - /* - * bits to clear in current->personality - * recalculated for each bprm->file. - */ - unsigned int pf_per_clear; unsigned int per_clear; /* bits to clear in current->personality */ int argc, envc; const char * filename; /* Name of binary as seen by procps */ diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 1e295ba12c0d..adbc6603abba 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -50,7 +50,7 @@ LSM_HOOK(int, 0, settime, const struct timespec64 *ts, const struct timezone *tz) LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages) LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm) -LSM_HOOK(int, 0, bprm_repopulate_creds, struct linux_binprm *bprm) +LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, struct file *file) LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committing_creds, struct linux_binprm *bprm) LSM_HOOK(void, LSM_RET_VOID, bprm_committed_creds, struct linux_binprm *bprm) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index f68076d440f3..c523c18efa0e 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -44,18 +44,18 @@ * request libc enable secure mode. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. - * @bprm_repopulate_creds: - * Assuming that the relevant bits of @bprm->cred->security have been - * previously set, examine @bprm->file and regenerate them. This is - * so that the credentials derived from the interpreter the code is - * actually going to run are used rather than credentials derived - * from a script. This done because the interpreter binary needs to - * reopen script, and may end up opening something completely different. - * This hook may also optionally check permissions (e.g. for - * transitions between security domains). - * The hook must set @bprm->active_secureexec to 1 if AT_SECURE should be set to + * @bprm_creds_from_file: + * If @file is setpcap, suid, sgid or otherwise marked to change + * privilege upon exec, update @bprm->cred to reflect that change. + * This is called after finding the binary that will be executed. + * without an interpreter. This ensures that the credentials will not + * be derived from a script that the binary will need to reopen, which + * when reopend may end up being a completely different file. This + * hook may also optionally check permissions (e.g. for transitions + * between security domains). + * The hook must set @bprm->secureexec to 1 if AT_SECURE should be set to * request libc enable secure mode. - * The hook must add to @bprm->pf_per_clear any personality flags that + * The hook must add to @bprm->per_clear any personality flags that * should be cleared from current->personality. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. diff --git a/include/linux/security.h b/include/linux/security.h index 6dcec9375e8f..8444fae7c5b9 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -140,7 +140,7 @@ extern int cap_capset(struct cred *new, const struct cred *old, const kernel_cap_t *effective, const kernel_cap_t *inheritable, const kernel_cap_t *permitted); -extern int cap_bprm_repopulate_creds(struct linux_binprm *bprm); +extern int cap_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file); extern int cap_inode_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size, int flags); extern int cap_inode_removexattr(struct dentry *dentry, const char *name); @@ -277,7 +277,7 @@ int security_syslog(int type); int security_settime64(const struct timespec64 *ts, const struct timezone *tz); int security_vm_enough_memory_mm(struct mm_struct *mm, long pages); int security_bprm_creds_for_exec(struct linux_binprm *bprm); -int security_bprm_repopulate_creds(struct linux_binprm *bprm); +int security_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file); int security_bprm_check(struct linux_binprm *bprm); void security_bprm_committing_creds(struct linux_binprm *bprm); void security_bprm_committed_creds(struct linux_binprm *bprm); @@ -575,9 +575,10 @@ static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm) return 0; } -static inline int security_bprm_repopulate_creds(struct linux_binprm *bprm) +static inline int security_bprm_creds_from_file(struct linux_binprm *bprm, + struct file *file) { - return cap_bprm_repopulate_creds(bprm); + return cap_bprm_creds_from_file(bprm, file); } static inline int security_bprm_check(struct linux_binprm *bprm) diff --git a/security/commoncap.c b/security/commoncap.c index 6de72d22dc6c..59bf3c1674c8 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -647,7 +647,8 @@ int get_vfs_caps_from_disk(const struct dentry *dentry, struct cpu_vfs_cap_data * its xattrs and, if present, apply them to the proposed credentials being * constructed by execve(). */ -static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_fcap) +static int get_file_caps(struct linux_binprm *bprm, struct file *file, + bool *effective, bool *has_fcap) { int rc = 0; struct cpu_vfs_cap_data vcaps; @@ -657,7 +658,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f if (!file_caps_enabled) return 0; - if (!mnt_may_suid(bprm->file->f_path.mnt)) + if (!mnt_may_suid(file->f_path.mnt)) return 0; /* @@ -665,10 +666,10 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_f * explicit that capability bits are limited to s_user_ns and its * descendants. */ - if (!current_in_userns(bprm->file->f_path.mnt->mnt_sb->s_user_ns)) + if (!current_in_userns(file->f_path.mnt->mnt_sb->s_user_ns)) return 0; - rc = get_vfs_caps_from_disk(bprm->file->f_path.dentry, &vcaps); + rc = get_vfs_caps_from_disk(file->f_path.dentry, &vcaps); if (rc < 0) { if (rc == -EINVAL) printk(KERN_NOTICE "Invalid argument reading file caps for %s\n", @@ -797,26 +798,27 @@ static inline bool nonroot_raised_pE(struct cred *new, const struct cred *old, } /** - * cap_bprm_repopulate_creds - Set up the proposed credentials for execve(). + * cap_bprm_creds_from_file - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds + * @file: The file to pull the credentials from * * Set up the proposed credentials for a new execution context being * constructed by execve(). The proposed creds in @bprm->cred is altered, * which won't take effect immediately. Returns 0 if successful, -ve on error. */ -int cap_bprm_repopulate_creds(struct linux_binprm *bprm) +int cap_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file) { + /* Process setpcap binaries and capabilities for uid 0 */ const struct cred *old = current_cred(); struct cred *new = bprm->cred; bool effective = false, has_fcap = false, is_setid; int ret; kuid_t root_uid; - new->cap_ambient = old->cap_ambient; if (WARN_ON(!cap_ambient_invariant_ok(old))) return -EPERM; - ret = get_file_caps(bprm, &effective, &has_fcap); + ret = get_file_caps(bprm, file, &effective, &has_fcap); if (ret < 0) return ret; @@ -826,7 +828,7 @@ int cap_bprm_repopulate_creds(struct linux_binprm *bprm) /* if we have fs caps, clear dangerous personality flags */ if (__cap_gained(permitted, new, old)) - bprm->pf_per_clear |= PER_CLEAR_ON_SETID; + bprm->per_clear |= PER_CLEAR_ON_SETID; /* Don't let someone trace a set[ug]id/setpcap binary with the revised * credentials unless they have the appropriate permit. @@ -889,7 +891,7 @@ int cap_bprm_repopulate_creds(struct linux_binprm *bprm) (!__is_real(root_uid, new) && (effective || __cap_grew(permitted, ambient, new)))) - bprm->active_secureexec = 1; + bprm->secureexec = 1; return 0; } @@ -1346,7 +1348,7 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(ptrace_traceme, cap_ptrace_traceme), LSM_HOOK_INIT(capget, cap_capget), LSM_HOOK_INIT(capset, cap_capset), - LSM_HOOK_INIT(bprm_repopulate_creds, cap_bprm_repopulate_creds), + LSM_HOOK_INIT(bprm_creds_from_file, cap_bprm_creds_from_file), LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv), LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv), LSM_HOOK_INIT(inode_getsecurity, cap_inode_getsecurity), diff --git a/security/security.c b/security/security.c index b890b7e2a765..259b8e750aa2 100644 --- a/security/security.c +++ b/security/security.c @@ -828,9 +828,9 @@ int security_bprm_creds_for_exec(struct linux_binprm *bprm) return call_int_hook(bprm_creds_for_exec, 0, bprm); } -int security_bprm_repopulate_creds(struct linux_binprm *bprm) +int security_bprm_creds_from_file(struct linux_binprm *bprm, struct file *file) { - return call_int_hook(bprm_repopulate_creds, 0, bprm); + return call_int_hook(bprm_creds_from_file, 0, bprm, file); } int security_bprm_check(struct linux_binprm *bprm)