linux-kernel - Re: Q: selinux_bprm_committed_creds() && signals/do

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 29 Apr 2009 08:58:09 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	James Morris <jmorris@...ei.org>
Cc:	David Howells <dhowells@...hat.com>,
	Eric Paris <eparis@...isplace.org>,
	Roland McGrath <roland@...hat.com>,
	Stephen Smalley <sds@...ho.nsa.gov>,
	linux-kernel@...r.kernel.org
Subject: Re: Q: selinux_bprm_committed_creds() && signals/do_wait

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?

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.

--
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/