From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out4.electric.net ([192.162.216.195]:65150 "EHLO smtp-out4.electric.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750881AbdLAJqN (ORCPT ); Fri, 1 Dec 2017 04:46:13 -0500 From: David Laight To: 'Solar Designer' CC: 'Salvatore Mesoraca' , "linux-kernel@vger.kernel.org" , "Kernel Hardening" , "linux-fsdevel@vger.kernel.org" , "Alexander Viro" , Jann Horn , Kees Cook , "Eric W. Biederman" Subject: RE: [PATCH v3 2/2] Protected O_CREAT open in sticky directories Date: Fri, 1 Dec 2017 09:46:28 +0000 Message-ID: References: <1511337706-8297-1-git-send-email-s.mesoraca16@gmail.com> <1511337706-8297-3-git-send-email-s.mesoraca16@gmail.com> <9fe9b2cd312748ddb31f63f9dc1b1ed8@AcuMS.aculab.com> <20171130175147.GA4124@openwall.com> In-Reply-To: <20171130175147.GA4124@openwall.com> Content-Language: en-US Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-fsdevel-owner@vger.kernel.org List-ID: From: Solar Designer > Sent: 30 November 2017 17:52 > > On Thu, Nov 30, 2017 at 04:53:06PM +0000, David Laight wrote: > > From: Salvatore Mesoraca > > > if a program tries to open a file, in a sticky directory, > > > with the O_CREAT flag and without the O_EXCL, it probably has a bug. > > > This feature allows to detect and potentially block programs that > > > act this way, it can be used to find vulnerabilities (like those > > > prevented by patch #1) and to do policy enforcement. > > > I presume the 'vulnerabilities' are related to symlinks being created > > just before the open? > > That's one way to exploit them. We already have a sysctl to try and > block that specific attack, and we're considering adding more, for other > attacks. But we'd also like an easy way to learn of the vulnerabilities > without anyone trying to exploit them yet, hence this patch. > > > Trouble is this change breaks a lot of general use of /tmp. > > That's general misuse of /tmp. Things like "command > /tmp/file" > without having pre-created the file with O_EXCL e.g. by mktemp(1). I'm sorry, I've been using Unix for over 30 years. /tmp is a place that temporary files were created - nothing special. Traditionally it was emptied on every boot. There was never anything that required files be created in any specific way. > > I always assumed that code that cared would use O_EXCL and > > everything else wasn't worth subverting. > > Opinions will vary on whether it's worth subverting or not, and someone > may reasonably want to configure this differently on different systems. > > > I found code in vi (and elsewhere) that subverted these checks > > by opening with O_WRONLY if stat() showed the file existed and > > O_CREAT|O_EXCL if it didn't. > > Yes, such misuses of /tmp and the like by use of those programs - where > the potential consequences would often be less severe (if your example > above is correct, it sounds like it'd be data spoofing but not > overwriting another file over a link) - could remain unnoticed. It seemed to me that code had been added to avoid issues caused by this policing of opens. But the code added is itself racy - so makes the whole thing pointless. > > I'm pretty sure that traditionally a lot of these opens were done > > with O_CREAT|O_TRUNC. > > Implementing that as unlink() followed by a create would stop > > 'random' (ok all) symlinks being followed. > > That's racy. We have O_EXCL, and it must be used along with O_CREAT > when the directory is untrusted. (If it were only about symlinks, we > also already have O_NOFOLLOW.) Not racy if done in the kernel itself. > > Overall I'm pretty sure this change will break things badly somewhere. > > Sure. We want it to visibly break what was subtly broken, so that the > breakage can be seen and corrected without an attempted attack. Maybe, but it will break some user system when they do a kernel update. It won't necessarily break a developer's system first. And, AFAICT, there is already code that subverts any tests by doing a check for the file existing and then selecting between two different types on open. David