* Re: compute_creds fixup in -mm [not found] <20040421010621.L22989@build.pdx.osdl.net> @ 2004-04-21 17:27 ` Andy Lutomirski 2004-04-21 18:13 ` Stephen Smalley 2004-04-21 19:07 ` Chris Wright 0 siblings, 2 replies; 9+ messages in thread From: Andy Lutomirski @ 2004-04-21 17:27 UTC (permalink / raw) To: Chris Wright, Andrew Morton, Linus Torvalds, sds, jmorris Cc: Andy Lutomirski, linux-kernel Chris Wright wrote: > [ I'm going through a bunch of security bugs/fixes, and I'm getting a little > bleary eyed so perhaps I'm missing something obvious. ] > > Andy, your patch appears to leave the core issue unfixed, unfortuantely. > [...] > This is still vulnerable. I think the problem is that a ptrace detach > never acquires the task_lock(), yet it will clear task->ptrace. So the > above code is still racey. > > ptrace_attach() > task_lock() > must_not_trace_exec() /*true, don't elevate uid*/ > ptrace_detach() > must_not_trace_exec() /*false, give full caps*/ > task_unlock() Yeesh! You're right. > > Code could be cleaned up to simply check must_not_trace_exec() only > once (like below, untested). Otherwise, it'd be nice to task_lock() > on __ptrace_unlink() before clearing task->ptrace, but this isn't proper > lock netsing. I think your patch is correct; I didn't do it that way because I didn't want to worry about whether it would change the semantics. I'm pretty sure now that it doesn't. This doesn't fix selinux, though -- its apply_creds hook just blindly calls commoncap's. In fact, this breaks all attempts to get nested capability modules right. The problem is that, AFAICS, not only does ptrace_detach not take task_lock, _exit() doesn't either. So you get an equivalent race for the shared state check. I see two ways to fix that: 1. something checks for shared state _once_ and saves it either in the binprm or passes it as a parameter to apply_creds. apply_creds needs to be changed so that the task_lock is taken by the caller. 2. Don't nest apply_creds. I'm favoring #1, as it doesn't break the whole programming model. Did I miss something? This one also (hopefully) fixes selinux. Patch against 2.6.5-mm5. fs/exec.c | 36 +++++++++++++++++++++--------------- include/linux/security.h | 19 +++++++++++++------ security/commoncap.c | 38 ++++++++++++-------------------------- security/dummy.c | 15 ++------------- security/selinux/hooks.c | 12 ++++-------- 5 files changed, 52 insertions(+), 68 deletions(-) --- linux-2.6.5-mm5/include/linux/security.h.ptlock 2004-04-21 08:52:49.904877920 -0700 +++ linux-2.6.5-mm5/include/linux/security.h 2004-04-21 09:12:46.540961584 -0700 @@ -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; +/* brpm_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) --- linux-2.6.5-mm5/fs/exec.c.ptlock 2004-04-21 08:50:37.767965784 -0700 +++ linux-2.6.5-mm5/fs/exec.c 2004-04-21 09:14:30.778115128 -0700 @@ -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 must_not_trace_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); + task_lock(current); + + security_bprm_apply_creds(bprm, must_not_trace_exec(current)); + + task_unlock(current); } EXPORT_SYMBOL(compute_creds); --- linux-2.6.5-mm5/security/selinux/hooks.c.ptlock 2004-04-21 08:57:16.947281304 -0700 +++ linux-2.6.5-mm5/security/selinux/hooks.c 2004-04-21 09:22:15.227508088 -0700 @@ -1746,7 +1746,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; @@ -1756,7 +1756,7 @@ struct rlimit *rlim, *initrlim; int rc, i; - secondary_ops->bprm_apply_creds(bprm); + secondary_ops->bprm_apply_creds(bprm, unsafe); tsec = current->security; @@ -1767,9 +1767,7 @@ 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)) { + if (unsafe & LSM_UNSAFE_SHARE) { rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS, PROCESS__SHARE, NULL, NULL); @@ -1782,14 +1780,13 @@ /* 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, NULL, &avd); if (!rc) tsec->sid = sid; - task_unlock(current); avc_audit(psec->sid, sid, SECCLASS_PROCESS, PROCESS__PTRACE, &avd, rc, NULL); if (rc) { @@ -1798,7 +1795,6 @@ } } else { tsec->sid = sid; - task_unlock(current); } /* Close files for which the new task SID is not authorized. */ --- linux-2.6.5-mm5/security/commoncap.c.ptlock 2004-04-21 08:54:16.824664104 -0700 +++ linux-2.6.5-mm5/security/commoncap.c 2004-04-21 09:24:01.468357024 -0700 @@ -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; } --- linux-2.6.5-mm5/security/dummy.c.ptlock 2004-04-21 08:56:00.608886504 -0700 +++ linux-2.6.5-mm5/security/dummy.c 2004-04-21 09:14:57.345076336 -0700 @@ -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 && !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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: compute_creds fixup in -mm 2004-04-21 17:27 ` compute_creds fixup in -mm Andy Lutomirski @ 2004-04-21 18:13 ` Stephen Smalley 2004-04-21 18:28 ` Chris Wright 2004-04-21 19:07 ` Chris Wright 1 sibling, 1 reply; 9+ messages in thread From: Stephen Smalley @ 2004-04-21 18:13 UTC (permalink / raw) To: Andy Lutomirski Cc: Chris Wright, Andrew Morton, Linus Torvalds, James Morris, lkml On Wed, 2004-04-21 at 13:27, Andy Lutomirski wrote: > This doesn't fix selinux, though -- its apply_creds hook just blindly calls > commoncap's. In fact, this breaks all attempts to get nested capability modules > right. The problem is that, AFAICS, not only does ptrace_detach not take > task_lock, _exit() doesn't either. So you get an equivalent race for the shared > state check. I see two ways to fix that: I didn't see Chris' patch. I assume that the worst case is unexpected program failure due to lack of capability, right? The SELinux security transitions aren't tied to the uid/capability evolution anyway, and SELinux provides its own separation among differing security domains, including the kernel access controls and enabling glibc secure mode. Side bar: Why does the uid/capability logic proceed under the old uid/capability set rather than aborting the exec? SELinux leaves the task SID unchanged and forces a SIGKILL, as we don't really want the program proceeding under the wrong permission set anyway. > 1. something checks for shared state _once_ and saves it either in the binprm or > passes it as a parameter to apply_creds. apply_creds needs to be changed so > that the task_lock is taken by the caller. Likely a good idea. > This one also (hopefully) fixes selinux. > > Patch against 2.6.5-mm5. Didn't apply for me. Also would help to rebase to 2.6.6-rc2-mm1. > --- linux-2.6.5-mm5/security/selinux/hooks.c.ptlock 2004-04-21 08:57:16.947281304 -0700 > +++ linux-2.6.5-mm5/security/selinux/hooks.c 2004-04-21 09:22:15.227508088 -0700 > @@ -1782,14 +1780,13 @@ > /* Check for ptracing, and update the task SID if ok. > Otherwise, leave SID unchanged and kill. */ > task_lock(current); Above line needs to be deleted, right? -- Stephen Smalley <sds@epoch.ncsc.mil> National Security Agency ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: compute_creds fixup in -mm 2004-04-21 18:13 ` Stephen Smalley @ 2004-04-21 18:28 ` Chris Wright 2004-04-21 18:42 ` Stephen Smalley 0 siblings, 1 reply; 9+ messages in thread From: Chris Wright @ 2004-04-21 18:28 UTC (permalink / raw) To: Stephen Smalley Cc: Andy Lutomirski, Chris Wright, Andrew Morton, Linus Torvalds, James Morris, lkml * Stephen Smalley (sds@epoch.ncsc.mil) wrote: > On Wed, 2004-04-21 at 13:27, Andy Lutomirski wrote: > > This doesn't fix selinux, though -- its apply_creds hook just blindly calls > > commoncap's. In fact, this breaks all attempts to get nested capability modules > > right. The problem is that, AFAICS, not only does ptrace_detach not take > > task_lock, _exit() doesn't either. So you get an equivalent race for the shared > > state check. I see two ways to fix that: > > I didn't see Chris' patch. I assume that the worst case is unexpected > program failure due to lack of capability, right? The SELinux security The opposite. You'd get a program with non-root euid, but full capability set, and AT_SECURE set false. My patch is below. --- a/security/commoncap.c~compute_creds-lock 2004-04-21 00:54:34.000000000 -0700 +++ b/security/commoncap.c 2004-04-21 01:01:00.000000000 -0700 @@ -135,28 +135,26 @@ 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 (must_not_trace_exec(current)) { + 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 */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: compute_creds fixup in -mm 2004-04-21 18:28 ` Chris Wright @ 2004-04-21 18:42 ` Stephen Smalley 2004-04-21 18:54 ` Chris Wright 2004-04-21 19:37 ` Andy Lutomirski 0 siblings, 2 replies; 9+ messages in thread From: Stephen Smalley @ 2004-04-21 18:42 UTC (permalink / raw) To: Chris Wright Cc: Andy Lutomirski, Andrew Morton, Linus Torvalds, James Morris, lkml On Wed, 2004-04-21 at 14:28, Chris Wright wrote: > * Stephen Smalley (sds@epoch.ncsc.mil) wrote: > > I didn't see Chris' patch. I assume that the worst case is unexpected > > program failure due to lack of capability, right? The SELinux security > > The opposite. You'd get a program with non-root euid, but full > capability set, and AT_SECURE set false. My patch is below. Sorry, I wasn't clear. I meant the worst case due to the share/ptrace state check being duplicated in SELinux and in commoncap, as opposed to being performed once as in Andy's patch. -- Stephen Smalley <sds@epoch.ncsc.mil> National Security Agency ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: compute_creds fixup in -mm 2004-04-21 18:42 ` Stephen Smalley @ 2004-04-21 18:54 ` Chris Wright 2004-04-21 19:37 ` Andy Lutomirski 1 sibling, 0 replies; 9+ messages in thread From: Chris Wright @ 2004-04-21 18:54 UTC (permalink / raw) To: Stephen Smalley Cc: Chris Wright, Andy Lutomirski, Andrew Morton, Linus Torvalds, James Morris, lkml * Stephen Smalley (sds@epoch.ncsc.mil) wrote: > On Wed, 2004-04-21 at 14:28, Chris Wright wrote: > > * Stephen Smalley (sds@epoch.ncsc.mil) wrote: > > > I didn't see Chris' patch. I assume that the worst case is unexpected > > > program failure due to lack of capability, right? The SELinux security > > > > The opposite. You'd get a program with non-root euid, but full > > capability set, and AT_SECURE set false. My patch is below. > > Sorry, I wasn't clear. I meant the worst case due to the share/ptrace > state check being duplicated in SELinux and in commoncap, as opposed to > being performed once as in Andy's patch. Ah, indeed. thanks, -chris -- Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: compute_creds fixup in -mm 2004-04-21 18:42 ` Stephen Smalley 2004-04-21 18:54 ` Chris Wright @ 2004-04-21 19:37 ` Andy Lutomirski 2004-04-21 20:10 ` Stephen Smalley 1 sibling, 1 reply; 9+ messages in thread From: Andy Lutomirski @ 2004-04-21 19:37 UTC (permalink / raw) To: Stephen Smalley Cc: Chris Wright, Andy Lutomirski, Andrew Morton, Linus Torvalds, James Morris, lkml Stephen Smalley wrote: > On Wed, 2004-04-21 at 14:28, Chris Wright wrote: > >>* Stephen Smalley (sds@epoch.ncsc.mil) wrote: >> >>>I didn't see Chris' patch. I assume that the worst case is unexpected >>>program failure due to lack of capability, right? The SELinux security >> >>The opposite. You'd get a program with non-root euid, but full >>capability set, and AT_SECURE set false. My patch is below. > > > Sorry, I wasn't clear. I meant the worst case due to the share/ptrace > state check being duplicated in SELinux and in commoncap, as opposed to > being performed once as in Andy's patch. > I was worried about sid changing but uid and caps staying the same if a ptrace_detach or _exit happens between the cap_bprm_apply_creds call and the rest of selinux_bprm_apply_creds. Remember the sendmail bug -- program failure due to lack of capabilities can cause privilege leaks (in this case selinux sid leaks). If this isn't a concern, then Chris' patch should be fine. On the other hand, some other LSM may prefer my version. Here's my corrected patch (thanks, Chris): --- linux-2.6.5-mm5/include/linux/security.h.ptlock 2004-04-21 08:52:49.904877920 -0700 +++ linux-2.6.5-mm5/include/linux/security.h 2004-04-21 09:12:46.540961584 -0700 @@ -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; +/* brpm_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) --- linux-2.6.5-mm5/fs/exec.c.ptlock 2004-04-21 08:50:37.767965784 -0700 +++ linux-2.6.5-mm5/fs/exec.c 2004-04-21 12:23:01.915761696 -0700 @@ -919,24 +919,28 @@ 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 must_not_trace_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); + task_lock(current); + security_bprm_apply_creds(bprm, must_not_trace_exec(current)); + task_unlock(current); } EXPORT_SYMBOL(compute_creds); --- linux-2.6.5-mm5/security/selinux/hooks.c.ptlock 2004-04-21 08:57:16.947281304 -0700 +++ linux-2.6.5-mm5/security/selinux/hooks.c 2004-04-21 12:23:58.245198320 -0700 @@ -1746,7 +1746,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; @@ -1756,7 +1756,7 @@ struct rlimit *rlim, *initrlim; int rc, i; - secondary_ops->bprm_apply_creds(bprm); + secondary_ops->bprm_apply_creds(bprm, unsafe); tsec = current->security; @@ -1767,9 +1767,7 @@ 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)) { + if (unsafe & LSM_UNSAFE_SHARE) { rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS, PROCESS__SHARE, NULL, NULL); @@ -1781,15 +1779,13 @@ /* 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, NULL, &avd); if (!rc) tsec->sid = sid; - task_unlock(current); avc_audit(psec->sid, sid, SECCLASS_PROCESS, PROCESS__PTRACE, &avd, rc, NULL); if (rc) { @@ -1798,7 +1794,6 @@ } } else { tsec->sid = sid; - task_unlock(current); } /* Close files for which the new task SID is not authorized. */ --- linux-2.6.5-mm5/security/commoncap.c.ptlock 2004-04-21 08:54:16.824664104 -0700 +++ linux-2.6.5-mm5/security/commoncap.c 2004-04-21 09:24:01.468357024 -0700 @@ -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; } --- linux-2.6.5-mm5/security/dummy.c.ptlock 2004-04-21 08:56:00.608886504 -0700 +++ linux-2.6.5-mm5/security/dummy.c 2004-04-21 09:14:57.345076336 -0700 @@ -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 && !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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: compute_creds fixup in -mm 2004-04-21 19:37 ` Andy Lutomirski @ 2004-04-21 20:10 ` Stephen Smalley 0 siblings, 0 replies; 9+ messages in thread From: Stephen Smalley @ 2004-04-21 20:10 UTC (permalink / raw) To: Andy Lutomirski Cc: Chris Wright, Andy Lutomirski, Andrew Morton, Linus Torvalds, James Morris, lkml On Wed, 2004-04-21 at 15:37, Andy Lutomirski wrote: > I was worried about sid changing but uid and caps staying the same if > a ptrace_detach or _exit happens between the cap_bprm_apply_creds call > and the rest of selinux_bprm_apply_creds. Remember the sendmail bug -- > program failure due to lack of capabilities can cause privilege leaks > (in this case selinux sid leaks). That particular issue shouldn't be a problem, as SELinux security transitions aren't controlled by Linux capabilities and SELinux specifically controls code execution (both entry into a domain and ability to execute anything else without changing domains). However, I do agree that it could yield an unexpected failure in the program that would be harmful, so I'm in favor of checking the state only once. -- Stephen Smalley <sds@epoch.ncsc.mil> National Security Agency ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: compute_creds fixup in -mm 2004-04-21 17:27 ` compute_creds fixup in -mm Andy Lutomirski 2004-04-21 18:13 ` Stephen Smalley @ 2004-04-21 19:07 ` Chris Wright 1 sibling, 0 replies; 9+ messages in thread From: Chris Wright @ 2004-04-21 19:07 UTC (permalink / raw) To: Andy Lutomirski Cc: Chris Wright, Andrew Morton, Linus Torvalds, sds, jmorris, linux-kernel * Andy Lutomirski (luto@myrealbox.com) wrote: > void compute_creds(struct linux_binprm *bprm) > { > - security_bprm_apply_creds(bprm); > + task_lock(current); > + > + security_bprm_apply_creds(bprm, must_not_trace_exec(current)); > + > + task_unlock(current); unecessary extra lines. > +static void selinux_bprm_apply_creds(struct linux_binprm *bprm, int unsafe) > @@ -1782,14 +1780,13 @@ > /* Check for ptracing, and update the task SID if ok. > Otherwise, leave SID unchanged and kill. */ > task_lock(current); oops ;-) > - if (current->ptrace & PT_PTRACED) { > + if (unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) { thanks, -chris -- Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <fa.n7n31hd.1k7ce37@ifi.uio.no>]
[parent not found: <fa.f9m11dm.206fg4@ifi.uio.no>]
* Re: compute_creds fixup in -mm [not found] ` <fa.f9m11dm.206fg4@ifi.uio.no> @ 2004-04-21 20:36 ` Andy Lutomirski 0 siblings, 0 replies; 9+ messages in thread From: Andy Lutomirski @ 2004-04-21 20:36 UTC (permalink / raw) To: Stephen Smalley Cc: Chris Wright, Andy Lutomirski, Andrew Morton, Linus Torvalds, James Morris, lkml [-- Attachment #1: Type: text/plain, Size: 160 bytes --] Apparently my mailer converts single leading spaces into pairs of leading spaces. Time to find a new mailer. Here's my patch again as an attachment. --Andy [-- Attachment #2: creds_fix_take2.patch --] [-- Type: text/plain, Size: 9159 bytes --] --- linux-2.6.5-mm5/include/linux/security.h.ptlock 2004-04-21 08:52:49.904877920 -0700 +++ linux-2.6.5-mm5/include/linux/security.h 2004-04-21 09:12:46.540961584 -0700 @@ -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; +/* brpm_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) --- linux-2.6.5-mm5/fs/exec.c.ptlock 2004-04-21 08:50:37.767965784 -0700 +++ linux-2.6.5-mm5/fs/exec.c 2004-04-21 12:23:01.915761696 -0700 @@ -919,24 +919,28 @@ 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 must_not_trace_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); + task_lock(current); + security_bprm_apply_creds(bprm, must_not_trace_exec(current)); + task_unlock(current); } EXPORT_SYMBOL(compute_creds); --- linux-2.6.5-mm5/security/selinux/hooks.c.ptlock 2004-04-21 08:57:16.947281304 -0700 +++ linux-2.6.5-mm5/security/selinux/hooks.c 2004-04-21 12:23:58.245198320 -0700 @@ -1746,7 +1746,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; @@ -1756,7 +1756,7 @@ struct rlimit *rlim, *initrlim; int rc, i; - secondary_ops->bprm_apply_creds(bprm); + secondary_ops->bprm_apply_creds(bprm, unsafe); tsec = current->security; @@ -1767,9 +1767,7 @@ 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)) { + if (unsafe & LSM_UNSAFE_SHARE) { rc = avc_has_perm(tsec->sid, sid, SECCLASS_PROCESS, PROCESS__SHARE, NULL, NULL); @@ -1781,15 +1779,13 @@ /* 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, NULL, &avd); if (!rc) tsec->sid = sid; - task_unlock(current); avc_audit(psec->sid, sid, SECCLASS_PROCESS, PROCESS__PTRACE, &avd, rc, NULL); if (rc) { @@ -1798,7 +1794,6 @@ } } else { tsec->sid = sid; - task_unlock(current); } /* Close files for which the new task SID is not authorized. */ --- linux-2.6.5-mm5/security/commoncap.c.ptlock 2004-04-21 08:54:16.824664104 -0700 +++ linux-2.6.5-mm5/security/commoncap.c 2004-04-21 09:24:01.468357024 -0700 @@ -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; } --- linux-2.6.5-mm5/security/dummy.c.ptlock 2004-04-21 08:56:00.608886504 -0700 +++ linux-2.6.5-mm5/security/dummy.c 2004-04-21 09:14:57.345076336 -0700 @@ -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 && !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) ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2004-04-21 20:37 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20040421010621.L22989@build.pdx.osdl.net>
2004-04-21 17:27 ` compute_creds fixup in -mm Andy Lutomirski
2004-04-21 18:13 ` Stephen Smalley
2004-04-21 18:28 ` Chris Wright
2004-04-21 18:42 ` Stephen Smalley
2004-04-21 18:54 ` Chris Wright
2004-04-21 19:37 ` Andy Lutomirski
2004-04-21 20:10 ` Stephen Smalley
2004-04-21 19:07 ` Chris Wright
[not found] <fa.n7n31hd.1k7ce37@ifi.uio.no>
[not found] ` <fa.f9m11dm.206fg4@ifi.uio.no>
2004-04-21 20:36 ` Andy Lutomirski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox