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

[Seth Forshee]

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: sta...@vger.kernel.org # 3.8+
Cc: Serge Hallyn <serge....@canonical.com>
Cc: "Eric W. Biederman" <ebie...@xmission.com>
Signed-off-by: Seth Forshee <seth.f...@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);
+
+ if (arg == 1 && ns_capable(ns, CAP_SYS_ADMIN)) {
/*
* Steal it away
*/
--
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Eric W. Biederman]

Seth Forshee <seth.f...@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: sta...@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.

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
> */
--

[Seth Forshee]

Thanks for the review. I'm not sure about the correctness of checking
all processes in the session versus just the session leader, since the
leader is the only task that really owns the tty in the sense of being
able to set and clear it for the session. But most of the time all the
tasks will be in the same namespace anyway.

I'm about to start testing a modified version of the above, and I'll
send and updated patch once I've finished.

Thanks,
Seth

[Seth Forshee]

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.

Cc: Serge Hallyn <serge....@canonical.com>
Cc: "Eric W. Biederman" <ebie...@xmission.com>
Signed-off-by: Seth Forshee <seth.f...@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:
--
1.8.3.2

[Eric W. Biederman]

Seth Forshee <seth.f...@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.

[Greg Kroah-Hartman]

On Fri, Jan 24, 2014 at 03:31:15PM -0800, Eric W. Biederman wrote:
> Seth Forshee <seth.f...@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.

No objection from me.

thanks,

greg k-h