Re: [Virtio-fs] Securing file handles

[Miklos Szeredi <mszeredi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

On Mon, Mar 8, 2021 at 2:39 PM Max Reitz <mreitz@redhat.com> wrote:

> Admittedly I’m not yet at the point where I feel comfortable doing
> changes to the kernel at all, so if you have the time, I’d appreciate
> it.  (If you don’t really have the time, I could try my hand first and
> then we’d see.)

I'd hate to context switch away from the fuse leases to the kernel
crypto, so it would have to wait some time...

But I've attached an incomplete patch that just missing the crypto
bits and testing.

Would you mind having a go at it?

>
> So AFAIU you want to put this in the kernel so we can get rid of needing
> the capability, because when you can only open handles that were
> previously generated for you, there wouldn’t be a security problem, right?

Something like that.

> But what about cases where a file is made inaccessible to some process
> between generating the handle and later opening it?  E.g. in
> /path/to/file, the “to” directory is changed to go-x (and the current
> user is not the owner), so opening /path/to/file wouldn’t be possible by
> path anymore.  Sure, if the FD remained open, you could still open the
> file anyway; but I consider it different in semantics.  (E.g. you could
> check that there are no processes that have “file” open anymore, and so
> you could assume that it’s now inaccessible.)

That could be a concern, yes.   Requiring CAP_DAC_READ_SEARCH in the
current user namespace, as my template patch does, might mitigate
those worries somewhat.

Thanks,
Miklos

[-- Attachment #2: fhandle-mac.patch --]
[-- Type: text/x-patch, Size: 4800 bytes --]

diff --git a/fs/fhandle.c b/fs/fhandle.c
index ec6feeccc276..17c066be0a5d 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -14,9 +14,40 @@
 #include "internal.h"
 #include "mount.h"
 
+static int fhandle_add_mac(struct file_handle *handle, size_t handle_alloc)
+{
+	struct files_struct *files = current->files;
+	struct fhandle_mac_key *key = READ_ONCE(files->key), *old;
+	size_t mac_size = 8; /* or whatever */
+
+	handle->handle_bytes += mac_size;
+
+	if (handle->handle_type == FILEID_INVALID ||
+	    handle->handle_bytes > handle_alloc)
+		return FILEID_INVALID;
+
+	if (!key) {
+		key = /* generate_key */;
+		if (!key)
+			return -ENOMEM;
+		old = cmpxchg(&files->key, NULL, key);
+		if (old) {
+			/* race */
+			kfree(key);
+			key = old;
+		}
+	}
+
+	/* add MAC to the end of the current handle using key */
+
+	handle->handle_type |= FILEID_MAC:
+
+	return 0;
+}
+
 static long do_sys_name_to_handle(struct path *path,
 				  struct file_handle __user *ufh,
-				  int __user *mnt_id)
+				  int __user *mnt_id, bool mac)
 {
 	long retval;
 	struct file_handle f_handle;
@@ -49,12 +80,20 @@ static long do_sys_name_to_handle(struct path *path,
 	retval = exportfs_encode_fh(path->dentry,
 				    (struct fid *)handle->f_handle,
 				    &handle_dwords,  0);
+	if (retval == -ENOSPC)
+		retval = FILEID_INVALID;
 	handle->handle_type = retval;
 	/* convert handle size to bytes */
 	handle_bytes = handle_dwords * sizeof(u32);
 	handle->handle_bytes = handle_bytes;
+	if (mac) {
+		retval = fhandle_add_mac(handle, f_handle.handle_bytes);
+		if (retval < 0)
+			goto out;
+		handle_bytes = handle->handle_bytes;
+	}
 	if ((handle->handle_bytes > f_handle.handle_bytes) ||
-	    (retval == FILEID_INVALID) || (retval == -ENOSPC)) {
+	    retval == FILEID_INVALID)
 		/* As per old exportfs_encode_fh documentation
 		 * we could return ENOSPC to indicate overflow
 		 * But file system returned 255 always. So handle
@@ -73,6 +112,7 @@ static long do_sys_name_to_handle(struct path *path,
 	    copy_to_user(ufh, handle,
 			 sizeof(struct file_handle) + handle_bytes))
 		retval = -EFAULT;
+out:
 	kfree(handle);
 	return retval;
 }
@@ -98,7 +138,7 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 	int lookup_flags;
 	int err;
 
-	if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
+	if ((flag & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH | AT_HANDLE_MAC)) != 0)
 		return -EINVAL;
 
 	lookup_flags = (flag & AT_SYMLINK_FOLLOW) ? LOOKUP_FOLLOW : 0;
@@ -106,7 +146,8 @@ SYSCALL_DEFINE5(name_to_handle_at, int, dfd, const char __user *, name,
 		lookup_flags |= LOOKUP_EMPTY;
 	err = user_path_at(dfd, name, lookup_flags, &path);
 	if (!err) {
-		err = do_sys_name_to_handle(&path, handle, mnt_id);
+		err = do_sys_name_to_handle(&path, handle, mnt_id,
+			flag & AT_HANDLE_MAC);
 		path_put(&path);
 	}
 	return err;
@@ -147,6 +188,14 @@ static int do_handle_to_path(int mountdirfd, struct file_handle *handle,
 		retval = PTR_ERR(path->mnt);
 		goto out_err;
 	}
+
+	if (handle->handle_type & FILEID_MAC) {
+		/* verify mac using current->files->key */
+		handle->handle_bytes -= 8;
+	} else if(!ns_capable(path->mnt->mnt_sb->s_user_ns, CAP_DAC_READ_SEARCH)) {
+		return -EPERM;
+	}
+
 	/* change the handle size to multiple of sizeof(u32) */
 	handle_dwords = handle->handle_bytes >> 2;
 	path->dentry = exportfs_decode_fh(path->mnt,
@@ -176,7 +225,7 @@ static int handle_to_path(int mountdirfd, struct file_handle __user *ufh,
 	 * directory. Ideally we would like CAP_DAC_SEARCH.
 	 * But we don't have that
 	 */
-	if (!capable(CAP_DAC_READ_SEARCH)) {
+	if (!ns_capable(current_user_ns(), CAP_DAC_READ_SEARCH)) {
 		retval = -EPERM;
 		goto out_err;
 	}
diff --git a/fs/file.c b/fs/file.c
index f3a4bac2cbe9..9e41b8beea52 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -420,6 +420,8 @@ void put_files_struct(struct files_struct *files)
 		/* free the arrays if they are not embedded */
 		if (fdt != &files->fdtab)
 			__free_fdtable(fdt);
+
+		fhandle_key_free(files->key);
 		kmem_cache_free(files_cachep, files);
 	}
 }
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index fe848901fcc3..a6fdd9bbe98a 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -113,6 +113,9 @@ enum fid_type {
 	 * Filesystems must not use 0xff file ID.
 	 */
 	FILEID_INVALID = 0xff,
+
+	/* OR-ed with the actual ID; used by the fhandle API. */
+	FILEID_MAC = 0x10000000;
 };
 
 struct fid {
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index d0e78174874a..1895d21435ac 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -56,6 +56,8 @@ struct files_struct {
 
 	struct fdtable __rcu *fdt;
 	struct fdtable fdtab;
+
+	struct fhandle_mac_key *key;
   /*
    * written part on a separate cache line in SMP
    */