make TIOCSTI ioctl require CAP_SYS_ADMIN

Commit Message

Matt Brown April 19, 2017, 3:45 a.m. UTC

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
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 <[e-mail]>
---
 drivers/tty/tty_io.c |  4 ++++
 include/linux/tty.h  |  2 ++
 kernel/sysctl.c      | 12 ++++++++++++
 security/Kconfig     | 13 +++++++++++++
 4 files changed, 31 insertions(+)

Comments

Serge E. Hallyn April 19, 2017, 4:58 a.m. UTC | #1

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 <[e-mail]>
> ---
>  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;
>  	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,
> +		.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?

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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook April 19, 2017, 5:20 a.m. UTC | #2

On Tue, Apr 18, 2017 at 9:58 PM, Serge E. Hallyn <[e-mail]> 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 <[e-mail]>

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

James Morris April 19, 2017, 11:18 a.m. UTC | #3

On Tue, 18 Apr 2017, Matt Brown wrote:

> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity
> project in-kernel.

It seems like an ugly hack to an ugly feature (CAP_SYS_ADMIN barely makes 
sense here), and rather than sprinkling these types of things throughout 
the kernel, I wonder if it might be better to implement it via LSM, in the 
YAMA module.



- James

Matt Brown April 19, 2017, 11:21 p.m. UTC | #4

On 04/19/2017 12:58 AM, 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: <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.
>

I assume you are talking about this CVE:
https://bugzilla.redhat.com/show_bug.cgi?id=1411256

In retrospect, is there any way that an escape from a privileged
container with the this bug could have been prevented?

>> 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 <[e-mail]>
>> ---
>>  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;
>>  	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,
>> +		.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?
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matt Brown April 19, 2017, 11:43 p.m. UTC | #5

On 04/19/2017 01:20 AM, Kees Cook wrote:
> On Tue, Apr 18, 2017 at 9:58 PM, Serge E. Hallyn <[e-mail]> 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 <[e-mail]>
>
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Serge E. Hallyn April 19, 2017, 11:53 p.m. UTC | #6

Quoting Matt Brown ([e-mail]):
> On 04/19/2017 12:58 AM, 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: <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.
> >
> 
> I assume you are talking about this CVE:
> https://bugzilla.redhat.com/show_bug.cgi?id=1411256
> 
> In retrospect, is there any way that an escape from a privileged
> container with the this bug could have been prevented?

I don't know, that's what I was probing for.  Detecting that the pgrp
or session - heck, the pid namespace - has changed would seem like a
good indicator that it shouldn't be able to push.

> >>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 <[e-mail]>
> >>---
> >> 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;
> >> 	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,
> >>+		.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?
> >
> >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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matt Brown April 20, 2017, 12:08 a.m. UTC | #7

On 04/19/2017 07:18 AM, James Morris wrote:
> On Tue, 18 Apr 2017, Matt Brown wrote:
>
>> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity
>> project in-kernel.
>
> It seems like an ugly hack to an ugly feature (CAP_SYS_ADMIN barely makes
> sense here), and rather than sprinkling these types of things throughout
> the kernel, I wonder if it might be better to implement it via LSM, in the
> YAMA module.
>
>

CAP_SYS_ADMIN is already used in the TIOCSTI TTY code to allow
character insertion into TTYs other than the caller's controlling
terminal. This is done because different TTYs indicate a security
boundary that should only be able to be crossed by a privileged
process. This patch would merely extend this security boundary
protection to include unprivileged processes from utilizing a common
TTY to step across a security boundary.

>
> - James
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matt Brown April 20, 2017, 4:44 a.m. UTC | #8

On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
> Quoting Matt Brown ([e-mail]):
>> On 04/19/2017 12:58 AM, 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: <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.
>>>
>>
>> I assume you are talking about this CVE:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256
>>
>> In retrospect, is there any way that an escape from a privileged
>> container with the this bug could have been prevented?
>
> I don't know, that's what I was probing for.  Detecting that the pgrp
> or session - heck, the pid namespace - has changed would seem like a
> good indicator that it shouldn't be able to push.
>

pgrp and session won't do because in the case we are discussing
current->signal->tty is the same as tty.

This is the current check that is already in place:
  | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
  | 	return -EPERM;

The only thing I could find to detect the tty message coming from a
container is as follows:
  | task_active_pid_ns(current)->level

This will be zero when run on the host, but 1 when run inside a
container. However this is very much a hack and could probably break
some userland stuff where there are multiple levels of namespaces.

The real problem is that there are no TTY namespaces. I don't think we
can solve this problem for CAP_SYS_ADMIN containers unless we want to
introduce a config that allows one to override normal CAP_SYS_ADMIN
functionality by denying TIOCSTI ioctls for processes whom
task_active_pid_ns(current)->level is equal to 0.

In the mean time, I think we can go ahead with this feature to give
people the ability to lock down non CAP_SYS_ADMIN containers/processes.

>>>> 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 <[e-mail]>
>>>> ---
>>>> 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;
>>>> 	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,
>>>> +		.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?
>>>
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Serge E. Hallyn April 20, 2017, 3:19 p.m. UTC | #9

Quoting Matt Brown ([e-mail]):
> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
> >Quoting Matt Brown ([e-mail]):
> >>On 04/19/2017 12:58 AM, 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: <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.
> >>>
> >>
> >>I assume you are talking about this CVE:
> >>https://bugzilla.redhat.com/show_bug.cgi?id=1411256
> >>
> >>In retrospect, is there any way that an escape from a privileged
> >>container with the this bug could have been prevented?
> >
> >I don't know, that's what I was probing for.  Detecting that the pgrp
> >or session - heck, the pid namespace - has changed would seem like a
> >good indicator that it shouldn't be able to push.
> >
> 
> pgrp and session won't do because in the case we are discussing
> current->signal->tty is the same as tty.
> 
> This is the current check that is already in place:
>  | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>  | 	return -EPERM;

Yeah...

> The only thing I could find to detect the tty message coming from a
> container is as follows:
>  | task_active_pid_ns(current)->level
> 
> This will be zero when run on the host, but 1 when run inside a
> container. However this is very much a hack and could probably break
> some userland stuff where there are multiple levels of namespaces.

Yes.  This is also however why I don't like the current patch, because
capable() will never be true in a container, so nested containers break.

What does current->signal->tty->pgrp actually point to?  Can we take
it to be the pgrp which opened it?  Could we check
ns_capable(current_pid_ns()->user_ns, CAP_whatever) and get a meaningful
answer?

> The real problem is that there are no TTY namespaces. I don't think we
> can solve this problem for CAP_SYS_ADMIN containers unless we want to
> introduce a config that allows one to override normal CAP_SYS_ADMIN
> functionality by denying TIOCSTI ioctls for processes whom
> task_active_pid_ns(current)->level is equal to 0.
> 
> In the mean time, I think we can go ahead with this feature to give
> people the ability to lock down non CAP_SYS_ADMIN containers/processes.
> 
> >>>>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 <[e-mail]>
> >>>>---
> >>>>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;
> >>>>	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,
> >>>>+		.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?
> >>>
> >>>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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Serge E. Hallyn April 20, 2017, 3:24 p.m. UTC | #10

Quoting Serge E. Hallyn ([e-mail]):
> Quoting Matt Brown ([e-mail]):
> > On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
> > >Quoting Matt Brown ([e-mail]):
> > >>On 04/19/2017 12:58 AM, 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: <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.
> > >>>
> > >>
> > >>I assume you are talking about this CVE:
> > >>https://bugzilla.redhat.com/show_bug.cgi?id=1411256
> > >>
> > >>In retrospect, is there any way that an escape from a privileged
> > >>container with the this bug could have been prevented?
> > >
> > >I don't know, that's what I was probing for.  Detecting that the pgrp
> > >or session - heck, the pid namespace - has changed would seem like a
> > >good indicator that it shouldn't be able to push.
> > >
> > 
> > pgrp and session won't do because in the case we are discussing
> > current->signal->tty is the same as tty.
> > 
> > This is the current check that is already in place:
> >  | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
> >  | 	return -EPERM;
> 
> Yeah...
> 
> > The only thing I could find to detect the tty message coming from a
> > container is as follows:
> >  | task_active_pid_ns(current)->level
> > 
> > This will be zero when run on the host, but 1 when run inside a
> > container. However this is very much a hack and could probably break
> > some userland stuff where there are multiple levels of namespaces.
> 
> Yes.  This is also however why I don't like the current patch, because
> capable() will never be true in a container, so nested containers break.
> 
> What does current->signal->tty->pgrp actually point to?  Can we take
> it to be the pgrp which opened it?  Could we check
> ns_capable(current_pid_ns()->user_ns, CAP_whatever) and get a meaningful
> answer?
> 

Ok I see that's meaningless, you can't get pidns from pid.  We could
instead add a user_ns *owner to the struct tty and store the user_ns
of the task which opened it.  It's more invasive, but also more
meaningful.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matt Brown April 20, 2017, 5:15 p.m. UTC | #11

On 2017-04-20 11:19, Serge E. Hallyn wrote:
> Quoting Matt Brown ([e-mail]):
>> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
>> >Quoting Matt Brown ([e-mail]):
>> >>On 04/19/2017 12:58 AM, 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: <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.
>> >>>
>> >>
>> >>I assume you are talking about this CVE:
>> >>https://bugzilla.redhat.com/show_bug.cgi?id=1411256
>> >>
>> >>In retrospect, is there any way that an escape from a privileged
>> >>container with the this bug could have been prevented?
>> >
>> >I don't know, that's what I was probing for.  Detecting that the pgrp
>> >or session - heck, the pid namespace - has changed would seem like a
>> >good indicator that it shouldn't be able to push.
>> >
>> 
>> pgrp and session won't do because in the case we are discussing
>> current->signal->tty is the same as tty.
>> 
>> This is the current check that is already in place:
>>  | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>>  | 	return -EPERM;
> 
> Yeah...
> 
>> The only thing I could find to detect the tty message coming from a
>> container is as follows:
>>  | task_active_pid_ns(current)->level
>> 
>> This will be zero when run on the host, but 1 when run inside a
>> container. However this is very much a hack and could probably break
>> some userland stuff where there are multiple levels of namespaces.
> 
> Yes.  This is also however why I don't like the current patch, because
> capable() will never be true in a container, so nested containers 
> break.
> 

What do you mean by "capable() will never be true in a container"? My 
understanding
is that if a container is given CAP_SYS_ADMIN then 
capable(CAP_SYS_ADMIN) will return
true? I agree the hack I mentioned above would be a bad idea because it 
would break
nested containers, but the current patch would not IMO.

A better version of the hack could involve a config 
CONFIG_TIOCSTI_MAX_NS_LEVEL where
a check would be performed to ensure that 
task_active_pid_ns(current)->level is not
greater than the config value(an integer that is >= 0) .

Again, I think we both would agree that this is not the best solution. 
The clear
downside is that you could have multiple container layers where the 
desired security
boundaries happened to fall at different levels. Just throwing ideas 
around.

> What does current->signal->tty->pgrp actually point to?  Can we take
> it to be the pgrp which opened it?  Could we check
> ns_capable(current_pid_ns()->user_ns, CAP_whatever) and get a 
> meaningful
> answer?
> 
>> The real problem is that there are no TTY namespaces. I don't think we
>> can solve this problem for CAP_SYS_ADMIN containers unless we want to
>> introduce a config that allows one to override normal CAP_SYS_ADMIN
>> functionality by denying TIOCSTI ioctls for processes whom
>> task_active_pid_ns(current)->level is equal to 0.
>> 
>> In the mean time, I think we can go ahead with this feature to give
>> people the ability to lock down non CAP_SYS_ADMIN 
>> containers/processes.
>> 
>> >>>>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 <[e-mail]>
>> >>>>---
>> >>>>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;
>> >>>>	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,
>> >>>>+		.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?
>> >>>
>> >>>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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Serge E. Hallyn April 20, 2017, 5:41 p.m. UTC | #12

Quoting [e-mail] ([e-mail]):
> On 2017-04-20 11:19, Serge E. Hallyn wrote:
> >Quoting Matt Brown ([e-mail]):
> >>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
> >>>Quoting Matt Brown ([e-mail]):
> >>>>On 04/19/2017 12:58 AM, 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: <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.
> >>>>>
> >>>>
> >>>>I assume you are talking about this CVE:
> >>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256
> >>>>
> >>>>In retrospect, is there any way that an escape from a privileged
> >>>>container with the this bug could have been prevented?
> >>>
> >>>I don't know, that's what I was probing for.  Detecting that the pgrp
> >>>or session - heck, the pid namespace - has changed would seem like a
> >>>good indicator that it shouldn't be able to push.
> >>>
> >>
> >>pgrp and session won't do because in the case we are discussing
> >>current->signal->tty is the same as tty.
> >>
> >>This is the current check that is already in place:
> >> | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
> >> | 	return -EPERM;
> >
> >Yeah...
> >
> >>The only thing I could find to detect the tty message coming from a
> >>container is as follows:
> >> | task_active_pid_ns(current)->level
> >>
> >>This will be zero when run on the host, but 1 when run inside a
> >>container. However this is very much a hack and could probably break
> >>some userland stuff where there are multiple levels of namespaces.
> >
> >Yes.  This is also however why I don't like the current patch, because
> >capable() will never be true in a container, so nested containers
> >break.
> >
> 
> What do you mean by "capable() will never be true in a container"?
> My understanding
> is that if a container is given CAP_SYS_ADMIN then
> capable(CAP_SYS_ADMIN) will return
> true?

No, capable(X) checks for X with respect to the initial user namespace.
So for root-owned containers it will be true, but containers running in
non-initial user namespaces cannot pass that check.

To check for privilege with respect to another user namespace, you need
to use ns_capable.  But for that you need a user_ns to target.

>  I agree the hack I mentioned above would be a bad idea because
> it would break
> nested containers, but the current patch would not IMO.
> 
> A better version of the hack could involve a config
> CONFIG_TIOCSTI_MAX_NS_LEVEL where
> a check would be performed to ensure that
> task_active_pid_ns(current)->level is not
> greater than the config value(an integer that is >= 0) .

Yeah.  That would break a different set of cases than the capable
check, I assume.  A smaller set, I think.

> Again, I think we both would agree that this is not the best
> solution. The clear
> downside is that you could have multiple container layers where the
> desired security
> boundaries happened to fall at different levels. Just throwing ideas
> around.

Yup, appreciated.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Matt Brown April 21, 2017, 5:09 a.m. UTC | #13

On 04/20/2017 01:41 PM, Serge E. Hallyn wrote:
> Quoting [e-mail] ([e-mail]):
>> On 2017-04-20 11:19, Serge E. Hallyn wrote:
>>> Quoting Matt Brown ([e-mail]):
>>>> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
>>>>> Quoting Matt Brown ([e-mail]):
>>>>>> On 04/19/2017 12:58 AM, 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: <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.
>>>>>>>
>>>>>>
>>>>>> I assume you are talking about this CVE:
>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256
>>>>>>
>>>>>> In retrospect, is there any way that an escape from a privileged
>>>>>> container with the this bug could have been prevented?
>>>>>
>>>>> I don't know, that's what I was probing for.  Detecting that the pgrp
>>>>> or session - heck, the pid namespace - has changed would seem like a
>>>>> good indicator that it shouldn't be able to push.
>>>>>
>>>>
>>>> pgrp and session won't do because in the case we are discussing
>>>> current->signal->tty is the same as tty.
>>>>
>>>> This is the current check that is already in place:
>>>> | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>>>> | 	return -EPERM;
>>>
>>> Yeah...
>>>
>>>> The only thing I could find to detect the tty message coming from a
>>>> container is as follows:
>>>> | task_active_pid_ns(current)->level
>>>>
>>>> This will be zero when run on the host, but 1 when run inside a
>>>> container. However this is very much a hack and could probably break
>>>> some userland stuff where there are multiple levels of namespaces.
>>>
>>> Yes.  This is also however why I don't like the current patch, because
>>> capable() will never be true in a container, so nested containers
>>> break.
>>>
>>
>> What do you mean by "capable() will never be true in a container"?
>> My understanding
>> is that if a container is given CAP_SYS_ADMIN then
>> capable(CAP_SYS_ADMIN) will return
>> true?
>
> No, capable(X) checks for X with respect to the initial user namespace.
> So for root-owned containers it will be true, but containers running in
> non-initial user namespaces cannot pass that check.
>
> To check for privilege with respect to another user namespace, you need
> to use ns_capable.  But for that you need a user_ns to target.
>

How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ?

current_user_ns() was found in include/linux/cred.h

>>  I agree the hack I mentioned above would be a bad idea because
>> it would break
>> nested containers, but the current patch would not IMO.
>>
>> A better version of the hack could involve a config
>> CONFIG_TIOCSTI_MAX_NS_LEVEL where
>> a check would be performed to ensure that
>> task_active_pid_ns(current)->level is not
>> greater than the config value(an integer that is >= 0) .
>
> Yeah.  That would break a different set of cases than the capable
> check, I assume.  A smaller set, I think.
>
>> Again, I think we both would agree that this is not the best
>> solution. The clear
>> downside is that you could have multiple container layers where the
>> desired security
>> boundaries happened to fall at different levels. Just throwing ideas
>> around.
>
> Yup, appreciated.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Serge E. Hallyn April 21, 2017, 5:24 a.m. UTC | #14

On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote:
> On 04/20/2017 01:41 PM, Serge E. Hallyn wrote:
> >Quoting [e-mail] ([e-mail]):
> >>On 2017-04-20 11:19, Serge E. Hallyn wrote:
> >>>Quoting Matt Brown ([e-mail]):
> >>>>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
> >>>>>Quoting Matt Brown ([e-mail]):
> >>>>>>On 04/19/2017 12:58 AM, 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: <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.
> >>>>>>>
> >>>>>>
> >>>>>>I assume you are talking about this CVE:
> >>>>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256
> >>>>>>
> >>>>>>In retrospect, is there any way that an escape from a privileged
> >>>>>>container with the this bug could have been prevented?
> >>>>>
> >>>>>I don't know, that's what I was probing for.  Detecting that the pgrp
> >>>>>or session - heck, the pid namespace - has changed would seem like a
> >>>>>good indicator that it shouldn't be able to push.
> >>>>>
> >>>>
> >>>>pgrp and session won't do because in the case we are discussing
> >>>>current->signal->tty is the same as tty.
> >>>>
> >>>>This is the current check that is already in place:
> >>>>| if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
> >>>>| 	return -EPERM;
> >>>
> >>>Yeah...
> >>>
> >>>>The only thing I could find to detect the tty message coming from a
> >>>>container is as follows:
> >>>>| task_active_pid_ns(current)->level
> >>>>
> >>>>This will be zero when run on the host, but 1 when run inside a
> >>>>container. However this is very much a hack and could probably break
> >>>>some userland stuff where there are multiple levels of namespaces.
> >>>
> >>>Yes.  This is also however why I don't like the current patch, because
> >>>capable() will never be true in a container, so nested containers
> >>>break.
> >>>
> >>
> >>What do you mean by "capable() will never be true in a container"?
> >>My understanding
> >>is that if a container is given CAP_SYS_ADMIN then
> >>capable(CAP_SYS_ADMIN) will return
> >>true?
> >
> >No, capable(X) checks for X with respect to the initial user namespace.
> >So for root-owned containers it will be true, but containers running in
> >non-initial user namespaces cannot pass that check.
> >
> >To check for privilege with respect to another user namespace, you need
> >to use ns_capable.  But for that you need a user_ns to target.
> >
> 
> How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ?
> 
> current_user_ns() was found in include/linux/cred.h

Any user can create a new user namespace and pass the above check.  What we
want is to find the user namespace which opened the tty.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook April 21, 2017, 6:01 a.m. UTC | #15

On Thu, Apr 20, 2017 at 10:24 PM, Serge E. Hallyn <[e-mail]> wrote:
> On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote:
>> On 04/20/2017 01:41 PM, Serge E. Hallyn wrote:
>> >Quoting [e-mail] ([e-mail]):
>> >>On 2017-04-20 11:19, Serge E. Hallyn wrote:
>> >>>Quoting Matt Brown ([e-mail]):
>> >>>>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
>> >>>>>Quoting Matt Brown ([e-mail]):
>> >>>>>>On 04/19/2017 12:58 AM, 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: <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.
>> >>>>>>>
>> >>>>>>
>> >>>>>>I assume you are talking about this CVE:
>> >>>>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256
>> >>>>>>
>> >>>>>>In retrospect, is there any way that an escape from a privileged
>> >>>>>>container with the this bug could have been prevented?
>> >>>>>
>> >>>>>I don't know, that's what I was probing for.  Detecting that the pgrp
>> >>>>>or session - heck, the pid namespace - has changed would seem like a
>> >>>>>good indicator that it shouldn't be able to push.
>> >>>>>
>> >>>>
>> >>>>pgrp and session won't do because in the case we are discussing
>> >>>>current->signal->tty is the same as tty.
>> >>>>
>> >>>>This is the current check that is already in place:
>> >>>>| if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>> >>>>|         return -EPERM;
>> >>>
>> >>>Yeah...
>> >>>
>> >>>>The only thing I could find to detect the tty message coming from a
>> >>>>container is as follows:
>> >>>>| task_active_pid_ns(current)->level
>> >>>>
>> >>>>This will be zero when run on the host, but 1 when run inside a
>> >>>>container. However this is very much a hack and could probably break
>> >>>>some userland stuff where there are multiple levels of namespaces.
>> >>>
>> >>>Yes.  This is also however why I don't like the current patch, because
>> >>>capable() will never be true in a container, so nested containers
>> >>>break.
>> >>>
>> >>
>> >>What do you mean by "capable() will never be true in a container"?
>> >>My understanding
>> >>is that if a container is given CAP_SYS_ADMIN then
>> >>capable(CAP_SYS_ADMIN) will return
>> >>true?
>> >
>> >No, capable(X) checks for X with respect to the initial user namespace.
>> >So for root-owned containers it will be true, but containers running in
>> >non-initial user namespaces cannot pass that check.
>> >
>> >To check for privilege with respect to another user namespace, you need
>> >to use ns_capable.  But for that you need a user_ns to target.
>> >
>>
>> How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ?
>>
>> current_user_ns() was found in include/linux/cred.h
>
> Any user can create a new user namespace and pass the above check.  What we
> want is to find the user namespace which opened the tty.

Can we use file->cred->user_ns? Hm, but I see there isn't really a
single file associated with tty_struct.

-Kees

Matt Brown April 22, 2017, 5:09 p.m. UTC | #16

On 04/21/2017 01:24 AM, Serge E. Hallyn wrote:
> On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote:
>> On 04/20/2017 01:41 PM, Serge E. Hallyn wrote:
>>> Quoting [e-mail] ([e-mail]):
>>>> On 2017-04-20 11:19, Serge E. Hallyn wrote:
>>>>> Quoting Matt Brown ([e-mail]):
>>>>>> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
>>>>>>> Quoting Matt Brown ([e-mail]):
>>>>>>>> On 04/19/2017 12:58 AM, 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: <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.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I assume you are talking about this CVE:
>>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256
>>>>>>>>
>>>>>>>> In retrospect, is there any way that an escape from a privileged
>>>>>>>> container with the this bug could have been prevented?
>>>>>>>
>>>>>>> I don't know, that's what I was probing for.  Detecting that the pgrp
>>>>>>> or session - heck, the pid namespace - has changed would seem like a
>>>>>>> good indicator that it shouldn't be able to push.
>>>>>>>
>>>>>>
>>>>>> pgrp and session won't do because in the case we are discussing
>>>>>> current->signal->tty is the same as tty.
>>>>>>
>>>>>> This is the current check that is already in place:
>>>>>> | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>>>>>> | 	return -EPERM;
>>>>>
>>>>> Yeah...
>>>>>
>>>>>> The only thing I could find to detect the tty message coming from a
>>>>>> container is as follows:
>>>>>> | task_active_pid_ns(current)->level
>>>>>>
>>>>>> This will be zero when run on the host, but 1 when run inside a
>>>>>> container. However this is very much a hack and could probably break
>>>>>> some userland stuff where there are multiple levels of namespaces.
>>>>>
>>>>> Yes.  This is also however why I don't like the current patch, because
>>>>> capable() will never be true in a container, so nested containers
>>>>> break.
>>>>>
>>>>
>>>> What do you mean by "capable() will never be true in a container"?
>>>> My understanding
>>>> is that if a container is given CAP_SYS_ADMIN then
>>>> capable(CAP_SYS_ADMIN) will return
>>>> true?
>>>
>>> No, capable(X) checks for X with respect to the initial user namespace.
>>> So for root-owned containers it will be true, but containers running in
>>> non-initial user namespaces cannot pass that check.
>>>
>>> To check for privilege with respect to another user namespace, you need
>>> to use ns_capable.  But for that you need a user_ns to target.
>>>
>>
>> How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ?
>>
>> current_user_ns() was found in include/linux/cred.h
>
> Any user can create a new user namespace and pass the above check.  What we
> want is to find the user namespace which opened the tty.
>

I believe I have a working solution that I can show in the next version
of the patch later today, but I just want to run the logic by you first.

I added: "struct user_namespace *owner_user_ns;" as a field in
tty_struct (include/linux/tty.h) Note: I am totally open to suggestions
for a better name.

Then I added "tty->owner_user_ns = current_user_ns();" to the
alloc_tty_struct function. (drivers/tty/tty_io.c)

When testing with a docker container, running in a different user
namespace, I printed out current_user_ns()->level, which returned 1,
and tty->owner_user_ns->level, which returned 0. This seems to prove
that I am correctly storing the user namespace which opened the tty.

Please let me know if there are any edge cases that I am missing with
this approach.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Serge E. Hallyn April 22, 2017, 7:50 p.m. UTC | #17

Quoting Matt Brown ([e-mail]):
> On 04/21/2017 01:24 AM, Serge E. Hallyn wrote:
> >On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote:
> >>On 04/20/2017 01:41 PM, Serge E. Hallyn wrote:
> >>>Quoting [e-mail] ([e-mail]):
> >>>>On 2017-04-20 11:19, Serge E. Hallyn wrote:
> >>>>>Quoting Matt Brown ([e-mail]):
> >>>>>>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote:
> >>>>>>>Quoting Matt Brown ([e-mail]):
> >>>>>>>>On 04/19/2017 12:58 AM, 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: <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.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>I assume you are talking about this CVE:
> >>>>>>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256
> >>>>>>>>
> >>>>>>>>In retrospect, is there any way that an escape from a privileged
> >>>>>>>>container with the this bug could have been prevented?
> >>>>>>>
> >>>>>>>I don't know, that's what I was probing for.  Detecting that the pgrp
> >>>>>>>or session - heck, the pid namespace - has changed would seem like a
> >>>>>>>good indicator that it shouldn't be able to push.
> >>>>>>>
> >>>>>>
> >>>>>>pgrp and session won't do because in the case we are discussing
> >>>>>>current->signal->tty is the same as tty.
> >>>>>>
> >>>>>>This is the current check that is already in place:
> >>>>>>| if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
> >>>>>>| 	return -EPERM;
> >>>>>
> >>>>>Yeah...
> >>>>>
> >>>>>>The only thing I could find to detect the tty message coming from a
> >>>>>>container is as follows:
> >>>>>>| task_active_pid_ns(current)->level
> >>>>>>
> >>>>>>This will be zero when run on the host, but 1 when run inside a
> >>>>>>container. However this is very much a hack and could probably break
> >>>>>>some userland stuff where there are multiple levels of namespaces.
> >>>>>
> >>>>>Yes.  This is also however why I don't like the current patch, because
> >>>>>capable() will never be true in a container, so nested containers
> >>>>>break.
> >>>>>
> >>>>
> >>>>What do you mean by "capable() will never be true in a container"?
> >>>>My understanding
> >>>>is that if a container is given CAP_SYS_ADMIN then
> >>>>capable(CAP_SYS_ADMIN) will return
> >>>>true?
> >>>
> >>>No, capable(X) checks for X with respect to the initial user namespace.
> >>>So for root-owned containers it will be true, but containers running in
> >>>non-initial user namespaces cannot pass that check.
> >>>
> >>>To check for privilege with respect to another user namespace, you need
> >>>to use ns_capable.  But for that you need a user_ns to target.
> >>>
> >>
> >>How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ?
> >>
> >>current_user_ns() was found in include/linux/cred.h
> >
> >Any user can create a new user namespace and pass the above check.  What we
> >want is to find the user namespace which opened the tty.
> >
> 
> I believe I have a working solution that I can show in the next version
> of the patch later today, but I just want to run the logic by you first.
> 
> I added: "struct user_namespace *owner_user_ns;" as a field in
> tty_struct (include/linux/tty.h) Note: I am totally open to suggestions
> for a better name.
> 
> Then I added "tty->owner_user_ns = current_user_ns();" to the
> alloc_tty_struct function. (drivers/tty/tty_io.c)

That's what I was hoping could work.  Then you can check ns_capable
with respect to that.

You'll want to grab a reference to the user_ns, and drop it on
final close, but otherwise this sounds good to me.  I don't really
know the tty layer well though so we'll need some sanity checking
from someone who does.

> When testing with a docker container, running in a different user
> namespace, I printed out current_user_ns()->level, which returned 1,
> and tty->owner_user_ns->level, which returned 0. This seems to prove
> that I am correctly storing the user namespace which opened the tty.
> 
> Please let me know if there are any edge cases that I am missing with
> this approach.

Thanks for posting this!  This seems like the best solution to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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