linux-kernel - Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require CAP_SYS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170515215752.4e9f3826@alans-desktop>
Date:   Mon, 15 May 2017 21:57:52 +0100
From:   Alan Cox <gnomes@...rguk.ukuu.org.uk>
To:     Matt Brown <matt@...tt.com>
Cc:     serge@...lyn.com, gregkh@...uxfoundation.org, jslaby@...e.com,
        akpm@...ux-foundation.org, jannh@...gle.com, keescook@...omium.org,
        jmorris@...ei.org, kernel-hardening@...ts.openwall.com,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 0/2] security: tty: make TIOCSTI ioctl require
 CAP_SYS_ADMIN

O> I'm not implying that my patch is supposed to provide safety for
> "hundreds of other" issues. I'm looking to provide a way to lock down a
> single TTY ioctl that has caused real security issues to arise. For

In other words you are not actually fixing anything.

> this reason, it's completely incorrect to say that this feature is
> snake oil. My patch does exactly what it claims to do. No more no less.
> 
> > In addition your change to allow it to be used by root in the guest
> > completely invalidates any protection you have because I can push
> >
> > "rm -rf /\n"
> >
> > as root in my namespace and exit
> >
> > The tty buffers are not flushed across the context change so the shell
> > you return to gets the input and oh dear....  
> 
> This is precisely what my patch prevents! With my protection enabled, a
> container will only be able to use the TIOCSTI ioctl on a tty if that
> container has CAP_SYS_ADMIN in the user namespace in which the tty was
> created.

Which is not necessarily the namespace of the process that next issues a
read().

This is snake oil. There is a correct and proper process for this use
case. That proper process is to create a new pty/tty pair. There are two
cases

- processes that do it right in which case the attacker is screwed and we
  don't need to mess up TIOCSTI handling for no reason.

- processes that do it wrong. If they do it wrong then they'll also use
  all the other holes and attacks available via the same path which are
  completely unfixable without making your system basically unusable.


So can we please apply the minimum of common sense reasoning to this and
drop the patch.

Alan