sgid clearing rules?

From: Miklos Szeredi <miklos@szeredi.hu>
To: linux-fsdevel@vger.kernel.org, Christian Brauner <brauner@kernel.org>
Cc: fstests <fstests@vger.kernel.org>,
	LSM <linux-security-module@vger.kernel.org>
Subject: sgid clearing rules?
Date: Mon, 21 Nov 2022 14:14:13 +0100	[thread overview]
Message-ID: <CAJfpegsVAUUg5p6DbL1nA_oRF4Bui+saqbFjjYn=VYtd-N2Xew@mail.gmail.com> (raw)

I'm looking at sgid clearing in case of file modification.  Seems like
the intent was:

 - if not a regular file, then don't clear
 - else if task has CAP_FSETID in init_user_ns, then don't clear
 - else if group exec is set, then clear
 - else if gid is in task's group list, then don't clear
 - else if gid and uid are mapped in current namespace and task has
CAP_FSETID in current namespace, then don't clear
 - else clear

However behavior seems to deviate from that if group exec is clear and
*suid* bit is not set.  The reason is that inode_has_no_xattr() will
set S_NOSEC and __file_remove_privs() will bail out before even
starting to interpret the rules.

This behavior is also codified in xfstests.  E.g. generic/683.out:

Test 1 - qa_user, non-exec file falloc
6666 -rwSrwSrw- TEST_DIR/683/a
666 -rw-rw-rw- TEST_DIR/683/a

vs

Test 9 - qa_user, non-exec file falloc, only sgid
2666 -rw-rwSrw- TEST_DIR/683/a
2666 -rw-rwSrw- TEST_DIR/683/a

and

Test 3 - qa_user, user-exec file falloc
6766 -rwsrwSrw- TEST_DIR/683/a
766 -rwxrw-rw- TEST_DIR/683/a

vs

_user, user-exec file falloc, only sgid
2766 -rwxrwSrw- TEST_DIR/683/a
2766 -rwxrwSrw- TEST_DIR/683/a

Should the S_IXGRP test be purged from is_sxid() to make this consistent?

Thanks,
Miklos