From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967656AbdDSXoN (ORCPT ); Wed, 19 Apr 2017 19:44:13 -0400 Received: from relay7-d.mail.gandi.net ([217.70.183.200]:58728 "EHLO relay7-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967533AbdDSXoK (ORCPT ); Wed, 19 Apr 2017 19:44:10 -0400 X-Originating-IP: 72.66.113.207 Subject: Re: [PATCH] make TIOCSTI ioctl require CAP_SYS_ADMIN To: Kees Cook , "Serge E. Hallyn" References: <20170419034526.18565-1-matt@nmatt.com> <20170419045813.GA17990@mail.hallyn.com> Cc: James Morris , Greg KH , Jiri Slaby , Andrew Morton , Jann Horn , "kernel-hardening@lists.openwall.com" , linux-security-module , LKML From: Matt Brown Message-ID: <7772b08e-f637-57c3-6882-d596f291eab0@nmatt.com> Date: Wed, 19 Apr 2017 19:43:35 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/19/2017 01:20 AM, Kees Cook wrote: > On Tue, Apr 18, 2017 at 9:58 PM, 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: >>> 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: >>> >>> >>> Signed-off-by: Matt Brown > > 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 >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -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 >