On Mon, 25 Feb 2013 20:26:21 -0800
ebied...@xmission.com (Eric W. Biederman) wrote:
> Dwight Engen <dwight.en...@oracle.com> writes:
>
> > On Sun, 24 Feb 2013 21:12:59 -0800
> > ebied...@xmission.com (Eric W. Biederman) wrote:
> >
> >> Serge Hallyn <serge.hal...@ubuntu.com> writes:
> >>
> >> > Quoting Dwight Engen (dwight.en...@oracle.com):
> >> >> I finally got around to testing out user namespaces. Very nice
> >> >> to to have container root not be kuid 0! One thing that I
> >> >> noticed was that mingetty in the container was failing because
> >> >> the call to vhangup(2) failed (and thus no lxc-console). I
> >> >> could patch the container to start mingetty with --nohangup,
> >> >> but that feels like a workaround and wouldn't be good when the
> >> >> terminal got reused in the container. Instead I patched my
> >> >> kernel with:
> >> >>
> >> >> diff --git a/fs/open.c b/fs/open.c
> >> >> index 9b33c0c..7c54d1d7 100644
> >> >> --- a/fs/open.c
> >> >> +++ b/fs/open.c
> >> >> @@ -1059,7 +1059,7 @@ EXPORT_SYMBOL(sys_close);
> >> >> */
> >> >> SYSCALL_DEFINE0(vhangup)
> >> >> {
> >> >> - if (capable(CAP_SYS_TTY_CONFIG)) {
> >> >> + if (ns_capable(current_user_ns(), CAP_SYS_TTY_CONFIG))
> >> >> {
> >> >
> >> > Note there is a nsown_capable(CAP_SYS_TTY_CONFIG) shortcut for
> >> > this, but
> >
> > Ahh, I missed that, thanks!
> >
> >> >> tty_vhangup_self();
> >> >> return 0;
> >> >> }
> >> >>
> >> >> This lets mingetty work, but I'm not so sure it safe to allow a
> >> >> CAP_SYS_TTY_CONFIG capable process in the namespace hangup
> >> >> whatever terminal it might be able to open and get to be its
> >> >> controlling terminal. I guess the terminal would have to be
> >> >> open()able or TIOCSCTTY'able in the container, but is that
> >> >> enough protection? Thoughts?
> >> >
> >> > I don't think nsown_capable() is sufficient here. Rather, we
> >> > need to check against the user namespace owning
> >> > get_current_tty(). So what is that, the
> >> > get_current_tty()->pgrp->upid[level]->user_ns ? Or can we walk
> >> > over the get_current_tty()->tty_files and see if any of those
> >> > match?
> >
> > I agree I don't think nsown_capable() is enough. I'm not sure what
> > we should be checking though. I don't think we can use pgrp for the
> > same disassociate reason as tty->session below.
> >
> >> > (Eric cc:d as this may be something he's already thought about.)
> >>
> >> I have not thought about a less than superprivilege revoke.
> >>
> >> Is this vhangup happening on pseudo-tty acting as /dev/console?
> >
> > Yes, its done on the ptys lxc set up for the
> > container's /dev/console and /dev/tty[1-4].
> >
> >> Part of me really wants to say the answer should be don't do that
> >> then.
> >
> > I think the reason mingetty wants to do the vhangup is still valid
> > in the container case though? ie. it wants to make sure any process
> > that has fds refering to the slave pty from the last login session
> > get set to hung up ops.
> >
> >> Reading the code the current unprivileged uses of tty_vhangup are:
> >> - The pty master closing.
> >> - A session group leader exiting with a controlling tty (that is
> >> not a pty).
> >>
> >> Which suggests that if you are root in the user namespace of the
> >> session that currently owns the tty it is reasonable to force a
> >> hangup for regulary ttys. I don't know about ptys.
> >>
> >> I guess that would require capabilities in
> >> ns_of_pid(tty->session)->user_ns.
> >>
> >> Being able to kill the process that is the pty master also seems
> >> reasonable.
> >>
> >> However I don't think either of those really answer the question
> >> for when it is ok send vhangup to a pty whose master lies outside
> >> of the current user namespace. Which I am pretty certain is the
> >> case in question.
> >
> > What defines which user namespace a pty is in? I guess I'm not sure
> > if the pty master should be considered outside the new user
> > namespace or not. lxc creates the pty pair while in the parent ns,
> > and chowns the slave into a uid in the new user ns. I guess it
> > could fchown the master too, though it doesn't right now. The
> > mingetty attempting vhangup is in the new user ns.
> >
> > ns_of_pid(tty->session)->user_ns is likely also the new user ns,
> > but I think we can't just use that in a ns_capable check because
> > tty->session might be NULL if the the tty has been disassociated as
> > the controlling terminal. I think we need a check that will work
> > even after disassociation since there could still be fds that need
> > hang up.
>
> For pty's since they only have the single device node. We can
> probably do kuid_has_mapping and kgid_has_mapping to see if we should
> have super user privileges over the pty. But that is specific to
> ptys on /dev/pts. Normal devices potentially can have device nodes
> with different permissions in different places so we can't figure out
> an owner for the device.
>
> Eric
>
Yeah, I agree we don't want something pty specific. The following patch
assumes we define a tty as belonging to the user ns of its session
leader (and if it doesn't have one then to init_user_ns) as you first
suggested. I added locking which avoids the race with
disassociate_tty() and ensures the user ns doesn't get unrefed by way of
put_pid(tty->session).
Note that I think TIOCVHANGUP should be checking for CAP_SYS_TTY_CONFIG
instead of CAP_SYS_ADMIN to be consistent with vhangup(2), but I did
not change that in the refactoring here.
--
>From 697f842ffc709312e5775e3d1d0782079c3070dc Mon Sep 17 00:00:00 2001
From: Dwight Engen <dwight.en...@oracle.com>
Date: Fri, 1 Mar 2013 13:49:58 -0500
Subject: [PATCH] make vhangup and TIOCVHANGUP namespace aware
Signed-off-by: Dwight Engen <dwight.en...@oracle.com>
---
drivers/tty/tty_io.c | 36 ++++++++++++++++++++++++++++++------
fs/open.c | 6 +-----
include/linux/tty.h | 2 +-
3 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index a057db8..764d4e7 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -104,6 +104,7 @@
#include <linux/kmod.h>
#include <linux/nsproxy.h>
+#include <linux/pid_namespace.h>
#undef TTY_DEBUG_HANGUP
@@ -722,6 +723,29 @@ void tty_vhangup(struct tty_struct *tty)
EXPORT_SYMBOL(tty_vhangup);
+/**
+ * tty_vhangup_check_cap - process vhangup checking for capablity
+ *
+ * Perform a vhangup on the given tty
+ */
+
+static int tty_vhangup_check_cap(struct tty_struct *tty, int cap)
+{
+ unsigned long flags;
+ int retval = 0;
+ struct user_namespace *ns = &init_user_ns;
+
+ spin_lock_irqsave(&tty->ctrl_lock, flags);
+ if (tty->session)
+ ns = ns_of_pid(tty->session)->user_ns;
+ if (!ns_capable(ns, cap))
+ retval = -EPERM;
+ spin_unlock_irqrestore(&tty->ctrl_lock, flags);
+
+ if (!retval)
+ tty_vhangup(tty);
+ return retval;
+}
/**
* tty_vhangup_self - process vhangup for own ctty
@@ -729,15 +753,18 @@ EXPORT_SYMBOL(tty_vhangup);
* Perform a vhangup on the current controlling tty
*/
-void tty_vhangup_self(void)
+int tty_vhangup_self(void)
{
struct tty_struct *tty;
+ int retval = 0;
tty = get_current_tty();
if (tty) {
- tty_vhangup(tty);
+ retval = tty_vhangup_check_cap(tty, CAP_SYS_TTY_CONFIG);
tty_kref_put(tty);
}
+
+ return retval;
}
/**
@@ -2710,10 +2737,7 @@ long tty_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
case TIOCSETD:
return tiocsetd(tty, p);
case TIOCVHANGUP:
- if (!capable(CAP_SYS_ADMIN))
- return -EPERM;
- tty_vhangup(tty);
- return 0;
+ return tty_vhangup_check_cap(tty, CAP_SYS_ADMIN);
case TIOCGDEV:
{
unsigned int ret = new_encode_dev(tty_devnum(real_tty));
diff --git a/fs/open.c b/fs/open.c
index 9b33c0c..19ac16e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1059,11 +1059,7 @@ EXPORT_SYMBOL(sys_close);
*/
SYSCALL_DEFINE0(vhangup)
{
- if (capable(CAP_SYS_TTY_CONFIG)) {
- tty_vhangup_self();
- return 0;
- }
- return -EPERM;
+ return tty_vhangup_self();
}
/*
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 8db1b56..c9d0e9c 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -379,7 +379,7 @@ extern int tty_signal(int sig, struct tty_struct *tty);
extern void tty_hangup(struct tty_struct *tty);
extern void tty_vhangup(struct tty_struct *tty);
extern void tty_vhangup_locked(struct tty_struct *tty);
-extern void tty_vhangup_self(void);
+extern int tty_vhangup_self(void);
extern void tty_unhangup(struct file *filp);
extern int tty_hung_up_p(struct file *filp);
extern void do_SAK(struct tty_struct *tty);
--
1.7.12.3
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_feb
_______________________________________________
Lxc-devel mailing list
Lxc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
