[PATCH 1/3] signals: cleanup security_task_kill() usage/implementation

[PATCH 1/3] signals: cleanup security_task_kill() usage/implementation

[Oleg Nesterov]

Every implementation of ->task_kill() does nothing when the signal comes from
the kernel. This is correct, but means that check_kill_permission() should call
security_task_kill() only for SI_FROMUSER() case, and we can remove the same
check from ->task_kill() implementations.

(sadly, check_kill_permission() is the last user of signal->session/__session
 but we can't s/task_session_nr/task_session/ here).

NOTE: Eric W. Biederman pointed out cap_task_kill() should die, and I think he
is very right.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

 kernel/signal.c            |   27 ++++++++++++++-------------
 security/commoncap.c       |    3 ---
 security/selinux/hooks.c   |    3 ---
 security/smack/smack_lsm.c |    9 ---------
 4 files changed, 14 insertions(+), 28 deletions(-)

--- 25/kernel/signal.c~1_LSM_KILL	2008-02-25 18:12:57.000000000 +0300
+++ 25/kernel/signal.c	2008-02-25 18:15:38.000000000 +0300
@@ -526,22 +526,23 @@ static int rm_from_queue(unsigned long m
 static int check_kill_permission(int sig, struct siginfo *info,
 				 struct task_struct *t)
 {
-	int error = -EINVAL;
+	int error;
+
 	if (!valid_signal(sig))
-		return error;
+		return -EINVAL;
 
-	if (info == SEND_SIG_NOINFO || (!is_si_special(info) && SI_FROMUSER(info))) {
-		error = audit_signal_info(sig, t); /* Let audit system see the signal */
-		if (error)
-			return error;
-		error = -EPERM;
-		if (((sig != SIGCONT) ||
-			(task_session_nr(current) != task_session_nr(t)))
-		    && (current->euid ^ t->suid) && (current->euid ^ t->uid)
-		    && (current->uid ^ t->suid) && (current->uid ^ t->uid)
-		    && !capable(CAP_KILL))
+	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
+		return 0;
+
+	error = audit_signal_info(sig, t); /* Let audit system see the signal */
+	if (error)
 		return error;
-	}
+
+	if (((sig != SIGCONT) || (task_session_nr(current) != task_session_nr(t)))
+	    && (current->euid ^ t->suid) && (current->euid ^ t->uid)
+	    && (current->uid ^ t->suid) && (current->uid ^ t->uid)
+	    && !capable(CAP_KILL))
+		return -EPERM;
 
 	return security_task_kill(t, info, sig, 0);
 }
--- 25/security/commoncap.c~1_LSM_KILL	2008-02-25 18:07:54.000000000 +0300
+++ 25/security/commoncap.c	2008-02-25 18:52:16.000000000 +0300
@@ -543,9 +543,6 @@ int cap_task_setnice (struct task_struct
 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
--- 25/security/smack/smack_lsm.c~1_LSM_KILL	2008-02-15 16:59:20.000000000 +0300
+++ 25/security/smack/smack_lsm.c	2008-02-25 18:54:01.000000000 +0300
@@ -1094,15 +1094,6 @@ static int smack_task_kill(struct task_s
 			   int sig, u32 secid)
 {
 	/*
-	 * Special cases where signals really ought to go through
-	 * in spite of policy. Stephen Smalley suggests it may
-	 * make sense to change the caller so that it doesn't
-	 * bother with the LSM hook in these cases.
-	 */
-	if (info != SEND_SIG_NOINFO &&
-	    (is_si_special(info) || SI_FROMKERNEL(info)))
-		return 0;
-	/*
 	 * Sending a signal requires that the sender
 	 * can write the receiver.
 	 */
--- 25/security/selinux/hooks.c~1_LSM_KILL	2008-02-15 16:59:20.000000000 +0300
+++ 25/security/selinux/hooks.c	2008-02-25 18:57:23.000000000 +0300
@@ -3194,9 +3194,6 @@ static int selinux_task_kill(struct task
 	if (rc)
 		return rc;
 
-	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
-		return 0;
-
 	if (!sig)
 		perm = PROCESS__SIGNULL; /* null signal; existence test */
 	else

Re: [PATCH 1/3] signals: cleanup security_task_kill() usage/implementation

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@tv-sign.ru):
> Every implementation of ->task_kill() does nothing when the signal comes from
> the kernel. This is correct, but means that check_kill_permission() should call
> security_task_kill() only for SI_FROMUSER() case, and we can remove the same
> check from ->task_kill() implementations.
> 
> (sadly, check_kill_permission() is the last user of signal->session/__session
>  but we can't s/task_session_nr/task_session/ here).
> 
> NOTE: Eric W. Biederman pointed out cap_task_kill() should die, and I think he
> is very right.

And at this point I agree.  We started out with a pretty strict version
designed to prevent unprivileged code from signaling code owned by the
same user but with more privileged.  We've at this point reduced the
check twice to prevent regressions and I think where we're at right now
is that it's meaningless.

Does anyone object to removing cap_task_kill() altogether?

(sigh)

-serge

> Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
> 
>  kernel/signal.c            |   27 ++++++++++++++-------------
>  security/commoncap.c       |    3 ---
>  security/selinux/hooks.c   |    3 ---
>  security/smack/smack_lsm.c |    9 ---------
>  4 files changed, 14 insertions(+), 28 deletions(-)
> 
> --- 25/kernel/signal.c~1_LSM_KILL	2008-02-25 18:12:57.000000000 +0300
> +++ 25/kernel/signal.c	2008-02-25 18:15:38.000000000 +0300
> @@ -526,22 +526,23 @@ static int rm_from_queue(unsigned long m
>  static int check_kill_permission(int sig, struct siginfo *info,
>  				 struct task_struct *t)
>  {
> -	int error = -EINVAL;
> +	int error;
> +
>  	if (!valid_signal(sig))
> -		return error;
> +		return -EINVAL;
> 
> -	if (info == SEND_SIG_NOINFO || (!is_si_special(info) && SI_FROMUSER(info))) {
> -		error = audit_signal_info(sig, t); /* Let audit system see the signal */
> -		if (error)
> -			return error;
> -		error = -EPERM;
> -		if (((sig != SIGCONT) ||
> -			(task_session_nr(current) != task_session_nr(t)))
> -		    && (current->euid ^ t->suid) && (current->euid ^ t->uid)
> -		    && (current->uid ^ t->suid) && (current->uid ^ t->uid)
> -		    && !capable(CAP_KILL))
> +	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
> +		return 0;
> +
> +	error = audit_signal_info(sig, t); /* Let audit system see the signal */
> +	if (error)
>  		return error;
> -	}
> +
> +	if (((sig != SIGCONT) || (task_session_nr(current) != task_session_nr(t)))
> +	    && (current->euid ^ t->suid) && (current->euid ^ t->uid)
> +	    && (current->uid ^ t->suid) && (current->uid ^ t->uid)
> +	    && !capable(CAP_KILL))
> +		return -EPERM;
> 
>  	return security_task_kill(t, info, sig, 0);
>  }
> --- 25/security/commoncap.c~1_LSM_KILL	2008-02-25 18:07:54.000000000 +0300
> +++ 25/security/commoncap.c	2008-02-25 18:52:16.000000000 +0300
> @@ -543,9 +543,6 @@ int cap_task_setnice (struct task_struct
>  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
> --- 25/security/smack/smack_lsm.c~1_LSM_KILL	2008-02-15 16:59:20.000000000 +0300
> +++ 25/security/smack/smack_lsm.c	2008-02-25 18:54:01.000000000 +0300
> @@ -1094,15 +1094,6 @@ static int smack_task_kill(struct task_s
>  			   int sig, u32 secid)
>  {
>  	/*
> -	 * Special cases where signals really ought to go through
> -	 * in spite of policy. Stephen Smalley suggests it may
> -	 * make sense to change the caller so that it doesn't
> -	 * bother with the LSM hook in these cases.
> -	 */
> -	if (info != SEND_SIG_NOINFO &&
> -	    (is_si_special(info) || SI_FROMKERNEL(info)))
> -		return 0;
> -	/*
>  	 * Sending a signal requires that the sender
>  	 * can write the receiver.
>  	 */
> --- 25/security/selinux/hooks.c~1_LSM_KILL	2008-02-15 16:59:20.000000000 +0300
> +++ 25/security/selinux/hooks.c	2008-02-25 18:57:23.000000000 +0300
> @@ -3194,9 +3194,6 @@ static int selinux_task_kill(struct task
>  	if (rc)
>  		return rc;
> 
> -	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
> -		return 0;
> -
>  	if (!sig)
>  		perm = PROCESS__SIGNULL; /* null signal; existence test */
>  	else