Messages in this thread | | | From | (Eric W. Biederman) | Date | Tue, 21 Jan 2014 15:12:26 -0800 | Subject | Re: [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 > */
| |