diff mbox

[3/4] restrict unprivileged TIOCSTI tty ioctl

Message ID 20170417060706.28674-4-matt@nmatt.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matt Brown April 17, 2017, 6:07 a.m. UTC
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(+)

Comments

Greg Kroah-Hartman April 17, 2017, 6:53 a.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jann Horn April 17, 2017, 2:18 p.m. UTC | #2
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Brown April 17, 2017, 4:18 p.m. UTC | #3
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

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