Re: [PATCH v2] tty: Allow stealing of controlling ttys within user namespaces

[ebiederm@xmission.com (Eric W. Biederman)]

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 which currently owns the tty.
>
> Update this code to only require CAP_SYS_ADMIN in the user
> namespaces of the target session's tasks, allowing the tty to be
> stolen from sessions whose tasks are in the same or lesser
> privileged user namespaces.

This code looks essentially correct.  I would like to look at it a bit
more before we merge it, just to ensure something silly hasn't been
missed, but the only thing that concerns me at this point is are we
checking the proper per task bits.

The case I am currently worrying about is a task that does something
privileged drops perms sets dumpable and then calls setns() on the
userns.

So I think we may have to solve the dumpable problem at the same time as
we solve this issue.

Now I don't know if it makes sense to take this through the tty tree or
my userns tree.  I am inclined to take it through the userns tree simply
because I am reviewing it and I have seen the several failed attempts at
this but if Greg wants it in the tty tree I won't object.

What I do want to do is be especially careful with a patch like this so
we don't accidentally introduce a DAC policy hole, and cause security
problems for people.  Bugs like that don't do anyone any good.

> 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 | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index c74a00a..558e6dc 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -2410,17 +2410,32 @@ 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)) {
> -			/*
> -			 * Steal it away
> -			 */
> -			read_lock(&tasklist_lock);
> -			session_clear_tty(tty->session);
> -			read_unlock(&tasklist_lock);
> -		} else {
> +		struct user_namespace *user_ns;
> +		struct task_struct *p;
> +
> +		if (arg != 1) {
>  			ret = -EPERM;
>  			goto unlock;
>  		}
> +
> +		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(&tasklist_lock);
> +				ret = -EPERM;
> +				goto unlock;
> +			}
> +			rcu_read_unlock();
> +		} while_each_pid_task(tty->session, PIDTYPE_SID, p);
> +
> +		/*
> +		 * Steal it away
> +		 */
> +		session_clear_tty(tty->session);
> +		read_unlock(&tasklist_lock);
>  	}
>  	proc_set_tty(current, tty);
>  unlock: