Patchset to Restrict Unprivileged TIOCSTI TTY Command Injection

Patchset to Restrict Unprivileged TIOCSTI TTY Command Injection

[Matt Brown]

The following patchset reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity project in-kernel. The purpose of this feature is to restrict unprivileged users from injecting commands into other processes in the same tty session by using the TIOCSTI ioctl.

It creates the kernel config SECURITY_TIOCSTI_RESTRICT and the sysctl kernel.tiocsti_restrict to control this feature. I modeled most of the code style and naming conventions off of SECURITY_DMESG_RESTRICT.

 drivers/tty/tty_io.c |  4 ++++
 include/linux/tty.h  |  2 ++
 kernel/sysctl.c      | 12 ++++++++++++
 security/Kconfig     | 12 ++++++++++++
 4 files changed, 30 insertions(+)

[PATCH 1/4] added SECURITY_TIOCSTI_RESTRICT kernel config
[PATCH 2/4] add tiocsti_restrict variable
[PATCH 3/4] restrict unprivileged TIOCSTI tty ioctl
[PATCH 4/4] added kernel.tiocsti_restrict sysctl

[PATCH 1/4] added SECURITY_TIOCSTI_RESTRICT kernel config

[Matt Brown]

adding the kernel config SECURITY_TIOCSTI_RESTRICT in order to allow
the user to restrict unprivileged command injection using TIOCSTI
tty ioctls

Signed-off-by: Matt Brown <matt@nmatt.com>
---
 security/Kconfig | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/security/Kconfig b/security/Kconfig
index 3ff1bf9..d757bcb 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -18,6 +18,18 @@ 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 in the same tty session using the TIOCSTI ioctl
+
+	  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

Re: [PATCH 1/4] added SECURITY_TIOCSTI_RESTRICT kernel config

[Greg KH]

On Mon, Apr 17, 2017 at 02:07:03AM -0400, Matt Brown wrote:
> adding the kernel config SECURITY_TIOCSTI_RESTRICT in order to allow
> the user to restrict unprivileged command injection using TIOCSTI
> tty ioctls

"unpriviledged command injection"?  That sounds a bit "odd", don't you
think?

> 
> Signed-off-by: Matt Brown <matt@nmatt.com>
> ---
>  security/Kconfig | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/security/Kconfig b/security/Kconfig
> index 3ff1bf9..d757bcb 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -18,6 +18,18 @@ 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 in the same tty session using the TIOCSTI ioctl

Tabs and spaces?

Since tty sessions are usually separated by different users, how would
they have the same one and yet need something like this?

Also, why not put this in the tty config section?

And finally, this patch on its own doesn't do anything :(

thanks,

greg k-h

Re: [PATCH 1/4] added SECURITY_TIOCSTI_RESTRICT kernel config

[Matt Brown]

On 04/17/2017 02:50 AM, Greg KH wrote:
> On Mon, Apr 17, 2017 at 02:07:03AM -0400, Matt Brown wrote:
>> adding the kernel config SECURITY_TIOCSTI_RESTRICT in order to allow
>> the user to restrict unprivileged command injection using TIOCSTI
>> tty ioctls
>
> "unpriviledged command injection"?  That sounds a bit "odd", don't you
> think?
>
>>
>> Signed-off-by: Matt Brown <matt@nmatt.com>
>> ---
>>  security/Kconfig | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/security/Kconfig b/security/Kconfig
>> index 3ff1bf9..d757bcb 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -18,6 +18,18 @@ 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 in the same tty session using the TIOCSTI ioctl
>
> Tabs and spaces?
>

Sorry about that. Used the wrong vimrc for part of the Kconfig. Will
fix in updated patch.

> Since tty sessions are usually separated by different users, how would
> they have the same one and yet need something like this?
>
> Also, why not put this in the tty config section?
>
> And finally, this patch on its own doesn't do anything :(
>

I will take your input, update my code, and resubmit as a single
patch.

> thanks,
>
> greg k-h
>

Re: [PATCH 1/4] added SECURITY_TIOCSTI_RESTRICT kernel config

[Alan Cox]

> Since tty sessions are usually separated by different users, how would
> they have the same one and yet need something like this?
> 
> Also, why not put this in the tty config section?

The normal attack use case people argue about is a rogue process on the
users machine sitting there waiting until the user has logged in to a
remote machine and is idle and then doing stuff.

Attackers of course don't bother doing that because it's easier to get
the user to run a different ssh client instead.

Alan

Re: [kernel-hardening] Re: [PATCH 1/4] added SECURITY_TIOCSTI_RESTRICT kernel config

[Kees Cook]

On Tue, Apr 18, 2017 at 6:40 AM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>> Since tty sessions are usually separated by different users, how would
>> they have the same one and yet need something like this?
>>
>> Also, why not put this in the tty config section?
>
> The normal attack use case people argue about is a rogue process on the
> users machine sitting there waiting until the user has logged in to a
> remote machine and is idle and then doing stuff.

It's still a threat, though, and adding this with default n to allow
the more paranoid builders a chance to mitigate it seems reasonable to
me.

> Attackers of course don't bother doing that because it's easier to get
> the user to run a different ssh client instead.

Attackers will always take the easiest route, so we have to keep
killing the low hanging fruit.

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 2/4] add tiocsti_restrict variable

[Matt Brown]

adding extern variable tiocsti_restrict to allow it to be included
in sysctl

Signed-off-by: Matt Brown <matt@nmatt.com>
---
 include/linux/tty.h | 2 ++
 1 file changed, 2 insertions(+)

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

Re: [PATCH 2/4] add tiocsti_restrict variable

[Greg KH]

On Mon, Apr 17, 2017 at 02:07:04AM -0400, Matt Brown wrote:
> adding extern variable tiocsti_restrict to allow it to be included
> in sysctl
> 
> Signed-off-by: Matt Brown <matt@nmatt.com>
> ---
>  include/linux/tty.h | 2 ++
>  1 file changed, 2 insertions(+)

I'm all for breaking up changes to be tiny logical pieces, but this is
really not needed.  Please make it "one logical feature" per patch.

thanks,

greg k-h

[PATCH 3/4] restrict unprivileged TIOCSTI tty ioctl

[Matt Brown]

this patch depends on patch 1 and 2

enforces restrictions on unprivileged users injecting commands
into other processes in the same tty session using the TIOCSTI ioctl

Signed-off-by: Matt Brown <matt@nmatt.com>
---
 drivers/tty/tty_io.c | 4 ++++
 1 file changed, 4 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))
-- 
2.10.2

Re: [PATCH 3/4] restrict unprivileged TIOCSTI tty ioctl

[Greg KH]

On Mon, Apr 17, 2017 at 02:07:05AM -0400, Matt Brown wrote:
> this patch depends on patch 1 and 2
> 
> enforces restrictions on unprivileged users injecting commands
> into other processes in the same tty session using the TIOCSTI ioctl
> 
> Signed-off-by: Matt Brown <matt@nmatt.com>
> ---
>  drivers/tty/tty_io.c | 4 ++++
>  1 file changed, 4 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;

So, what type of "normal" userspace operations did you just break here?
What type of "not normal" did you break/change?

Why tie this to CAP_SYS_ADMIN as well?  That wasn't listed in your
Kconfig help text.  This seems like an additional capabilities
dependancy that odds are, most people do not want...

>  	if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>  		return -EPERM;

And finally, why doesn't this original check handle what you want to do
already?

I don't understand your "threat model" you wish to address by this
change series, please be a lot more explicit in your patch changelog
descriptions.

thanks,

greg k-h

Re: [kernel-hardening] Re: [PATCH 3/4] restrict unprivileged TIOCSTI tty ioctl

[Jann Horn]

On Mon, Apr 17, 2017 at 8:53 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Apr 17, 2017 at 02:07:05AM -0400, Matt Brown wrote:
>> this patch depends on patch 1 and 2
>>
>> enforces restrictions on unprivileged users injecting commands
>> into other processes in the same tty session using the TIOCSTI ioctl
>>
>> Signed-off-by: Matt Brown <matt@nmatt.com>
>> ---
>>  drivers/tty/tty_io.c | 4 ++++
>>  1 file changed, 4 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;
>
> So, what type of "normal" userspace operations did you just break here?
> What type of "not normal" did you break/change?

Looking at
<https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>,
it looks like there are a couple users whose behavior would probably
change when run by unprivileged users - in particular agetty, csh, xemacs
and tcsh.


> Why tie this to CAP_SYS_ADMIN as well?  That wasn't listed in your
> Kconfig help text.  This seems like an additional capabilities
> dependancy that odds are, most people do not want...
>
>>       if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>>               return -EPERM;

I think CAP_SYS_ADMIN is a logical choice here. AFAIK
CAP_SYS_ADMIN is normally used for random functionality that permits
users to bypass normal access controls without constraints that clearly
map to existing capabilities. See seccomp() without NNP,
proc_dointvec_minmax_sysadmin() and so on.

I guess you could also argue for CAP_SYS_TTY_CONFIG, but I don't
think that fits well, given that this is more about using a TTY in a
privileged way than about configuring it.


> And finally, why doesn't this original check handle what you want to do
> already?
>
> I don't understand your "threat model" you wish to address by this
> change series, please be a lot more explicit in your patch changelog
> descriptions.

While I don't know what Matt Brown's threat model is here, I like the
patch for the following reason:

For me, there are two usecases for having UIDs on a Linux
system. The first usecase is to isolate human users from each other.
The second usecase are service accounts, where the same human
administrator has access to multiple UIDs, each of which is used for
one specific service, reducing the impact of a compromise of a single
service.

In the "isolated services" usecase, the machine's administrator needs
to be able to access the various service accounts in some way. One
way that integrates well with existing tools is to log in as root, then
use su or sudo to run a shell with the service's UID with reduced
privileges.

The only issue I know of that makes this dangerous in the default
configuration is that the tty file descriptor becomes accessible to the
shell running under the service's UID, allowing the service to e.g.
use ptrace() or .bashrc or so to inject code into the service-privileged
shell, then abuse TIOCSTI to take over root's shell.

So, the reason the additional check is necessary is that there are
usecases where in practice, TTY file descriptors are shared over
privilege boundaries, and thereforce, access to the TTY fd should not
automatically grant the level of access that is normally only available
using a PTY master fd.

sudo can mitigate this using the use_pty config option, which creates
a new PTY and forwards I/O to and from that PTY, but this option is
off by default. I think the version of su that e.g. ships in Debian can't
even be configured to mitigate this.

In my opinion, while it's possible for system administrators or
programmers to avoid this pitfall if they know enough about how TTY
devices work, this behavior is highly unintuitive.

Also, quoting part of the help text for grsecurity's config option
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.

Basically, TIOCSTI permits exactly what Yama is designed to
prevent: It lets different processes in one session compromise
each other.


For context, in case someone in this thread hasn't seen it yet:
http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/
is a writeup about the issue and some prior discussions about it.

Re: [kernel-hardening] Re: [PATCH 3/4] restrict unprivileged TIOCSTI tty ioctl

[Matt Brown]

On 04/17/2017 10:18 AM, Jann Horn wrote:
> On Mon, Apr 17, 2017 at 8:53 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Mon, Apr 17, 2017 at 02:07:05AM -0400, Matt Brown wrote:
>>> this patch depends on patch 1 and 2
>>>
>>> enforces restrictions on unprivileged users injecting commands
>>> into other processes in the same tty session using the TIOCSTI ioctl
>>>
>>> Signed-off-by: Matt Brown <matt@nmatt.com>
>>> ---
>>>  drivers/tty/tty_io.c | 4 ++++
>>>  1 file changed, 4 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;
>>
>> So, what type of "normal" userspace operations did you just break here?
>> What type of "not normal" did you break/change?
>
> Looking at
> <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI>,
> it looks like there are a couple users whose behavior would probably
> change when run by unprivileged users - in particular agetty, csh, xemacs
> and tcsh.
>

This is why I set this Kconfig to default to no.

This is also why I think this belongs under security and not tty. This
is more of a security feature than a tty feature.

>
>> Why tie this to CAP_SYS_ADMIN as well?  That wasn't listed in your
>> Kconfig help text.  This seems like an additional capabilities
>> dependancy that odds are, most people do not want...
>>
>>>       if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN))
>>>               return -EPERM;
>
> I think CAP_SYS_ADMIN is a logical choice here. AFAIK
> CAP_SYS_ADMIN is normally used for random functionality that permits
> users to bypass normal access controls without constraints that clearly
> map to existing capabilities. See seccomp() without NNP,
> proc_dointvec_minmax_sysadmin() and so on.
>
> I guess you could also argue for CAP_SYS_TTY_CONFIG, but I don't
> think that fits well, given that this is more about using a TTY in a
> privileged way than about configuring it.
>

I can update the Kconfig help text to mention the use of CAP_SYS_ADMIN.

>
>> And finally, why doesn't this original check handle what you want to do
>> already?
>>
>> I don't understand your "threat model" you wish to address by this
>> change series, please be a lot more explicit in your patch changelog
>> descriptions.
>
> While I don't know what Matt Brown's threat model is here, I like the
> patch for the following reason:
>
> For me, there are two usecases for having UIDs on a Linux
> system. The first usecase is to isolate human users from each other.
> The second usecase are service accounts, where the same human
> administrator has access to multiple UIDs, each of which is used for
> one specific service, reducing the impact of a compromise of a single
> service.
>
> In the "isolated services" usecase, the machine's administrator needs
> to be able to access the various service accounts in some way. One
> way that integrates well with existing tools is to log in as root, then
> use su or sudo to run a shell with the service's UID with reduced
> privileges.
>
> The only issue I know of that makes this dangerous in the default
> configuration is that the tty file descriptor becomes accessible to the
> shell running under the service's UID, allowing the service to e.g.
> use ptrace() or .bashrc or so to inject code into the service-privileged
> shell, then abuse TIOCSTI to take over root's shell.
>
> So, the reason the additional check is necessary is that there are
> usecases where in practice, TTY file descriptors are shared over
> privilege boundaries, and thereforce, access to the TTY fd should not
> automatically grant the level of access that is normally only available
> using a PTY master fd.
>
> sudo can mitigate this using the use_pty config option, which creates
> a new PTY and forwards I/O to and from that PTY, but this option is
> off by default. I think the version of su that e.g. ships in Debian can't
> even be configured to mitigate this.
>
> In my opinion, while it's possible for system administrators or
> programmers to avoid this pitfall if they know enough about how TTY
> devices work, this behavior is highly unintuitive.
>
> Also, quoting part of the help text for grsecurity's config option
> 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.
>
> Basically, TIOCSTI permits exactly what Yama is designed to
> prevent: It lets different processes in one session compromise
> each other.
>
>
> For context, in case someone in this thread hasn't seen it yet:
> http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/
> is a writeup about the issue and some prior discussions about it.
>

I couldn't have explained my threat model and rational better myself.

I can take any other feedback and roll it into a single updated patch.

[PATCH 4/4] added kernel.tiocsti_restrict sysctl

[Matt Brown]

this patch depends on patches 1, 2 and 3

Allow CONFIG_SECURITY_TIOCSTI_RESTRICT to be controlled via sysctl

Signed-off-by: Matt Brown <matt@nmatt.com>
---
 kernel/sysctl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

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