Message-ID: <7772b08e-f637-57c3-6882-d596f291eab0@nmatt.com>
Date:   Wed, 19 Apr 2017 19:43:35 -0400
From:   Matt Brown <matt@...tt.com>
To:     Kees Cook <keescook@...omium.org>,
        "Serge E. Hallyn" <serge@...lyn.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 04/19/2017 01:20 AM, Kees Cook wrote:
> 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?
>

I can include that in the next version of the patch. Any suggestions on
what the warning ought to say?

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

Will add in next patch version.

>
>>> +             .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.
>

I will reorganize the commit message as you suggested. As for the
Kconfig name, I'm open to calling it something else. However, I thought
basing it off of SECURITY_DMESG_RESTRICT made sense.

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

Powered by blists - more mailing lists