On Thu, Jun 2, 2022 at 1:31 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> 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.
>
TBH, I am having a hard time following the expected vs. actual
behavior in all the cases at all points in time.
Christoph,
As the author of this patch, do you have an opinion w.r.t backporting
this patch alongs with vs. independent of followup fixes?
wait for future fixes yet to come?
Thanks,
Amir.
