Re: [PATCH] file capabilities: allow sigcont within session (v2)

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@xxxxxxxxxxxxxx):
> On Wed, 2007-10-31 at 18:49 -0500, Serge E. Hallyn wrote:
> > >From 5bff8967f45a35f858b96ca673d9bf98eac53d49 Mon Sep 17 00:00:00 2001
> > From: Serge E. Hallyn <serue@xxxxxxxxxx>
> > Date: Wed, 31 Oct 2007 11:22:04 -0500
> > Subject: [PATCH 1/1] file capabilities: allow sigcont within session (v2)
> > 
> > (This is a proposed fix to http://bugzilla.kernel.org/show_bug.cgi?id=9247)
> > 
> > Allow sigcont to be sent to a process with greater capabilities
> > if it is in the same session.  Otherwise, a shell from which
> > I've started a root shell and done 'suspend' can't be restarted
> > by the parent shell.
> > 
> > Also don't do file-capabilities signaling checks when uids for
> > the processes don't match, since the standard check_kill_permission
> > will have done those checks.
> 
> Description doesn't match the code.

Egads.  I knew I should've just kept that part out of it for the first
patch...

New patch on top of previous one is appended.

Thanks.

> And in the non-matching uid case,
> check_kill_permission typically returns an error before it reaches
> cap_task_kill (modulo the special cases).

Typically, but when it doesn't, then the file capabilities shouldn't get
in the way of check_kill_permission() granting permission.  The file
capabilities 

> 
> > 
> > Signed-off-by: Serge E. Hallyn <serue@xxxxxxxxxx>
> > ---
> >  security/commoncap.c |    9 +++++++++
> >  1 files changed, 9 insertions(+), 0 deletions(-)
> > 
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index bf67871..4de6857 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -526,6 +526,15 @@ int cap_task_kill(struct task_struct *p, struct siginfo *info,
> >  	if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
> >  		return 0;
> >  
> > +	/* if tasks have same uid, then check_kill_permission did check */
> > +	if (current->uid == p->uid || current->euid == p->uid ||
> > +		current->uid == p->suid || current->euid == p->suid)
> > +		return 0;
> 
> I'm confused - if you are allowing all signals within the same uid, then

No I was confused.  I wanted to allow for tasks with different uids.

But in fact that's not safe anyway.  A binary can be setuid and owned by
a non-root user user1, have file capabilities, and be executed by user2.

(Anyway given how grossly my code missed my erroneous intentions, I need
to add some signal tests to my file capabilities tests - and get those
tests into LTP)

> what was the point of having a cap_task_kill at all?  cap_task_kill was
> supposed to prevent a process with lesser capabilities from killing a
> process with more capabilities, even if they have the same uid, so that
> when you have a program marked with file capabilities instead of a
> setuid-0 program, that program can't be sent arbitrary signals by the
> caller.
> 
> > +
> > +	/* 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.
> -- 
> Stephen Smalley
> National Security Agency

Thanks, Stephen.