linux-kernel - Re: [PATCH 1/3] signals: cleanup security_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20080225181542.GA10896@sergelap.austin.ibm.com>
Date:	Mon, 25 Feb 2008 12:15:42 -0600
From:	"Serge E. Hallyn" <serue@...ibm.com>
To:	Oleg Nesterov <oleg@...sign.ru>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Casey Schaufler <casey@...aufler-ca.com>,
	David Quigley <dpquigl@...ho.nsa.gov>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Eric Paris <eparis@...hat.com>,
	Harald Welte <laforge@...monks.org>,
	Pavel Emelyanov <xemul@...nvz.org>,
	"Serge E. Hallyn" <serue@...ibm.com>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] signals: cleanup security_task_kill()
	usage/implementation

Quoting Oleg Nesterov (oleg@...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@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/