Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN

[Matt Brown]

On 04/19/2017 12:58 AM, Serge E. Hallyn wrote:
> On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote:
> > This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity
> > project in-kernel.
> >
> > This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding
> > sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI
> > ioctl calls from non CAP_SYS_ADMIN users.
> >
> > Possible effects on userland:
> >
> > There could be a few user programs that would be effected by this
> > change.
> > See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>
> > notable programs are: agetty, csh, xemacs and tcsh
> >
> > However, I still believe that this change is worth it given that the
> > Kconfig defaults to n. This will be a feature that is turned on for the
>
> It's not worthless, but note that for instance before this was fixed
> in lxc, this patch would not have helped with escapes from privileged
> containers.

I assume you are talking about this CVE:
https://bugzilla.redhat.com/show_bug.cgi?id=1411256

In retrospect, is there any way that an escape from a privileged
container with the this bug could have been prevented?

> > same reason that people activate it when using grsecurity. Users of this
> > opt-in feature will realize that they are choosing security over some OS
> > features like unprivileged TIOCSTI ioctls, as should be clear in the
> > Kconfig help message.
> >
> > Threat Model/Patch Rational:
> >
> > >From grsecurity's config for GRKERNSEC_HARDEN_TTY.
> >
> >  | There are very few legitimate uses for this functionality and it
> >  | has made vulnerabilities in several 'su'-like programs possible in
> >  | the past.  Even without these vulnerabilities, it provides an
> >  | attacker with an easy mechanism to move laterally among other
> >  | processes within the same user's compromised session.
> >
> > So if one process within a tty session becomes compromised it can follow
> > that additional processes, that are thought to be in different security
> > boundaries, can be compromised as a result. When using a program like su
> > or sudo, these additional processes could be in a tty session where TTY file
> > descriptors are indeed shared over privilege boundaries.
> >
> > This is also an excellent writeup about the issue:
> > <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/>
> >
> > Signed-off-by: Matt Brown <matt@xxxxxxxxx>
> > ---
> >  drivers/tty/tty_io.c |  4 ++++
> >  include/linux/tty.h  |  2 ++
> >  kernel/sysctl.c      | 12 ++++++++++++
> >  security/Kconfig     | 13 +++++++++++++
> >  4 files changed, 31 insertions(+)
> >
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index e6d1a65..31894e8 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on)
> >   *	FIXME: may race normal receive processing
> >   */
> >
> > +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT);
> > +
> >  static int tiocsti(struct tty_struct *tty, char __user *p)
> >  {
> >  	char ch, mbz = 0;
> >  	struct tty_ldisc *ld;
> >
> > +	if (tiocsti_restrict && !capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> >  	if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
> >  		return -EPERM;
> >  	if (get_user(ch, p))
> > diff --git a/include/linux/tty.h b/include/linux/tty.h
> > index 1017e904..7011102 100644
> > --- a/include/linux/tty.h
> > +++ b/include/linux/tty.h
> > @@ -342,6 +342,8 @@ struct tty_file_private {
> >  	struct list_head list;
> >  };
> >
> > +extern int tiocsti_restrict;
> > +
> >  /* tty magic number */
> >  #define TTY_MAGIC		0x5401
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index acf0a5a..68d1363 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -67,6 +67,7 @@
> >  #include <linux/kexec.h>
> >  #include <linux/bpf.h>
> >  #include <linux/mount.h>
> > +#include <linux/tty.h>
> >
> >  #include <linux/uaccess.h>
> >  #include <asm/processor.h>
> > @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = {
> >  		.extra2		= &two,
> >  	},
> >  #endif
> > +#if defined CONFIG_TTY
> > +	{
> > +		.procname	= "tiocsti_restrict",
> > +		.data		= &tiocsti_restrict,
> > +		.maxlen		= sizeof(int),
> > +		.mode		= 0644,
> > +		.proc_handler	= proc_dointvec_minmax_sysadmin,
> > +		.extra1		= &zero,
> > +		.extra2		= &one,
> > +	},
> > +#endif
> >  	{
> >  		.procname	= "ngroups_max",
> >  		.data		= &ngroups_max,
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 3ff1bf9..7d13331 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT
> >
> >  	  If you are unsure how to answer this question, answer N.
> >
> > +config SECURITY_TIOCSTI_RESTRICT
>
> This is an odd way to name this.  Shouldn't the name reflect that it
> is setting the default, rather than enabling the feature?
>
> Besides that, I'm ok with the patch.
>
> > +	bool "Restrict unprivileged use of tiocsti command injection"
> > +	default n
> > +	help
> > +	  This enforces restrictions on unprivileged users injecting commands
> > +	  into other processes which share a tty session using the TIOCSTI
> > +	  ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN.
> > +
> > +	  If this option is not selected, no restrictions will be enforced
> > +	  unless the tiocsti_restrict sysctl is explicitly set to (1).
> > +
> > +	  If you are unsure how to answer this question, answer N.
> > +
> >  config SECURITY
> >  	bool "Enable different security models"
> >  	depends on SYSFS
> > --
> > 2.10.2