Subject: Re: Q: selinux_bprm_committed_creds() && signals/do_wait
From: Stephen Smalley <sds@tycho.nsa.gov>
To: Oleg Nesterov <oleg@redhat.com>
Cc: James Morris <jmorris@namei.org>, David Howells <dhowells@redhat.com>,
       Eric Paris <eparis@parisplace.org>, Roland McGrath <roland@redhat.com>,
       linux-kernel@vger.kernel.org
In-Reply-To: <20090429065809.GA477@redhat.com>
References: <20090428223025.GA11997@redhat.com>
	 <alpine.LRH.2.00.0904291024001.5246@tundra.namei.org>
	 <20090429065809.GA477@redhat.com>
Content-Type: text/plain
Organization: National Security Agency
Date: Wed, 29 Apr 2009 08:20:30 -0400
Message-Id: <1241007630.18249.141.camel@localhost.localdomain>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2737
Lines: 73

On Wed, 2009-04-29 at 08:58 +0200, Oleg Nesterov wrote:
> On 04/29, James Morris wrote:
> >
> > On Wed, 29 Apr 2009, Oleg Nesterov wrote:
> >
> > > selinux_bprm_committed_creds:
> > >
> > > 	rc = avc_has_perm()
> > > 	if (rc) {
> > > 		flush_signals(current);
> > >
> > > This doesn't look right. If the task was SIGKILL'ed we must not proceed,
> > > the task should die. The fix is simple, we should check SIGNAL_GROUP_EXIT
> > > and do nothing in this case, the task will exit before return to user
> > > space. If SIGNAL_GROUP_EXIT is set, it is just wrong to drop SIGKILL and
> > > continue.
> >
> > I'm not quite sure what you're asking.  This is a permission check to see
> > if the new task can inherit the signal state of the parent,
> 
> we can flush the signal which was sent after we changed SID/cred and passed
> the new permission checks,
> 
> > and if not,
> > the new task's signal state is flushed.
> >
> > Where does a consideration of SIGKILL arise?
> 
> It is not possible to flush SIGKILL. Once SIGKILL (or another fatal signal)
> is queued, it sets SIGNAL_GROUP_EXIT which can't be and must not be cleared.
> 
> But, there is no need to flush SIGKILL. The task will exit. If it was sent
> before we changed SID, we can pretend the task has died before exec().
> 
> > > But, before fixing, I'd like to understand why we are doing
> > >
> > > 		flush_signal_handlers(current, 1);
> > > 		sigemptyset(&current->blocked);
> > >
> > > later. Could someone explain ? This looks unneeded.
> >
> > This is part of clearing all the signal state in the child.
> 
> This doesn't explain why we are doing this ;)
> 
> Why do we need to s/IGN/DFL/ and why do we clear ->blocked ? How this can
> help from the security pov?

We don't want the caller to be able to arrange conditions that prevent
correct handling of signals (e.g. SIGHUP) by the callee.  That was
motivated by a specific attack against newrole, but was a general issue
for any program that runs in a more trusted domain than its caller.

As I recall, I based the logic in part on existing logic in
call_usermodehelper().

> In fact this looks a bit wrong. The only way to ensure we can't lose the
> signal during exec() is to block it beforehand, then install the handler
> after exec(). s/IGN/DFL/ doesn't look good too.
> 
> But, if we really need this for security (selinux is a black magic to me),
> then the above doesn't matter. Please help to understand.
> 
> Oleg.
-- 
Stephen Smalley
National Security Agency

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/