From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753266AbdGJJFJ (ORCPT ); Mon, 10 Jul 2017 05:05:09 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:37527 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750812AbdGJJFF (ORCPT ); Mon, 10 Jul 2017 05:05:05 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Kees Cook Cc: Linus Torvalds , Andy Lutomirski , David Howells , Serge Hallyn , John Johansen , Casey Schaufler , Alexander Viro , Michal Hocko , Ben Hutchings , Hugh Dickins , Oleg Nesterov , "Jason A. Donenfeld" , Rik van Riel , James Morris , Greg Ungerer , Ingo Molnar , Nicolas Pitre , Stephen Smalley , Paul Moore , Vivek Goyal , =?utf-8?Q?Micka=C3=ABl_Sala=C3=BCn?= , Tetsuo Handa , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, selinux@tycho.nsa.gov References: <1499673451-66160-1-git-send-email-keescook@chromium.org> <1499673451-66160-3-git-send-email-keescook@chromium.org> Date: Mon, 10 Jul 2017 03:57:07 -0500 In-Reply-To: <1499673451-66160-3-git-send-email-keescook@chromium.org> (Kees Cook's message of "Mon, 10 Jul 2017 00:57:25 -0700") Message-ID: <87a84cr7oc.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1dUUck-0008NF-2W;;;mid=<87a84cr7oc.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.213.87;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19ayQeztZbHLVBre9P4hZDLL66f6jk/0jU= X-SA-Exim-Connect-IP: 67.3.213.87 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.7 XMSubLong Long Subject * 0.0 TVD_RCVD_IP Message was received from an IP address * 1.2 LotsOfNums_01 BODY: Lots of long strings of numbers * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa04 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_01 4+ unique symbols in subject * 0.0 T_TooManySym_03 6+ unique symbols in subject * 0.0 T_TooManySym_02 5+ unique symbols in subject X-Spam-DCC: XMission; sa04 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: *;Kees Cook X-Spam-Relay-Country: X-Spam-Timing: total 5315 ms - load_scoreonly_sql: 0.05 (0.0%), signal_user_changed: 3.7 (0.1%), b_tie_ro: 2.5 (0.0%), parse: 1.82 (0.0%), extract_message_metadata: 22 (0.4%), get_uri_detail_list: 6 (0.1%), tests_pri_-1000: 10 (0.2%), tests_pri_-950: 1.73 (0.0%), tests_pri_-900: 1.45 (0.0%), tests_pri_-400: 49 (0.9%), check_bayes: 47 (0.9%), b_tokenize: 22 (0.4%), b_tok_get_all: 13 (0.2%), b_comp_prob: 4.0 (0.1%), b_tok_touch_all: 4.9 (0.1%), b_finish: 0.71 (0.0%), tests_pri_0: 408 (7.7%), check_dkim_signature: 0.71 (0.0%), check_dkim_adsp: 3.0 (0.1%), tests_pri_500: 4812 (90.5%), poll_dns_idle: 4806 (90.4%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH v2 2/8] exec: Move security_bprm_secureexec() earlier X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Kees Cook writes: > There are several places where exec needs to know if a privilege-gain has > happened. These should be using the results of security_bprm_secureexec() > but it is getting (needlessly) called very late. It is hard to tell at a glance but I believe this introduces a regression. cap_bprm_set_creds is currently called before cap_bprm_secureexec and it has a number of cases such as no_new_privs and ptrace that can result in some of the precomputed credential changes not happening. Without accounting for that I believe your cap_bprm_securexec now returns a postive value too early. > Instead, move this earlier in the exec code, to the start of the point > of no return in setup_new_exec(). Here, the new creds have already > been calculated (and stored in bprm->cred), which is normally what > security_bprm_secureexec() wants to examine. Since it's moved earlier, > LSMs hooking bprm_secureexec() need to be adjusted to use the creds in > bprm: > > $ git grep LSM_HOOK_INIT.*bprm_secureexec > apparmor/lsm.c: LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec), > commoncap.c: LSM_HOOK_INIT(bprm_secureexec, cap_bprm_secureexec), > selinux/hooks.c: LSM_HOOK_INIT(bprm_secureexec, selinux_bprm_secureexec), > smack/smack_lsm.c: LSM_HOOK_INIT(bprm_secureexec, smack_bprm_secureexec), > > AppArmor does not access creds in apparmor_bprm_secureexec. > > Capabilities needed to be adjusted to use bprm creds. > > SELinux needed to be adjusted to use bprm creds for the security structure. > > Smack needed to be adjusted to use bprm creds for the security structure. > > The result of the bprm_secureexec() hook is saved in a new bprm field > "secureexec" so it can be queried later (just AT_SECURE currently). > > Signed-off-by: Kees Cook > --- > fs/binfmt_elf.c | 2 +- > fs/binfmt_elf_fdpic.c | 2 +- > fs/exec.c | 5 +++++ > include/linux/binfmts.h | 3 ++- > include/linux/lsm_hooks.h | 3 ++- > security/commoncap.c | 4 +--- > security/selinux/hooks.c | 2 +- > security/smack/smack_lsm.c | 2 +- > 8 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c > index 5075fd5c62c8..7f6ec4dac13d 100644 > --- a/fs/binfmt_elf.c > +++ b/fs/binfmt_elf.c > @@ -254,7 +254,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 7842ae661e34..b92e37fb53aa 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1337,6 +1337,11 @@ EXPORT_SYMBOL(would_dump); > > void setup_new_exec(struct linux_binprm * bprm) > { > + if (security_bprm_secureexec(bprm)) { > + /* Record for AT_SECURE. */ > + bprm->secureexec = 1; > + } > + > 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 05488da3aee9..1afaa303cad0 100644 > --- a/include/linux/binfmts.h > +++ b/include/linux/binfmts.h > @@ -27,9 +27,10 @@ struct linux_binprm { > unsigned int > cred_prepared:1,/* true if creds already prepared (multiple > * preps happen for interpreters) */ > - 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 */ > + secureexec:1; /* true when gaining privileges */ > #ifdef __alpha__ > unsigned int taso:1; > #endif > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 080f34e66017..d1bd24fb4a33 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -72,7 +72,8 @@ > * 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. > + * should enable secure mode. Called before bprm_committing_creds(), > + * so pending credentials are in @bprm->cred. > * @bprm contains the linux_binprm structure. > * > * Security hooks for filesystem operations. > diff --git a/security/commoncap.c b/security/commoncap.c > index 7abebd782d5e..482d3aac2fc6 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -624,12 +624,10 @@ int cap_bprm_set_creds(struct linux_binprm *bprm) > * 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. > */ > int cap_bprm_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)) { > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 819fd6858b49..9381c8474cf4 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2420,7 +2420,7 @@ static int selinux_bprm_set_creds(struct linux_binprm *bprm) > > static int selinux_bprm_secureexec(struct linux_binprm *bprm) > { > - const struct task_security_struct *tsec = current_security(); > + const struct task_security_struct *tsec = bprm->cred->security; > u32 sid, osid; > int atsecure = 0; > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 658f5d8c7e76..13cf9e66d5fe 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -975,7 +975,7 @@ static void smack_bprm_committing_creds(struct linux_binprm *bprm) > */ > static int smack_bprm_secureexec(struct linux_binprm *bprm) > { > - struct task_smack *tsp = current_security(); > + struct task_smack *tsp = bprm->cred->security; > > if (tsp->smk_task != tsp->smk_forked) > return 1;