svn commit: r347191 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs

[Alan Somers]

Author: asomers
Date: Mon May  6 16:54:35 2019
New Revision: 347191
URL: https://svnweb.freebsd.org/changeset/base/347191

Log:
  fusefs: Fix another obscure permission handling bug
  
  Don't allow unprivileged users to set SGID on files to whose group they
  don't belong.  This is slightly different than what POSIX says we should do
  (clear sgid on return from a successful chmod), but it matches what UFS
  currently does.
  
  Reported by:	pjdfstest
  Sponsored by:	The FreeBSD Foundation

Modified:
  projects/fuse2/sys/fs/fuse/fuse_vnops.c
  projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc

Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c
==============================================================================
--- projects/fuse2/sys/fs/fuse/fuse_vnops.c	Mon May  6 16:22:45 2019	(r347190)
+++ projects/fuse2/sys/fs/fuse/fuse_vnops.c	Mon May  6 16:54:35 2019	(r347191)
@@ -1589,6 +1589,17 @@ fuse_vnop_setattr(struct vop_setattr_args *ap)
 		if (checkperm && vp->v_type != VDIR && (vap->va_mode & S_ISTXT)
 		    && 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);
+			if (!groupmember(old_va.va_gid, cred)) {
+				err = priv_check_cred(cred, PRIV_VFS_SETGID);
+				if (err)
+					return (err);
+			}
+		}
 		accmode |= VADMIN;
 	}
 

Modified: projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc	Mon May  6 16:22:45 2019	(r347190)
+++ projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc	Mon May  6 16:54:35 2019	(r347191)
@@ -225,6 +225,27 @@ void expect_setxattr(int error)
 }
 };
 
+/* Return a group to which this user does not belong */
+static gid_t excluded_group()
+{
+	int i, ngroups = 64;
+	gid_t newgid, groups[ngroups];
+
+	getgrouplist(getlogin(), getegid(), groups, &ngroups);
+	for (newgid = 0; newgid >= 0; newgid++) {
+		bool belongs = false;
+
+		for (i = 0; i < ngroups; i++) {
+			if (groups[i] == newgid)
+				belongs = true;
+		}
+		if (!belongs)
+			break;
+	}
+	/* newgid is now a group to which the current user does not belong */
+	return newgid;
+}
+
 TEST_F(Access, eacces)
 {
 	const char FULLPATH[] = "mountpoint/some_file.txt";
@@ -304,27 +325,13 @@ TEST_F(Chgrp, eperm)
 	const char RELPATH[] = "some_file.txt";
 	const uint64_t ino = 42;
 	const mode_t mode = 0755;
-	int ngroups = 64;
-	gid_t groups[ngroups];
 	uid_t uid;
 	gid_t gid, newgid;
-	int i;
 
 	uid = geteuid();
 	gid = getegid();
-	getgrouplist(getlogin(), getegid(), groups, &ngroups);
-	for (newgid = 0; newgid >= 0; newgid++) {
-		bool belongs = false;
+	newgid = excluded_group();
 
-		for (i = 0; i < ngroups; i++) {
-			if (groups[i] == newgid)
-				belongs = true;
-		}
-		if (!belongs)
-			break;
-	}
-	/* newgid is now a group to which the current user does not belong */
-
 	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(
@@ -779,6 +786,33 @@ TEST_F(Setattr, eacces)
 
 	expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1);
 	expect_lookup(RELPATH, ino, S_IFREG | oldmode, UINT64_MAX, 0);
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([](auto in) {
+			return (in->header.opcode == FUSE_SETATTR);
+		}, Eq(true)),
+		_)
+	).Times(0);
+
+	EXPECT_NE(0, chmod(FULLPATH, newmode));
+	EXPECT_EQ(EPERM, errno);
+}
+
+/* 
+ * Setting the sgid bit should fail for an unprivileged user who doesn't belong
+ * to the file's group
+ */
+TEST_F(Setattr, sgid_by_non_group_member)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const uint64_t ino = 42;
+	const mode_t oldmode = 0755;
+	const mode_t newmode = 02755;
+	uid_t uid = geteuid();
+	gid_t gid = excluded_group();
+
+	expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1);
+	expect_lookup(RELPATH, ino, S_IFREG | oldmode, UINT64_MAX, uid, gid);
 	EXPECT_CALL(*m_mock, process(
 		ResultOf([](auto in) {
 			return (in->header.opcode == FUSE_SETATTR);