xfsqa: add testcase for ->setattr permission checking

Index: xfs-cmds-git/xfstests/192
===================================================================

xfstests/192 new file +177 -0

@@ -0,0 +1,177 @@
+#! /bin/sh
+# FS QA Test No. 192
+#
+# Test permission checks in ->setattr
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2008 Christoph Hellwig.
+#-----------------------------------------------------------------------
+#
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup_files; exit \$status" 0 1 2 3 15
+tag="added by qa $seq"
+
+#
+# For some tests we need a secondary group for the qa_user.  Currently
+# that's not available in the framework, so the tests using it are
+# commented out.
+#
+#group2=foo
+
+#
+# Create two files, one owned by root, one by the qa_user
+#
+_create_files()
+{
+	touch test.root
+	touch test.${qa_user}
+	chown ${qa_user}:${qa_user} test.${qa_user}
+}
+
+#
+# Remove our files again
+#
+_cleanup_files()
+{
+	rm -f test.${qa_user}
+	rm -f test.root
+}
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs xfs nfs udf
+_supported_os Linux
+
+_require_user
+_need_to_be_root
+
+
+#
+# make sure we have a normal umask set
+#
+umask 022
+
+
+#
+# Test the ATTR_UID case
+#
+echo
+echo "testing ATTR_UID"
+echo
+
+_create_files
+
+echo "user: chown root owned file to qa_user (should fail)"
+su ${qa_user} -c "chown root test.${qa_user}"
+
+echo "user: chown root owned file to root (should fail)"
+su ${qa_user} -c "chown root test.root"
+
+echo "user: chown qa_user owned file to qa_user (should succeed)"
+su ${qa_user} -c "chown ${qa_user} test.${qa_user}"
+
+# this would work without _POSIX_CHOWN_RESTRICTED
+echo "user: chown qa_user owned file to root (should fail)"
+su ${qa_user} -c "chown ${qa_user} test.root"
+
+_cleanup_files
+
+#
+# Test the ATTR_GID case
+#
+echo
+echo "testing ATTR_GID"
+echo
+
+_create_files
+
+echo "user: chgrp root owned file to root (should fail)"
+su ${qa_user} -c "chgrp root test.root"
+
+echo "user: chgrp qa_user owned file to root (should fail)"
+su ${qa_user} -c "chgrp root test.${qa_user}"
+
+echo "user: chgrp root owned file to qa_user (should fail)"
+su ${qa_user} -c "chgrp ${qa_user} test.root"
+
+echo "user: chgrp qa_user owned file to qa_user (should suceed)"
+su ${qa_user} -c "chgrp ${qa_user} test.${qa_user}"
+
+#echo "user: chgrp qa_user owned file to secondary group (should suceed)"
+#su ${qa_user} -c "chgrp ${group2} test.${qa_user}"
+
+_cleanup_files
+
+
+#
+# Test the ATTR_MODE case
+#
+echo
+echo "testing ATTR_MODE"
+echo
+
+_create_files
+
+echo "user: chmod a+r on qa_user owned file (should succeed)"
+su ${qa_user} -c "chmod a+r test.${qa_user}"
+
+echo "user: chmod a+r on root owned file (should fail)"
+su ${qa_user} -c "chmod a+r test.root"
+
+#
+# Setup a file owned by the qa_user, but with a group ID that
+# is not present in the qa_users group list (use root to make it easier for it)
+# and mark it with set sgid bit
+#
+echo "check that the sgid bit is cleared"
+chown ${qa_user}:root test.${qa_user}
+chmod g+s test.${qa_user}
+
+# and let the qa_user change permission bits
+su ${qa_user} -c "chmod a+w test.${qa_user}"
+stat -c '%A' test.${qa_user}
+
+#
+# Setup a file owned by the qa_user and with the suid bit set.
+# A chown by root should not clean the suid bit.
+#
+echo "check that suid bit is not cleared"
+chmod u+s test.${qa_user}
+chmod a+w test.${qa_user}
+stat -c '%A' test.${qa_user}
+
+_cleanup_files
+
+
+#
+# Test ATTR_*TIMES_SET
+#
+echo
+echo "testing ATTR_*TIMES_SET"
+echo
+
+_create_files
+
+echo "user: touch qa_user file (should succeed)"
+su ${qa_user} -c "touch test.${qa_user}"
+
+echo "user: touch root file (should fail)"
+su ${qa_user} -c "touch test.root"
+
+_cleanup_files
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0

Index: xfs-cmds-git/xfstests/group
===================================================================

xfstests/group +1 -0

@@ -291,3 +291,4 @@
 189 mount auto
 190 rw auto
 191 nfs4acl auto
+192 auto metadata

Index: xfs-cmds-git/xfstests/192.out
===================================================================

xfstests/192.out new file +28 -0

@@ -0,0 +1,38 @@
+QA output created by 192
+
+testing ATTR_UID
+
+user: chown root owned file to qa_user (should fail)
+chown: changing ownership of `test.fsgqa': Operation not permitted
+user: chown root owned file to root (should fail)
+chown: changing ownership of `test.root': Operation not permitted
+user: chown qa_user owned file to qa_user (should succeed)
+user: chown qa_user owned file to root (should fail)
+chown: changing ownership of `test.root': Operation not permitted
+
+testing ATTR_GID
+
+user: chgrp root owned file to root (should fail)
+chgrp: changing group of `test.root': Operation not permitted
+user: chgrp qa_user owned file to root (should fail)
+chgrp: changing group of `test.fsgqa': Operation not permitted
+user: chgrp root owned file to qa_user (should fail)
+chgrp: changing group of `test.root': Operation not permitted
+user: chgrp qa_user owned file to qa_user (should suceed)
+
+testing ATTR_MODE
+
+user: chmod a+r on qa_user owned file (should succeed)
+user: chmod a+r on root owned file (should fail)
+chmod: changing permissions of `test.root': Operation not permitted
+check that the sgid bit is cleared

Index: xfs-cmds-git/xfstests/192
===================================================================

xfstests/192 new file +64 -0

@@ -0,0 +1,177 @@
+#! /bin/sh
+# FS QA Test No. 192
+#
+# Test permission checks in ->setattr
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2008 Christoph Hellwig.
+#-----------------------------------------------------------------------
+#
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup_files; exit \$status" 0 1 2 3 15
+tag="added by qa $seq"
+
+#
+# For some tests we need a secondary group for the qa_user.  Currently
+# that's not available in the framework, so the tests using it are
+# commented out.
+#
+#group2=foo
+
+#
+# Create two files, one owned by root, one by the qa_user
+#
+_create_files()
+{
+	touch test.root
+	touch test.${qa_user}
+	chown ${qa_user}:${qa_user} test.${qa_user}
+}
+
+#
+# Remove our files again
+#
+_cleanup_files()
+{
+	rm -f test.${qa_user}
+	rm -f test.root
+}
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# real QA test starts here
+_supported_fs xfs nfs udf
+_supported_os Linux
+
+_require_user
+_need_to_be_root
+
+
+#
+# make sure we have a normal umask set
+#
+umask 022
+

1.

xfstests/192 line 77

Quoted 3 lines

xfstests/192 new file +3 -0

+echo "user: chown root owned file to qa_user (should fail)"
+su ${qa_user} -c "chown root test.${qa_user}"
+

I think the description and command above don't match. I think we have a swap with subtest 4 below. Need to either swap descriptions or commands.

xfstests/192 line 148

Quoted 4 lines

xfstests/192 new file +4 -0

+#
+# Setup a file owned by the qa_user and with the suid bit set.
+# A chown by root should not clean the suid bit.
+#

Typos:
s/clean/clear/

s/suceed/succeed/ in a couple of places.

* It looks like you test the clearing of suid/sgid bits
for setting the mode permission bits and not
for setting ownership as the description suggests;
i.e. you test with chmod instead of chown for clearing of suid/sgid bits

Quoted 7 lines · Timothy Shimmin

1.

xfstests/192 line 77

Quoted 3 lines

xfstests/192 new file +3 -0

+echo "user: chown root owned file to qa_user (should fail)"
+su ${qa_user} -c "chown root test.${qa_user}"
+

I think the description and command above don't match. I think we have a swap with subtest 4 below. Need to either swap descriptions or commands.

Yes, I swapped the descriptions.

Quoted 8 lines

xfstests/192 line 148

Quoted 4 lines

xfstests/192 new file +4 -0

+#
+# Setup a file owned by the qa_user and with the suid bit set.
+# A chown by root should not clean the suid bit.
+#

Typos:
s/clean/clear/

s/suceed/succeed/ in a couple of places.

Yeah.

Quoted 4 lines

* It looks like you test the clearing of suid/sgid bits
for setting the mode permission bits and not
for setting ownership as the description suggests;
i.e. you test with chmod instead of chown for clearing of suid/sgid bits

Yes, that's also what I intended too,

as XFS had some code to clear
the suid bits for changing permissions, but those shouldn't happen for the restricted_chown case (and don't even happen in the XFS code, it's just not obvious when reading the old setattr implementation).

While for sgid we want to clear it on mode changes if the gid is not in the group list. So what needs fixing here is once again the comment.

Btw, I just noticed you checked in another testcase as 192. Do you want a respin or do you want to fix it up yourself?

For chown:
"If the specified file is a regular file, one or more of the S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, and the process does not have appropriate privileges, the set-user-ID (S_ISUID) and set-group-ID (S_ISGID) bits of the file mode shall be cleared upon successful return from chown(). If the specified file is a regular file, one or more of the S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, and the process has appropriate privileges, it is implementation-defined whether the set-user-ID and set-group-ID bits are altered. If the chown() function is successfully invoked on a file that is not a regular file and one or more of the S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, the set-user-ID and set-group-ID bits may be cleared."

reg file + mode-bits set + no appropriate privileges -> clear suid,sgid reg file + mode-bits set + appropriate privileges -> maybe clear suid,sgid non reg file + mode-bits set + chown success on file (??) -> maybe clear suid/sgid