[PATCH] properly split capset_check+capset_set

[PATCH] properly split capset_check+capset_set

[Serge E. Hallyn]

The attached patch removes checks from kernel/capability.c which are
redundant with cap_capset_check() code, and moves the capset_check()
calls to immediately before the capset_set() calls.  This allows
capset_check() to accurately check the setter's permission to set caps
on the target.  Please apply.

thanks,
-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-mm1/kernel/capability.c
===================================================================
--- linux-2.6.10-mm1.orig/kernel/capability.c	2005-01-04 10:23:54.000000000 -0600
+++ linux-2.6.10-mm1/kernel/capability.c	2005-01-04 10:32:48.000000000 -0600
@@ -85,34 +85,60 @@ out:
  * cap_set_pg - set capabilities for all processes in a given process
  * group.  We call this holding task_capability_lock and tasklist_lock.
  */
-static inline void cap_set_pg(int pgrp, kernel_cap_t *effective,
+static inline int cap_set_pg(int pgrp, kernel_cap_t *effective,
 			      kernel_cap_t *inheritable,
 			      kernel_cap_t *permitted)
 {
 	task_t *g, *target;
+	int ret = -EPERM;
+	int found = 0;
 
 	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);
+				ret = 0;
+			}
+			found = 1;
+		}
 	} while_each_task_pid(pgrp, PIDTYPE_PGID, g);
+
+	if (!found)
+	     ret = 0;
+	return ret;
 }
 
 /*
  * cap_set_all - set capabilities for all processes other than init
  * and self.  We call this holding task_capability_lock and tasklist_lock.
  */
-static inline void cap_set_all(kernel_cap_t *effective,
+static inline int cap_set_all(kernel_cap_t *effective,
 			       kernel_cap_t *inheritable,
 			       kernel_cap_t *permitted)
 {
      task_t *g, *target;
+     int ret = -EPERM;
+     int found = 0;
 
      do_each_thread(g, target) {
              if (target == current || target->pid == 1)
                      continue;
+             found = 1;
+	     if (security_capset_check(target, effective, inheritable,
+						permitted))
+		     continue;
+	     ret = 0;
 	     security_capset_set(target, effective, inheritable, permitted);
      } while_each_thread(g, target);
+
+     if (!found)
+	     ret = 0;
+     return ret;
 }
 
 /*
@@ -167,36 +193,23 @@ asmlinkage long sys_capset(cap_user_head
      } else
                target = current;
 
-     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;
-
-     /* verify restrictions on target's new Permitted set */
-     if (!cap_issubset(permitted, cap_combine(target->cap_permitted,
-                       current->cap_permitted)))
-             goto out;
-
-     /* verify the _new_Effective_ is a subset of the _new_Permitted_ */
-     if (!cap_issubset(effective, permitted))
-             goto out;
-
      ret = 0;
 
      /* having verified that the proposed changes are legal,
            we now put them into effect. */
      if (pid < 0) {
              if (pid == -1)  /* all procs other than current and init */
-                     cap_set_all(&effective, &inheritable, &permitted);
+                     ret = cap_set_all(&effective, &inheritable, &permitted);
 
              else            /* all procs in process group */
-                     cap_set_pg(-pid, &effective, &inheritable, &permitted);
+                     ret = cap_set_pg(-pid, &effective, &inheritable,
+		     					&permitted);
      } else {
-	     security_capset_set(target, &effective, &inheritable, &permitted);
+	     ret = security_capset_check(target, &effective, &inheritable,
+	     						&permitted);
+	     if (!ret)
+		     security_capset_set(target, &effective, &inheritable,
+		     					&permitted);
      }
 
 out:
Index: linux-2.6.10-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-mm1.orig/security/selinux/hooks.c	2005-01-04 10:23:54.000000000 -0600
+++ linux-2.6.10-mm1/security/selinux/hooks.c	2005-01-04 10:24:19.000000000 -0600
@@ -1390,12 +1390,6 @@ static int selinux_capset_check(struct t
 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);
 }
 

Re: [PATCH] properly split capset_check+capset_set

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> The attached patch removes checks from kernel/capability.c which are
> redundant with cap_capset_check() code, and moves the capset_check()
> calls to immediately before the capset_set() calls.  This allows
> capset_check() to accurately check the setter's permission to set caps
> on the target.  Please apply.
> 
> thanks,
> -serge
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Signed-off-by: Chris Wright <chrisw@osdl.org>

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] properly split capset_check+capset_set

[Stephen Smalley]

On Tue, 2005-01-04 at 11:27, Serge E. Hallyn wrote:
> The attached patch removes checks from kernel/capability.c which are
> redundant with cap_capset_check() code, and moves the capset_check()
> calls to immediately before the capset_set() calls.  This allows
> capset_check() to accurately check the setter's permission to set caps
> on the target.  Please apply.
> 
> thanks,
> -serge
> 
> Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Signed-off-by:  Stephen Smalley <sds@epoch.ncsc.mil>

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] properly split capset_check+capset_set

[Alan Cox]

On Maw, 2005-01-04 at 16:27, Serge E. Hallyn wrote:
> The attached patch removes checks from kernel/capability.c which are
> redundant with cap_capset_check() code, and moves the capset_check()
> calls to immediately before the capset_set() calls.  This allows
> capset_check() to accurately check the setter's permission to set caps
> on the target.  Please apply.

Why does this help ?

A partial failure now returns no error ?


);
> +		while_each_thread(g, target) {
> +			if (!security_capset_check(target, effective,
> +							inheritable,
> +							permitted)) {
> +				security_capset_set(target, effective,
> +							inheritable,
> +							permitted);
> +				ret = 0;
> +			}
> +			found = 1;

Re: [PATCH] properly split capset_check+capset_set

[Chris Wright]

* Alan Cox (alan@lxorguk.ukuu.org.uk) wrote:
> On Maw, 2005-01-04 at 16:27, Serge E. Hallyn wrote:
> > The attached patch removes checks from kernel/capability.c which are
> > redundant with cap_capset_check() code, and moves the capset_check()
> > calls to immediately before the capset_set() calls.  This allows
> > capset_check() to accurately check the setter's permission to set caps
> > on the target.  Please apply.
> 
> Why does this help ?

Without this change, the check was done without knowing who the target
really was, so the code that sets it had to check as well.

> A partial failure now returns no error ?

It never did.  Now it behaves the same as signal delivery which returns
success if any signals were delivered, and failure if none were delivered.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

[PATCH] Properly split capset_check+capset_set

[Serge E. Hallyn]

In kernel/capability.c, capset_check verifies whether or not the
destination process is permitted to undergo the capability change,
while capset_set (1) checks the permission of current to set the
destination process' capability and (2) performs the change.

As Stephen Smalley pointed out, the cap_capset_check code is redundant
with what is hardcoded in kernel/capability.c:sys_capset().  On the
other hand, because the security_capset_set hook is responsible for
doing both an authorization check and doing the actual change,
(particularly, in the case of a cap_set_all or cap_set_pg), when
stacking security modules, the first module may complete the
capset_set before the second module refuses permission.

The attached patch (against 2.6.10-rc3-mm1 w/ ioctl patch) removes the
redundant cap_capset_check hook and moves the security_capset_check
call to just before security_capset_set.  The selinux_capset_set hook
now simply sets the capability (through its secondary), while
selinux_capset_check checks the authorization permission.

thanks,
-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-rc3-mm1/include/linux/security.h
===================================================================
--- linux-2.6.10-rc3-mm1.orig/include/linux/security.h	2004-12-13 12:17:35.000000000 -0600
+++ linux-2.6.10-rc3-mm1/include/linux/security.h	2004-12-13 17:57:41.053436296 -0600
@@ -41,7 +41,6 @@ extern int cap_capable (struct task_stru
 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);
@@ -1943,7 +1942,7 @@ static inline int security_capset_check 
 					 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-mm1/kernel/capability.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/kernel/capability.c	2004-12-13 12:17:36.000000000 -0600
+++ linux-2.6.10-rc3-mm1/kernel/capability.c	2004-12-13 12:18:40.000000000 -0600
@@ -93,8 +93,12 @@ static inline void cap_set_pg(int pgrp, 
 
 	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 @@ static inline void cap_set_all(kernel_ca
      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 @@ asmlinkage long sys_capset(cap_user_head
 
      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 @@ asmlinkage long sys_capset(cap_user_head
              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-mm1/security/capability.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/capability.c	2004-12-13 15:56:16.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/capability.c	2004-12-13 17:57:41.084431584 -0600
@@ -27,7 +27,6 @@
 static struct security_operations capability_ops = {
 	.ptrace =			cap_ptrace,
 	.capget =			cap_capget,
-	.capset_check =			cap_capset_check,
 	.capset_set =			cap_capset_set,
 	.capable =			cap_capable,
 	.settime =			cap_settime,
Index: linux-2.6.10-rc3-mm1/security/commoncap.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/commoncap.c	2004-12-13 12:17:36.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/commoncap.c	2004-12-13 12:18:40.000000000 -0600
@@ -75,32 +75,6 @@ int cap_capget (struct task_struct *targ
 	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)
 {
@@ -406,7 +380,6 @@ EXPORT_SYMBOL(cap_capable);
 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-mm1/security/root_plug.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/root_plug.c	2004-10-18 16:55:28.000000000 -0500
+++ linux-2.6.10-rc3-mm1/security/root_plug.c	2004-12-13 17:57:41.085431432 -0600
@@ -86,7 +86,6 @@ static struct security_operations rootpl
 	/* 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-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/hooks.c	2004-12-13 12:17:36.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/hooks.c	2004-12-13 17:57:38.456831040 -0600
@@ -1393,24 +1393,12 @@ static int selinux_capget(struct task_st
 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);
 }
 

Re: [PATCH] Properly split capset_check+capset_set

[Stephen Smalley]

On Wed, 2004-12-15 at 14:48, Serge E. Hallyn wrote:
> The attached patch (against 2.6.10-rc3-mm1 w/ ioctl patch) removes the
> redundant cap_capset_check hook and moves the security_capset_check
> call to just before security_capset_set.  The selinux_capset_set hook
> now simply sets the capability (through its secondary), while
> selinux_capset_check checks the authorization permission.

One minor complaint:  the caller of capset() will no longer receive any
error code if security_capset_check() fails.  I don't think that it is
necessary to preserve any error return in the cases where capset() is
being applied to multiple processes, but in the case where it is being
applied to a single specific process, it would be nice if any error
return from security_capset_check() would be returned to the caller.

I also wonder whether the existing hardcoded checks should be moved into
the new security_capset_check() hook function for dummy and commoncap so
that they will be applied to the real target, even when pid < 0. 
Otherwise, those hardcoded checks seem bogus in the pid < 0 case, as
they are then applied to current rather than to the true targets.

-- 
Stephen Smalley <sds@epoch.ncsc.mil>
National Security Agency

Re: [PATCH] Properly split capset_check+capset_set

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@epoch.ncsc.mil):
> On Wed, 2004-12-15 at 14:48, Serge E. Hallyn wrote:
> > The attached patch (against 2.6.10-rc3-mm1 w/ ioctl patch) removes the
> > redundant cap_capset_check hook and moves the security_capset_check
> > call to just before security_capset_set.  The selinux_capset_set hook
> > now simply sets the capability (through its secondary), while
> > selinux_capset_check checks the authorization permission.
> 
> One minor complaint:  the caller of capset() will no longer receive any
> error code if security_capset_check() fails.  I don't think that it is

Good point.  I was thinking in terms of cap_capset_check(), and lack of
that permission still returns an error, but selinux_capset_check's
return value in particular is not being reported.

How about the attached patch?

> necessary to preserve any error return in the cases where capset() is
> being applied to multiple processes, but in the case where it is being
> applied to a single specific process, it would be nice if any error
> return from security_capset_check() would be returned to the caller.

In the case of pid<0, would we want to do something like "return an error
if none of the sets was allowed, else return 0", or is that too ugly?

> I also wonder whether the existing hardcoded checks should be moved into
> the new security_capset_check() hook function for dummy and commoncap so
> that they will be applied to the real target, even when pid < 0. 
> Otherwise, those hardcoded checks seem bogus in the pid < 0 case, as
> they are then applied to current rather than to the true targets.

True, this (testing applicability of caps to the real targets in pid<0 case)
certainly seems like a good thing, so the attached patch leaves that
check in cap_capset_check, and just removes it from sys_capset() instead.

thanks,
-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-rc3-mm1/kernel/capability.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/kernel/capability.c	2004-12-15 17:14:39.080628184 -0600
+++ linux-2.6.10-rc3-mm1/kernel/capability.c	2004-12-15 17:23:43.617845944 -0600
@@ -93,8 +93,14 @@ static inline void cap_set_pg(int pgrp, 
 
 	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 +117,9 @@ static inline void cap_set_all(kernel_ca
      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);
 }
@@ -167,24 +176,6 @@ asmlinkage long sys_capset(cap_user_head
      } else
                target = current;
 
-     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;
-
-     /* verify restrictions on target's new Permitted set */
-     if (!cap_issubset(permitted, cap_combine(target->cap_permitted,
-                       current->cap_permitted)))
-             goto out;
-
-     /* verify the _new_Effective_ is a subset of the _new_Permitted_ */
-     if (!cap_issubset(effective, permitted))
-             goto out;
-
      ret = 0;
 
      /* having verified that the proposed changes are legal,
@@ -196,7 +187,11 @@ asmlinkage long sys_capset(cap_user_head
              else            /* all procs in process group */
                      cap_set_pg(-pid, &effective, &inheritable, &permitted);
      } else {
-	     security_capset_set(target, &effective, &inheritable, &permitted);
+	     ret = security_capset_check(target, &effective, &inheritable,
+	     						&permitted);
+	     if (!ret)
+		     security_capset_set(target, &effective, &inheritable,
+		     					&permitted);
      }
 
 out:
Index: linux-2.6.10-rc3-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/hooks.c	2004-12-15 17:14:39.081628032 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/hooks.c	2004-12-15 17:25:55.381814776 -0600
@@ -1405,12 +1405,6 @@ static int selinux_capset_check(struct t
 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);
 }
 

Re: [PATCH] Properly split capset_check+capset_set

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> Quoting Stephen Smalley (sds@epoch.ncsc.mil):
> > necessary to preserve any error return in the cases where capset() is
> > being applied to multiple processes, but in the case where it is being
> > applied to a single specific process, it would be nice if any error
> > return from security_capset_check() would be returned to the caller.
> 
> In the case of pid<0, would we want to do something like "return an error
> if none of the sets was allowed, else return 0", or is that too ugly?

The signal code returns an error if any delivery failed.  So, there's at
least precedence for that type of behaviour.

> > I also wonder whether the existing hardcoded checks should be moved into
> > the new security_capset_check() hook function for dummy and commoncap so
> > that they will be applied to the real target, even when pid < 0. 
> > Otherwise, those hardcoded checks seem bogus in the pid < 0 case, as
> > they are then applied to current rather than to the true targets.
> 
> True, this (testing applicability of caps to the real targets in pid<0 case)
> certainly seems like a good thing, so the attached patch leaves that
> check in cap_capset_check, and just removes it from sys_capset() instead.

Yeah, it is inconsistent right now.  Don't forget, the dummy code doesn't
even allow for capability setting.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] Properly split capset_check+capset_set

[Serge E. Hallyn]

Quoting Chris Wright (chrisw@osdl.org):
> * Serge E. Hallyn (serue@us.ibm.com) wrote:
> > Quoting Stephen Smalley (sds@epoch.ncsc.mil):
> > > necessary to preserve any error return in the cases where capset() is
> > > being applied to multiple processes, but in the case where it is being
> > > applied to a single specific process, it would be nice if any error
> > > return from security_capset_check() would be returned to the caller.
> > 
> > In the case of pid<0, would we want to do something like "return an error
> > if none of the sets was allowed, else return 0", or is that too ugly?
> 
> The signal code returns an error if any delivery failed.  So, there's at
> least precedence for that type of behaviour.

Ok, the attached patch returns an error if none of the capsets succeeded,
else returns 0.  That seems to me to make the most sense from the caller's
point of view.

> > True, this (testing applicability of caps to the real targets in pid<0 case)
> > certainly seems like a good thing, so the attached patch leaves that
> > check in cap_capset_check, and just removes it from sys_capset() instead.
> 
> Yeah, it is inconsistent right now.  Don't forget, the dummy code doesn't
> even allow for capability setting.

So is the attached patch sufficient?  (With it, dummy will always refuse
the capset_set, as it did before)

thanks,
-serge

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

Index: linux-2.6.10-rc3-mm1/kernel/capability.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/kernel/capability.c	2004-12-15 17:14:39.000000000 -0600
+++ linux-2.6.10-rc3-mm1/kernel/capability.c	2004-12-16 11:15:22.699524536 -0600
@@ -85,34 +85,52 @@ out:
  * cap_set_pg - set capabilities for all processes in a given process
  * group.  We call this holding task_capability_lock and tasklist_lock.
  */
-static inline void cap_set_pg(int pgrp, kernel_cap_t *effective,
+static inline int cap_set_pg(int pgrp, kernel_cap_t *effective,
 			      kernel_cap_t *inheritable,
 			      kernel_cap_t *permitted)
 {
 	task_t *g, *target;
+	int ret = -EPERM;
 
 	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);
+				ret = 0;
+			}
+		}
 	} while_each_task_pid(pgrp, PIDTYPE_PGID, g);
+
+	return ret;
 }
 
 /*
  * cap_set_all - set capabilities for all processes other than init
  * and self.  We call this holding task_capability_lock and tasklist_lock.
  */
-static inline void cap_set_all(kernel_cap_t *effective,
+static inline int cap_set_all(kernel_cap_t *effective,
 			       kernel_cap_t *inheritable,
 			       kernel_cap_t *permitted)
 {
      task_t *g, *target;
+     int ret = -EPERM;
 
      do_each_thread(g, target) {
              if (target == current || target->pid == 1)
                      continue;
+	     if (security_capset_check(target, effective, inheritable,
+						permitted))
+		     continue;
+	     ret = 0;
 	     security_capset_set(target, effective, inheritable, permitted);
      } while_each_thread(g, target);
+
+     return ret;
 }
 
 /*
@@ -167,36 +185,23 @@ asmlinkage long sys_capset(cap_user_head
      } else
                target = current;
 
-     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;
-
-     /* verify restrictions on target's new Permitted set */
-     if (!cap_issubset(permitted, cap_combine(target->cap_permitted,
-                       current->cap_permitted)))
-             goto out;
-
-     /* verify the _new_Effective_ is a subset of the _new_Permitted_ */
-     if (!cap_issubset(effective, permitted))
-             goto out;
-
      ret = 0;
 
      /* having verified that the proposed changes are legal,
            we now put them into effect. */
      if (pid < 0) {
              if (pid == -1)  /* all procs other than current and init */
-                     cap_set_all(&effective, &inheritable, &permitted);
+                     ret = cap_set_all(&effective, &inheritable, &permitted);
 
              else            /* all procs in process group */
-                     cap_set_pg(-pid, &effective, &inheritable, &permitted);
+                     ret = cap_set_pg(-pid, &effective, &inheritable,
+		     					&permitted);
      } else {
-	     security_capset_set(target, &effective, &inheritable, &permitted);
+	     ret = security_capset_check(target, &effective, &inheritable,
+	     						&permitted);
+	     if (!ret)
+		     security_capset_set(target, &effective, &inheritable,
+		     					&permitted);
      }
 
 out:
Index: linux-2.6.10-rc3-mm1/security/selinux/hooks.c
===================================================================
--- linux-2.6.10-rc3-mm1.orig/security/selinux/hooks.c	2004-12-15 17:14:39.000000000 -0600
+++ linux-2.6.10-rc3-mm1/security/selinux/hooks.c	2004-12-15 17:25:55.000000000 -0600
@@ -1405,12 +1405,6 @@ static int selinux_capset_check(struct t
 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);
 }
 

Re: [PATCH] Properly split capset_check+capset_set

[Chris Wright]

* Serge E. Hallyn (serue@us.ibm.com) wrote:
> As Stephen Smalley pointed out, the cap_capset_check code is redundant
> with what is hardcoded in kernel/capability.c:sys_capset().  On the
> other hand, because the security_capset_set hook is responsible for
> doing both an authorization check and doing the actual change,
> (particularly, in the case of a cap_set_all or cap_set_pg), when
> stacking security modules, the first module may complete the
> capset_set before the second module refuses permission.

The problem is that the module was (theoretically) allowed to manage
capability bits all on its own.  I think it's a bit of a braindamaged
idea though, and the bits should just stay in the ->cap_* fields.

> The attached patch (against 2.6.10-rc3-mm1 w/ ioctl patch) removes the
> redundant cap_capset_check hook and moves the security_capset_check
> call to just before security_capset_set.  The selinux_capset_set hook
> now simply sets the capability (through its secondary), while
> selinux_capset_check checks the authorization permission.

I think Stephen mentioned this already, but you lose an error now,
where you continue in cap_set_all().

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net