On Wed, Oct 12, 2016 at 10:35:49AM -0700, Eric Biggers wrote: > Cc +fstests@xxxxxxxxxxxxxxx > > On Wed, Oct 12, 2016 at 09:16:51AM +0200, Jan Kara wrote: > > Hi, > > > > On Tue 11-10-16 16:11:01, Eric Biggers wrote: > > > On Mon, Sep 19, 2016 at 05:42:48PM +0200, Jan Kara wrote: > > > > When file permissions are modified via chmod(2) and the user is not in > > > > the owning group or capable of CAP_FSETID, the setgid bit is cleared in > > > > inode_change_ok(). Setting a POSIX ACL via setxattr(2) sets the file > > > > permissions as well as the new ACL, but doesn't clear the setgid bit in > > > > a similar way; this allows to bypass the check in chmod(2). Fix that. > > > > > > > > > > This patch is causing xfstests generic/314 to fail. This test is supposed to > > > test "SGID inheritance on subdirectories", and the failure is because subdir2 > > > unexpectedly ends up without a SGID bit. This happens because the following > > > commands now result in the SGID bit on the parent directory "$TEST_DIR/$seq-dir" > > > being cleared rather than set: > > > > > > mkdir $TEST_DIR/$seq-dir > > > chown $qa_user:12345 $TEST_DIR/$seq-dir > > > chmod 2775 $TEST_DIR/$seq-dir > > > su $qa_user -c "setfacl -m u:$qa_user:rwx,d:u:$qa_user:rwx $TEST_DIR/$seq-dir" > > > > > > Is this the expected behavior now? > > > > Yes, this is expected behavior - $qa_user is not in group 12345 and thus he > > could not set sgid bit himself. So once mode is modified by the user (and > > the setfacl command you presented will touch file mode) sgid bit is expected > > to be cleared - this is to be consistent with the behavior when: > > > > chmod 2755 $TEST_DIR/$seq-dir > > > > done by $qa_user would clear the sgid bit as well. > > > > Honza > > -- > > Jan Kara <jack@xxxxxxxx> > > SUSE Labs, CR > > Is this true even though we're talking about a directory, not a regular file? > > From the POSIX man page for chmod (man 3 chmod): > > If the calling process does not have appropriate privileges, and if the > group ID of the file does not match the effective group ID or one of > the supplementary group IDs and if the file is a regular file, bit > S_ISGID (set-group-ID on execution) in the file's mode shall be cleared > upon successful return from chmod(). > > Note the "is a regular file". Granted, Linux already cleared the SGID bit on > directories too, so your patch is consistent with that existing behavior. But > if "directories too" is really the "correct" behavior, it looks like there need > to be xfstests updates: > > * generic/314, which is supposed to test SGID inheritance, should be > updated to not depend on any specific SGID clearing behavior. As-is > this test is failing. > > * generic/375, which is a new test that is supposed to test > SGID clearing, possibly should test both a regular file and a > directory. Right now it only tests a regular file. > > Thoughts on this? Send patches to fix/update the tests. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
