2005-01-04 16:33:20

by Serge E. Hallyn

[permalink] [raw]
Subject: [PATCH] properly split capset_check+capset_set

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 <[email protected]>

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);
}


2005-01-04 20:15:58

by Chris Wright

[permalink] [raw]
Subject: Re: [PATCH] properly split capset_check+capset_set

* Serge E. Hallyn ([email protected]) 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 <[email protected]>

Signed-off-by: Chris Wright <[email protected]>

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

2005-01-04 22:09:49

by Stephen Smalley

[permalink] [raw]
Subject: Re: [PATCH] properly split capset_check+capset_set

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 <[email protected]>

Signed-off-by: Stephen Smalley <[email protected]>

--
Stephen Smalley <[email protected]>
National Security Agency

2005-01-04 23:46:40

by Chris Wright

[permalink] [raw]
Subject: Re: [PATCH] properly split capset_check+capset_set

* Alan Cox ([email protected]) 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

2005-01-04 23:33:06

by Alan

[permalink] [raw]
Subject: Re: [PATCH] properly split capset_check+capset_set

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;