Re: [PATCH v2 RESEND] posix_acl: Clear SGID bit when setting file permissions

[Dave Chinner <david@fromorbit.com>]

On Wed, Oct 12, 2016 at 10:35:49AM -0700, Eric Biggers wrote:
> Cc +fstests@vger.kernel.org
> 
> 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@suse.com>
> > 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@fromorbit.com