| Message ID | 1470786857-27366-1-git-send-email-john.stultz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Aug 10, 2016 at 11:36 AM, Kees Cook <keescook@chromium.org> wrote: > On Tue, Aug 9, 2016 at 4:54 PM, John Stultz <john.stultz@linaro.org> wrote: >> In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) >> to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds >> when p == current, but the CAP_SYS_NICE doesn't. >> >> Thus while the previous commit was intended to loosen the needed >> privledges to modify a processes timerslack, it needlessly restricted >> a task modifying its own timerslack via the proc/<tid>/timerslack_ns >> (which is permitted also via the PR_SET_TIMERSLACK method). >> >> This patch corrects this by checking if p == current before checking >> the CAP_SYS_NICE value. >> >> This patch applies on top of my two previous patches currently in -mm >> >> 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 | 34 +++++++++++++++++++--------------- >> 1 file changed, 19 insertions(+), 15 deletions(-) >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index 02f8389..01c3c2d 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -2281,15 +2281,17 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, >> if (!p) >> return -ESRCH; >> >> - if (!capable(CAP_SYS_NICE)) { >> - count = -EPERM; >> - goto out; >> - } >> + if (p != current) { >> + if (!capable(CAP_SYS_NICE)) { >> + count = -EPERM; >> + goto out; >> + } >> >> - err = security_task_setscheduler(p); >> - if (err) { >> - count = err; >> - goto out; >> + err = security_task_setscheduler(p); >> + if (err) { >> + count = err; >> + goto out; >> + } >> } > > This entirely bypasses LSM when p == current. Is that intended? I wasn't entierly sure. I didn't think PR_SET_TIMERSLACK has a security hook, but looking again I now see the top-level security_task_prctl() check, so maybe not skipping it in this case would be good? thanks -john
On Wed, Aug 10, 2016 at 12:03 PM, John Stultz <john.stultz@linaro.org> wrote: > On Wed, Aug 10, 2016 at 11:36 AM, Kees Cook <keescook@chromium.org> wrote: >> On Tue, Aug 9, 2016 at 4:54 PM, John Stultz <john.stultz@linaro.org> wrote: >>> In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) >>> to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds >>> when p == current, but the CAP_SYS_NICE doesn't. >>> >>> Thus while the previous commit was intended to loosen the needed >>> privledges to modify a processes timerslack, it needlessly restricted >>> a task modifying its own timerslack via the proc/<tid>/timerslack_ns >>> (which is permitted also via the PR_SET_TIMERSLACK method). >>> >>> This patch corrects this by checking if p == current before checking >>> the CAP_SYS_NICE value. >>> >>> This patch applies on top of my two previous patches currently in -mm >>> >>> 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 | 34 +++++++++++++++++++--------------- >>> 1 file changed, 19 insertions(+), 15 deletions(-) >>> >>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>> index 02f8389..01c3c2d 100644 >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -2281,15 +2281,17 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, >>> if (!p) >>> return -ESRCH; >>> >>> - if (!capable(CAP_SYS_NICE)) { >>> - count = -EPERM; >>> - goto out; >>> - } >>> + if (p != current) { >>> + if (!capable(CAP_SYS_NICE)) { >>> + count = -EPERM; >>> + goto out; >>> + } >>> >>> - err = security_task_setscheduler(p); >>> - if (err) { >>> - count = err; >>> - goto out; >>> + err = security_task_setscheduler(p); >>> + if (err) { >>> + count = err; >>> + goto out; >>> + } >>> } >> >> This entirely bypasses LSM when p == current. Is that intended? > > I wasn't entierly sure. I didn't think PR_SET_TIMERSLACK has a > security hook, but looking again I now see the top-level > security_task_prctl() check, so maybe not skipping it in this case > would be good? So thinking about this some more. I'm really not sure what the right thing is. Since the LSM check for security_task_setscheduler(), is different from the security_task_prctl() check, it seems odd to have different checks for different interfaces which in the p==current case are really are the same. Suggestions? thanks -john
On Wed, Aug 10, 2016 at 1:01 PM, Arjan van de Ven <arjan@linux.intel.com> wrote: > On 8/10/2016 12:03 PM, John Stultz wrote: > >> I wasn't entierly sure. I didn't think PR_SET_TIMERSLACK has a >> security hook, but looking again I now see the top-level >> security_task_prctl() check, so maybe not skipping it in this case >> would be good? > > > the easy fix would be to add back the ptrace check.. just either ptrace-able > OR CAP_SYS_NICE ;) Well, I worry that just adds more complexity to trying to understand it. p==current OR CAP_SYS_NICE makes the most sense to me. > then you can prove you only added new stuff as well, and have all the LSM > from before The LSM bits (and how consistent or inconsistent they can be) is really the part that I have the most concern about, and I'm not sure what the best approach would be. thanks -john
On Wed, Aug 10, 2016 at 2:02 PM, Kees Cook <keescook@chromium.org> wrote: > On Wed, Aug 10, 2016 at 11:36 AM, Kees Cook <keescook@chromium.org> wrote: >> On Tue, Aug 9, 2016 at 4:54 PM, John Stultz <john.stultz@linaro.org> wrote: >>> In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) >>> to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds >>> when p == current, but the CAP_SYS_NICE doesn't. >>> >>> Thus while the previous commit was intended to loosen the needed >>> privledges to modify a processes timerslack, it needlessly restricted >>> a task modifying its own timerslack via the proc/<tid>/timerslack_ns >>> (which is permitted also via the PR_SET_TIMERSLACK method). >>> >>> This patch corrects this by checking if p == current before checking >>> the CAP_SYS_NICE value. >>> >>> This patch applies on top of my two previous patches currently in -mm >>> >>> 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 | 34 +++++++++++++++++++--------------- >>> 1 file changed, 19 insertions(+), 15 deletions(-) >>> >>> diff --git a/fs/proc/base.c b/fs/proc/base.c >>> index 02f8389..01c3c2d 100644 >>> --- a/fs/proc/base.c >>> +++ b/fs/proc/base.c >>> @@ -2281,15 +2281,17 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, >>> if (!p) >>> return -ESRCH; >>> >>> - if (!capable(CAP_SYS_NICE)) { >>> - count = -EPERM; >>> - goto out; >>> - } >>> + if (p != current) { >>> + if (!capable(CAP_SYS_NICE)) { >>> + count = -EPERM; >>> + goto out; >>> + } >>> >>> - err = security_task_setscheduler(p); >>> - if (err) { >>> - count = err; >>> - goto out; >>> + err = security_task_setscheduler(p); >>> + if (err) { >>> + count = err; >>> + goto out; >>> + } >>> } >> >> This entirely bypasses LSM when p == current. Is that intended? > > I take back my concern. :) I think this is correct (as you mention in > the thread: the prctl LSM hook already fired), so until there is a But did it? The prctrl hook is just for the prctrl interface. The proc/<tid>/timerslack_ns is separate. This is part of my confusion here, mostly in that I'm not really sure I have a good sense of philosophy for LSM hooks. Are these just interface guards/hooks, or are we trying to map the hook to the underlying action being taken? As with the prctrl interface, it seems like its just an interface guard, but the /proc/<tid>/timerslack_ns interface checking security_task_setscheduler() seems to be more connected to the underlying action being done by changing the timerslack value. thanks -john
diff --git a/fs/proc/base.c b/fs/proc/base.c index 02f8389..01c3c2d 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2281,15 +2281,17 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, if (!p) return -ESRCH; - if (!capable(CAP_SYS_NICE)) { - count = -EPERM; - goto out; - } + if (p != current) { + if (!capable(CAP_SYS_NICE)) { + count = -EPERM; + goto out; + } - err = security_task_setscheduler(p); - if (err) { - count = err; - goto out; + err = security_task_setscheduler(p); + if (err) { + count = err; + goto out; + } } task_lock(p); @@ -2315,14 +2317,16 @@ static int timerslack_ns_show(struct seq_file *m, void *v) if (!p) return -ESRCH; - if (!capable(CAP_SYS_NICE)) { - err = -EPERM; - goto out; - } + if (p != current) { - err = security_task_getscheduler(p); - if (err) - goto out; + if (!capable(CAP_SYS_NICE)) { + err = -EPERM; + goto out; + } + err = security_task_getscheduler(p); + if (err) + goto out; + } task_lock(p); seq_printf(m, "%llu\n", p->timer_slack_ns);
In changing from checking ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) to capable(CAP_SYS_NICE), I missed that ptrace_my_access succeeds when p == current, but the CAP_SYS_NICE doesn't. Thus while the previous commit was intended to loosen the needed privledges to modify a processes timerslack, it needlessly restricted a task modifying its own timerslack via the proc/<tid>/timerslack_ns (which is permitted also via the PR_SET_TIMERSLACK method). This patch corrects this by checking if p == current before checking the CAP_SYS_NICE value. This patch applies on top of my two previous patches currently in -mm 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 | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) -- 1.9.1