Message-ID: <CAGXu5jKp1OUqGK5hhm9SA-0TiYa9AvH5HBW-Knj9f3Jb5CujQw@mail.gmail.com>
Date:   Tue, 18 Apr 2017 22:20:20 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Serge E. Hallyn" <serge@...lyn.com>, Matt Brown <matt@...tt.com>
Cc:     James Morris <jmorris@...ei.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jann Horn <jannh@...gle.com>,
        "kernel-hardening@...ts.openwall.com" 
        <kernel-hardening@...ts.openwall.com>,
        linux-security-module <linux-security-module@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN

On Tue, Apr 18, 2017 at 9:58 PM, Serge E. Hallyn <serge@...lyn.com> 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@...tt.com>

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

Powered by blists - more mailing lists