On Thu, Jan 05, 2023 at 03:53:36PM +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. Remove the obsolete tests as
> they're not a security issue and will cause spurious warnings on older
> distro kernels.
>
> Note, that only with v6.2 setgid inheritance works correctly for
> overlayfs in the write path. Before this the setgid bit was always
retained
(I'll fix that missing when I merge this patch)
>
> 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>
> ---
> Changes in v3:
> - Miklos Szeredi <miklos@xxxxxxxxxx>
> - Instead of fixing up the test which changed, remove them.
> - Link to v2: https://lore.kernel.org/r/20230103-fstests-setgid-v6-2-v2-1-9c70ee2a4113@xxxxxxxxxx
This version looks good to me, old kernel and new kernel (v6.2) all test passed.
If there's not more review points from VFS part, I'll merge this patch.
Reviewed-by: Zorro Lang <zlang@xxxxxxxxxx>
>
> Changes in v2:
> - Darrick J. Wong <djwong@xxxxxxxxxx>:
> - Also update generic/686 and generic/687.
> - Link to v1: https://lore.kernel.org/r/20230103-fstests-setgid-v6-2-v1-1-b8972c303ebe@xxxxxxxxxx
> ---
> tests/generic/673 | 16 ++--------------
> tests/generic/673.out | 16 ++--------------
> tests/generic/683 | 16 ++--------------
> tests/generic/683.out | 12 ++----------
> tests/generic/684 | 16 ++--------------
> tests/generic/684.out | 12 ++----------
> tests/generic/685 | 16 ++--------------
> tests/generic/685.out | 12 ++----------
> tests/generic/686 | 16 ++--------------
> tests/generic/686.out | 12 ++----------
> tests/generic/687 | 16 ++--------------
> tests/generic/687.out | 12 ++----------
> 12 files changed, 24 insertions(+), 148 deletions(-)
>
> diff --git a/tests/generic/673 b/tests/generic/673
> index 6d1f49ea..ac8b8c09 100755
> --- a/tests/generic/673
> +++ b/tests/generic/673
> @@ -102,26 +102,14 @@ setup_testfile
> chmod a+rwxs $SCRATCH_MNT/a
> commit_and_check
>
> -#Commit to a non-exec file by an unprivileged user leaves sgid.
> -echo "Test 9 - qa_user, non-exec file, only sgid"
> -setup_testfile
> -chmod a+rw,g+rws $SCRATCH_MNT/a
> -commit_and_check "$qa_user"
> -
> #Commit to a group-exec file by an unprivileged user clears sgid
> -echo "Test 10 - qa_user, group-exec file, only sgid"
> +echo "Test 9 - qa_user, group-exec file, only sgid"
> setup_testfile
> chmod a+rw,g+rwxs $SCRATCH_MNT/a
> commit_and_check "$qa_user"
>
> -#Commit to a user-exec file by an unprivileged user clears sgid
> -echo "Test 11 - qa_user, user-exec file, only sgid"
> -setup_testfile
> -chmod a+rw,u+x,g+rws $SCRATCH_MNT/a
> -commit_and_check "$qa_user"
> -
> #Commit to a all-exec file by an unprivileged user clears sgid.
> -echo "Test 12 - qa_user, all-exec file, only sgid"
> +echo "Test 10 - qa_user, all-exec file, only sgid"
> setup_testfile
> chmod a+rwx,g+rwxs $SCRATCH_MNT/a
> commit_and_check "$qa_user"
> diff --git a/tests/generic/673.out b/tests/generic/673.out
> index 0817857d..4276fa01 100644
> --- a/tests/generic/673.out
> +++ b/tests/generic/673.out
> @@ -47,25 +47,13 @@ Test 8 - root, all-exec file
> 3784de23efab7a2074c9ec66901e39e5 SCRATCH_MNT/a
> 6777 -rwsrwsrwx SCRATCH_MNT/a
>
> -Test 9 - qa_user, non-exec file, only sgid
> -310f146ce52077fcd3308dcbe7632bb2 SCRATCH_MNT/a
> -2666 -rw-rwSrw- SCRATCH_MNT/a
> -3784de23efab7a2074c9ec66901e39e5 SCRATCH_MNT/a
> -2666 -rw-rwSrw- SCRATCH_MNT/a
> -
> -Test 10 - qa_user, group-exec file, only sgid
> +Test 9 - qa_user, group-exec file, only sgid
> 310f146ce52077fcd3308dcbe7632bb2 SCRATCH_MNT/a
> 2676 -rw-rwsrw- SCRATCH_MNT/a
> 3784de23efab7a2074c9ec66901e39e5 SCRATCH_MNT/a
> 676 -rw-rwxrw- SCRATCH_MNT/a
>
> -Test 11 - qa_user, user-exec file, only sgid
> -310f146ce52077fcd3308dcbe7632bb2 SCRATCH_MNT/a
> -2766 -rwxrwSrw- SCRATCH_MNT/a
> -3784de23efab7a2074c9ec66901e39e5 SCRATCH_MNT/a
> -2766 -rwxrwSrw- SCRATCH_MNT/a
> -
> -Test 12 - qa_user, all-exec file, only sgid
> +Test 10 - qa_user, all-exec file, only sgid
> 310f146ce52077fcd3308dcbe7632bb2 SCRATCH_MNT/a
> 2777 -rwxrwsrwx SCRATCH_MNT/a
> 3784de23efab7a2074c9ec66901e39e5 SCRATCH_MNT/a
> diff --git a/tests/generic/683 b/tests/generic/683
> index eea8d21b..304b1a48 100755
> --- a/tests/generic/683
> +++ b/tests/generic/683
> @@ -110,26 +110,14 @@ setup_testfile
> chmod a+rwxs $junk_file
> commit_and_check "" "$verb" 64k 64k
>
> -# Commit to a non-exec file by an unprivileged user leaves sgid.
> -echo "Test 9 - qa_user, non-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a group-exec file by an unprivileged user clears sgid
> -echo "Test 10 - qa_user, group-exec file $verb, only sgid"
> +echo "Test 9 - qa_user, group-exec file $verb, only sgid"
> setup_testfile
> chmod a+rw,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
>
> -# Commit to a user-exec file by an unprivileged user clears sgid
> -echo "Test 11 - qa_user, user-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,u+x,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a all-exec file by an unprivileged user clears sgid.
> -echo "Test 12 - qa_user, all-exec file $verb, only sgid"
> +echo "Test 10 - qa_user, all-exec file $verb, only sgid"
> setup_testfile
> chmod a+rwx,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
> diff --git a/tests/generic/683.out b/tests/generic/683.out
> index ca29f6e6..de18ea5f 100644
> --- a/tests/generic/683.out
> +++ b/tests/generic/683.out
> @@ -31,19 +31,11 @@ Test 8 - root, all-exec file falloc
> 6777 -rwsrwsrwx TEST_DIR/683/a
> 6777 -rwsrwsrwx TEST_DIR/683/a
>
> -Test 9 - qa_user, non-exec file falloc, only sgid
> -2666 -rw-rwSrw- TEST_DIR/683/a
> -2666 -rw-rwSrw- TEST_DIR/683/a
> -
> -Test 10 - qa_user, group-exec file falloc, only sgid
> +Test 9 - qa_user, group-exec file falloc, only sgid
> 2676 -rw-rwsrw- TEST_DIR/683/a
> 676 -rw-rwxrw- TEST_DIR/683/a
>
> -Test 11 - qa_user, user-exec file falloc, only sgid
> -2766 -rwxrwSrw- TEST_DIR/683/a
> -2766 -rwxrwSrw- TEST_DIR/683/a
> -
> -Test 12 - qa_user, all-exec file falloc, only sgid
> +Test 10 - qa_user, all-exec file falloc, only sgid
> 2777 -rwxrwsrwx TEST_DIR/683/a
> 777 -rwxrwxrwx TEST_DIR/683/a
>
> diff --git a/tests/generic/684 b/tests/generic/684
> index 541dbeb4..1ebffb01 100755
> --- a/tests/generic/684
> +++ b/tests/generic/684
> @@ -110,26 +110,14 @@ setup_testfile
> chmod a+rwxs $junk_file
> commit_and_check "" "$verb" 64k 64k
>
> -# Commit to a non-exec file by an unprivileged user leaves sgid.
> -echo "Test 9 - qa_user, non-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a group-exec file by an unprivileged user clears sgid
> -echo "Test 10 - qa_user, group-exec file $verb, only sgid"
> +echo "Test 9 - qa_user, group-exec file $verb, only sgid"
> setup_testfile
> chmod a+rw,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
>
> -# Commit to a user-exec file by an unprivileged user clears sgid
> -echo "Test 11 - qa_user, user-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,u+x,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a all-exec file by an unprivileged user clears sgid.
> -echo "Test 12 - qa_user, all-exec file $verb, only sgid"
> +echo "Test 10 - qa_user, all-exec file $verb, only sgid"
> setup_testfile
> chmod a+rwx,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
> diff --git a/tests/generic/684.out b/tests/generic/684.out
> index 2e084ced..da5ada5e 100644
> --- a/tests/generic/684.out
> +++ b/tests/generic/684.out
> @@ -31,19 +31,11 @@ Test 8 - root, all-exec file fpunch
> 6777 -rwsrwsrwx TEST_DIR/684/a
> 6777 -rwsrwsrwx TEST_DIR/684/a
>
> -Test 9 - qa_user, non-exec file fpunch, only sgid
> -2666 -rw-rwSrw- TEST_DIR/684/a
> -2666 -rw-rwSrw- TEST_DIR/684/a
> -
> -Test 10 - qa_user, group-exec file fpunch, only sgid
> +Test 9 - qa_user, group-exec file fpunch, only sgid
> 2676 -rw-rwsrw- TEST_DIR/684/a
> 676 -rw-rwxrw- TEST_DIR/684/a
>
> -Test 11 - qa_user, user-exec file fpunch, only sgid
> -2766 -rwxrwSrw- TEST_DIR/684/a
> -2766 -rwxrwSrw- TEST_DIR/684/a
> -
> -Test 12 - qa_user, all-exec file fpunch, only sgid
> +Test 10 - qa_user, all-exec file fpunch, only sgid
> 2777 -rwxrwsrwx TEST_DIR/684/a
> 777 -rwxrwxrwx TEST_DIR/684/a
>
> diff --git a/tests/generic/685 b/tests/generic/685
> index 29eca1a8..e4ada8e7 100755
> --- a/tests/generic/685
> +++ b/tests/generic/685
> @@ -110,26 +110,14 @@ setup_testfile
> chmod a+rwxs $junk_file
> commit_and_check "" "$verb" 64k 64k
>
> -# Commit to a non-exec file by an unprivileged user leaves sgid.
> -echo "Test 9 - qa_user, non-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a group-exec file by an unprivileged user clears sgid
> -echo "Test 10 - qa_user, group-exec file $verb, only sgid"
> +echo "Test 9 - qa_user, group-exec file $verb, only sgid"
> setup_testfile
> chmod a+rw,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
>
> -# Commit to a user-exec file by an unprivileged user clears sgid
> -echo "Test 11 - qa_user, user-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,u+x,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a all-exec file by an unprivileged user clears sgid.
> -echo "Test 12 - qa_user, all-exec file $verb, only sgid"
> +echo "Test 10 - qa_user, all-exec file $verb, only sgid"
> setup_testfile
> chmod a+rwx,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
> diff --git a/tests/generic/685.out b/tests/generic/685.out
> index e611da3e..03eef362 100644
> --- a/tests/generic/685.out
> +++ b/tests/generic/685.out
> @@ -31,19 +31,11 @@ Test 8 - root, all-exec file fzero
> 6777 -rwsrwsrwx TEST_DIR/685/a
> 6777 -rwsrwsrwx TEST_DIR/685/a
>
> -Test 9 - qa_user, non-exec file fzero, only sgid
> -2666 -rw-rwSrw- TEST_DIR/685/a
> -2666 -rw-rwSrw- TEST_DIR/685/a
> -
> -Test 10 - qa_user, group-exec file fzero, only sgid
> +Test 9 - qa_user, group-exec file fzero, only sgid
> 2676 -rw-rwsrw- TEST_DIR/685/a
> 676 -rw-rwxrw- TEST_DIR/685/a
>
> -Test 11 - qa_user, user-exec file fzero, only sgid
> -2766 -rwxrwSrw- TEST_DIR/685/a
> -2766 -rwxrwSrw- TEST_DIR/685/a
> -
> -Test 12 - qa_user, all-exec file fzero, only sgid
> +Test 10 - qa_user, all-exec file fzero, only sgid
> 2777 -rwxrwsrwx TEST_DIR/685/a
> 777 -rwxrwxrwx TEST_DIR/685/a
>
> diff --git a/tests/generic/686 b/tests/generic/686
> index a8ec23d5..d56aa7cc 100755
> --- a/tests/generic/686
> +++ b/tests/generic/686
> @@ -110,26 +110,14 @@ setup_testfile
> chmod a+rwxs $junk_file
> commit_and_check "" "$verb" 64k 64k
>
> -# Commit to a non-exec file by an unprivileged user leaves sgid.
> -echo "Test 9 - qa_user, non-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a group-exec file by an unprivileged user clears sgid
> -echo "Test 10 - qa_user, group-exec file $verb, only sgid"
> +echo "Test 9 - qa_user, group-exec file $verb, only sgid"
> setup_testfile
> chmod a+rw,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
>
> -# Commit to a user-exec file by an unprivileged user clears sgid
> -echo "Test 11 - qa_user, user-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,u+x,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a all-exec file by an unprivileged user clears sgid.
> -echo "Test 12 - qa_user, all-exec file $verb, only sgid"
> +echo "Test 10 - qa_user, all-exec file $verb, only sgid"
> setup_testfile
> chmod a+rwx,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
> diff --git a/tests/generic/686.out b/tests/generic/686.out
> index aa1e6471..562e1ab9 100644
> --- a/tests/generic/686.out
> +++ b/tests/generic/686.out
> @@ -31,19 +31,11 @@ Test 8 - root, all-exec file finsert
> 6777 -rwsrwsrwx TEST_DIR/686/a
> 6777 -rwsrwsrwx TEST_DIR/686/a
>
> -Test 9 - qa_user, non-exec file finsert, only sgid
> -2666 -rw-rwSrw- TEST_DIR/686/a
> -2666 -rw-rwSrw- TEST_DIR/686/a
> -
> -Test 10 - qa_user, group-exec file finsert, only sgid
> +Test 9 - qa_user, group-exec file finsert, only sgid
> 2676 -rw-rwsrw- TEST_DIR/686/a
> 676 -rw-rwxrw- TEST_DIR/686/a
>
> -Test 11 - qa_user, user-exec file finsert, only sgid
> -2766 -rwxrwSrw- TEST_DIR/686/a
> -2766 -rwxrwSrw- TEST_DIR/686/a
> -
> -Test 12 - qa_user, all-exec file finsert, only sgid
> +Test 10 - qa_user, all-exec file finsert, only sgid
> 2777 -rwxrwsrwx TEST_DIR/686/a
> 777 -rwxrwxrwx TEST_DIR/686/a
>
> diff --git a/tests/generic/687 b/tests/generic/687
> index ff3e2fe1..3a7f1fd5 100755
> --- a/tests/generic/687
> +++ b/tests/generic/687
> @@ -110,26 +110,14 @@ setup_testfile
> chmod a+rwxs $junk_file
> commit_and_check "" "$verb" 64k 64k
>
> -# Commit to a non-exec file by an unprivileged user leaves sgid.
> -echo "Test 9 - qa_user, non-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a group-exec file by an unprivileged user clears sgid
> -echo "Test 10 - qa_user, group-exec file $verb, only sgid"
> +echo "Test 9 - qa_user, group-exec file $verb, only sgid"
> setup_testfile
> chmod a+rw,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
>
> -# Commit to a user-exec file by an unprivileged user clears sgid
> -echo "Test 11 - qa_user, user-exec file $verb, only sgid"
> -setup_testfile
> -chmod a+rw,u+x,g+rws $junk_file
> -commit_and_check "$qa_user" "$verb" 64k 64k
> -
> # Commit to a all-exec file by an unprivileged user clears sgid.
> -echo "Test 12 - qa_user, all-exec file $verb, only sgid"
> +echo "Test 10 - qa_user, all-exec file $verb, only sgid"
> setup_testfile
> chmod a+rwx,g+rwxs $junk_file
> commit_and_check "$qa_user" "$verb" 64k 64k
> diff --git a/tests/generic/687.out b/tests/generic/687.out
> index c5116c27..f72f6d30 100644
> --- a/tests/generic/687.out
> +++ b/tests/generic/687.out
> @@ -31,19 +31,11 @@ Test 8 - root, all-exec file fcollapse
> 6777 -rwsrwsrwx TEST_DIR/687/a
> 6777 -rwsrwsrwx TEST_DIR/687/a
>
> -Test 9 - qa_user, non-exec file fcollapse, only sgid
> -2666 -rw-rwSrw- TEST_DIR/687/a
> -2666 -rw-rwSrw- TEST_DIR/687/a
> -
> -Test 10 - qa_user, group-exec file fcollapse, only sgid
> +Test 9 - qa_user, group-exec file fcollapse, only sgid
> 2676 -rw-rwsrw- TEST_DIR/687/a
> 676 -rw-rwxrw- TEST_DIR/687/a
>
> -Test 11 - qa_user, user-exec file fcollapse, only sgid
> -2766 -rwxrwSrw- TEST_DIR/687/a
> -2766 -rwxrwSrw- TEST_DIR/687/a
> -
> -Test 12 - qa_user, all-exec file fcollapse, only sgid
> +Test 10 - qa_user, all-exec file fcollapse, only sgid
> 2777 -rwxrwsrwx TEST_DIR/687/a
> 777 -rwxrwxrwx TEST_DIR/687/a
>
>
> ---
> base-commit: fbd489798b31e32f0eaefcd754326a06aa5b166f
> change-id: 20230103-fstests-setgid-v6-2-4ce5852d11e2
>
