Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN
From: Kees Cook
Date: Wed Apr 19 2017 - 01:20:30 EST
On Tue, Apr 18, 2017 at 9:58 PM, Serge E. Hallyn <serge@xxxxxxxxxx> 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.
>
>> 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>
Thanks for working on this! I think it'll be nice to have available.
>> ---
>> 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;
I wonder if it might be worth adding a pr_warn_ratelimited() here to
help people identify either programs that want to use this feature or
actual attacks?
>> 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,
Since this is a new sysctl, it'll need to get documented in
Documentation/sysctl/kernel.txt as part of this patch.
>> + .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?
This is modeled after SECURITY_DMESG_RESTRICT. I think the Kconfig
name is fine (given the other one), but it'd be worth maybe
reorganizing the commit message to say "this introduces
tiocsti_restrict sysctl, whose default is controlled via
CONFIG_SECURITY_TIOCSTI_RESTRICT" or similar. Right now the commit
message doesn't, I don't think, make clear what the config does.
>
> 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
-Kees
--
Kees Cook
Pixel Security