Re: [RFC][PATCH 1/2 v2] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[Kees Cook]

On Fri, Jul 15, 2016 at 11:42 AM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> On Fri, Jul 15, 2016 at 10:51 AM, Nick Kralevich <nnk@xxxxxxxxxx> wrote:
>> On Fri, Jul 15, 2016 at 10:24 AM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
>>> +       if (!capable(CAP_SYS_NICE))
>>> +               return -EPERM;
>>> +
>>
>> Since you're going the LSM route (from your second patch of this
>
> Well, you suggested it, so I sent out an RFC. I'm not married to it yet. :)
>
>
>> series), the capability check above should be moved to the LSM hook in
>> security/commoncap.c.  Only one security call to
>> security_task_settimerslack is needed, which will cover the standard
>> capabilities check as well as the SELinux check.
>
> Huh. Ok. I was looking at the implementation of nice(), which does:
>
>  if (increment < 0 && !can_nice(current, nice))
>                 return -EPERM;
> retval = security_task_setnice(current, nice);
> if (retval)
> ...
>
> Which made it seem like standard checks are done first, then finer
> grain lsm checks second.

I'm on the fence about this: it can be argued that if it's a cap check
it should live in the commoncap.c checks, but most of our cap checks
for these kinds of access controls are directly in the function, prior
the the security_* calls. I've added James and Casey who may have a
more well constructed rationale for doing this one way or the other.

> (...and now you can guess where my accidental "current" usage in the
> next patch came from :)
>
>
>>
>>>         p = get_proc_task(inode);
>>>         if (!p)
>>>                 return -ESRCH;
>>>
>>
>> Per your patch #2, you'd call security_task_settimerslack here. This
>> would call into the capability LSM hook you added in
>> security/commoncap.c
>
> Though I was hoping to keep the CAP_SYS_PTRACE -> CAP_SYS_NICE change
> first, then add the LSM hooks, as it makes the needed ABI change more
> obvious. I worry swapping it around with the LSM hook being added
> first makes it significantly less obvious, as (at least for me) the
> security_task_* functions get indirect and difficult to follow quickly
> ("wait, why are we checking SETSCHED for nice?").
>
> A side curiosity: why does "git grep PROCESS__SETSCHED" miss the
> definition? Is the av_permissions.h file somehow caught by .gitignore?
>
> thanks
> -john

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security