| Message ID | 20220601104547.260949-2-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs stable candidate patches for 5.10.y (part 2) | expand |
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.... 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. Cheers, Dave.
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. 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? To be on the safe side, until there is consensus about the best way to fix LTS, I will omit this fix from my weekly post to stable. Thanks, Amir.
On Thu, Jun 02, 2022 at 10:52:38AM +1000, Dave Chinner 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.... > > 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. There were a few outstanding issues and we didn't receive a new submission for them right before or during the merge window. I'm at a conference this week but I'll get back to review the patches and associated tests on Monday. Christian
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
On Thu, Jun 2, 2022 at 1:31 PM Christian Brauner <brauner@kernel.org> 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@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. > 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.
On Wed, Jun 08, 2022 at 11:25:10AM +0300, Amir Goldstein wrote: > 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? To me backporting it seems good and useful, as it fixes a relatively big problem. The remaining issues seem minor compared to that.
On Wed, Jun 8, 2022 at 11:26 AM Christoph Hellwig <hch@lst.de> wrote: > > On Wed, Jun 08, 2022 at 11:25:10AM +0300, Amir Goldstein wrote: > > 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? > > To me backporting it seems good and useful, as it fixes a relatively > big problem. The remaining issues seem minor compared to that. > Thanks! Dave, I am not seeing any progress with the remaining VFS issues, so rather not hold up fixing this "big problem" on future changes. Do the two opinions from Christian and Christoph help you ease your mind about the concerns that you raised? Anyway, I now have these two patches queued and tests so I can send them along in the same batch, whenever that is: e014f37db xfs: use setattr_copy to set vfs inode attributes 01ea173e1 xfs: fix up non-directory creation in SGID directories The setattr_copy patch is also part of Leah's candidates for 5.15, so if you approve, I will include both patches above in my next posting (after part 3) to align with Leah's 5.15 candidates. Thanks, Amir.
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index e958b1c74561..e20d8af80216 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -802,6 +802,7 @@ xfs_ialloc( xfs_buf_t **ialloc_context, xfs_inode_t **ipp) { + struct inode *dir = pip ? VFS_I(pip) : NULL; struct xfs_mount *mp = tp->t_mountp; xfs_ino_t ino; xfs_inode_t *ip; @@ -847,18 +848,17 @@ xfs_ialloc( return error; ASSERT(ip != NULL); inode = VFS_I(ip); - inode->i_mode = mode; set_nlink(inode, nlink); - inode->i_uid = current_fsuid(); inode->i_rdev = rdev; ip->i_d.di_projid = prid; - if (pip && XFS_INHERIT_GID(pip)) { - inode->i_gid = VFS_I(pip)->i_gid; - if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode)) - inode->i_mode |= S_ISGID; + if (dir && !(dir->i_mode & S_ISGID) && + (mp->m_flags & XFS_MOUNT_GRPID)) { + inode->i_uid = current_fsuid(); + inode->i_gid = dir->i_gid; + inode->i_mode = mode; } else { - inode->i_gid = current_fsgid(); + inode_init_owner(inode, dir, mode); } /*