On Sun, Dec 01, 2019 at 03:24:11AM +0300, Dmitry Marakasov
wrote:> * Konstantin Belousov (kostikbel at gmail.com) wrote:
>
> > > > I'm helping to investigate some userspace issue [1],
where kill(-1, SIGKILL)
> > > > fails with EPERM. I've managed to isolate this case in a
small program:
> > > >
> > > >
> > > > ```
> > > > #include <err.h>
> > > > #include <errno.h>
> > > > #include <signal.h>
> > > > #include <stdio.h>
> > > > #include <string.h>
> > > > #include <unistd.h>
> > > >
> > > > int main() {
> > > > if (setuid(66) == -1) // uucp, just for the test
> > > > err(1, "setuid");
> > > >
> > > > int res = kill(-1, 0); // <- fails with EPERM
> > > > fprintf(stderr, "kill(-1, 0) result=%d,
errno=%s\n", res, strerror(errno));
> > > >
> > > > return 0;
> > > > }
> > > > ```
> > > >
> > > > when run from root on 12.1 kill call fails with EPERM.
However I cannot
> > > > comprehend what it is caused by and how it's even
possible: kill(2) manpage
> > > > says that with pid=-1 kill should only send (and in this
case of sig=0,
> > > > /not/ send) signals to the processes belonging to the
current uid, so there
> > > > should be no permission problems. I've also looked into
the kernel code
> > > > (sys_kill, killpg1), and it matches to what manpage says, I
see no way
> > > > for it to return EPERM: sys_kill() should fall through to
the switch, call
> > > > killpg1() with all=1 and killpg1() if(all) branch may only
set `ret` to
> > > > either 0 or ESRCH. Am I missing something, or is there a
problem somewhere?
> > >
> > > It looks like I have misread the `else if' path of this core.
> > >
> > > if (all) {
> > > /*
> > > * broadcast
> > > */
> > > sx_slock(&allproc_lock);
> > > FOREACH_PROC_IN_SYSTEM(p) {
> > > if (p->p_pid <= 1 || p->p_flag &
P_SYSTEM ||
> > > p == td->td_proc || p->p_state == PRS_NEW)
{
> > > continue;
> > > }
> > > PROC_LOCK(p);
> > > err = p_cansignal(td, p, sig);
> > > if (err == 0) {
> > > if (sig)
> > > pksignal(p, sig, ksi);
> > > ret = err;
> > > }
> > > else if (ret == ESRCH)
> > > ret = err;
> > > PROC_UNLOCK(p);
> > > }
> > > sx_sunlock(&allproc_lock);
> > > } ...
> > >
> > > so it's clear now where EPERM comes from. However it looks
like the
> > > behavior contradicts the manpage - there are no signs of check
that
> > > the signalled process has the same uid as the caller.
> >
> > I am not sure what you mean by 'signs of check'. Look at
p_cansignal()
> > and cr_cansignal() implementation.
>
> I've meant that according to the manpage
>
> If pid is -1:
> If the user has super-user privileges, the signal is sent to
all
> processes excluding system processes (with P_SYSTEM flag set),
> process with ID 1 (usually init(8)), and the process sending
the
> signal. If the user is not the super user, the signal is sent
to
> all processes with the same uid as the user excluding the
process
> sending the signal. No error is returned if any process could
be
> signaled.
>
> IMO there should be an additional check in this condition:
>
> if (p->p_pid <= 1 || p->p_flag & P_SYSTEM ||
> p == td->td_proc || p->p_state == PRS_NEW) {
> continue;
> }
>
> E.g. something like
>
> if (p->p_pid <= 1 || p->p_flag & P_SYSTEM ||
> p == td->td_proc || p->p_state == PRS_NEW ||
> (td->td_ucred->cr_ruid != 0 &&
> p->td_ucred->cr_ruid != td->td_ucred->cr_ruid) {
> continue;
> }
>
> e.g. it should not even attempt to signal processes with other uids.
Why ? You are trying to outguess p_cansignal(), which could deny
action for much more reasons, so you would get EPERM still, e.g. if the
target is suid. Or, p_cansignal() also might allow to send the signal
even for mismatched uids, again look at it code.
I might guess that your complain is really about a different aspect
of it. If you look at the posix description of the EPERM error from
kill(2) (really kill(3)), it says
[EPERM] The process does not have permission to send the signal to
any receiving process.
In other words, we should not return EPERM if we signalled at least one
of the process.
Is this the problem ?