Subject: Re: creds_fix_take2.patch
From: Chris Wright <[email-masked]>
To: Andrew Morton <[email-masked]>
Cc: Stephen Smalley <[email-masked]>, [email-masked], [email-masked], [email-masked]
Date: Thu, 22 Apr 2004 17:41:09 -0700

* Andrew Morton ([email-masked]) wrote:
> Stephen Smalley <[email-masked]> 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(&current->fs->count) > 1 ||
-		     atomic_read(&current->files->count) > 1 ||
-		     atomic_read(&current->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(&current->parent->wait_chldexit);
+
+lock_out:
+		task_lock(current);
+		return;
 	}
 }