From 4e83d6555e99880df9d89decd5ef1009a4a9f68b Mon Sep 17 00:00:00 2001
From: Alan Somers <asomers@FreeBSD.org>
Date: Tue, 7 May 2019 01:27:23 +0000
Subject: [PATCH] fusefs: allow the null chown and null chgrp

Even an unprivileged user should be able to chown a file to its current
owner, or chgrp it to its current group.  Those are no-ops.

Reported by:	pjdfstest
Sponsored by:	The FreeBSD Foundation
---
 sys/fs/fuse/fuse_vnops.c                   | 45 +++++++++++++++++-----
 tests/sys/fs/fusefs/default_permissions.cc | 37 ++++++++++++++----
 2 files changed, 65 insertions(+), 17 deletions(-)

diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c
index 920cffc4e5bd..b5b2ab9208d5 100644
--- a/sys/fs/fuse/fuse_vnops.c
+++ b/sys/fs/fuse/fuse_vnops.c
@@ -1519,15 +1519,21 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
 	struct thread *td = curthread;
 	struct mount *mp;
 	struct fuse_data *data;
+	struct vattr old_va;
 	int dataflags;
-	int err = 0;
+	int err = 0, err2;
 	accmode_t accmode = 0;
 	bool checkperm;
+	gid_t cr_gid;
 
 	mp = vnode_mount(vp);
 	data = fuse_get_mpdata(mp);
 	dataflags = data->dataflags;
 	checkperm = dataflags & FSESS_DEFAULT_PERMISSIONS;
+	if (cred->cr_ngroups > 0)
+		cr_gid = cred->cr_groups[0];
+	else
+		cr_gid = 0;
 
 	if (fuse_isdeadfs(vp)) {
 		return ENXIO;
@@ -1537,10 +1543,20 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
 		if (checkperm) {
 			/* Only root may change a file's owner */
 			err = priv_check_cred(cred, PRIV_VFS_CHOWN);
-			if (err)
-				return err;;
-		}
-		accmode |= VADMIN;
+			if (err) {
+				/* As a special case, allow the null chown */
+				err2 = fuse_internal_getattr(vp, &old_va, cred,
+					td);
+				if (err2)
+					return (err2);
+				if (vap->va_uid != old_va.va_uid)
+					return err;
+				else
+					accmode |= VADMIN;
+			} else
+				accmode |= VADMIN;
+		} else
+			accmode |= VADMIN;
 	}
 	if (vap->va_gid != (gid_t)VNOVAL) {
 		if (checkperm && !groupmember(vap->va_gid, cred))
@@ -1550,10 +1566,20 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
 			 * groups 
 			 */
 			err = priv_check_cred(cred, PRIV_VFS_CHOWN);
-			if (err)
-				return err;
-		}
-		accmode |= VADMIN;
+			if (err) {
+				/* As a special case, allow the null chgrp */
+				err2 = fuse_internal_getattr(vp, &old_va, cred,
+					td);
+				if (err2)
+					return (err2);
+				if (vap->va_gid != old_va.va_gid)
+					return err;
+				else
+					accmode |= VADMIN;
+			} else
+				accmode |= VADMIN;
+		} else
+			accmode |= VADMIN;
 	}
 	if (vap->va_size != VNOVAL) {
 		switch (vp->v_type) {
@@ -1591,7 +1617,6 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
 		    && priv_check_cred(cred, PRIV_VFS_STICKYFILE))
 			return EFTYPE;
 		if (checkperm && (vap->va_mode & S_ISGID)) {
-			struct vattr old_va;
 			err = fuse_internal_getattr(vp, &old_va, cred, td);
 			if (err)
 				return (err);
diff --git a/tests/sys/fs/fusefs/default_permissions.cc b/tests/sys/fs/fusefs/default_permissions.cc
index a538bae237ea..c17754426b2e 100644
--- a/tests/sys/fs/fusefs/default_permissions.cc
+++ b/tests/sys/fs/fusefs/default_permissions.cc
@@ -294,6 +294,34 @@ TEST_F(Access, ok)
 	ASSERT_EQ(0, access(FULLPATH, access_mode)) << strerror(errno);
 }
 
+/* Unprivileged users may chown a file to their own uid */
+TEST_F(Chown, chown_to_self)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const uint64_t ino = 42;
+	const mode_t mode = 0755;
+	uid_t uid;
+
+	uid = geteuid();
+
+	expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1, uid);
+	expect_lookup(RELPATH, ino, S_IFREG | mode, UINT64_MAX, uid);
+	/* The OS may optimize chown by omitting the redundant setattr */
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([](auto in) {
+			return (in->header.opcode == FUSE_SETATTR);
+		}, Eq(true)),
+		_)
+	).WillRepeatedly(Invoke(ReturnImmediate([=](auto in __unused, auto out){
+		SET_OUT_HEADER_LEN(out, attr);
+		out->body.attr.attr.mode = S_IFREG | mode;
+		out->body.attr.attr.uid = uid;
+	})));
+
+	EXPECT_EQ(0, chown(FULLPATH, uid, -1)) << strerror(errno);
+}
+
 /* Only root may change a file's owner */
 TEST_F(Chown, eperm)
 {
@@ -357,19 +385,14 @@ TEST_F(Chgrp, ok)
 
 	expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1, uid, gid);
 	expect_lookup(RELPATH, ino, S_IFREG | mode, UINT64_MAX, uid, gid);
-	EXPECT_CALL(*m_mock, process(
-		ResultOf([](auto in) {
-			return (in->header.opcode == FUSE_SETATTR);
-		}, Eq(true)),
-		_)
-	).Times(0);
+	/* The OS may optimize chgrp by omitting the redundant setattr */
 	EXPECT_CALL(*m_mock, process(
 		ResultOf([](auto in) {
 			return (in->header.opcode == FUSE_SETATTR &&
 				in->header.nodeid == ino);
 		}, Eq(true)),
 		_)
-	).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto out) {
+	).WillRepeatedly(Invoke(ReturnImmediate([=](auto in __unused, auto out){
 		SET_OUT_HEADER_LEN(out, attr);
 		out->body.attr.attr.mode = S_IFREG | mode;
 		out->body.attr.attr.uid = uid;