Re: Q: selinux_bprm_committed_creds() && signals/do_wait

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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: [PATCH] ptrace/coredump/exit_group deadlock
    ... by segfaulting the task and triggering a core dump while some other ... stopped task can't wakeup because it has SIGNAL_GROUP_EXIT set, ... in which a SIGKILL is left pending before the thread stops. ... be selected from the queue in preference to any stop signals. ...
    (Linux-Kernel)
  • Re: Signals to cinit
    ... it can mask the next SIGKILL ... Currently, when the global init has the pending SIGKILL, we can't ... We can drop other SIG_DFL signals from the same namespace early as well. ... must know it can't reliably kill cinit with SIGTERM. ...
    (Linux-Kernel)
  • Re: [PATCH] ptrace: simplify ptrace_stop()->sigkill_pending() path
    ... clear was the exact list of problem scenarios you were considering. ... problem scenario is when the SIGKILL was already posted before we took the ... Add a flag meaning "all ... about other fatal signals which cause SIGNAL_GROUP_EXIT? ...
    (Linux-Kernel)
  • Re: Apache hacks (./atac, d0s.txt)
    ... > Looks like it ignores all the `kill` signals, not sure how I can actually ... > kill it... ... SIGKILL cannot be blocked, ... DCC is the ability to ...
    (Bugtraq)
  • Re: COSEINC Linux Advisory #1: Linux Kernel Parent Process Death Signal Vulnerability
    ... it prevents a non-malicious caller from using PDEATHSIG ... Bypass permission checks for sending signals ). ... The ability to send signals to a process is subject to security ... any bug which allows these restrictions to be ...
    (Bugtraq)