Return-Path: Received: from localhost (bix [127.0.0.1]) by localhost.localdomain (8.12.10/8.12.10) with ESMTP id i3N0iWvL021299 for ; Thu, 22 Apr 2004 17:44:32 -0700 Received: from bix [127.0.0.1] by localhost with POP3 (fetchmail-6.2.0) for akpm@localhost (single-drop); Thu, 22 Apr 2004 17:44:32 -0700 (PDT) Received: from build.pdx.osdl.net (build.pdx.osdl.net [172.20.1.2]) by mail.osdl.org (8.11.6/8.11.6) with ESMTP id i3N0fA208106; Thu, 22 Apr 2004 17:41:10 -0700 Received: (from chrisw@localhost) by build.pdx.osdl.net (8.11.6/8.11.6) id i3N0f9e08249; Thu, 22 Apr 2004 17:41:09 -0700 Date: Thu, 22 Apr 2004 17:41:09 -0700 From: Chris Wright To: Andrew Morton Cc: Stephen Smalley , chrisw@osdl.org, luto@myrealbox.com, jmorris@redhat.com Subject: Re: creds_fix_take2.patch Message-ID: <20040422174109.F21045@build.pdx.osdl.net> References: <20040422013221.37f77875.akpm@osdl.org> <1082634891.10587.20.camel@moss-spartans.epoch.ncsc.mil> <1082638997.10587.58.camel@moss-spartans.epoch.ncsc.mil> <20040422131306.38f1bbec.akpm@osdl.org> <20040422131557.B21045@build.pdx.osdl.net> <20040422132834.15700a68.akpm@osdl.org> <1082667982.10587.322.camel@moss-spartans.epoch.ncsc.mil> <1082668484.10587.338.camel@moss-spartans.epoch.ncsc.mil> <20040422143057.0ed1d6b9.akpm@osdl.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <20040422143057.0ed1d6b9.akpm@osdl.org>; from akpm@osdl.org on Thu, Apr 22, 2004 at 02:30:57PM -0700 X-Spam-Status: No, hits=-4.9 required=1.0 tests=BAYES_00 autolearn=ham version=2.60 X-Spam-Checker-Version: SpamAssassin 2.60 (1.212-2003-09-23-exp) on bix X-Spam-Level: * Andrew Morton (akpm@osdl.org) wrote: > Stephen Smalley wrote: > > > > On Thu, 2004-04-22 at 17:06, Stephen Smalley wrote: > > > Untested patch below relative to Andy's patch should eliminate the > > > nested locking introduced by his patch for SELinux. > > > > Ok, compiles, boots, and doesn't deadlock upon exercising those > > permission checks. > > Thanks. Could you or Andy please prepare a fresh patch against > Linus's tree? Here's an updated patch against Linus's tree. Updates include: - Stephen's patch to drop task_lock as needed - s/must_must_not_trace_exec/unsafe_exec/ - move unsafe_exec() call above security_bprm_apply_creds() call rather than in call for readability. - fix dummy hook to honor the case where root is ptracing - couple minor formatting/spelling fixes I've tested the dummy and capabilities bits. thanks, -chris -- Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net ===== fs/exec.c 1.111 vs edited ===== --- 1.111/fs/exec.c Wed Apr 21 02:11:57 2004 +++ edited/fs/exec.c Thu Apr 22 16:56:30 2004 @@ -919,24 +919,30 @@ EXPORT_SYMBOL(prepare_binprm); -/* - * This function is used to produce the new IDs and capabilities - * from the old ones and the file's capabilities. - * - * The formula used for evolving capabilities is: - * - * pI' = pI - * (***) pP' = (fP & X) | (fI & pI) - * pE' = pP' & fE [NB. fE is 0 or ~0] - * - * I=Inheritable, P=Permitted, E=Effective // p=process, f=file - * ' indicates post-exec(), and X is the global 'cap_bset'. - * - */ +static inline int unsafe_exec(struct task_struct *p) +{ + int unsafe = 0; + if (p->ptrace & PT_PTRACED) { + if (p->ptrace & PT_PTRACE_CAP) + unsafe |= LSM_UNSAFE_PTRACE_CAP; + else + unsafe |= LSM_UNSAFE_PTRACE; + } + if (atomic_read(&p->fs->count) > 1 || + atomic_read(&p->files->count) > 1 || + atomic_read(&p->sighand->count) > 1) + unsafe |= LSM_UNSAFE_SHARE; + + return unsafe; +} void compute_creds(struct linux_binprm *bprm) { - security_bprm_apply_creds(bprm); + int unsafe; + task_lock(current); + unsafe = unsafe_exec(current); + security_bprm_apply_creds(bprm, unsafe); + task_unlock(current); } EXPORT_SYMBOL(compute_creds); ===== include/linux/security.h 1.32 vs edited ===== --- 1.32/include/linux/security.h Wed Apr 21 02:11:57 2004 +++ edited/include/linux/security.h Thu Apr 22 16:56:30 2004 @@ -44,7 +44,7 @@ extern int cap_capset_check (struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted); extern void cap_capset_set (struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted); extern int cap_bprm_set_security (struct linux_binprm *bprm); -extern void cap_bprm_apply_creds (struct linux_binprm *bprm); +extern void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe); extern int cap_bprm_secureexec(struct linux_binprm *bprm); extern int cap_inode_setxattr(struct dentry *dentry, char *name, void *value, size_t size, int flags); extern int cap_inode_removexattr(struct dentry *dentry, char *name); @@ -86,6 +86,11 @@ struct sched_param; struct swap_info_struct; +/* bprm_apply_creds unsafe reasons */ +#define LSM_UNSAFE_SHARE 1 +#define LSM_UNSAFE_PTRACE 2 +#define LSM_UNSAFE_PTRACE_CAP 4 + #ifdef CONFIG_SECURITY /** @@ -112,6 +117,8 @@ * also perform other state changes on the process (e.g. closing open * file descriptors to which access is no longer granted if the attributes * were changed). + * bprm_apply_creds is called under task_lock. @unsafe indicates various + * reasons why it may be unsafe to change security state. * @bprm contains the linux_binprm structure. * @bprm_set_security: * Save security information in the bprm->security field, typically based @@ -1026,7 +1033,7 @@ int (*bprm_alloc_security) (struct linux_binprm * bprm); void (*bprm_free_security) (struct linux_binprm * bprm); - void (*bprm_apply_creds) (struct linux_binprm * bprm); + void (*bprm_apply_creds) (struct linux_binprm * bprm, int unsafe); int (*bprm_set_security) (struct linux_binprm * bprm); int (*bprm_check_security) (struct linux_binprm * bprm); int (*bprm_secureexec) (struct linux_binprm * bprm); @@ -1290,9 +1297,9 @@ { security_ops->bprm_free_security (bprm); } -static inline void security_bprm_apply_creds (struct linux_binprm *bprm) +static inline void security_bprm_apply_creds (struct linux_binprm *bprm, int unsafe) { - security_ops->bprm_apply_creds (bprm); + security_ops->bprm_apply_creds (bprm, unsafe); } static inline int security_bprm_set (struct linux_binprm *bprm) { @@ -1962,9 +1969,9 @@ static inline void security_bprm_free (struct linux_binprm *bprm) { } -static inline void security_bprm_apply_creds (struct linux_binprm *bprm) +static inline void security_bprm_apply_creds (struct linux_binprm *bprm, int unsafe) { - cap_bprm_apply_creds (bprm); + cap_bprm_apply_creds (bprm, unsafe); } static inline int security_bprm_set (struct linux_binprm *bprm) ===== security/commoncap.c 1.6 vs edited ===== --- 1.6/security/commoncap.c Wed Apr 21 02:12:12 2004 +++ edited/security/commoncap.c Thu Apr 22 16:56:30 2004 @@ -115,15 +115,7 @@ return 0; } -static inline int must_not_trace_exec (struct task_struct *p) -{ - return ((p->ptrace & PT_PTRACED) && !(p->ptrace & PT_PTRACE_CAP)) - || atomic_read(&p->fs->count) > 1 - || atomic_read(&p->files->count) > 1 - || atomic_read(&p->sighand->count) > 1; -} - -void cap_bprm_apply_creds (struct linux_binprm *bprm) +void cap_bprm_apply_creds (struct linux_binprm *bprm, int unsafe) { /* Derived from fs/exec.c:compute_creds. */ kernel_cap_t new_permitted, working; @@ -133,30 +125,25 @@ current->cap_inheritable); new_permitted = cap_combine (new_permitted, working); - task_lock(current); - - if (bprm->e_uid != current->uid || bprm->e_gid != current->gid) { + if (bprm->e_uid != current->uid || bprm->e_gid != current->gid || + !cap_issubset (new_permitted, current->cap_permitted)) { current->mm->dumpable = 0; - if (must_not_trace_exec(current) && !capable(CAP_SETUID)) { - bprm->e_uid = current->uid; - bprm->e_gid = current->gid; + if (unsafe & ~LSM_UNSAFE_PTRACE_CAP) { + if (!capable(CAP_SETUID)) { + bprm->e_uid = current->uid; + bprm->e_gid = current->gid; + } + if (!capable (CAP_SETPCAP)) { + new_permitted = cap_intersect (new_permitted, + current->cap_permitted); + } } } current->suid = current->euid = current->fsuid = bprm->e_uid; current->sgid = current->egid = current->fsgid = bprm->e_gid; - if (!cap_issubset (new_permitted, current->cap_permitted)) { - current->mm->dumpable = 0; - - if (must_not_trace_exec (current) && !capable (CAP_SETPCAP)) { - new_permitted = cap_intersect (new_permitted, - current-> - cap_permitted); - } - } - /* For init, we want to retain the capabilities set * in the init_task struct. Thus we skip the usual * capability rules */ @@ -167,7 +154,6 @@ } /* AUD: Audit candidate if current->cap_effective is set */ - task_unlock(current); current->keep_capabilities = 0; } ===== security/dummy.c 1.36 vs edited ===== --- 1.36/security/dummy.c Wed Apr 21 02:12:12 2004 +++ edited/security/dummy.c Thu Apr 22 16:56:30 2004 @@ -171,21 +171,12 @@ return; } -static inline int must_not_trace_exec (struct task_struct *p) +static void dummy_bprm_apply_creds (struct linux_binprm *bprm, int unsafe) { - return ((p->ptrace & PT_PTRACED) && !(p->ptrace & PT_PTRACE_CAP)) - || atomic_read(&p->fs->count) > 1 - || atomic_read(&p->files->count) > 1 - || atomic_read(&p->sighand->count) > 1; -} - -static void dummy_bprm_apply_creds (struct linux_binprm *bprm) -{ - task_lock(current); if (bprm->e_uid != current->uid || bprm->e_gid != current->gid) { current->mm->dumpable = 0; - if (must_not_trace_exec(current) && !capable(CAP_SETUID)) { + if ((unsafe & ~LSM_UNSAFE_PTRACE_CAP) && !capable(CAP_SETUID)) { bprm->e_uid = current->uid; bprm->e_gid = current->gid; } @@ -193,8 +184,6 @@ current->suid = current->euid = current->fsuid = bprm->e_uid; current->sgid = current->egid = current->fsgid = bprm->e_gid; - - task_unlock(current); } static int dummy_bprm_set_security (struct linux_binprm *bprm) ===== security/selinux/hooks.c 1.35 vs edited ===== --- 1.35/security/selinux/hooks.c Wed Apr 21 02:15:14 2004 +++ edited/security/selinux/hooks.c Thu Apr 22 16:56:57 2004 @@ -1745,7 +1745,7 @@ spin_unlock(&files->file_lock); } -static void selinux_bprm_apply_creds(struct linux_binprm *bprm) +static void selinux_bprm_apply_creds(struct linux_binprm *bprm, int unsafe) { struct task_security_struct *tsec, *psec; struct bprm_security_struct *bsec; @@ -1755,7 +1755,7 @@ struct rlimit *rlim, *initrlim; int rc, i; - secondary_ops->bprm_apply_creds(bprm); + secondary_ops->bprm_apply_creds(bprm, unsafe); tsec = current->security; @@ -1766,22 +1766,22 @@ if (tsec->sid != sid) { /* Check for shared state. If not ok, leave SID unchanged and kill. */ - if ((atomic_read(¤t->fs->count) > 1 || - atomic_read(¤t->files->count) > 1 || - atomic_read(¤t->sighand->count) > 1)) { - rc = avc_has_perm(tsec->sid, sid, + if (unsafe & LSM_UNSAFE_SHARE) { + rc = avc_has_perm_noaudit(tsec->sid, sid, SECCLASS_PROCESS, PROCESS__SHARE, - NULL, NULL); + NULL, &avd); if (rc) { + task_unlock(current); + avc_audit(tsec->sid, sid, SECCLASS_PROCESS, + PROCESS__SHARE, &avd, rc, NULL); force_sig_specific(SIGKILL, current); - return; + goto lock_out; } } /* Check for ptracing, and update the task SID if ok. Otherwise, leave SID unchanged and kill. */ - task_lock(current); - if (current->ptrace & PT_PTRACED) { + if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) { psec = current->parent->security; rc = avc_has_perm_noaudit(psec->sid, sid, SECCLASS_PROCESS, PROCESS__PTRACE, @@ -1793,7 +1793,7 @@ PROCESS__PTRACE, &avd, rc, NULL); if (rc) { force_sig_specific(SIGKILL, current); - return; + goto lock_out; } } else { tsec->sid = sid; @@ -1846,6 +1846,10 @@ /* Wake up the parent if it is waiting so that it can recheck wait permission to the new task SID. */ wake_up_interruptible(¤t->parent->wait_chldexit); + +lock_out: + task_lock(current); + return; } }