orangefs: posix read and write on open files.
diff mbox series

Message ID 20191120154111.9788-1-hubcap@kernel.org
State New
Headers show
Series
  • orangefs: posix read and write on open files.
Related show

Commit Message

hubcap@kernel.org Nov. 20, 2019, 3:41 p.m. UTC
From: Mike Marshall <hubcap@omnibond.com>

Orangefs doesn't have an open function. Orangefs performs
permission checks on files each time they are accessed.
Users create files with arbitrary permissions. A user
might create a file with a mode that doesn't include write,
or change the mode of a file he has opened to one that doesn't
include write. Posix says the user can write on the file
anyway since he was able to open it.

Orangefs through the kernel module needs to seem posixy, so
when someone creates or chmods a file to a mode that disallows owner
read and/or write, we'll call orangefs_posix_open to stamp a
temporary S_IRWXU acl on it in userspace without telling the kernel
about the acl, and remove the acl later when the kernel passes through
file_operations->release.

This fixes known real-world problems: git, for example,
uses openat(AT_FDCWD, argv[1], O_RDWR|O_CREAT|O_EXCL, 0444)
on some important files it later tries to write on during clone.
We don't actually know use cases where people chmod their open files to
un-writability and then try to write on them, but they
should be able to if they want to :-).

Signed-off-by: Mike Marshall <hubcap@omnibond.com>
---
 fs/orangefs/file.c            | 16 ++++++++++++
 fs/orangefs/inode.c           | 15 +++++++++++
 fs/orangefs/namei.c           | 10 ++++++-
 fs/orangefs/orangefs-kernel.h |  3 +++
 fs/orangefs/orangefs-utils.c  | 49 +++++++++++++++++++++++++++++++++++
 5 files changed, 92 insertions(+), 1 deletion(-)

Comments

Linus Torvalds Nov. 20, 2019, 5:06 p.m. UTC | #1
On Wed, Nov 20, 2019 at 7:41 AM <hubcap@kernel.org> wrote:
>
> From: Mike Marshall <hubcap@omnibond.com>
>
> Orangefs doesn't have an open function. Orangefs performs
> permission checks on files each time they are accessed.

This is completely broken, and your fix doesn't even fix the brokenness.

Giving a user access rights as a workaround for the breakage is wrong,
and has nothing at all to do with POSIX. It just breaks things even
more in other ways - now you open other processes to re-open the file
when they really really shouldn't be able to. So your "fix" is quite
possibly a security issue.

Also, the much more common case - that your patch doesn't fix - is
that a file is opened with one set of credentials, and then used with
another set entirely. Trying to use some kind of ACL to say "original
opener can write to this" is wrong, and doesn't fix that.

For example, the file may be opened by root, and then root drops all
privileges and reverts to the original user. The file should still be
writable, even though the UID changed.

(Another case of that is to just transfer the fd over unix domain
sockets to a different process entirely, but that is much more
unusual).

The fact is, permission checks at the time of access are simply
*wrong*. They cannot work. No amount of "give the file a fake ACL"
will ever make it work.

The permission checks are done at open time. After that, they are
simply not done. That's the POSIX model.

                  Linus

Patch
diff mbox series

diff --git a/fs/orangefs/file.c b/fs/orangefs/file.c
index a5612abc0936..c582e7bc5d40 100644
--- a/fs/orangefs/file.c
+++ b/fs/orangefs/file.c
@@ -513,6 +513,22 @@  static int orangefs_file_release(struct inode *inode, struct file *file)
 		}
 
 	}
+	/*
+	 * Check to see if we're affecting posixness by keeping a temporary
+	 * owner rw ACL on this file while it is open, so that the process
+	 * that has it open can read and write it no matter what the mode is.
+	 * If there is a temporary ACL on the file, clean it and
+	 * its associated inode flag up. See the comments in
+	 * orangefs_posix_open for more info.
+	 */
+	if (ORANGEFS_I(inode)->opened) {
+		ORANGEFS_I(inode)->opened = 0;
+		orangefs_inode_setxattr(inode,
+			XATTR_NAME_POSIX_ACL_ACCESS,
+			NULL,
+			0,
+			0);
+	}
 	return 0;
 }
 
diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index efb12197da18..af9eb24ad7c6 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -880,6 +880,20 @@  int __orangefs_setattr(struct inode *inode, struct iattr *iattr)
 		}
 	}
 
+	/*
+	 * if it seems like someone might be fixing to chmod an open file into
+	 * unread or unwritability, use the orangefs_posix_open hat-trick to
+	 * posixly provide read and writability.
+	 */
+	if ((iattr->ia_mode) &&
+	    (!ORANGEFS_I(inode)->opened) &&
+	    (iattr->ia_valid & ATTR_MODE) &&
+	    (!(iattr->ia_mode & S_IRUSR) || (!(iattr->ia_mode & S_IWUSR)))) {
+		ret = orangefs_posix_open(inode);
+		if (ret)
+			goto out;
+	}
+
 	if (iattr->ia_valid & ATTR_SIZE) {
 		ret = orangefs_setattr_size(inode, iattr);
 		if (ret)
@@ -1060,6 +1074,7 @@  static int orangefs_set_inode(struct inode *inode, void *data)
 	hash_init(ORANGEFS_I(inode)->xattr_cache);
 	ORANGEFS_I(inode)->mapping_time = jiffies - 1;
 	ORANGEFS_I(inode)->bitlock = 0;
+	ORANGEFS_I(inode)->opened = 0;
 	return 0;
 }
 
diff --git a/fs/orangefs/namei.c b/fs/orangefs/namei.c
index 3e7cf3d0a494..040ef164563e 100644
--- a/fs/orangefs/namei.c
+++ b/fs/orangefs/namei.c
@@ -86,7 +86,15 @@  static int orangefs_create(struct inode *dir,
 	iattr.ia_valid |= ATTR_MTIME | ATTR_CTIME;
 	iattr.ia_mtime = iattr.ia_ctime = current_time(dir);
 	__orangefs_setattr(dir, &iattr);
-	ret = 0;
+
+	/*
+	 * If you can open (or create) a file, Posix says you should
+	 * be able to read or write to the file without regard to
+	 * the file's mode until the fd is closed.
+	 */
+	if (!(mode & S_IRUSR) || (!(mode & S_IWUSR)))
+		ret = orangefs_posix_open(inode);
+
 out:
 	op_release(new_op);
 	gossip_debug(GOSSIP_NAME_DEBUG,
diff --git a/fs/orangefs/orangefs-kernel.h b/fs/orangefs/orangefs-kernel.h
index 34a6c99fa29b..cad7a7e08e34 100644
--- a/fs/orangefs/orangefs-kernel.h
+++ b/fs/orangefs/orangefs-kernel.h
@@ -198,6 +198,7 @@  struct orangefs_inode_s {
 	kuid_t attr_uid;
 	kgid_t attr_gid;
 	unsigned long bitlock;
+	int opened;
 
 	DECLARE_HASHTABLE(xattr_cache, 4);
 };
@@ -405,6 +406,8 @@  ssize_t do_readv_writev(enum ORANGEFS_io_type, struct file *, loff_t *,
 /*
  * defined in orangefs-utils.c
  */
+int orangefs_posix_open(struct inode *inode);
+
 __s32 fsid_of_op(struct orangefs_kernel_op_s *op);
 
 ssize_t orangefs_inode_getxattr(struct inode *inode,
diff --git a/fs/orangefs/orangefs-utils.c b/fs/orangefs/orangefs-utils.c
index d4b7ae763186..3c60c65ec445 100644
--- a/fs/orangefs/orangefs-utils.c
+++ b/fs/orangefs/orangefs-utils.c
@@ -452,6 +452,55 @@  int orangefs_inode_setattr(struct inode *inode)
 	return ret;
 }
 
+/*
+ * Orangefs doesn't have an open function. Orangefs performs
+ * permission checks on files each time they are accessed.
+ * Users create files with arbitrary permissions. A user
+ * might create a file with a mode that doesn't include write,
+ * or change the mode of a file he has opened to one that doesn't
+ * include write. Posix says the user can write on the file
+ * anyway since he was able to open it.
+ *
+ * Orangefs through the kernel module needs to seem posixy, so
+ * when someone creates or chmods a file to a mode that disallows owner
+ * read and/or write, we'll call orangefs_posix_open to stamp a
+ * temporary S_IRWXU acl on it in userspace without telling the kernel
+ * about the acl, and remove the acl later when the kernel passes through
+ * file_operations->release.
+ *
+ * This fixes known real-world problems: git, for example,
+ * uses openat(AT_FDCWD, argv[1], O_RDWR|O_CREAT|O_EXCL, 0444)
+ * on some important files it later tries to write on during clone.
+ * We don't actually know use cases where people chmod their open files to
+ * un-writability and then try to write on them, but they
+ * should be able to if they want to :-).
+ */
+int orangefs_posix_open(struct inode *inode) {
+	struct posix_acl_xattr_header *posix_header;
+	struct posix_acl_xattr_entry *posix_entry;
+	void *buffer;
+	int size = sizeof(struct posix_acl_xattr_header) +
+			sizeof(struct posix_acl_xattr_entry);
+	const char *name = XATTR_NAME_POSIX_ACL_ACCESS;
+	int ret;
+
+	buffer = kmalloc(size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	ORANGEFS_I(inode)->opened = 1;
+	posix_header = buffer;
+	posix_header->a_version = POSIX_ACL_XATTR_VERSION;
+	posix_entry = (void *)(posix_header + 1);
+	posix_entry->e_tag = ACL_USER;
+	posix_entry->e_perm = S_IRWXU >> 6;
+	posix_entry->e_id = from_kuid(&init_user_ns, current_fsuid());
+
+	ret = orangefs_inode_setxattr(inode, name, buffer, size, 0);
+	kfree(buffer);
+	return ret;
+}
+
 /*
  * The following is a very dirty hack that is now a permanent part of the
  * ORANGEFS protocol. See protocol.h for more error definitions.