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

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@xxxxxxxxxx):
> 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@xxxxxxxxxx>
> 
>  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
--
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/