linux-kernel - Re: [RFC][PATCH] proc: Relax /proc/<tid>/timerslack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAFJ0LnH_qkGPtGh5VmBUevq7=WS3zceWetSzHV-_GSVduXh7=A@mail.gmail.com>
Date:	Thu, 14 Jul 2016 12:55:40 -0700
From:	Nick Kralevich <nnk@...gle.com>
To:	John Stultz <john.stultz@...aro.org>
Cc:	lkml <linux-kernel@...r.kernel.org>,
	Kees Cook <keescook@...omium.org>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Oren Laadan <orenl@...lrox.com>,
	Ruchi Kandoi <kandoiruchi@...gle.com>,
	Rom Lemarchand <romlem@...roid.com>,
	Todd Kjos <tkjos@...gle.com>, Colin Cross <ccross@...roid.com>,
	Dmitry Shmidt <dimitrysh@...gle.com>,
	Elliott Hughes <enh@...gle.com>,
	Android Kernel Team <kernel-team@...roid.com>
Subject: Re: [RFC][PATCH] proc: Relax /proc/<tid>/timerslack_ns capability requirements

On Thu, Jul 14, 2016 at 11:50 AM, John Stultz <john.stultz@...aro.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@...omium.org>
> Cc: "Serge E. Hallyn" <serge@...lyn.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> CC: Arjan van de Ven <arjan@...ux.intel.com>
> Cc: Oren Laadan <orenl@...lrox.com>
> Cc: Ruchi Kandoi <kandoiruchi@...gle.com>
> Cc: Rom Lemarchand <romlem@...roid.com>
> Cc: Todd Kjos <tkjos@...gle.com>
> Cc: Colin Cross <ccross@...roid.com>
> Cc: Nick Kralevich <nnk@...gle.com>
> Cc: Dmitry Shmidt <dimitrysh@...gle.com>
> Cc: Elliott Hughes <enh@...gle.com>
> Cc: Android Kernel Team <kernel-team@...roid.com>
> Signed-off-by: John Stultz <john.stultz@...aro.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@...gle.com | 650.214.4037