LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/3] signals: cleanup security_task_kill() usage/implementation
@ 2008-02-25 17:42 Oleg Nesterov
  2008-02-25 18:15 ` Serge E. Hallyn
  0 siblings, 1 reply; 2+ messages in thread
From: Oleg Nesterov @ 2008-02-25 17:42 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Casey Schaufler, David Quigley, Eric W. Biederman, Eric Paris,
	Harald Welte, Pavel Emelyanov, Serge E. Hallyn, Stephen Smalley,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH 1/3] signals: cleanup security_task_kill() usage/implementation
  2008-02-25 17:42 [PATCH 1/3] signals: cleanup security_task_kill() usage/implementation Oleg Nesterov
@ 2008-02-25 18:15 ` Serge E. Hallyn
  0 siblings, 0 replies; 2+ messages in thread
From: Serge E. Hallyn @ 2008-02-25 18:15 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Casey Schaufler, David Quigley, Eric W. Biederman,
	Eric Paris, Harald Welte, Pavel Emelyanov, Serge E. Hallyn,
	Stephen Smalley, linux-kernel

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2008-02-25 18:16 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-25 17:42 [PATCH 1/3] signals: cleanup security_task_kill() usage/implementation Oleg Nesterov
2008-02-25 18:15 ` Serge E. Hallyn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).