[RFC][PATCH] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[RFC][PATCH] proc: Relax /proc/<tid>/timerslack_ns capability requirements

[John Stultz]

When an interface to allow a task to change another tasks
timerslack was first proposed, it was suggested that something
greater then CAP_SYS_NICE would be needed, as a task could be
delayed further then what normally could be done with nice
adjustments.

So CAP_SYS_PTRACE was adopted instead for what became the
/proc/<tid>/timerslack_ns interface. However, for Android (where
this feature originates), giving the system_server
CAP_SYS_PTRACE would allow it to observe and modify all tasks
memory. This is considered too high a privilege level for only
needing to change the timerslack.

After some discussion, it was realized that a CAP_SYS_NICE
process can set a task as SCHED_FIFO, so they could fork some
spinning processes and set them all SCHED_FIFO 99, in effect
delaying all other tasks for an infinite amount of time.

So as a CAP_SYS_NICE task can already cause trouble for other
tasks, using it as a required capability for accessing and
modifying /proc/<tid>/timerslack_ns seems sufficient.

Thus, this patch loosens the capability requirements to
CAP_SYS_NICE.

For ABI preservation, it still allows CAP_SYS_PTRACE tasks to
access/modify timerslack values, but I'm fine with removing
this if others agree.

Cc: Kees Cook <keescook@chromium.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
CC: Arjan van de Ven <arjan@linux.intel.com>
Cc: Oren Laadan <orenl@cellrox.com>
Cc: Ruchi Kandoi <kandoiruchi@google.com>
Cc: Rom Lemarchand <romlem@android.com>
Cc: Todd Kjos <tkjos@google.com>
Cc: Colin Cross <ccross@android.com>
Cc: Nick Kralevich <nnk@google.com>
Cc: Dmitry Shmidt <dimitrysh@google.com>
Cc: Elliott Hughes <enh@google.com>
Cc: Android Kernel Team <kernel-team@android.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 fs/proc/base.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index a11eb71..d32033e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2281,7 +2281,8 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
 	if (!p)
 		return -ESRCH;
 
-	if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
+	if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
+	    capable(CAP_SYS_NICE)) {
 		task_lock(p);
 		if (slack_ns == 0)
 			p->timer_slack_ns = p->default_timer_slack_ns;
@@ -2306,7 +2307,8 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
 	if (!p)
 		return -ESRCH;
 
-	if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
+	if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
+	    capable(CAP_SYS_NICE)) {
 		task_lock(p);
 		seq_printf(m, "%llu\n", p->timer_slack_ns);
 		task_unlock(p);
-- 
1.9.1

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

[Kees Cook]

On Thu, Jul 14, 2016 at 11:50 AM, John Stultz <john.stultz@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc/<tid>/timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc/<tid>/timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE.
>
> For ABI preservation, it still allows CAP_SYS_PTRACE tasks to
> access/modify timerslack values, but I'm fine with removing
> this if others agree.

Is anything actually using this ABI yet? (Regardless, I'm fine
allowing both caps.)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> CC: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Oren Laadan <orenl@cellrox.com>
> Cc: Ruchi Kandoi <kandoiruchi@google.com>
> Cc: Rom Lemarchand <romlem@android.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Nick Kralevich <nnk@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Elliott Hughes <enh@google.com>
> Cc: Android Kernel Team <kernel-team@android.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  fs/proc/base.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..d32033e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2281,7 +2281,8 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>         if (!p)
>                 return -ESRCH;
>
> -       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +           capable(CAP_SYS_NICE)) {
>                 task_lock(p);
>                 if (slack_ns == 0)
>                         p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2306,7 +2307,8 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>         if (!p)
>                 return -ESRCH;
>
> -       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +           capable(CAP_SYS_NICE)) {
>                 task_lock(p);
>                 seq_printf(m, "%llu\n", p->timer_slack_ns);
>                 task_unlock(p);
> --
> 1.9.1
>



-- 
Kees Cook
Chrome OS & Brillo Security

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

[John Stultz]

On Thu, Jul 14, 2016 at 11:52 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Jul 14, 2016 at 11:50 AM, John Stultz <john.stultz@linaro.org> wrote:
>>
>> For ABI preservation, it still allows CAP_SYS_PTRACE tasks to
>> access/modify timerslack values, but I'm fine with removing
>> this if others agree.
>
> Is anything actually using this ABI yet? (Regardless, I'm fine
> allowing both caps.)

Not that I'm aware of... but there may be someone somewhere.

Given it landed in 4.6, I suspect its fairly safe to remove.

thanks
-john

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

[Nick Kralevich]

On Thu, Jul 14, 2016 at 11:50 AM, John Stultz <john.stultz@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc/<tid>/timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc/<tid>/timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE.
>
> For ABI preservation, it still allows CAP_SYS_PTRACE tasks to
> access/modify timerslack values, but I'm fine with removing
> this if others agree.
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> CC: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Oren Laadan <orenl@cellrox.com>
> Cc: Ruchi Kandoi <kandoiruchi@google.com>
> Cc: Rom Lemarchand <romlem@android.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Colin Cross <ccross@android.com>
> Cc: Nick Kralevich <nnk@google.com>
> Cc: Dmitry Shmidt <dimitrysh@google.com>
> Cc: Elliott Hughes <enh@google.com>
> Cc: Android Kernel Team <kernel-team@android.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  fs/proc/base.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..d32033e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2281,7 +2281,8 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf,
>         if (!p)
>                 return -ESRCH;
>
> -       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +           capable(CAP_SYS_NICE)) {


Like others, I would prefer if we remove the ptrace_may_access()
checks. It seems unnecessary and makes the code slightly more
complicated than necessary.

However, if we can't do that, at a minimum, we should reorder the
checks so that capable(CAP_SYS_NICE) is checked first. Both
ptrace_may_access() and capable() will generate denials if the
appropriate SELinux permission isn't present. People will immediately
see the denial and start adding ptrace allow rules, defeating the
purpose of this change.

In addition, the current capable() check is very course grain. In
Android, for instance, system_server should have the ability to adjust
timerslack values for applications, but shouldn't necessarily apply
those same timerslack values to other privileged processes such as
init. I would recommend we add an LSM hook (include/linux/lsm_hooks.h)
here, to allow security modules to make policy decisions over whether
a timerslack value should be modifiable at all. It would also be good
to provide an SELinux implementation of the hook, which could be as
simple as:

  security/selinux/hooks.c:

  static int selinux_task_settimerslack(struct task_struct *p,
timerslack_value value)
  {
    return current_has_perm(p, PROCESS__SETSCHED);
  }

This would allow SELinux fine grain control over which timer slack
values could be adjusted.

>                 task_lock(p);
>                 if (slack_ns == 0)
>                         p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2306,7 +2307,8 @@ static int timerslack_ns_show(struct seq_file *m, void *v)
>         if (!p)
>                 return -ESRCH;
>
> -       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +           capable(CAP_SYS_NICE)) {
>                 task_lock(p);
>                 seq_printf(m, "%llu\n", p->timer_slack_ns);
>                 task_unlock(p);
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | nnk@google.com | 650.214.4037