On Fri, 2020-04-24 at 15:52 +0200, Solar Designer wrote:
On Fri, Apr 24, 2020 at 12:07:15AM +0100, Ben Hutchings wrote:
3.16.83-rc1 review patch. If anyone has any objections, please let me know.
I do. This patch is currently known-buggy, see this thread:
https://www.openwall.com/lists/oss-security/2020/01/28/2
It is (partially) fixed with these newer commits in 5.5 and 5.5.2:
commit d0cb50185ae942b03c4327be322055d622dc79f6 Author: Al Viro viro@zeniv.linux.org.uk Date: Sun Jan 26 09:29:34 2020 -0500
do_last(): fetch directory ->i_mode and ->i_uid before it's too latemay_create_in_sticky() call is done when we already have dropped the reference to dir. Fixes: 30aba6656f61e (namei: allow restricted O_CREAT of FIFOs and regular files) Signed-off-by: Al Viro viro@zeniv.linux.org.uk
commit d76341d93dedbcf6ed5a08dfc8bce82d3e9a772b Author: Al Viro viro@zeniv.linux.org.uk Date: Sat Feb 1 16:26:45 2020 +0000
vfs: fix do_last() regressioncommit 6404674acd596de41fd3ad5f267b4525494a891a upstream.
[...]
At least inclusion of the above fixes is mandatory for any backports.
I know, and those are the next 2 patches in the series.
Also, I think no one has fixed the logic of may_create_in_sticky() so that it wouldn't unintentionally apply the "protection" when the file is neither a FIFO nor a regular file (something I found and mentioned in the oss-security posting above).
[...]
I think the implementation of may_create_in_sticky() should be rewritten such that it'd directly correspond to the textual description in the comment above. As we've seen, trying to write the code "more optimally" resulted in its logic actually being different from the description.
Meanwhile, I think backporting known-so-buggy code is a bad idea.
I can see that it's not quite right, but does it matter in practice? Directories and symlinks are handled separately; sockets can't be opened anyway; block and character devices wonn't normally appear in a sticky directory.
Ben.