Message-ID: <CAG48ez1NBnrsPnHN6D9nbOJP6+Q6zEV9vfx9q7ME4Eti-vRmhQ@mail.gmail.com>
Date:   Mon, 17 Apr 2017 16:18:14 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Matt Brown <matt@...tt.com>, jmorris@...ei.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com
Subject: Re: [kernel-hardening] Re: [PATCH 3/4] restrict unprivileged TIOCSTI
 tty ioctl

On Mon, Apr 17, 2017 at 8:53 AM, Greg KH <gregkh@...uxfoundation.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@...tt.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.

Powered by blists - more mailing lists