Re: [RFC] [Stacking v4 3/3] Cleaned up stacker patch

From: Serge E. Hallyn (serue@private)
Date: Wed Dec 08 2004 - 05:26:55 PST


Quoting Stephen Smalley (sds@private):
...
> capset_set than for other hooks.  Possibly reflects a more basic problem
> in the capset code and hooks; capset_check is supposed to handle
> security checking while capset_set is supposed to just update state, but
> we often don't have the specific target task in capset_check, so
> capset_set has to apply a check too for completeness.  Also, looking at
> the core kernel, it looks like capset_check isn't serving a useful
> purpose anymore, as the core kernel is applying the same checks
> directly.  capget may not be a real issue, as it is only getting the
> bits into a kernel data structure and will not copy to userspace if we
> returned an error.  Possibly need two separate hooks at the same points
> where capset_set is presently being invoked to cleanly separate
> permission checking hooks from state update.

So something like the following patch might be appropriate?  It removes
the redundant capset_check code, moving capset_check to right before each
call to capset_set, and moves the selinux_capset_set authentication code
into capset_check.

thanks,
-serge

Index: linux-2.6.10-rc3-bk2/include/linux/security.h
===================================================================
--- linux-2.6.10-rc3-bk2.orig/include/linux/security.h	2004-12-07 19:35:52.249237440 -0600
+++ linux-2.6.10-rc3-bk2/include/linux/security.h	2004-12-07 19:42:44.489567384 -0600
@@ -41,7 +41,6 @@
 extern int cap_settime (struct timespec *ts, struct timezone *tz);
 extern int cap_ptrace (struct task_struct *parent, struct task_struct *child);
 extern int cap_capget (struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
-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, int unsafe);
@@ -1932,7 +1931,7 @@
 					 kernel_cap_t *inheritable,
 					 kernel_cap_t *permitted)
 {
-	return cap_capset_check (target, effective, inheritable, permitted);
+	return 0;
 }
 
 static inline void security_capset_set (struct task_struct *target,
Index: linux-2.6.10-rc3-bk2/kernel/capability.c
===================================================================
--- linux-2.6.10-rc3-bk2.orig/kernel/capability.c	2004-12-07 19:35:52.659175120 -0600
+++ linux-2.6.10-rc3-bk2/kernel/capability.c	2004-12-07 20:30:19.482542480 -0600
@@ -93,8 +93,12 @@
 
 	do_each_task_pid(pgrp, PIDTYPE_PGID, g) {
 		target = g;
-		while_each_thread(g, target)
-			security_capset_set(target, effective, inheritable, permitted);
+		while_each_thread(g, target) {
+			if (!security_capset_check(target, effective,
+					inheritable, permitted))
+				security_capset_set(target, effective,
+					inheritable, permitted);
+		}
 	} while_each_task_pid(pgrp, PIDTYPE_PGID, g);
 }
 
@@ -111,6 +115,9 @@
      do_each_thread(g, target) {
              if (target == current || target->pid == 1)
                      continue;
+	     if (security_capset_check(target, effective, inheritable,
+				permitted))
+		     continue;
 	     security_capset_set(target, effective, inheritable, permitted);
      } while_each_thread(g, target);
 }
@@ -169,9 +176,6 @@
 
      ret = -EPERM;
 
-     if (security_capset_check(target, &effective, &inheritable, &permitted))
-	     goto out;
-
      if (!cap_issubset(inheritable, cap_combine(target->cap_inheritable,
                        current->cap_permitted)))
              goto out;
@@ -196,7 +200,10 @@
              else            /* all procs in process group */
                      cap_set_pg(-pid, &effective, &inheritable, &permitted);
      } else {
-	     security_capset_set(target, &effective, &inheritable, &permitted);
+	     if (!security_capset_check(target, &effective, &inheritable,
+				&permitted))
+		     security_capset_set(target, &effective, &inheritable,
+					&permitted);
      }
 
 out:
Index: linux-2.6.10-rc3-bk2/security/commoncap.c
===================================================================
--- linux-2.6.10-rc3-bk2.orig/security/commoncap.c	2004-12-07 19:35:54.010969616 -0600
+++ linux-2.6.10-rc3-bk2/security/commoncap.c	2004-12-07 19:42:13.580266312 -0600
@@ -75,32 +75,6 @@
 	return 0;
 }
 
-int cap_capset_check (struct task_struct *target, kernel_cap_t *effective,
-		      kernel_cap_t *inheritable, kernel_cap_t *permitted)
-{
-	/* Derived from kernel/capability.c:sys_capset. */
-	/* verify restrictions on target's new Inheritable set */
-	if (!cap_issubset (*inheritable,
-			   cap_combine (target->cap_inheritable,
-					current->cap_permitted))) {
-		return -EPERM;
-	}
-
-	/* verify restrictions on target's new Permitted set */
-	if (!cap_issubset (*permitted,
-			   cap_combine (target->cap_permitted,
-					current->cap_permitted))) {
-		return -EPERM;
-	}
-
-	/* verify the _new_Effective_ is a subset of the _new_Permitted_ */
-	if (!cap_issubset (*effective, *permitted)) {
-		return -EPERM;
-	}
-
-	return 0;
-}
-
 void cap_capset_set (struct task_struct *target, kernel_cap_t *effective,
 		     kernel_cap_t *inheritable, kernel_cap_t *permitted)
 {
@@ -398,7 +372,6 @@
 EXPORT_SYMBOL(cap_settime);
 EXPORT_SYMBOL(cap_ptrace);
 EXPORT_SYMBOL(cap_capget);
-EXPORT_SYMBOL(cap_capset_check);
 EXPORT_SYMBOL(cap_capset_set);
 EXPORT_SYMBOL(cap_bprm_set_security);
 EXPORT_SYMBOL(cap_bprm_apply_creds);
Index: linux-2.6.10-rc3-bk2/security/root_plug.c
===================================================================
--- linux-2.6.10-rc3-bk2.orig/security/root_plug.c	2004-10-18 16:55:28.000000000 -0500
+++ linux-2.6.10-rc3-bk2/security/root_plug.c	2004-12-07 19:45:22.286578592 -0600
@@ -86,7 +86,6 @@
 	/* Use the capability functions for some of the hooks */
 	.ptrace =			cap_ptrace,
 	.capget =			cap_capget,
-	.capset_check =			cap_capset_check,
 	.capset_set =			cap_capset_set,
 	.capable =			cap_capable,
 
Index: linux-2.6.10-rc3-bk2/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-bk2.orig/security/selinux/hooks.c	2004-12-07 19:35:54.051963384 -0600
+++ linux-2.6.10-rc3-bk2/security/selinux/hooks.c	2004-12-07 19:44:41.366799344 -0600
@@ -1391,24 +1391,12 @@
 static int selinux_capset_check(struct task_struct *target, kernel_cap_t *effective,
                                 kernel_cap_t *inheritable, kernel_cap_t *permitted)
 {
-	int error;
-
-	error = secondary_ops->capset_check(target, effective, inheritable, permitted);
-	if (error)
-		return error;
-
 	return task_has_perm(current, target, PROCESS__SETCAP);
 }
 
 static void selinux_capset_set(struct task_struct *target, kernel_cap_t *effective,
                                kernel_cap_t *inheritable, kernel_cap_t *permitted)
 {
-	int error;
-
-	error = task_has_perm(current, target, PROCESS__SETCAP);
-	if (error)
-		return;
-
 	secondary_ops->capset_set(target, effective, inheritable, permitted);
 }
 



This archive was generated by hypermail 2.1.3 : Wed Dec 08 2004 - 05:27:34 PST