Message-ID: <a6b8f9ab-d5f8-51fb-0481-89907c43289f@nmatt.com>
Date:   Wed, 19 Apr 2017 19:21:51 -0400
From:   Matt Brown <matt@...tt.com>
To:     "Serge E. Hallyn" <serge@...lyn.com>
Cc:     jmorris@...ei.org, gregkh@...uxfoundation.org, jslaby@...e.com,
        akpm@...ux-foundation.org, jannh@...gle.com, keescook@...omium.org,
        kernel-hardening@...ts.openwall.com,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN

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@...tt.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

Powered by blists - more mailing lists