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@xxxxxxxxxxxxx> wrote:
> >
> > On Wed, Jun 01, 2022 at 01:45:40PM +0300, Amir Goldstein wrote:
> > > From: Christoph Hellwig <hch@xxxxxx>
> > >
> > > 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
