From: Matt Brown <matt@nmatt.com>
To: Kees Cook <keescook@chromium.org>, "Serge E. Hallyn" <serge@hallyn.com>
Cc: James Morris <jmorris@namei.org>,
Greg KH <gregkh@linuxfoundation.org>,
Jiri Slaby <jslaby@suse.com>,
Andrew Morton <akpm@linux-foundation.org>,
Jann Horn <jannh@google.com>,
"kernel-hardening@lists.openwall.com"
<kernel-hardening@lists.openwall.com>,
linux-security-module <linux-security-module@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN
Date: Wed, 19 Apr 2017 19:43:35 -0400 [thread overview]
Message-ID: <7772b08e-f637-57c3-6882-d596f291eab0@nmatt.com> (raw)
In-Reply-To: <CAGXu5jKp1OUqGK5hhm9SA-0TiYa9AvH5HBW-Knj9f3Jb5CujQw@mail.gmail.com>
On 04/19/2017 01:20 AM, Kees Cook wrote:
> On Tue, Apr 18, 2017 at 9:58 PM, Serge E. Hallyn <serge@hallyn.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@nmatt.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
>
next prev parent reply other threads:[~2017-04-19 23:44 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-19 3:45 [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN Matt Brown
2017-04-19 4:58 ` Serge E. Hallyn
2017-04-19 5:20 ` Kees Cook
2017-04-19 23:43 ` Matt Brown [this message]
2017-04-19 23:21 ` Matt Brown
2017-04-19 23:53 ` Serge E. Hallyn
2017-04-20 4:44 ` Matt Brown
2017-04-20 15:19 ` Serge E. Hallyn
2017-04-20 15:24 ` [kernel-hardening] " Serge E. Hallyn
2017-04-20 17:15 ` matt
2017-04-20 17:41 ` Serge E. Hallyn
2017-04-21 5:09 ` Matt Brown
2017-04-21 5:24 ` Serge E. Hallyn
2017-04-21 6:01 ` Kees Cook
2017-04-22 17:09 ` Matt Brown
2017-04-22 19:50 ` Serge E. Hallyn
2017-04-19 11:18 ` James Morris
2017-04-20 0:08 ` Matt Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7772b08e-f637-57c3-6882-d596f291eab0@nmatt.com \
--to=matt@nmatt.com \
--cc=akpm@linux-foundation.org \
--cc=gregkh@linuxfoundation.org \
--cc=jannh@google.com \
--cc=jmorris@namei.org \
--cc=jslaby@suse.com \
--cc=keescook@chromium.org \
--cc=kernel-hardening@lists.openwall.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=serge@hallyn.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).