Re: Fw: [PATCH 1/1] file capabilities: simplify signal check

[Oleg Nesterov <oleg@tv-sign.ru>]

On 02/23, Eric W. Biederman wrote:
>
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > um, is that code namespace-clean?
> 
> Choke, gag.
> 
> There are uid namespace issues but since no one has finished the
> uid namespace that I am aware of that is minor.
> 
> However the code does not appear clean/maintainable.  The normal linux
> signal sending policy has already been enforce before we get to this
> point.
> 
> So unless I am totally mistaken the code should read:
> 
> int cap_task_kill(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid)
> {
>         if (info != SEND_SIG_NOINFO && (is_si_special(info) || SI_FROMKERNEL(info)))
>                 return 0;
> 
>         if (!cap_issubset(p->cap_permitted, current->cap_permitted))
>                 return -EPERM;
> 
> 	return 0;
> }
> 
> Although doing it that way violates:
>        /*
>          * Running a setuid root program raises your capabilities.
>          * Killing your own setuid root processes was previously
>          * allowed.
>          * We must preserve legacy signal behavior in this case.
>          */
> 
> 
> Which says to me the code should really read:
> int cap_task_kill(struct task_struct *p, struct siginfo *info,
>                                 int sig, u32 secid)
> {
> 	return 0;
> }
> 
> The entire point of defining cap_task_kill under
> CONFIG_SECURITY_FILE_CAPABLITIES appears to be deny killing processes
> with more caps.  Killing processes that we could ordinarily kill 
> which have more caps appears to be precisely the case we have decided
> to allow.  So the patched version of cap_task_kill appears to be an
> expensive way of doing nothing, just waiting for someone to complain
> about the last couple of cases it denies until it is truly a noop.

(Can't comment, I never understood this security magic, but Eric's
 explanation looks very reasonable to me).


I just have an almost off-topic (sorry ;) question. Do we really need
kill_pid_info_as_uid() ? Harald Welte cc'ed.

>From "[PATCH] Fix signal sending in usbdevio on async URB completion"
commit 46113830a18847cff8da73005e57bc49c2f95a56

	> If a process issues an URB from userspace and (starts to) terminate
	> before the URB comes back, we run into the issue described above.  This
	> is because the urb saves a pointer to "current" when it is posted to the
	> device, but there's no guarantee that this pointer is still valid
	> afterwards.
	>
	> In fact, there are three separate issues:
	>
	> 1) the pointer to "current" can become invalid, since the task could be
	>    completely gone when the URB completion comes back from the device.
	>
	> 2) Even if the saved task pointer is still pointing to a valid task_struct,
	>    task_struct->sighand could have gone meanwhile.
	>
	> 3) Even if the process is perfectly fine, permissions may have changed,
	>    and we can no longer send it a signal.

The problems 1) and 2) are solved by converting to a struct pid. Is 3) a real
problem? The task which does ioctl(USBDEVFS_SUBMITURB) explicitly asks to send
the signal to it, should we deny the signal even if it changes its credentials
in some way?

Just curious. Thanks,

Oleg.