Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN

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

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.

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