lkml.org 
[lkml]   [2014]   [Jan]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] tty: Allow stealing of controlling ttys within user namespaces
Seth Forshee <seth.forshee@canonical.com> writes:

> root is allowed to steal ttys from other sessions, but it
> requires system-wide CAP_SYS_ADMIN and therefore is not possible
> for root within a user namespace. This should be allowed so long
> as the process doing the stealing is privileged towards the
> session leader which currently owns the tty.
>
> Update the tty code to only require CAP_SYS_ADMIN in the
> namespace of the target session leader when stealing a tty. Fall
> back to using init_user_ns to preserve the existing behavior for
> system-wide root.
>
> Cc: stable@vger.kernel.org # 3.8+

This is not a regression of any form, nor is it obviously correct so
this does not count as a stable material.

> Cc: Serge Hallyn <serge.hallyn@canonical.com>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
> drivers/tty/tty_io.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index c74a00a..1c47f16 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2410,7 +2410,19 @@ static int tiocsctty(struct tty_struct *tty, int arg)
> * This tty is already the controlling
> * tty for another session group!
> */
> - if (arg == 1 && capable(CAP_SYS_ADMIN)) {
> + struct user_namespace *ns = &init_user_ns;
> + struct task_struct *p;
> +
> + read_lock(&tasklist_lock);
> + do_each_pid_task(tty->session, PIDTYPE_SID, p) {
> + if (p->signal->leader) {
> + ns = task_cred_xxx(p, user_ns);
> + break;
> + }
> + } while_each_pid_task(tty->session, PIDTYPE_SID, p);
> + read_unlock(&tasklist_lock);

Ugh. That appears to be both racy (what protects the user_ns from going
away?) and a possibly allowing revoking a tty from a more privileged processes tty.

However I do see a form that can easily verify we won't revoke a tty from a
more privileged process.

if (arg == 1) {
struct user_namespace *user_ns;
read_lock(&tasklist_lock);
do_each_pid_task(tty->session, PIDTYPE_SID, p) {
rcu_read_lock();
user_ns = task_cred_xxx(p, user_ns);
if (!ns_capable(user_ns, CAP_SYS_ADMIN)) {
rcu_read_unlock();
read_unlock(&task_list_lock);
ret = -EPERM;
goto out_unlock;
}
rcu_read_unlock();
}
/* Don't drop the the tasklist_lock before
* stealing the tasks or the set of tasks can
* change, and we only have permission for this set
* of tasks.
*/
/*
* Steal it away
*/
session_clear_tty(tty->session);
read_unlock(&task_list_lock);
} else {
ret = -EPERM;
goto out_unlock;
}

My code above is ugly and could use some cleaning up but it should be
correct with respect to this issue.

Eric


> + if (arg == 1 && ns_capable(user_ns, CAP_SYS_ADMIN)) {
> /*
> * Steal it away
> */


\
 
 \ /
  Last update: 2014-01-22 00:41    [W:0.048 / U:2.748 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site