On Tue, Jan 03, 2023 at 09:47:37AM -0800, Darrick J. Wong wrote:
> On Tue, Jan 03, 2023 at 04:28:20PM +0100, Christian Brauner wrote:
> > Over mutiple kernel releases we have reworked setgid inheritance
> > significantly due to long-standing security issues, security issues that
> > were reintroduced after they were fixed, and the subtle and difficult
> > inheritance rules that plagued individual filesystems. We have lifted
> > setgid inheritance into the VFS proper in earlier patches. Starting with
> > kernel v6.2 we have made setgid inheritance consistent between the write
> > and setattr (ch{mod,own}) paths.
> >
> > The gist of the requirement is that in order to inherit the setgid bit
> > the user needs to be in the group of the file or have CAP_FSETID in
> > their user namespace. Otherwise the setgid bit will be dropped
> > irregardless of the file's executability. Change the tests accordingly
> > and annotate them with the commits that changed the behavior.
> >
> > Note, that only with v6.2 setgid inheritance works correctly for
> > overlayfs in the write path. Before this the setgid bit was always
> > retained.
> >
> > Link: https://lore.kernel.org/linux-ext4/CAOQ4uxhmCgyorYVtD6=n=khqwUc=MPbZs+y=sqt09XbGoNm_tA@xxxxxxxxxxxxxx
> > Link: https://lore.kernel.org/linux-fsdevel/20221212112053.99208-1-brauner@xxxxxxxxxx
> > Link: https://lore.kernel.org/linux-fsdevel/20221122142010.zchf2jz2oymx55qi@wittgenstein
> > Cc: Amir Goldstein <amir73il@xxxxxxxxx>
> > Cc: Zorro Lang <zlang@xxxxxxxxxx>
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> > ---
> > tests/generic/673 | 12 +++++++++---
> > tests/generic/673.out | 8 ++++----
> > tests/generic/683 | 11 ++++++++---
> > tests/generic/683.out | 8 ++++----
> > tests/generic/684 | 11 ++++++++---
> > tests/generic/684.out | 8 ++++----
> > tests/generic/685 | 11 ++++++++---
> > tests/generic/685.out | 8 ++++----
> > 8 files changed, 49 insertions(+), 28 deletions(-)
> >
> > diff --git a/tests/generic/673 b/tests/generic/673
> > index 6d1f49ea..1d8e4184 100755
> > --- a/tests/generic/673
> > +++ b/tests/generic/673
> > @@ -23,6 +23,12 @@ _require_scratch_reflink
> > _scratch_mkfs >> $seqres.full
> > _scratch_mount
> > _require_congruent_file_oplen $SCRATCH_MNT 1048576
> > +
> > +# Due to multiple security issues and potential for subtle bugs around setgid
> > +# inheritance the rules in the write and chmod/chown paths have been made
> > +# consistent and are enforced by the VFS since kernel 6.2.
> > +_fixed_in_kernel_version "v6.2"
>
> Uh... will this new behavior get ported to the LTS kernels too, or are
I plan on backporting this but I would like to wait for a little while
longer into the v6.2 cycle before doing this as everyone is just coming
out of the holiday season trying to catch up.
> we simply changing the kernel behavior here?
Yes, this is a behavioral change. I made sure to mention the risks and
reasons involved on the list along the patches and in the pull requests
that were merged.
>
> Also, this patch does correct the new test failures that I saw as soon
> as I pulled 6.2-rc1, but it doesn't fix the finsert and fcollapse tests:
Indeed I have missed them because they were skipped:
generic/686 [not run] xfs_io finsert failed (old kernel/wrong fs?)
generic/687 [not run] xfs_io fcollapse failed (old kernel/wrong fs?)
I'll update the series accordingly. Thanks for pointing that out.
Christian
