Re: [PATCH] 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 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
>  			 */