From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 35F85C54FCB for ; Fri, 24 Apr 2020 13:58:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1F82920776 for ; Fri, 24 Apr 2020 13:58:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728327AbgDXN65 (ORCPT ); Fri, 24 Apr 2020 09:58:57 -0400 Received: from mother.openwall.net ([195.42.179.200]:37296 "HELO mother.openwall.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726667AbgDXN64 (ORCPT ); Fri, 24 Apr 2020 09:58:56 -0400 X-Greylist: delayed 400 seconds by postgrey-1.27 at vger.kernel.org; Fri, 24 Apr 2020 09:58:55 EDT Received: (qmail 32561 invoked from network); 24 Apr 2020 13:52:14 -0000 Received: from localhost (HELO pvt.openwall.com) (127.0.0.1) by localhost with SMTP; 24 Apr 2020 13:52:14 -0000 Received: by pvt.openwall.com (Postfix, from userid 503) id 5E3FBAB5C7; Fri, 24 Apr 2020 15:52:05 +0200 (CEST) Date: Fri, 24 Apr 2020 15:52:05 +0200 From: Solar Designer To: Ben Hutchings Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org, akpm@linux-foundation.org, Denis Kirjanov , Dan Carpenter , Al Viro , Kees Cook , Salvatore Mesoraca , Linus Torvalds Subject: Re: [PATCH 3.16 208/245] namei: allow restricted O_CREAT of FIFOs and regular files Message-ID: <20200424135205.GA27204@openwall.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Date: Sun Jan 26 09:29:34 2020 -0500 do_last(): fetch directory ->i_mode and ->i_uid before it's too late may_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 commit d76341d93dedbcf6ed5a08dfc8bce82d3e9a772b Author: Al Viro Date: Sat Feb 1 16:26:45 2020 +0000 vfs: fix do_last() regression commit 6404674acd596de41fd3ad5f267b4525494a891a upstream. Brown paperbag time: fetching ->i_uid/->i_mode really should've been done from nd->inode. I even suggested that, but the reason for that has slipped through the cracks and I went for dir->d_inode instead - made for more "obvious" patch. Analysis: - at the entry into do_last() and all the way to step_into(): dir (aka nd->path.dentry) is known not to have been freed; so's nd->inode and it's equal to dir->d_inode unless we are already doomed to -ECHILD. inode of the file to get opened is not known. - after step_into(): inode of the file to get opened is known; dir might be pointing to freed memory/be negative/etc. - at the call of may_create_in_sticky(): guaranteed to be out of RCU mode; inode of the file to get opened is known and pinned; dir might be garbage. The last was the reason for the original patch. Except that at the do_last() entry we can be in RCU mode and it is possible that nd->path.dentry->d_inode has already changed under us. In that case we are going to fail with -ECHILD, but we need to be careful; nd->inode is pointing to valid struct inode and it's the same as nd->path.dentry->d_inode in "won't fail with -ECHILD" case, so we should use that. Reported-by: "Rantala, Tommi T. (Nokia - FI/Espoo)" Reported-by: syzbot+190005201ced78a74ad6@syzkaller.appspotmail.com Wearing-brown-paperbag: Al Viro Cc: stable@kernel.org Fixes: d0cb50185ae9 ("do_last(): fetch directory ->i_mode and ->i_uid before it's too late") Signed-off-by: Al Viro Signed-off-by: Linus Torvalds Signed-off-by: Greg Kroah-Hartman At least inclusion of the above fixes is mandatory for any backports. 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). > +/** > + * may_create_in_sticky - Check whether an O_CREAT open in a sticky directory > + * should be allowed, or not, on files that already > + * exist. > + * @dir: the sticky parent directory > + * @inode: the inode of the file to open > + * > + * Block an O_CREAT open of a FIFO (or a regular file) when: > + * - sysctl_protected_fifos (or sysctl_protected_regular) is enabled > + * - the file already exists > + * - we are in a sticky directory > + * - we don't own the file > + * - the owner of the directory doesn't own the file > + * - the directory is world writable > + * If the sysctl_protected_fifos (or sysctl_protected_regular) is set to 2 > + * the directory doesn't have to be world writable: being group writable will > + * be enough. > + * > + * Returns 0 if the open is allowed, -ve on error. > + */ > +static int may_create_in_sticky(struct dentry * const dir, > + struct inode * const inode) > +{ > + if ((!sysctl_protected_fifos && S_ISFIFO(inode->i_mode)) || > + (!sysctl_protected_regular && S_ISREG(inode->i_mode)) || > + likely(!(dir->d_inode->i_mode & S_ISVTX)) || > + uid_eq(inode->i_uid, dir->d_inode->i_uid) || > + uid_eq(current_fsuid(), inode->i_uid)) > + return 0; > + > + if (likely(dir->d_inode->i_mode & 0002) || > + (dir->d_inode->i_mode & 0020 && > + ((sysctl_protected_fifos >= 2 && S_ISFIFO(inode->i_mode)) || > + (sysctl_protected_regular >= 2 && S_ISREG(inode->i_mode))))) { > + return -EACCES; > + } > + return 0; > +} 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. Alexander