Messages in this thread Patch in this message | | | Date | Wed, 15 Dec 2004 17:22:22 -0600 | From | "Serge E. Hallyn" <> | Subject | Re: [PATCH] Properly split capset_check+capset_set |
| |
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); } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |