From ddb4a1442def2a78b91a85b4251fb712ef23662b Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:23 -0700 Subject: exec: Rename bprm->cred_prepared to called_set_creds The cred_prepared bprm flag has a misleading name. It has nothing to do with the bprm_prepare_cred hook, and actually tracks if bprm_set_creds has been called. Rename this flag and improve its comment. Cc: David Howells Cc: Stephen Smalley Cc: Casey Schaufler Signed-off-by: Kees Cook Acked-by: John Johansen Acked-by: James Morris Acked-by: Paul Moore Acked-by: Serge Hallyn --- fs/binfmt_flat.c | 2 +- fs/exec.c | 2 +- include/linux/binfmts.h | 8 ++++++-- security/apparmor/domain.c | 2 +- security/selinux/hooks.c | 2 +- security/smack/smack_lsm.c | 2 +- security/tomoyo/tomoyo.c | 2 +- 7 files changed, 12 insertions(+), 8 deletions(-) diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index a1e6860b6f46..604a176df0c2 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -890,7 +890,7 @@ static int load_flat_shared_library(int id, struct lib_info *libs) * as we're past the point of no return and are dealing with shared * libraries. */ - bprm.cred_prepared = 1; + bprm.called_set_creds = 1; res = prepare_binprm(&bprm); diff --git a/fs/exec.c b/fs/exec.c index 62175cbcc801..a0fff86269e4 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1548,7 +1548,7 @@ int prepare_binprm(struct linux_binprm *bprm) retval = security_bprm_set_creds(bprm); if (retval) return retval; - bprm->cred_prepared = 1; + bprm->called_set_creds = 1; memset(bprm->buf, 0, BINPRM_BUF_SIZE); return kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE); diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 3ae9013eeaaa..9023e1d2d5cd 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -25,8 +25,12 @@ struct linux_binprm { struct mm_struct *mm; unsigned long p; /* current top of mem */ unsigned int - cred_prepared:1,/* true if creds already prepared (multiple - * preps happen for interpreters) */ + /* + * 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, cap_effective:1;/* true if has elevated effective capabilities, * false if not; except for init which inherits * its parent's caps anyway */ diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index d0594446ae3f..67ec52cfc523 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -758,7 +758,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) file_inode(bprm->file)->i_mode }; - if (bprm->cred_prepared) + if (bprm->called_set_creds) return 0; ctx = cred_ctx(bprm->cred); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 33fd061305c4..1db40195d178 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2356,7 +2356,7 @@ 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->cred_prepared) + if (bprm->called_set_creds) return 0; old_tsec = current_security(); diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 463af86812c7..db8e16ec223b 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -917,7 +917,7 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm) struct superblock_smack *sbsp; int rc; - if (bprm->cred_prepared) + if (bprm->called_set_creds) return 0; isp = inode->i_security; diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 130b4fa4f65f..d25b705360e0 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -76,7 +76,7 @@ static int tomoyo_bprm_set_creds(struct linux_binprm *bprm) * Do only if this function is called for the first time of an execve * operation. */ - if (bprm->cred_prepared) + if (bprm->called_set_creds) return 0; #ifndef CONFIG_SECURITY_TOMOYO_OMIT_USERSPACE_LOADER /* -- cgit v1.2.3 From a9208e42ba99bfe63bdf5f76aaf0193ad3805f02 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:30 -0700 Subject: exec: Correct comments about "point of no return" In commit 221af7f87b97 ("Split 'flush_old_exec' into two functions"), the comment about the point of no return should have stayed in flush_old_exec() since it refers to "bprm->mm = NULL;" line, but prior changes in commits c89681ed7d0e ("remove steal_locks()"), and fd8328be874f ("sanitize handling of shared descriptor tables in failing execve()") made it look like it meant the current->sas_ss_sp line instead. The comment was referring to the fact that once bprm->mm is NULL, all failures from a binfmt load_binary hook (e.g. load_elf_binary), will get SEGV raised against current. Move this comment and expand the explanation a bit, putting it above the assignment this time, and add details about the true nature of "point of no return" being the call to flush_old_exec() itself. This also removes an erroneous commet about when credentials are being installed. That has its own dedicated function, install_exec_creds(), which carries a similar (and correct) comment, so remove the bogus comment where installation is not actually happening. Cc: David Howells Cc: Eric W. Biederman Signed-off-by: Kees Cook Acked-by: "Eric W. Biederman" Acked-by: Serge Hallyn --- fs/exec.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index a0fff86269e4..26b98072be50 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1259,6 +1259,12 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec) perf_event_comm(tsk, 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). + */ int flush_old_exec(struct linux_binprm * bprm) { int retval; @@ -1286,7 +1292,13 @@ int flush_old_exec(struct linux_binprm * bprm) if (retval) goto out; - bprm->mm = NULL; /* We're using it now */ + /* + * After clearing bprm->mm (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. + */ + bprm->mm = NULL; set_fs(USER_DS); current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | @@ -1333,7 +1345,6 @@ void setup_new_exec(struct linux_binprm * bprm) { arch_pick_mmap_layout(current->mm); - /* This is the point of no return */ current->sas_ss_sp = current->sas_ss_size = 0; if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid())) @@ -1351,7 +1362,6 @@ void setup_new_exec(struct linux_binprm * bprm) */ current->mm->task_size = TASK_SIZE; - /* install the new credentials */ if (!uid_eq(bprm->cred->uid, current_euid()) || !gid_eq(bprm->cred->gid, current_egid())) { current->pdeath_signal = 0; -- cgit v1.2.3 From c425e189ffd7720c881fe9ccd7143cea577f6d03 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:22 -0700 Subject: binfmt: Introduce secureexec flag The bprm_secureexec hook can be moved earlier. Right now, it is called during create_elf_tables(), via load_binary(), via search_binary_handler(), via exec_binprm(). Nearly all (see exception below) state used by bprm_secureexec is created during the bprm_set_creds hook, called from prepare_binprm(). For all LSMs (except commoncaps described next), only the first execution of bprm_set_creds takes any effect (they all check bprm->called_set_creds which prepare_binprm() sets after the first call to the bprm_set_creds hook). However, all these LSMs also only do anything with bprm_secureexec when they detected a secure state during their first run of bprm_set_creds. Therefore, it is functionally identical to move the detection into bprm_set_creds, since the results from secureexec here only need to be based on the first call to the LSM's bprm_set_creds hook. The single exception is that the commoncaps secureexec hook also examines euid/uid and egid/gid differences which are controlled by bprm_fill_uid(), via prepare_binprm(), which can be called multiple times (e.g. binfmt_script, binfmt_misc), and may clear the euid/egid for the final load (i.e. the script interpreter). However, while commoncaps specifically ignores bprm->cred_prepared, and runs its bprm_set_creds hook each time prepare_binprm() may get called, it needs to base the secureexec decision on the final call to bprm_set_creds. As a result, it will need special handling. To begin this refactoring, this adds the secureexec flag to the bprm struct, and calls the secureexec hook during setup_new_exec(). This is safe since all the cred work is finished (and past the point of no return). This explicit call will be removed in later patches once the hook has been removed. Cc: David Howells Signed-off-by: Kees Cook Reviewed-by: John Johansen Acked-by: Serge Hallyn Reviewed-by: James Morris --- fs/binfmt_elf.c | 2 +- fs/binfmt_elf_fdpic.c | 2 +- fs/exec.c | 2 ++ include/linux/binfmts.h | 8 +++++++- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 879ff9c7ffd0..3b7dda91b07b 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -252,7 +252,7 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec, NEW_AUX_ENT(AT_EUID, from_kuid_munged(cred->user_ns, cred->euid)); NEW_AUX_ENT(AT_GID, from_kgid_munged(cred->user_ns, cred->gid)); NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid)); - NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm)); + NEW_AUX_ENT(AT_SECURE, bprm->secureexec); NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes); #ifdef ELF_HWCAP2 NEW_AUX_ENT(AT_HWCAP2, ELF_HWCAP2); diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index cf93a4fad012..5aa9199dfb13 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -650,7 +650,7 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, NEW_AUX_ENT(AT_EUID, (elf_addr_t) from_kuid_munged(cred->user_ns, cred->euid)); NEW_AUX_ENT(AT_GID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->gid)); NEW_AUX_ENT(AT_EGID, (elf_addr_t) from_kgid_munged(cred->user_ns, cred->egid)); - NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm)); + NEW_AUX_ENT(AT_SECURE, bprm->secureexec); NEW_AUX_ENT(AT_EXECFN, bprm->exec); #ifdef ARCH_DLINFO diff --git a/fs/exec.c b/fs/exec.c index 26b98072be50..0f361115c88f 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1343,6 +1343,8 @@ EXPORT_SYMBOL(would_dump); void setup_new_exec(struct linux_binprm * bprm) { + bprm->secureexec |= security_bprm_secureexec(bprm); + arch_pick_mmap_layout(current->mm); current->sas_ss_sp = current->sas_ss_size = 0; diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 9023e1d2d5cd..16838ba7ee75 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -31,9 +31,15 @@ struct linux_binprm { * binfmt_script/misc). */ called_set_creds:1, - cap_effective:1;/* true if has elevated effective capabilities, + cap_effective:1,/* true if has elevated effective capabilities, * false if not; except for init which inherits * its parent's caps anyway */ + /* + * 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. + */ + secureexec:1; #ifdef __alpha__ unsigned int taso:1; #endif -- cgit v1.2.3 From 993b3ab0642e57da5de6bef11dd50db7e2fc3b7e Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:24 -0700 Subject: apparmor: Refactor to remove bprm_secureexec hook The AppArmor bprm_secureexec hook can be merged with the bprm_set_creds hook since it's dealing with the same information, and all of the details are finalized during the first call to the bprm_set_creds hook via prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored via bprm->called_set_creds). Here, all the comments describe how secureexec is actually calculated during bprm_set_creds, so this actually does it, drops the bprm flag that was being used internally by AppArmor, and drops the bprm_secureexec hook. Signed-off-by: Kees Cook Acked-by: John Johansen Reviewed-by: James Morris Acked-by: Serge Hallyn --- security/apparmor/domain.c | 19 +------------------ security/apparmor/include/domain.h | 1 - security/apparmor/include/file.h | 3 --- security/apparmor/lsm.c | 1 - 4 files changed, 1 insertion(+), 23 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 67ec52cfc523..17a601c67b62 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -807,7 +807,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) aa_label_printk(new, GFP_ATOMIC); dbg_printk("\n"); } - bprm->unsafe |= AA_SECURE_X_NEEDED; + bprm->secureexec = 1; } if (label->proxy != new->proxy) { @@ -843,23 +843,6 @@ audit: goto done; } -/** - * apparmor_bprm_secureexec - determine if secureexec is needed - * @bprm: binprm for exec (NOT NULL) - * - * Returns: %1 if secureexec is needed else %0 - */ -int apparmor_bprm_secureexec(struct linux_binprm *bprm) -{ - /* the decision to use secure exec is computed in set_creds - * and stored in bprm->unsafe. - */ - if (bprm->unsafe & AA_SECURE_X_NEEDED) - return 1; - - return 0; -} - /* * Functions for self directed profile change */ diff --git a/security/apparmor/include/domain.h b/security/apparmor/include/domain.h index bab5810b6e9a..24c5976d6143 100644 --- a/security/apparmor/include/domain.h +++ b/security/apparmor/include/domain.h @@ -30,7 +30,6 @@ struct aa_domain { #define AA_CHANGE_STACK 8 int apparmor_bprm_set_creds(struct linux_binprm *bprm); -int apparmor_bprm_secureexec(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/include/file.h b/security/apparmor/include/file.h index 001e40073ff9..4c2c8ac8842f 100644 --- a/security/apparmor/include/file.h +++ b/security/apparmor/include/file.h @@ -101,9 +101,6 @@ static inline struct aa_label *aa_get_file_label(struct aa_file_ctx *ctx) #define AA_X_INHERIT 0x4000 #define AA_X_UNCONFINED 0x8000 -/* AA_SECURE_X_NEEDED - is passed in the bprm->unsafe field */ -#define AA_SECURE_X_NEEDED 0x8000 - /* need to make conditional which ones are being set */ struct path_cond { kuid_t uid; diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 867bcd154c7e..7a82c0f61452 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -694,7 +694,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(bprm_set_creds, apparmor_bprm_set_creds), LSM_HOOK_INIT(bprm_committing_creds, apparmor_bprm_committing_creds), LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds), - LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec), LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit), }; -- cgit v1.2.3 From 62874c3adf709b884ceb0c61c35ab3794b3b0e95 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:25 -0700 Subject: selinux: Refactor to remove bprm_secureexec hook The SELinux bprm_secureexec hook can be merged with the bprm_set_creds hook since it's dealing with the same information, and all of the details are finalized during the first call to the bprm_set_creds hook via prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored via bprm->called_set_creds). Here, the test can just happen at the end of the bprm_set_creds hook, and the bprm_secureexec hook can be dropped. Cc: Stephen Smalley Signed-off-by: Kees Cook Acked-by: Paul Moore Tested-by: Paul Moore Acked-by: Serge Hallyn Reviewed-by: James Morris Reviewed-by: Andy Lutomirski --- security/selinux/hooks.c | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 1db40195d178..a1f5f5ddfba7 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2442,30 +2442,17 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) /* Clear any possibly unsafe personality bits on exec: */ bprm->per_clear |= PER_CLEAR_ON_SETID; - } - - return 0; -} - -static int selinux_bprm_secureexec(struct linux_binprm *bprm) -{ - const struct task_security_struct *tsec = current_security(); - u32 sid, osid; - int atsecure = 0; - - sid = tsec->sid; - osid = tsec->osid; - if (osid != sid) { /* Enable secure mode for SIDs transitions unless the noatsecure permission is granted between the two SIDs, i.e. ahp returns 0. */ - atsecure = avc_has_perm(osid, sid, - SECCLASS_PROCESS, - PROCESS__NOATSECURE, NULL); + rc = avc_has_perm(old_tsec->sid, new_tsec->sid, + SECCLASS_PROCESS, PROCESS__NOATSECURE, + NULL); + bprm->secureexec |= !!rc; } - return !!atsecure; + return 0; } static int match_file(const void *p, struct file *file, unsigned fd) @@ -6266,7 +6253,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(bprm_set_creds, selinux_bprm_set_creds), LSM_HOOK_INIT(bprm_committing_creds, selinux_bprm_committing_creds), LSM_HOOK_INIT(bprm_committed_creds, selinux_bprm_committed_creds), - LSM_HOOK_INIT(bprm_secureexec, selinux_bprm_secureexec), LSM_HOOK_INIT(sb_alloc_security, selinux_sb_alloc_security), LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security), -- cgit v1.2.3 From ccbb6e1065fa9cd27f2bf406e8c5d5cf0273f554 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:26 -0700 Subject: smack: Refactor to remove bprm_secureexec hook The Smack bprm_secureexec hook can be merged with the bprm_set_creds hook since it's dealing with the same information, and all of the details are finalized during the first call to the bprm_set_creds hook via prepare_binprm() (subsequent calls due to binfmt_script, etc, are ignored via bprm->called_set_creds). Here, the test can just happen at the end of the bprm_set_creds hook, and the bprm_secureexec hook can be dropped. Signed-off-by: Kees Cook Acked-by: Serge Hallyn Reviewed-by: James Morris Reviewed-by: Casey Schaufler --- security/smack/smack_lsm.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index db8e16ec223b..791d028207b9 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -950,6 +950,10 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm) bsp->smk_task = isp->smk_task; bprm->per_clear |= PER_CLEAR_ON_SETID; + /* Decide if this is a secure exec. */ + if (bsp->smk_task != bsp->smk_forked) + bprm->secureexec = 1; + return 0; } @@ -967,22 +971,6 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm) current->pdeath_signal = 0; } -/** - * smack_bprm_secureexec - Return the decision to use secureexec. - * @bprm: binprm for exec - * - * Returns 0 on success. - */ -static int smack_bprm_secureexec(struct linux_binprm *bprm) -{ - struct task_smack *tsp = current_security(); - - if (tsp->smk_task != tsp->smk_forked) - return 1; - - return 0; -} - /* * Inode hooks */ @@ -4646,7 +4634,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds), LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds), - LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec), LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security), LSM_HOOK_INIT(inode_free_security, smack_inode_free_security), -- cgit v1.2.3 From 46d98eb4e1d2bc225f661879e0e157a952107598 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:27 -0700 Subject: commoncap: Refactor to remove bprm_secureexec hook The commoncap implementation of the bprm_secureexec hook is the only LSM that depends on the final call to its bprm_set_creds hook (since it may be called for multiple files, it ignores bprm->called_set_creds). As a result, it cannot safely _clear_ bprm->secureexec since other LSMs may have set it. Instead, remove the bprm_secureexec hook by introducing a new flag to bprm specific to commoncap: cap_elevated. This is similar to cap_effective, but that is used for a specific subset of elevated privileges, and exists solely to track state from bprm_set_creds to bprm_secureexec. As such, it will be removed in the next patch. Here, set the new bprm->cap_elevated flag when setuid/setgid has happened from bprm_fill_uid() or fscapabilities have been prepared. This temporarily moves the bprm_secureexec hook to a static inline. The helper will be removed in the next patch; this makes the step easier to review and bisect, since this does not introduce any changes to inputs nor outputs to the "elevated privileges" calculation. The new flag is merged with the bprm->secureexec flag in setup_new_exec() since this marks the end of any further prepare_binprm() calls. Cc: Andy Lutomirski Signed-off-by: Kees Cook Reviewed-by: Andy Lutomirski Acked-by: James Morris Acked-by: Serge Hallyn --- fs/exec.c | 7 +++++++ include/linux/binfmts.h | 7 +++++++ include/linux/security.h | 3 +-- security/commoncap.c | 12 ++++++++---- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 0f361115c88f..1536bc4502cc 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1345,6 +1345,13 @@ void setup_new_exec(struct linux_binprm * bprm) { bprm->secureexec |= security_bprm_secureexec(bprm); + /* + * 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; + arch_pick_mmap_layout(current->mm); current->sas_ss_sp = current->sas_ss_size = 0; diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 16838ba7ee75..213c61fa3780 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -34,6 +34,13 @@ struct linux_binprm { cap_effective:1,/* true if has elevated effective capabilities, * false if not; except for init which inherits * its parent's caps anyway */ + /* + * 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. + */ + cap_elevated:1, /* * Set by bprm_set_creds hook to indicate a privilege-gaining * exec has happened. Used to sanitize execution environment diff --git a/include/linux/security.h b/include/linux/security.h index b6ea1dc9cc9d..f89832ccdf55 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -85,7 +85,6 @@ extern int cap_capset(struct cred *new, const struct cred *old, const kernel_cap_t *inheritable, const kernel_cap_t *permitted); extern int cap_bprm_set_creds(struct linux_binprm *bprm); -extern int cap_bprm_secureexec(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); @@ -543,7 +542,7 @@ static inline void security_bprm_committed_creds(struct linux_binprm *bprm) static inline int security_bprm_secureexec(struct linux_binprm *bprm) { - return cap_bprm_secureexec(bprm); + return 0; } static inline int security_sb_alloc(struct super_block *sb) diff --git a/security/commoncap.c b/security/commoncap.c index 7abebd782d5e..abb6050c8083 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -481,6 +481,8 @@ out: return rc; } +static int is_secureexec(struct linux_binprm *bprm); + /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -614,11 +616,14 @@ skip: if (WARN_ON(!cap_ambient_invariant_ok(new))) return -EPERM; + /* Check for privilege-elevated exec. */ + bprm->cap_elevated = is_secureexec(bprm); + return 0; } /** - * cap_bprm_secureexec - Determine whether a secure execution is required + * is_secureexec - Determine whether a secure execution is required * @bprm: The execution parameters * * Determine whether a secure execution is required, return 1 if it is, and 0 @@ -627,9 +632,9 @@ skip: * The credentials have been committed by this point, and so are no longer * available through @bprm->cred. */ -int cap_bprm_secureexec(struct linux_binprm *bprm) +static int is_secureexec(struct linux_binprm *bprm) { - const struct cred *cred = current_cred(); + const struct cred *cred = bprm->cred; kuid_t root_uid = make_kuid(cred->user_ns, 0); if (!uid_eq(cred->uid, root_uid)) { @@ -1079,7 +1084,6 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = { 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_secureexec, cap_bprm_secureexec), LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv), LSM_HOOK_INIT(inode_killpriv, cap_inode_killpriv), LSM_HOOK_INIT(mmap_addr, cap_mmap_addr), -- cgit v1.2.3 From ee67ae7ef6ff499137292ac8a9dfe86096796283 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:28 -0700 Subject: commoncap: Move cap_elevated calculation into bprm_set_creds Instead of a separate function, open-code the cap_elevated test, which lets us entirely remove bprm->cap_effective (to use the local "effective" variable instead), and more accurately examine euid/egid changes via the existing local "is_setid". The following LTP tests were run to validate the changes: # ./runltp -f syscalls -s cap # ./runltp -f securebits # ./runltp -f cap_bounds # ./runltp -f filecaps All kernel selftests for capabilities and exec continue to pass as well. Signed-off-by: Kees Cook Reviewed-by: James Morris Acked-by: Serge Hallyn Reviewed-by: Andy Lutomirski --- include/linux/binfmts.h | 3 --- security/commoncap.c | 52 ++++++++++--------------------------------------- 2 files changed, 10 insertions(+), 45 deletions(-) diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h index 213c61fa3780..fb44d6180ca0 100644 --- a/include/linux/binfmts.h +++ b/include/linux/binfmts.h @@ -31,9 +31,6 @@ struct linux_binprm { * binfmt_script/misc). */ called_set_creds:1, - cap_effective:1,/* true if has elevated effective capabilities, - * false if not; except for init which inherits - * its parent's caps anyway */ /* * True if most recent call to the commoncaps bprm_set_creds * hook (due to multiple prepare_binprm() calls from the diff --git a/security/commoncap.c b/security/commoncap.c index abb6050c8083..d8e26fb9781d 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -285,15 +285,6 @@ int cap_capset(struct cred *new, return 0; } -/* - * Clear proposed capability sets for execve(). - */ -static inline void bprm_clear_caps(struct linux_binprm *bprm) -{ - cap_clear(bprm->cred->cap_permitted); - bprm->cap_effective = false; -} - /** * cap_inode_need_killpriv - Determine if inode change affects privileges * @dentry: The inode/dentry in being changed with change marked ATTR_KILL_PRIV @@ -443,7 +434,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c int rc = 0; struct cpu_vfs_cap_data vcaps; - bprm_clear_caps(bprm); + cap_clear(bprm->cred->cap_permitted); if (!file_caps_enabled) return 0; @@ -476,13 +467,11 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c out: if (rc) - bprm_clear_caps(bprm); + cap_clear(bprm->cred->cap_permitted); return rc; } -static int is_secureexec(struct linux_binprm *bprm); - /** * cap_bprm_set_creds - Set up the proposed credentials for execve(). * @bprm: The execution parameters, including the proposed creds @@ -587,8 +576,6 @@ skip: if (WARN_ON(!cap_ambient_invariant_ok(new))) return -EPERM; - bprm->cap_effective = effective; - /* * Audit candidate if current->cap_effective is set * @@ -617,35 +604,16 @@ skip: return -EPERM; /* Check for privilege-elevated exec. */ - bprm->cap_elevated = is_secureexec(bprm); - - return 0; -} - -/** - * is_secureexec - Determine whether a secure execution is required - * @bprm: The execution parameters - * - * Determine whether a secure execution is required, return 1 if it is, and 0 - * if it is not. - * - * The credentials have been committed by this point, and so are no longer - * available through @bprm->cred. - */ -static int is_secureexec(struct linux_binprm *bprm) -{ - const struct cred *cred = bprm->cred; - kuid_t root_uid = make_kuid(cred->user_ns, 0); - - if (!uid_eq(cred->uid, root_uid)) { - if (bprm->cap_effective) - return 1; - if (!cap_issubset(cred->cap_permitted, cred->cap_ambient)) - return 1; + bprm->cap_elevated = 0; + if (is_setid) { + bprm->cap_elevated = 1; + } else if (!uid_eq(new->uid, root_uid)) { + if (effective || + !cap_issubset(new->cap_permitted, new->cap_ambient)) + bprm->cap_elevated = 1; } - return (!uid_eq(cred->euid, cred->uid) || - !gid_eq(cred->egid, cred->gid)); + return 0; } /** -- cgit v1.2.3 From 2af622802696e1dbe28d81c8ea6355dc30800396 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:29 -0700 Subject: LSM: drop bprm_secureexec hook This removes the bprm_secureexec hook since the logic has been folded into the bprm_set_creds hook for all LSMs now. Cc: Eric W. Biederman Signed-off-by: Kees Cook Reviewed-by: John Johansen Acked-by: James Morris Acked-by: Serge Hallyn --- fs/exec.c | 2 -- include/linux/lsm_hooks.h | 14 +++++--------- include/linux/security.h | 6 ------ security/security.c | 5 ----- 4 files changed, 5 insertions(+), 22 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 1536bc4502cc..eca0cb550a06 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1343,8 +1343,6 @@ EXPORT_SYMBOL(would_dump); void setup_new_exec(struct linux_binprm * bprm) { - bprm->secureexec |= security_bprm_secureexec(bprm); - /* * Once here, prepare_binrpm() will not be called any more, so * the final state of setuid/setgid/fscaps can be merged into the diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 3a90febadbe2..d1c7bef25691 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -43,7 +43,11 @@ * 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. + * 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. * @bprm contains the linux_binprm structure. * Return 0 if the hook is successful and permission is granted. * @bprm_check_security: @@ -71,12 +75,6 @@ * linux_binprm structure. This hook is a good place to perform state * changes on the process such as clearing out non-inheritable signal * state. This is called immediately after commit_creds(). - * @bprm_secureexec: - * Return a boolean value (0 or 1) indicating whether a "secure exec" - * is required. The flag is passed in the auxiliary table - * on the initial stack to the ELF interpreter to indicate whether libc - * should enable secure mode. - * @bprm contains the linux_binprm structure. * * Security hooks for filesystem operations. * @@ -1388,7 +1386,6 @@ union security_list_options { int (*bprm_set_creds)(struct linux_binprm *bprm); int (*bprm_check_security)(struct linux_binprm *bprm); - int (*bprm_secureexec)(struct linux_binprm *bprm); void (*bprm_committing_creds)(struct linux_binprm *bprm); void (*bprm_committed_creds)(struct linux_binprm *bprm); @@ -1710,7 +1707,6 @@ struct security_hook_heads { struct list_head vm_enough_memory; struct list_head bprm_set_creds; struct list_head bprm_check_security; - struct list_head bprm_secureexec; struct list_head bprm_committing_creds; struct list_head bprm_committed_creds; struct list_head sb_alloc_security; diff --git a/include/linux/security.h b/include/linux/security.h index f89832ccdf55..974bb9b0996c 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -231,7 +231,6 @@ 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); void security_bprm_committed_creds(struct linux_binprm *bprm); -int security_bprm_secureexec(struct linux_binprm *bprm); int security_sb_alloc(struct super_block *sb); void security_sb_free(struct super_block *sb); int security_sb_copy_data(char *orig, char *copy); @@ -540,11 +539,6 @@ static inline void security_bprm_committed_creds(struct linux_binprm *bprm) { } -static inline int security_bprm_secureexec(struct linux_binprm *bprm) -{ - return 0; -} - static inline int security_sb_alloc(struct super_block *sb) { return 0; diff --git a/security/security.c b/security/security.c index 30132378d103..afc34f46c6c5 100644 --- a/security/security.c +++ b/security/security.c @@ -351,11 +351,6 @@ void security_bprm_committed_creds(struct linux_binprm *bprm) call_void_hook(bprm_committed_creds, bprm); } -int security_bprm_secureexec(struct linux_binprm *bprm) -{ - return call_int_hook(bprm_secureexec, 0, bprm); -} - int security_sb_alloc(struct super_block *sb) { return call_int_hook(sb_alloc_security, 0, sb); -- cgit v1.2.3 From e37fdb785a5f95ecadf43b773c97f676500ac7b8 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:31 -0700 Subject: exec: Use secureexec for setting dumpability The examination of "current" to decide dumpability is wrong. This was a check of and euid/uid (or egid/gid) mismatch in the existing process, not the newly created one. This appears to stretch back into even the "history.git" tree. Luckily, dumpability is later set in commit_creds(). In earlier kernel versions before creds existed, similar checks also existed late in the exec flow, covering up the mistake as far back as I could find. Note that because the commit_creds() check examines differences of euid, uid, egid, gid, and capabilities between the old and new creds, it would look like the setup_new_exec() dumpability test could be entirely removed. However, the secureexec test may cover a different set of tests (specific to the LSMs) than what commit_creds() checks for. So, fix this test to use secureexec (the removed euid tests are redundant to the commoncap secureexec checks now). Cc: David Howells Signed-off-by: Kees Cook Acked-by: Serge Hallyn Reviewed-by: James Morris --- fs/exec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/exec.c b/fs/exec.c index eca0cb550a06..3536437ffd76 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1354,7 +1354,7 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size = 0; - if (uid_eq(current_euid(), current_uid()) && gid_eq(current_egid(), current_gid())) + if (!bprm->secureexec) set_dumpable(current->mm, SUID_DUMP_USER); else set_dumpable(current->mm, suid_dumpable); -- cgit v1.2.3 From a70423dfbc58402cc2573f95b7e842024aff7162 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:32 -0700 Subject: exec: Use secureexec for clearing pdeath_signal Like dumpability, clearing pdeath_signal happens both in setup_new_exec() and later in commit_creds(). The test in setup_new_exec() is different from all other privilege comparisons, though: it is checking the new cred (bprm) uid vs the old cred (current) euid. This appears to be a bug, introduced by commit a6f76f23d297 ("CRED: Make execve() take advantage of copy-on-write credentials"): - if (bprm->e_uid != current_euid() || - bprm->e_gid != current_egid()) { - set_dumpable(current->mm, suid_dumpable); + if (bprm->cred->uid != current_euid() || + bprm->cred->gid != current_egid()) { It was bprm euid vs current euid (and egids), but the effective got dropped. Nothing in the exec flow changes bprm->cred->uid (nor gid). The call traces are: prepare_bprm_creds() prepare_exec_creds() prepare_creds() memcpy(new_creds, old_creds, ...) security_prepare_creds() (unimplemented by commoncap) ... prepare_binprm() bprm_fill_uid() resets euid/egid to current euid/egid sets euid/egid on bprm based on set*id file bits security_bprm_set_creds() cap_bprm_set_creds() handle all caps-based manipulations so this test is effectively a test of current_uid() vs current_euid(), which is wrong, just like the prior dumpability tests were wrong. The commit log says "Clear pdeath_signal and set dumpable on certain circumstances that may not be covered by commit_creds()." This may be meaning the earlier old euid vs new euid (and egid) test that got changed. Luckily, as with dumpability, this is all masked by commit_creds() which performs old/new euid and egid tests and clears pdeath_signal. And again, like dumpability, we should include LSM secureexec logic for pdeath_signal clearing. For example, Smack goes out of its way to clear pdeath_signal when it finds a secureexec condition. Cc: David Howells Signed-off-by: Kees Cook Acked-by: Serge Hallyn Reviewed-by: James Morris --- fs/exec.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 3536437ffd76..7a9288551d62 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1369,8 +1369,7 @@ void setup_new_exec(struct linux_binprm * bprm) */ current->mm->task_size = TASK_SIZE; - if (!uid_eq(bprm->cred->uid, current_euid()) || - !gid_eq(bprm->cred->gid, current_egid())) { + if (bprm->secureexec) { current->pdeath_signal = 0; } else { if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) -- cgit v1.2.3 From 35b372b76f7153142cd4838ef1e1e094d115f46f Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:33 -0700 Subject: smack: Remove redundant pdeath_signal clearing This removes the redundant pdeath_signal clearing in Smack: the check in smack_bprm_committing_creds() matches the check in smack_bprm_set_creds() (which used to be in the now-removed smack_bprm_securexec() hook) and since secureexec is now being checked for clearing pdeath_signal, this is redundant to the common exec code. Signed-off-by: Kees Cook Acked-by: Serge Hallyn Reviewed-by: James Morris Reviewed-by: Casey Schaufler --- security/smack/smack_lsm.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 791d028207b9..319add31b4a4 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -957,20 +957,6 @@ static int smack_bprm_set_creds(struct linux_binprm *bprm) return 0; } -/** - * smack_bprm_committing_creds - Prepare to install the new credentials - * from bprm. - * - * @bprm: binprm for exec - */ -static void smack_bprm_committing_creds(struct linux_binprm *bprm) -{ - struct task_smack *bsp = bprm->cred->security; - - if (bsp->smk_task != bsp->smk_forked) - current->pdeath_signal = 0; -} - /* * Inode hooks */ @@ -4633,7 +4619,6 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(sb_parse_opts_str, smack_parse_opts_str), LSM_HOOK_INIT(bprm_set_creds, smack_bprm_set_creds), - LSM_HOOK_INIT(bprm_committing_creds, smack_bprm_committing_creds), LSM_HOOK_INIT(inode_alloc_security, smack_inode_alloc_security), LSM_HOOK_INIT(inode_free_security, smack_inode_free_security), -- cgit v1.2.3 From 473d89639db0aaa0799616b397584ba4f58cd8e1 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:34 -0700 Subject: exec: Consolidate dumpability logic Since it's already valid to set dumpability in the early part of setup_new_exec(), we can consolidate the logic into a single place. The BINPRM_FLAGS_ENFORCE_NONDUMP is set during would_dump() calls before setup_new_exec(), so its test is safe to move as well. Signed-off-by: Kees Cook Acked-by: Serge Hallyn Reviewed-by: James Morris --- fs/exec.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 7a9288551d62..3006c1c24304 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1354,10 +1354,12 @@ void setup_new_exec(struct linux_binprm * bprm) current->sas_ss_sp = current->sas_ss_size = 0; - if (!bprm->secureexec) - set_dumpable(current->mm, SUID_DUMP_USER); - else + /* Figure out dumpability. */ + if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP || + bprm->secureexec) set_dumpable(current->mm, suid_dumpable); + else + set_dumpable(current->mm, SUID_DUMP_USER); arch_setup_new_exec(); perf_event_exec(); @@ -1371,9 +1373,6 @@ void setup_new_exec(struct linux_binprm * bprm) if (bprm->secureexec) { current->pdeath_signal = 0; - } else { - if (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP) - set_dumpable(current->mm, suid_dumpable); } /* An exec changes our domain. We are no longer part of the thread -- cgit v1.2.3 From 64701dee4178eb4a771b8b36cd86560f5b0e2460 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:35 -0700 Subject: exec: Use sane stack rlimit under secureexec For a secureexec, before memory layout selection has happened, reset the stack rlimit to something sane to avoid the caller having control over the resulting layouts. $ ulimit -s 8192 $ ulimit -s unlimited $ /bin/sh -c 'ulimit -s' unlimited $ sudo /bin/sh -c 'ulimit -s' 8192 Cc: Linus Torvalds Signed-off-by: Kees Cook Reviewed-by: James Morris Acked-by: Serge Hallyn --- fs/exec.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/fs/exec.c b/fs/exec.c index 3006c1c24304..3235cbd85efa 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1350,6 +1350,18 @@ void setup_new_exec(struct linux_binprm * bprm) */ bprm->secureexec |= bprm->cap_elevated; + if (bprm->secureexec) { + /* + * 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 (current->signal->rlim[RLIMIT_STACK].rlim_cur > _STK_LIM) + current->signal->rlim[RLIMIT_STACK].rlim_cur = _STK_LIM; + } + arch_pick_mmap_layout(current->mm); current->sas_ss_sp = current->sas_ss_size = 0; -- cgit v1.2.3 From fe8993b3a05cbba6318a54e0f85901aaea6fc244 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 18 Jul 2017 15:25:36 -0700 Subject: exec: Consolidate pdeath_signal clearing Instead of an additional secureexec check for pdeath_signal, just move it up into the initial secureexec test. Neither perf nor arch code touches pdeath_signal, so the relocation shouldn't change anything. Signed-off-by: Kees Cook Acked-by: Serge Hallyn --- fs/exec.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fs/exec.c b/fs/exec.c index 3235cbd85efa..01a9fb9d8ac3 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1351,6 +1351,9 @@ void setup_new_exec(struct linux_binprm * bprm) bprm->secureexec |= bprm->cap_elevated; if (bprm->secureexec) { + /* Make sure parent cannot signal privileged process. */ + current->pdeath_signal = 0; + /* * For secureexec, reset the stack limit to sane default to * avoid bad behavior from the prior rlimits. This has to @@ -1383,10 +1386,6 @@ void setup_new_exec(struct linux_binprm * bprm) */ current->mm->task_size = TASK_SIZE; - if (bprm->secureexec) { - current->pdeath_signal = 0; - } - /* An exec changes our domain. We are no longer part of the thread group */ current->self_exec_id++; -- cgit v1.2.3