Re: [PATCH 5.10 CANDIDATE 1/8] xfs: fix up non-directory creation in SGID directories

From: Christian Brauner <brauner@kernel.org>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Dave Chinner <david@fromorbit.com>,
	"Darrick J . Wong" <djwong@kernel.org>,
	Brian Foster <bfoster@redhat.com>, Christoph Hellwig <hch@lst.de>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Adam Manzanares <a.manzanares@samsung.com>,
	Tyler Hicks <code@tyhicks.com>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	fstests <fstests@vger.kernel.org>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Yang Xu <xuyang2018.jy@fujitsu.com>
Subject: Re: [PATCH 5.10 CANDIDATE 1/8] xfs: fix up non-directory creation in SGID directories
Date: Thu, 2 Jun 2022 12:31:49 +0200	[thread overview]
Message-ID: <20220602103149.gc6b5hzkense5nrs@wittgenstein> (raw)
In-Reply-To: <CAOQ4uxjcumjxeWypahgYd9wLExLuipd9MTCc_8vfq6SFY7L4dA@mail.gmail.com>

On Thu, Jun 02, 2022 at 07:13:56AM +0300, Amir Goldstein wrote:
> On Thu, Jun 2, 2022 at 3:52 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Wed, Jun 01, 2022 at 01:45:40PM +0300, Amir Goldstein wrote:
> > > From: Christoph Hellwig <hch@lst.de>
> > >
> > > commit 01ea173e103edd5ec41acec65b9261b87e123fc2 upstream.
> > >
> > > XFS always inherits the SGID bit if it is set on the parent inode, while
> > > the generic inode_init_owner does not do this in a few cases where it can
> > > create a possible security problem, see commit 0fa3ecd87848
> > > ("Fix up non-directory creation in SGID directories") for details.
> >
> > inode_init_owner() introduces a bunch more SGID problems because
> > it strips the SGID bit from the mode passed to it, but all the code
> > outside it still sees the SGID bit set. IIRC, that means we do the
> > wrong thing when ACLs are present. IIRC, there's an LTP test for
> > this CVE now, and it also has a variant which uses ACLs and that
> > fails too....
> 
> Good point.
> I think Christian's vfstests probably tests more cases than what LTP
> does at this point.

I think so, yes. There will also be more tests coming into fstests.

> 
> Christian, Yang,
> 
> It would be nice if you could annotate the relevant fstests with
> _fixed_by_kernel_commit, which will make it easier to find
> all relevant commits to backport when tests are failing on LTS
> kernel.
> 
> >
> > I'm kinda wary about mentioning a security fix and then not
> > backporting the entire set of fixes the CVE requires in the same
> > patchset.  I have no idea what the status of the VFS level fixes
> > that are needed to fix this properly - I thought they were done and
> > reviewed, but they don't appear to be in 5.19 yet.
> >
> 
> No, it looks like tihs is still in review.
> 
> Christoph, Cristian, Yang,
> 
> What do you think is best to do w.r.t this patch?
> 
> Wait for all the current known issues to be fixed in upstream and then
> backport all known fixes?
> 
> Backport whatever fixes are available in upstream now at the same
> backport series?
> 
> Take this now and the rest later?

Imho, backporting this patch is useful. It fixes a basic issue.
What Dave mentioned is that if ACLs/umask are in play things become
order dependent I've pointed out on the patch that aims to fix this: If
no ACLs are supported then umask is stripped in vfs and if they are it's
stripped in the fs. So if umask strips S_IXGRP in the vfs then no setgid
bit is inherited. If it's stripped in the fs post inode_init_owner()
setgid bit is tripped and thus not inherited.. The vfs patch unifies
this behavior.

Christian