Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN

[Serge E. Hallyn]

Quoting Matt Brown (matt@xxxxxxxxx):
> 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?

I don't know, that's what I was probing for.  Detecting that the pgrp
or session - heck, the pid namespace - has changed would seem like a
good indicator that it shouldn't be able to push.

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