[lxc-devel] User namespaces

Thu Apr 11 17:18:14 UTC 2013

Dwight Engen <dwight.engen at oracle.com> writes:

> Hi Eric, any chance you've had a moment to mull this over any more?
> I've also CC'ed Jiri since he is listed in MAINTAINERS for the TTY
> layer :)

Honestly I really haven't.

For the most part I have been in feature freeze and bug fix mode.

It gets tricky getting the permission checks correct on something like
this and I can easily imagine more people being able to call
vhangup/revoke without care could easily translate into a DDOS attack.
Say revoking the file descriptor that logs system messages out the
serial console.

I think this problem would be easier if we had a devpts specific
solution as the concept of owner of a device node is much clearer there.
There is only one device node.

I would really like to see a combination of implementation and reasoning
that doesn't leave corner cases for me to worry about what the bad guys
are going to do with it.

Eric

> On Wed, 6 Mar 2013 09:58:53 -0600
> Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
>
>> Quoting Dwight Engen (dwight.engen at oracle.com):
>> > On Mon, 25 Feb 2013 20:26:21 -0800
>> > ebiederm at xmission.com (Eric W. Biederman) wrote:
>> ...
>> > > 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.engen at 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.engen at oracle.com>
>> 
>> It looks good to me.  Eric?
>> 
>> Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.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
>> > 