linux-kernel - Re: [PATCH] tty: Allow stealing of controlling ttys within user namespaces

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <877g9t0w11.fsf@xmission.com>
Date:	Tue, 21 Jan 2014 15:12:26 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Seth Forshee <seth.forshee@...onical.com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jslaby@...e.cz>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty: Allow stealing of controlling ttys within user namespaces

Seth Forshee <seth.forshee@...onical.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@...r.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@...onical.com>
> Cc: "Eric W. Biederman" <ebiederm@...ssion.com>
> Signed-off-by: Seth Forshee <seth.forshee@...onical.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
>  			 */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/