Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
On Thu, Oct 25, 2018 at 01:56:27PM -0500, Eric W. Biederman wrote: > > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > > in its effective capability set, but the current check looks in the root > > namespace instead of the process' user namespace. Since a process is > > allowed to do other activities controlled by CAP_SYS_NICE inside a > > namespace, it should also be able to adjust timerslack_ns. > > The goal seems legitimate. However the permission checks look wrong. > > In particular the choice of user namespace should be > "p->cred->user_ns". This will limit this to tasks that have > CAP_SYS_NICE in the same namespace as the task that is being modified. > > Testing file->f_cred->user_ns it is testing whoever opened the file and > that could be anyone. Thanks, that seems like the right answer here. I'll send a v2 to fix it. Thanks, Benjamin
Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
On Thu, Oct 25, 2018 at 01:56:27PM -0500, Eric W. Biederman wrote: > > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > > in its effective capability set, but the current check looks in the root > > namespace instead of the process' user namespace. Since a process is > > allowed to do other activities controlled by CAP_SYS_NICE inside a > > namespace, it should also be able to adjust timerslack_ns. > > The goal seems legitimate. However the permission checks look wrong. > > In particular the choice of user namespace should be > "p->cred->user_ns". This will limit this to tasks that have > CAP_SYS_NICE in the same namespace as the task that is being modified. > > Testing file->f_cred->user_ns it is testing whoever opened the file and > that could be anyone. Thanks, that seems like the right answer here. I'll send a v2 to fix it. Thanks, Benjamin
Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
bmgor...@google.com writes: > From: Benjamin Gordon > > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > in its effective capability set, but the current check looks in the root > namespace instead of the process' user namespace. Since a process is > allowed to do other activities controlled by CAP_SYS_NICE inside a > namespace, it should also be able to adjust timerslack_ns. The goal seems legitimate. However the permission checks look wrong. In particular the choice of user namespace should be "p->cred->user_ns". This will limit this to tasks that have CAP_SYS_NICE in the same namespace as the task that is being modified. Testing file->f_cred->user_ns it is testing whoever opened the file and that could be anyone. Eric > Signed-off-by: Benjamin Gordon > Cc: John Stultz > Cc: Kees Cook > Cc: "Serge E. Hallyn" > Cc: Thomas Gleixner > Cc: Arjan van de Ven > Cc: Oren Laadan > Cc: Ruchi Kandoi > Cc: Rom Lemarchand > Cc: Todd Kjos > Cc: Colin Cross > Cc: Nick Kralevich > Cc: Dmitry Shmidt > Cc: Elliott Hughes > Cc: Android Kernel Team > Cc: Andrew Morton > --- > fs/proc/base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7e9f07bf260d..4b50937dff80 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, > const char __user *buf, > return -ESRCH; > > if (p != current) { > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { > count = -EPERM; > goto out; > } > @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void > *v) > > if (p != current) { > > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { > err = -EPERM; > goto out; > }
Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
bmgor...@google.com writes: > From: Benjamin Gordon > > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > in its effective capability set, but the current check looks in the root > namespace instead of the process' user namespace. Since a process is > allowed to do other activities controlled by CAP_SYS_NICE inside a > namespace, it should also be able to adjust timerslack_ns. The goal seems legitimate. However the permission checks look wrong. In particular the choice of user namespace should be "p->cred->user_ns". This will limit this to tasks that have CAP_SYS_NICE in the same namespace as the task that is being modified. Testing file->f_cred->user_ns it is testing whoever opened the file and that could be anyone. Eric > Signed-off-by: Benjamin Gordon > Cc: John Stultz > Cc: Kees Cook > Cc: "Serge E. Hallyn" > Cc: Thomas Gleixner > Cc: Arjan van de Ven > Cc: Oren Laadan > Cc: Ruchi Kandoi > Cc: Rom Lemarchand > Cc: Todd Kjos > Cc: Colin Cross > Cc: Nick Kralevich > Cc: Dmitry Shmidt > Cc: Elliott Hughes > Cc: Android Kernel Team > Cc: Andrew Morton > --- > fs/proc/base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7e9f07bf260d..4b50937dff80 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, > const char __user *buf, > return -ESRCH; > > if (p != current) { > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { > count = -EPERM; > goto out; > } > @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void > *v) > > if (p != current) { > > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { > err = -EPERM; > goto out; > }
Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
On Wed, Oct 17, 2018 at 3:47 PM, wrote: > From: Benjamin Gordon > > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > in its effective capability set, but the current check looks in the root > namespace instead of the process' user namespace. Since a process is > allowed to do other activities controlled by CAP_SYS_NICE inside a > namespace, it should also be able to adjust timerslack_ns. > > Signed-off-by: Benjamin Gordon > Cc: John Stultz > Cc: Kees Cook > Cc: "Serge E. Hallyn" > Cc: Thomas Gleixner > Cc: Arjan van de Ven > Cc: Oren Laadan > Cc: Ruchi Kandoi > Cc: Rom Lemarchand > Cc: Todd Kjos > Cc: Colin Cross > Cc: Nick Kralevich > Cc: Dmitry Shmidt > Cc: Elliott Hughes > Cc: Android Kernel Team > Cc: Andrew Morton > --- > fs/proc/base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7e9f07bf260d..4b50937dff80 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, > const char __user *buf, > return -ESRCH; > > if (p != current) { > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { > count = -EPERM; > goto out; > } > @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void > *v) > > if (p != current) { > > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { > err = -EPERM; > goto out; > } Looks reasonable to me. Acked-by: John Stultz thanks -john
Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
On Wed, Oct 17, 2018 at 3:47 PM, wrote: > From: Benjamin Gordon > > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > in its effective capability set, but the current check looks in the root > namespace instead of the process' user namespace. Since a process is > allowed to do other activities controlled by CAP_SYS_NICE inside a > namespace, it should also be able to adjust timerslack_ns. > > Signed-off-by: Benjamin Gordon > Cc: John Stultz > Cc: Kees Cook > Cc: "Serge E. Hallyn" > Cc: Thomas Gleixner > Cc: Arjan van de Ven > Cc: Oren Laadan > Cc: Ruchi Kandoi > Cc: Rom Lemarchand > Cc: Todd Kjos > Cc: Colin Cross > Cc: Nick Kralevich > Cc: Dmitry Shmidt > Cc: Elliott Hughes > Cc: Android Kernel Team > Cc: Andrew Morton > --- > fs/proc/base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7e9f07bf260d..4b50937dff80 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, > const char __user *buf, > return -ESRCH; > > if (p != current) { > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { > count = -EPERM; > goto out; > } > @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void > *v) > > if (p != current) { > > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { > err = -EPERM; > goto out; > } Looks reasonable to me. Acked-by: John Stultz thanks -john
Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
On Wed, Oct 17, 2018 at 04:47:38PM -0600, bmgor...@google.com wrote: > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > in its effective capability set, but the current check looks in the root > namespace instead of the process' user namespace. Since a process is > allowed to do other activities controlled by CAP_SYS_NICE inside a > namespace, it should also be able to adjust timerslack_ns. > > Signed-off-by: Benjamin Gordon > Cc: John Stultz > Cc: Kees Cook > Cc: "Serge E. Hallyn" > Cc: Thomas Gleixner > Cc: Arjan van de Ven > Cc: Oren Laadan > Cc: Ruchi Kandoi > Cc: Rom Lemarchand > Cc: Todd Kjos > Cc: Colin Cross > Cc: Nick Kralevich > Cc: Dmitry Shmidt > Cc: Elliott Hughes > Cc: Android Kernel Team > Cc: Andrew Morton > --- > fs/proc/base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7e9f07bf260d..4b50937dff80 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, > const char __user *buf, > return -ESRCH; > > if (p != current) { > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { > count = -EPERM; > goto out; > } > @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void > *v) > > if (p != current) { > > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { > err = -EPERM; > goto out; > } > -- > 2.19.1.331.ge82ca0e54c-goog > Friendly ping. Does anybody have time to take a look at this? Thanks, Benjamin
Re: [PATCH] proc: use ns_capable instead of capable for timerslack_ns
On Wed, Oct 17, 2018 at 04:47:38PM -0600, bmgor...@google.com wrote: > Access to timerslack_ns is controlled by a process having CAP_SYS_NICE > in its effective capability set, but the current check looks in the root > namespace instead of the process' user namespace. Since a process is > allowed to do other activities controlled by CAP_SYS_NICE inside a > namespace, it should also be able to adjust timerslack_ns. > > Signed-off-by: Benjamin Gordon > Cc: John Stultz > Cc: Kees Cook > Cc: "Serge E. Hallyn" > Cc: Thomas Gleixner > Cc: Arjan van de Ven > Cc: Oren Laadan > Cc: Ruchi Kandoi > Cc: Rom Lemarchand > Cc: Todd Kjos > Cc: Colin Cross > Cc: Nick Kralevich > Cc: Dmitry Shmidt > Cc: Elliott Hughes > Cc: Android Kernel Team > Cc: Andrew Morton > --- > fs/proc/base.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7e9f07bf260d..4b50937dff80 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, > const char __user *buf, > return -ESRCH; > > if (p != current) { > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { > count = -EPERM; > goto out; > } > @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void > *v) > > if (p != current) { > > - if (!capable(CAP_SYS_NICE)) { > + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { > err = -EPERM; > goto out; > } > -- > 2.19.1.331.ge82ca0e54c-goog > Friendly ping. Does anybody have time to take a look at this? Thanks, Benjamin
[PATCH] proc: use ns_capable instead of capable for timerslack_ns
From: Benjamin Gordon Access to timerslack_ns is controlled by a process having CAP_SYS_NICE in its effective capability set, but the current check looks in the root namespace instead of the process' user namespace. Since a process is allowed to do other activities controlled by CAP_SYS_NICE inside a namespace, it should also be able to adjust timerslack_ns. Signed-off-by: Benjamin Gordon Cc: John Stultz Cc: Kees Cook Cc: "Serge E. Hallyn" Cc: Thomas Gleixner Cc: Arjan van de Ven Cc: Oren Laadan Cc: Ruchi Kandoi Cc: Rom Lemarchand Cc: Todd Kjos Cc: Colin Cross Cc: Nick Kralevich Cc: Dmitry Shmidt Cc: Elliott Hughes Cc: Android Kernel Team Cc: Andrew Morton --- fs/proc/base.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 7e9f07bf260d..4b50937dff80 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, return -ESRCH; if (p != current) { - if (!capable(CAP_SYS_NICE)) { + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { count = -EPERM; goto out; } @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void *v) if (p != current) { - if (!capable(CAP_SYS_NICE)) { + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { err = -EPERM; goto out; } -- 2.19.1.331.ge82ca0e54c-goog
[PATCH] proc: use ns_capable instead of capable for timerslack_ns
From: Benjamin Gordon Access to timerslack_ns is controlled by a process having CAP_SYS_NICE in its effective capability set, but the current check looks in the root namespace instead of the process' user namespace. Since a process is allowed to do other activities controlled by CAP_SYS_NICE inside a namespace, it should also be able to adjust timerslack_ns. Signed-off-by: Benjamin Gordon Cc: John Stultz Cc: Kees Cook Cc: "Serge E. Hallyn" Cc: Thomas Gleixner Cc: Arjan van de Ven Cc: Oren Laadan Cc: Ruchi Kandoi Cc: Rom Lemarchand Cc: Todd Kjos Cc: Colin Cross Cc: Nick Kralevich Cc: Dmitry Shmidt Cc: Elliott Hughes Cc: Android Kernel Team Cc: Andrew Morton --- fs/proc/base.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index 7e9f07bf260d..4b50937dff80 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2356,7 +2356,7 @@ static ssize_t timerslack_ns_write(struct file *file, const char __user *buf, return -ESRCH; if (p != current) { - if (!capable(CAP_SYS_NICE)) { + if (!ns_capable(file->f_cred->user_ns, CAP_SYS_NICE)) { count = -EPERM; goto out; } @@ -2393,7 +2393,7 @@ static int timerslack_ns_show(struct seq_file *m, void *v) if (p != current) { - if (!capable(CAP_SYS_NICE)) { + if (!ns_capable(seq_user_ns(m), CAP_SYS_NICE)) { err = -EPERM; goto out; } -- 2.19.1.331.ge82ca0e54c-goog