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

[Alan Somers]

Author: asomers
Date: Wed May  8 22:28:13 2019
New Revision: 347372
URL: https://svnweb.freebsd.org/changeset/base/347372

Log:
  fusefs: fix a permission handling bug during VOP_RENAME
  
  If the file to be renamed is a directory and it's going to get a new parent,
  then the user must have write permissions to that directory, because the
  ".." dirent must be changed.
  
  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	Wed May  8 21:26:11 2019	(r347371)
+++ projects/fuse2/sys/fs/fuse/fuse_vnops.c	Wed May  8 22:28:13 2019	(r347372)
@@ -1423,7 +1423,8 @@ fuse_vnop_rename(struct vop_rename_args *ap)
 	struct vnode *tvp = ap->a_tvp;
 	struct componentname *tcnp = ap->a_tcnp;
 	struct fuse_data *data;
-
+	bool newparent = fdvp != tdvp;
+	bool isdir = fvp->v_type == VDIR;
 	int err = 0;
 
 	if (fuse_isdeadfs(fdvp)) {
@@ -1442,7 +1443,17 @@ fuse_vnop_rename(struct vop_rename_args *ap)
 	 * under the source directory in the file system tree.
 	 * Linux performs this check at VFS level.
 	 */
+	/* 
+	 * If source is a directory, and it will get a new parent, user must
+	 * have write permission to it, so ".." can be modified.
+	 */
 	data = fuse_get_mpdata(vnode_mount(tdvp));
+	if (data->dataflags & FSESS_DEFAULT_PERMISSIONS && isdir && newparent) {
+		err = fuse_internal_access(fvp, VWRITE,
+			tcnp->cn_thread, tcnp->cn_cred);
+		if (err)
+			goto out;
+	}
 	sx_xlock(&data->rename_lock);
 	err = fuse_internal_rename(fdvp, fcnp, tdvp, tcnp);
 	if (err == 0) {

Modified: projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc	Wed May  8 21:26:11 2019	(r347371)
+++ projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc	Wed May  8 22:28:13 2019	(r347372)
@@ -833,6 +833,49 @@ TEST_F(Rename, eperm_on_sticky_srcdir)
 	ASSERT_EQ(EPERM, errno);
 }
 
+/* 
+ * A user cannot move out a subdirectory that he does not own, because that
+ * would require changing the subdirectory's ".." dirent
+ */
+TEST_F(Rename, eperm_for_subdirectory)
+{
+	const char FULLDST[] = "mountpoint/d/dst";
+	const char FULLSRC[] = "mountpoint/src";
+	const char RELDSTDIR[] = "d";
+	const char RELDST[] = "dst";
+	const char RELSRC[] = "src";
+	uint64_t ino = 42;
+	uint64_t dstdir_ino = 43;
+
+	expect_getattr(1, S_IFDIR | 0777, UINT64_MAX, 1, 0);
+	expect_lookup(RELSRC, ino, S_IFDIR | 0755, UINT64_MAX, 0);
+	expect_lookup(RELDSTDIR, dstdir_ino, S_IFDIR | 0777, UINT64_MAX, 0);
+	EXPECT_LOOKUP(dstdir_ino, RELDST).WillOnce(Invoke(ReturnErrno(ENOENT)));
+
+	ASSERT_EQ(-1, rename(FULLSRC, FULLDST));
+	ASSERT_EQ(EACCES, errno);
+}
+
+/*
+ * A user _can_ rename a subdirectory to which he lacks write permissions, if
+ * it will keep the same parent
+ */
+TEST_F(Rename, subdirectory_to_same_dir)
+{
+	const char FULLDST[] = "mountpoint/dst";
+	const char FULLSRC[] = "mountpoint/src";
+	const char RELDST[] = "dst";
+	const char RELSRC[] = "src";
+	uint64_t ino = 42;
+
+	expect_getattr(1, S_IFDIR | 0777, UINT64_MAX, 1, 0);
+	expect_lookup(RELSRC, ino, S_IFDIR | 0755, UINT64_MAX, 0);
+	EXPECT_LOOKUP(1, RELDST).WillOnce(Invoke(ReturnErrno(ENOENT)));
+	expect_rename(0);
+
+	ASSERT_EQ(0, rename(FULLSRC, FULLDST)) << strerror(errno);
+}
+
 TEST_F(Rename, eperm_on_sticky_dstdir)
 {
 	const char FULLDST[] = "mountpoint/d/dst";