Re: Fw: [PATCH 1/1] file capabilities: simplify signal check

[Eric W. Biederman]

Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> writes:

> um, is that code namespace-clean?

Choke, gag.

There are uid namespace issues but since no one has finished the
uid namespace that I am aware of that is minor.

However the code does not appear clean/maintainable.  The normal linux
signal sending policy has already been enforce before we get to this
point.

So unless I am totally mistaken the code should read:

int cap_task_kill(struct task_struct *p, struct siginfo *info,
                                int sig, u32 secid)
{
        if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
                return 0;

        if (!cap_issubset(p->cap_permitted, current->cap_permitted))
                return -EPERM;

	return 0;
}

Although doing it that way violates:
       /*
         * Running a setuid root program raises your capabilities.
         * Killing your own setuid root processes was previously
         * allowed.
         * We must preserve legacy signal behavior in this case.
         */


Which says to me the code should really read:
int cap_task_kill(struct task_struct *p, struct siginfo *info,
                                int sig, u32 secid)
{
	return 0;
}

The entire point of defining cap_task_kill under
CONFIG_SECURITY_FILE_CAPABLITIES appears to be deny killing processes
with more caps.  Killing processes that we could ordinarily kill 
which have more caps appears to be precisely the case we have decided
to allow.  So the patched version of cap_task_kill appears to be an
expensive way of doing nothing, just waiting for someone to complain
about the last couple of cases it denies until it is truly a noop.



> Thanks.
>
> Begin forwarded message:
>
> Date: Wed, 20 Feb 2008 10:15:50 -0600
> From: "Serge E. Hallyn" <serue@xxxxxxxxxx>
> To: lkml <linux-kernel@xxxxxxxxxxxxxxx>
> Subject: [PATCH 1/1] file capabilities: simplify signal check
>
>
>>From bd076c7245d02be0cc01b7c09bd7170ec5946492 Mon Sep 17 00:00:00 2001
> From: Serge E. Hallyn <serue@xxxxxxxxxx>
> Date: Sun, 17 Feb 2008 20:28:07 -0500
> Subject: [PATCH 1/1] file capabilities: simplify signal check
>
> Simplify the uid equivalence check in cap_task_kill().  Anyone
> can kill a process owned by the same uid.
>
> Without this patch wireshark is reported to fail.
>
> Signed-off-by: Serge E. Hallyn <serue@xxxxxxxxxx>
> Signed-off-by: Andrew G. Morgan <morgan@xxxxxxxxxx>
> ---
>  security/commoncap.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 5aba826..bb0c095 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -552,7 +552,7 @@ int cap_task_kill(struct task_struct *p, struct siginfo
> *info,
>  	 * allowed.
>  	 * We must preserve legacy signal behavior in this case.
>  	 */
> -	if (p->euid == 0 && p->uid == current->uid)
> +	if (p->uid == current->uid)
>  		return 0;
>  
>  	/* sigcont is permitted within same session */
> -- 
> 1.5.1.1.GIT

So it looks to me like we should just give up trying to deny a few
cases now.

diff --git a/security/commoncap.c b/security/commoncap.c
index 5aba826..c1d1fd7 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -540,41 +540,6 @@ int cap_task_setnice (struct task_struct *p, int nice)
 	return cap_safe_nice(p);
 }
 
-int cap_task_kill(struct task_struct *p, struct siginfo *info,
-				int sig, u32 secid)
-{
-	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
-		return 0;
-
-	/*
-	 * Running a setuid root program raises your capabilities.
-	 * Killing your own setuid root processes was previously
-	 * allowed.
-	 * We must preserve legacy signal behavior in this case.
-	 */
-	if (p->euid == 0 && p->uid == current->uid)
-		return 0;
-
-	/* sigcont is permitted within same session */
-	if (sig == SIGCONT && (task_session_nr(current) == task_session_nr(p)))
-		return 0;
-
-	if (secid)
-		/*
-		 * Signal sent as a particular user.
-		 * Capabilities are ignored.  May be wrong, but it's the
-		 * only thing we can do at the moment.
-		 * Used only by usb drivers?
-		 */
-		return 0;
-	if (cap_issubset(p->cap_permitted, current->cap_permitted))
-		return 0;
-	if (capable(CAP_KILL))
-		return 0;
-
-	return -EPERM;
-}
-
 /*
  * called from kernel/sys.c for prctl(PR_CABSET_DROP)
  * done without task_capability_lock() because it introduces
@@ -605,13 +570,13 @@ int cap_task_setnice (struct task_struct *p, int nice)
 {
 	return 0;
 }
+#endif
+
 int cap_task_kill(struct task_struct *p, struct siginfo *info,
 				int sig, u32 secid)
 {
 	return 0;
 }
-#endif
-
 void cap_task_reparent_to_init (struct task_struct *p)
 {
 	cap_set_init_eff(p->cap_effective);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/