Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN

["Serge E. Hallyn" <serge@hallyn.com>]

Quoting Matt Brown (matt@nmatt.com):
> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
> >Quoting Matt Brown (matt@nmatt.com):
> >>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.
> >
> 
> pgrp and session won't do because in the case we are discussing
> current->signal->tty is the same as tty.
> 
> This is the current check that is already in place:
>  | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>  | 	return -EPERM;

Yeah...

> The only thing I could find to detect the tty message coming from a
> container is as follows:
>  | task_active_pid_ns(current)->level
> 
> This will be zero when run on the host, but 1 when run inside a
> container. However this is very much a hack and could probably break
> some userland stuff where there are multiple levels of namespaces.

Yes.  This is also however why I don't like the current patch, because
capable() will never be true in a container, so nested containers break.

What does current->signal->tty->pgrp actually point to?  Can we take
it to be the pgrp which opened it?  Could we check
ns_capable(current_pid_ns()->user_ns, CAP_whatever) and get a meaningful
answer?

> The real problem is that there are no TTY namespaces. I don't think we
> can solve this problem for CAP_SYS_ADMIN containers unless we want to
> introduce a config that allows one to override normal CAP_SYS_ADMIN
> functionality by denying TIOCSTI ioctls for processes whom
> task_active_pid_ns(current)->level is equal to 0.
> 
> In the mean time, I think we can go ahead with this feature to give
> people the ability to lock down non CAP_SYS_ADMIN containers/processes.
> 
> >>>>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@nmatt.com>
> >>>>---
> >>>>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