On 02/23, Eric W. Biederman wrote:
>
> Andrew Morton <[EMAIL PROTECTED]> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
