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
