Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751185AbdFBOqh (ORCPT ); Fri, 2 Jun 2017 10:46:37 -0400 Received: from relay2-d.mail.gandi.net ([217.70.183.194]:41678 "EHLO relay2-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751125AbdFBOqf (ORCPT ); Fri, 2 Jun 2017 10:46:35 -0400 X-Originating-IP: 72.66.113.207 Subject: Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN To: Alan Cox , Kees Cook Cc: Casey Schaufler , Boris Lukashev , Greg KH , "Serge E. Hallyn" , "kernel-hardening@lists.openwall.com" , linux-security-module , linux-kernel References: <20170529213800.29438-1-matt@nmatt.com> <20170529213800.29438-3-matt@nmatt.com> <20170529232640.16211960@alans-desktop> <3738951f-7a4a-b37f-c695-21a2fcd45f76@schaufler-ca.com> <0e078ce7-5b62-f27c-3920-efc2ffdf342b@nmatt.com> <20170530132427.016053da@alans-desktop> <2ab8580e-bf8e-21bd-6bfa-33e5fa82400b@nmatt.com> <20170530235106.11aab25c@alans-desktop> <3bd4ff7b-6f7d-52b0-03f6-026bac79f11f@nmatt.com> <20170531005633.484a2e14@alans-desktop> <20170601140812.583cf0a5@alans-desktop> <20170601222432.6f593538@lxorguk.ukuu.org.uk> From: Matt Brown Message-ID: <2d0ad49c-886e-1caf-771a-d251957f614c@nmatt.com> Date: Fri, 2 Jun 2017 10:46:54 -0400 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <20170601222432.6f593538@lxorguk.ukuu.org.uk> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5528 Lines: 126 On 6/1/17 5:24 PM, Alan Cox wrote: >> There's a difference between "bugs" and "security bugs". Letting > > Not really, it's merely a matter of severity of result. A non security > bug that hoses your hard disk is to anyone but security nutcases at > least as bad as a security hole. > >> security bugs continue to get exploited because we want to flush out >> bugs seems insensitive to the people getting attacked. I'd rather >> protect against a class of bug than have to endless fix each bug. > > The others are security bugs too to varying degree > >>> I'm not against doing something to protect the container folks, but that >>> something as with Android is a whitelist of ioctls. And if we need to do >>> this with a kernel hook lets do it properly. >>> >>> Remember the namespace of the tty on creation >>> If the magic security flag is set then >>> Apply a whitelist to *any* tty ioctl call where the ns doesn't >>> match >>> >>> and we might as well just take the Android whitelist since they've kindly >>> built it for us all! >>> >>> In the tty layer it ends up being something around 10 lines of code and >>> some other file somewhere in security/ that's just a switch or similar >>> with the whitelisted ioctl codes in it. >>> >>> That (or a similar SELinux ruleset) would actually fix the problem. >>> SELinux would be better because it can also apply the rules when doing >>> things like su/sudo/... >> >> Just to play devil's advocate, wouldn't such a system continue to not >> address your physical-console concerns? I wouldn't want to limit the > > It would for the cases that a whitelist and container check covers - > because the whitelist wouldn't allow you to do anything but boring stuff > on the tty. TIOCSTI is just one of a whole range of differently stupid > and annoying opportunities. Containers do not and should not be able to > set the keymap, change the video mode, use console selection, make funny > beepy noises, access video I/O registers and all the other stuff like > that. Nothing is going to break if we have a fairly conservative > whitelist. > >> protection to only containers (but it's a good start), since it >> wouldn't protect people not using containers that still have a >> privileged TTY attached badly somewhere. > > How are you going to magically fix the problem. I'm not opposed to fixing > the real problem but right now it appears to be a product of wishful > thinking not programming. What's the piece of security code that > magically discerns the fact you are running something untrusted at the > other end of your tty. SELinux can do it via labelling but I don't see > any generic automatic way for the kernel to magically work out when to > whitelist and when not to. If there is a better magic rule than > differing-namespace then provide the code. > > You can't just disable TIOCSTI, it has users deal with it. You can > get away with disabling it for namespace crossing I think but if you do > that you need to disable a pile of others. > > (If it breaks containers blocking TIOCSTI then we need to have a good > look at algorithms for deciding when to flush the input queue on exiting > a container or somesuch) > >> If you're talking about wholistic SELinux policy, sure, I could >> imagine a wholistic fix. But for the tons of people without a >> comprehensive SELinux policy, the proposed protection continues to >> make sense. > > No it doesn't. It's completely useless unless you actually bother to > address the other exploit opportunities. > > Right now the proposal is a hack to do > > if (TIOCSTI && different_namespace && magic_flag) > This is not what my patch does. Mine is like: if (TIOCSTI && !ns_capable(tty->owner_user_ns, CAP_SYS_ADMIN) && magic_flag) in other words: if (TIOCSTI && (different_owner_user_ns || !CAP_SYS_ADMIN) && magic_flag) can you specify what you mean by different_namespace? which namespace? The reason I brought in the user namespace was to solve an edge case with nested containers. capable() will never be true in a container, so nested containers would break if we didn't do the ns_capable check. This is also the reason for the addition of owner_user_ns to tty_struct. The point of my patch is to prevent any type of tiocsti activity where the tty is given to an unprivileged process/container. The emphasis of my check is on the CAP_SYS_ADMIN part, not the namespace part. > the proper solution is > > if (!whitelisted(ioctl) && different_namespace && magic_flag) > > The former is snake oil, the latter actually deals with the problem space > for namespaced stuff comprehensively and is only a tiny amount more code. > > For non namespaced stuff it makes it no worse, if you don't allocate a > pty/tty pair properly then the gods are not going to magically save you > from on high sorry. And if you want to completely kill TIOCSTI even > though it's kind of pointless you can use seccomp. > > We can make things a bit better for the non-namespaced cases by providing > a new ioctl that turns on/off whitelisting for that tty so that the > process can do > > ioctl(tty_fd, TIOCTRUST, &zero); > execl("/home/foo/stupid-idea", "ooops", NULL); > > that's a simple per tty flag and a trivial one condition extra check to > the test above. We do need some way to change it back however and that's > a bit trickier given we don't want the stupid-idea tool to be able to but > we do want the invoking shell to - maybe you have to be session leader ? > > Alan >