[PATCH 0/2] fuse: Send file/inode security context during creation

[PATCH 0/2] fuse: Send file/inode security context during creation

[Vivek Goyal]

Hi,

When a file is created (create, mknod, mkdir, symlink), typically file
systems call  ecurity_inode_init_security() to initialize security
context of an inode. But this does not very well with remote filesystems
as inode is not there yet. Client will send a creation request to
server and once server has created the file, client will instantiate
the inode.

So filesystems like nfs and ceph use security_dentry_init_security()
instead. This takes in a dentry and returns the security context of
file if any. 

These patches call security_dentry_init_security() and send security
label of file along with creation request (FUSE_CREATE, FUSE_MKDIR,
FUSE_MKNOD, FUSE_SYMLINK). This will give server an opportunity
to create new file and also set security label (possibly atomically
where possible).

These patches are based on the work Chirantan Ekbote did some time
back but it never got upstreamed. So I have taken his patches,
made some modifications and posting again.

https://listman.redhat.com/archives/virtio-fs/2020-July/msg00014.html
https://listman.redhat.com/archives/virtio-fs/2020-July/msg00015.html

These patches will allow us to support SELinux on virtiofs.

Vivek Goyal (2):
  fuse: Add a flag FUSE_SECURITY_CTX
  fuse: Send security context of inode on file creation

 fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
 fs/fuse/fuse_i.h          |   3 +
 fs/fuse/inode.c           |   4 +-
 include/uapi/linux/fuse.h |  20 ++++++-
 4 files changed, 134 insertions(+), 7 deletions(-)

-- 
2.31.1

[Virtio-fs] [PATCH 0/2] fuse: Send file/inode security context during creation

[Vivek Goyal]

Hi,

When a file is created (create, mknod, mkdir, symlink), typically file
systems call  ecurity_inode_init_security() to initialize security
context of an inode. But this does not very well with remote filesystems
as inode is not there yet. Client will send a creation request to
server and once server has created the file, client will instantiate
the inode.

So filesystems like nfs and ceph use security_dentry_init_security()
instead. This takes in a dentry and returns the security context of
file if any. 

These patches call security_dentry_init_security() and send security
label of file along with creation request (FUSE_CREATE, FUSE_MKDIR,
FUSE_MKNOD, FUSE_SYMLINK). This will give server an opportunity
to create new file and also set security label (possibly atomically
where possible).

These patches are based on the work Chirantan Ekbote did some time
back but it never got upstreamed. So I have taken his patches,
made some modifications and posting again.

https://listman.redhat.com/archives/virtio-fs/2020-July/msg00014.html
https://listman.redhat.com/archives/virtio-fs/2020-July/msg00015.html

These patches will allow us to support SELinux on virtiofs.

Vivek Goyal (2):
  fuse: Add a flag FUSE_SECURITY_CTX
  fuse: Send security context of inode on file creation

 fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
 fs/fuse/fuse_i.h          |   3 +
 fs/fuse/inode.c           |   4 +-
 include/uapi/linux/fuse.h |  20 ++++++-
 4 files changed, 134 insertions(+), 7 deletions(-)

-- 
2.31.1

[PATCH 1/2] fuse: Add a flag FUSE_SECURITY_CTX

[Vivek Goyal]

Add the FUSE_SECURITY_CTX flag for the `flags` field of the
fuse_init_out struct.  When this flag is set the kernel will append the
security context for a newly created inode to the request (create,
mkdir, mknod, and symlink).  The server is responsible for ensuring that
the inode appears atomically (preferrably) with the requested security context.

For example, if the server is backed by a "real" linux file system then
it can write the security context value to
/proc/thread-self/attr/fscreate before making the syscall to create the
inode.

Vivek:
This patch is slightly modified version of patch from
Chirantan Ekbote <chirantan@chromium.org>. I made changes so that this
patch applies to latest kernel.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/uapi/linux/fuse.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 36ed092227fa..2fe54c80051a 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -184,6 +184,10 @@
  *
  *  7.34
  *  - add FUSE_SYNCFS
+ *
+ *  7.35
+ *  - add FUSE_SECURITY_CTX flag for fuse_init_out
+ *  - add security context to create, mkdir, symlink, and mknod requests
  */
 
 #ifndef _LINUX_FUSE_H
@@ -219,7 +223,7 @@
 #define FUSE_KERNEL_VERSION 7
 
 /** Minor version number of this interface */
-#define FUSE_KERNEL_MINOR_VERSION 34
+#define FUSE_KERNEL_MINOR_VERSION 35
 
 /** The node ID of the root inode */
 #define FUSE_ROOT_ID 1
@@ -336,6 +340,8 @@ struct fuse_file_lock {
  *			write/truncate sgid is killed only if file has group
  *			execute permission. (Same as Linux VFS behavior).
  * FUSE_SETXATTR_EXT:	Server supports extended struct fuse_setxattr_in
+ * FUSE_SECURITY_CTX:	add security context to create, mkdir, symlink, and
+ * 			mknod
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -367,6 +373,7 @@ struct fuse_file_lock {
 #define FUSE_SUBMOUNTS		(1 << 27)
 #define FUSE_HANDLE_KILLPRIV_V2	(1 << 28)
 #define FUSE_SETXATTR_EXT	(1 << 29)
+#define FUSE_SECURITY_CTX	(1 << 30)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.31.1

[Virtio-fs] [PATCH 1/2] fuse: Add a flag FUSE_SECURITY_CTX

[Vivek Goyal]

Add the FUSE_SECURITY_CTX flag for the `flags` field of the
fuse_init_out struct.  When this flag is set the kernel will append the
security context for a newly created inode to the request (create,
mkdir, mknod, and symlink).  The server is responsible for ensuring that
the inode appears atomically (preferrably) with the requested security context.

For example, if the server is backed by a "real" linux file system then
it can write the security context value to
/proc/thread-self/attr/fscreate before making the syscall to create the
inode.

Vivek:
This patch is slightly modified version of patch from
Chirantan Ekbote <chirantan@chromium.org>. I made changes so that this
patch applies to latest kernel.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 include/uapi/linux/fuse.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 36ed092227fa..2fe54c80051a 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -184,6 +184,10 @@
  *
  *  7.34
  *  - add FUSE_SYNCFS
+ *
+ *  7.35
+ *  - add FUSE_SECURITY_CTX flag for fuse_init_out
+ *  - add security context to create, mkdir, symlink, and mknod requests
  */
 
 #ifndef _LINUX_FUSE_H
@@ -219,7 +223,7 @@
 #define FUSE_KERNEL_VERSION 7
 
 /** Minor version number of this interface */
-#define FUSE_KERNEL_MINOR_VERSION 34
+#define FUSE_KERNEL_MINOR_VERSION 35
 
 /** The node ID of the root inode */
 #define FUSE_ROOT_ID 1
@@ -336,6 +340,8 @@ struct fuse_file_lock {
  *			write/truncate sgid is killed only if file has group
  *			execute permission. (Same as Linux VFS behavior).
  * FUSE_SETXATTR_EXT:	Server supports extended struct fuse_setxattr_in
+ * FUSE_SECURITY_CTX:	add security context to create, mkdir, symlink, and
+ * 			mknod
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -367,6 +373,7 @@ struct fuse_file_lock {
 #define FUSE_SUBMOUNTS		(1 << 27)
 #define FUSE_HANDLE_KILLPRIV_V2	(1 << 28)
 #define FUSE_SETXATTR_EXT	(1 << 29)
+#define FUSE_SECURITY_CTX	(1 << 30)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.31.1

[PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

When a new inode is created, send its security context to server along
with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
This gives server an opportunity to create new file and set security
context (possibly atomically). In all the configurations it might not
be possible to set context atomically.

Like nfs and ceph, use security_dentry_init_security() to dermine security
context of inode and send it with create, mkdir, mknod, and symlink requests.

Following is the information sent to server.

- struct fuse_secctx.
  This contains total size of security context which follows this structure.

- xattr name string.
  This string represents name of xattr which should be used while setting
  security context. As of now it is hardcoded to "security.selinux".

- security context.
  This is the actual security context whose size is specified in fuse_secctx
  struct.

This patch is modified version of patch from
Chirantan Ekbote <chirantan@chromium.org>

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
 fs/fuse/fuse_i.h          |   3 +
 fs/fuse/inode.c           |   4 +-
 include/uapi/linux/fuse.h |  11 ++++
 4 files changed, 126 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index d9b977c0f38d..439bde1ea329 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -17,6 +17,9 @@
 #include <linux/xattr.h>
 #include <linux/iversion.h>
 #include <linux/posix_acl.h>
+#include <linux/security.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
 
 static void fuse_advise_use_readdirplus(struct inode *dir)
 {
@@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
 	return ERR_PTR(err);
 }
 
+static int get_security_context(struct dentry *entry, umode_t mode,
+				void **security_ctx, u32 *security_ctxlen)
+{
+	struct fuse_secctx *fsecctx;
+	void *ctx, *full_ctx;
+	u32 ctxlen, full_ctxlen;
+	int err = 0;
+
+	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
+					    &ctxlen);
+	if (err) {
+		if (err != -EOPNOTSUPP)
+			goto out_err;
+		/* No LSM is supporting this security hook. Ignore error */
+		err = 0;
+		ctxlen = 0;
+	}
+
+	if (ctxlen > 0) {
+		/*
+		 * security_dentry_init_security() does not return the name
+		 * of lsm or xattr to which label belongs. As of now only
+		 * selinux implements this. Hence, hardcoding the name to
+		 * security.selinux.
+		 */
+		char *name = "security.selinux";
+		void *ptr;
+
+		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
+		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
+		if (!full_ctx) {
+			err = -ENOMEM;
+			kfree(ctx);
+			goto out_err;
+		}
+
+		ptr = full_ctx;
+		fsecctx = (struct fuse_secctx*) ptr;
+		fsecctx->size = ctxlen;
+		ptr += sizeof(*fsecctx);
+		strcpy(ptr, name);
+		ptr += strlen(name) + 1;
+		memcpy(ptr, ctx, ctxlen);
+		kfree(ctx);
+	} else {
+		full_ctxlen = sizeof(*fsecctx);
+		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
+		if (!full_ctx) {
+			err = -ENOMEM;
+			goto out_err;
+		}
+	}
+
+	*security_ctxlen = full_ctxlen;
+	*security_ctx = full_ctx;
+out_err:
+	return err;
+}
+
 /*
  * Atomic create+open operation
  *
@@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	struct fuse_entry_out outentry;
 	struct fuse_inode *fi;
 	struct fuse_file *ff;
+	void *security_ctx = NULL;
+	u32 security_ctxlen;
 
 	/* Userspace expects S_IFREG in create mode */
 	BUG_ON((mode & S_IFMT) != S_IFREG);
@@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	args.out_args[0].value = &outentry;
 	args.out_args[1].size = sizeof(outopen);
 	args.out_args[1].value = &outopen;
+
+	if (fm->fc->init_security) {
+		err = get_security_context(entry, mode, &security_ctx,
+					   &security_ctxlen);
+		if (err)
+			goto out_put_forget_req;
+
+		args.in_numargs = 3;
+		args.in_args[2].size = security_ctxlen;
+		args.in_args[2].value = security_ctx;
+	}
+
 	err = fuse_simple_request(fm, &args);
 	if (err)
 		goto out_free_ff;
@@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 
 out_free_ff:
 	fuse_file_free(ff);
+	kfree(security_ctx);
 out_put_forget_req:
 	kfree(forget);
 out_err:
@@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
  */
 static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 			    struct inode *dir, struct dentry *entry,
-			    umode_t mode)
+			    umode_t mode, bool init_security)
 {
 	struct fuse_entry_out outarg;
 	struct inode *inode;
 	struct dentry *d;
 	int err;
 	struct fuse_forget_link *forget;
+	void *security_ctx = NULL;
+	u32 security_ctxlen = 0;
 
 	if (fuse_is_bad(dir))
 		return -EIO;
@@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 	args->out_numargs = 1;
 	args->out_args[0].size = sizeof(outarg);
 	args->out_args[0].value = &outarg;
+
+	if (init_security) {
+		unsigned short idx = args->in_numargs;
+
+		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
+			err = -ENOMEM;
+			goto out_put_forget_req;
+		}
+
+		err = get_security_context(entry, mode, &security_ctx,
+					   &security_ctxlen);
+		if (err)
+			goto out_put_forget_req;
+
+		if (security_ctxlen > 0) {
+			args->in_args[idx].size = security_ctxlen;
+			args->in_args[idx].value = security_ctx;
+			args->in_numargs++;
+		}
+	}
+
 	err = fuse_simple_request(fm, args);
+	kfree(security_ctx);
 	if (err)
 		goto out_put_forget_req;
 
@@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
 	args.in_args[0].value = &inarg;
 	args.in_args[1].size = entry->d_name.len + 1;
 	args.in_args[1].value = entry->d_name.name;
-	return create_new_entry(fm, &args, dir, entry, mode);
+	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
 }
 
 static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
@@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 	args.in_args[0].value = &inarg;
 	args.in_args[1].size = entry->d_name.len + 1;
 	args.in_args[1].value = entry->d_name.name;
-	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
+	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
+				fm->fc->init_security);
 }
 
 static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
@@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
 	args.in_args[0].value = entry->d_name.name;
 	args.in_args[1].size = len;
 	args.in_args[1].value = link;
-	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
+	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
+				fm->fc->init_security);
 }
 
 void fuse_update_ctime(struct inode *inode)
@@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
 	args.in_args[0].value = &inarg;
 	args.in_args[1].size = newent->d_name.len + 1;
 	args.in_args[1].value = newent->d_name.name;
-	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
+	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
+			       false);
 	/* Contrary to "normal" filesystems it can happen that link
 	   makes two "logical" inodes point to the same "physical"
 	   inode.  We invalidate the attributes of the old one, so it
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 319596df5dc6..885f34f9967f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -765,6 +765,9 @@ struct fuse_conn {
 	/* Propagate syncfs() to server */
 	unsigned int sync_fs:1;
 
+	/* Initialize security xattrs when creating a new inode */
+	unsigned int init_security:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 36cd03114b6d..343bc9cfbd92 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 			}
 			if (arg->flags & FUSE_SETXATTR_EXT)
 				fc->setxattr_ext = 1;
+			if (arg->flags & FUSE_SECURITY_CTX)
+				fc->init_security = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
 		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
 		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
-		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
+		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		ia->in.flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 2fe54c80051a..777c773e143e 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -986,4 +986,15 @@ struct fuse_syncfs_in {
 	uint64_t	padding;
 };
 
+/*
+ * For each security context, send fuse_secctx with size of security context
+ * fuse_secctx will be followed by security context name and this in turn
+ * will be followed by actual context label.
+ * fuse_secctx, name, context
+ * */
+struct fuse_secctx {
+	uint32_t	size;
+	uint32_t	padding;
+};
+
 #endif /* _LINUX_FUSE_H */
-- 
2.31.1

[Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

When a new inode is created, send its security context to server along
with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
This gives server an opportunity to create new file and set security
context (possibly atomically). In all the configurations it might not
be possible to set context atomically.

Like nfs and ceph, use security_dentry_init_security() to dermine security
context of inode and send it with create, mkdir, mknod, and symlink requests.

Following is the information sent to server.

- struct fuse_secctx.
  This contains total size of security context which follows this structure.

- xattr name string.
  This string represents name of xattr which should be used while setting
  security context. As of now it is hardcoded to "security.selinux".

- security context.
  This is the actual security context whose size is specified in fuse_secctx
  struct.

This patch is modified version of patch from
Chirantan Ekbote <chirantan@chromium.org>

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
 fs/fuse/fuse_i.h          |   3 +
 fs/fuse/inode.c           |   4 +-
 include/uapi/linux/fuse.h |  11 ++++
 4 files changed, 126 insertions(+), 6 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index d9b977c0f38d..439bde1ea329 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -17,6 +17,9 @@
 #include <linux/xattr.h>
 #include <linux/iversion.h>
 #include <linux/posix_acl.h>
+#include <linux/security.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
 
 static void fuse_advise_use_readdirplus(struct inode *dir)
 {
@@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
 	return ERR_PTR(err);
 }
 
+static int get_security_context(struct dentry *entry, umode_t mode,
+				void **security_ctx, u32 *security_ctxlen)
+{
+	struct fuse_secctx *fsecctx;
+	void *ctx, *full_ctx;
+	u32 ctxlen, full_ctxlen;
+	int err = 0;
+
+	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
+					    &ctxlen);
+	if (err) {
+		if (err != -EOPNOTSUPP)
+			goto out_err;
+		/* No LSM is supporting this security hook. Ignore error */
+		err = 0;
+		ctxlen = 0;
+	}
+
+	if (ctxlen > 0) {
+		/*
+		 * security_dentry_init_security() does not return the name
+		 * of lsm or xattr to which label belongs. As of now only
+		 * selinux implements this. Hence, hardcoding the name to
+		 * security.selinux.
+		 */
+		char *name = "security.selinux";
+		void *ptr;
+
+		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
+		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
+		if (!full_ctx) {
+			err = -ENOMEM;
+			kfree(ctx);
+			goto out_err;
+		}
+
+		ptr = full_ctx;
+		fsecctx = (struct fuse_secctx*) ptr;
+		fsecctx->size = ctxlen;
+		ptr += sizeof(*fsecctx);
+		strcpy(ptr, name);
+		ptr += strlen(name) + 1;
+		memcpy(ptr, ctx, ctxlen);
+		kfree(ctx);
+	} else {
+		full_ctxlen = sizeof(*fsecctx);
+		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
+		if (!full_ctx) {
+			err = -ENOMEM;
+			goto out_err;
+		}
+	}
+
+	*security_ctxlen = full_ctxlen;
+	*security_ctx = full_ctx;
+out_err:
+	return err;
+}
+
 /*
  * Atomic create+open operation
  *
@@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	struct fuse_entry_out outentry;
 	struct fuse_inode *fi;
 	struct fuse_file *ff;
+	void *security_ctx = NULL;
+	u32 security_ctxlen;
 
 	/* Userspace expects S_IFREG in create mode */
 	BUG_ON((mode & S_IFMT) != S_IFREG);
@@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	args.out_args[0].value = &outentry;
 	args.out_args[1].size = sizeof(outopen);
 	args.out_args[1].value = &outopen;
+
+	if (fm->fc->init_security) {
+		err = get_security_context(entry, mode, &security_ctx,
+					   &security_ctxlen);
+		if (err)
+			goto out_put_forget_req;
+
+		args.in_numargs = 3;
+		args.in_args[2].size = security_ctxlen;
+		args.in_args[2].value = security_ctx;
+	}
+
 	err = fuse_simple_request(fm, &args);
 	if (err)
 		goto out_free_ff;
@@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 
 out_free_ff:
 	fuse_file_free(ff);
+	kfree(security_ctx);
 out_put_forget_req:
 	kfree(forget);
 out_err:
@@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
  */
 static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 			    struct inode *dir, struct dentry *entry,
-			    umode_t mode)
+			    umode_t mode, bool init_security)
 {
 	struct fuse_entry_out outarg;
 	struct inode *inode;
 	struct dentry *d;
 	int err;
 	struct fuse_forget_link *forget;
+	void *security_ctx = NULL;
+	u32 security_ctxlen = 0;
 
 	if (fuse_is_bad(dir))
 		return -EIO;
@@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
 	args->out_numargs = 1;
 	args->out_args[0].size = sizeof(outarg);
 	args->out_args[0].value = &outarg;
+
+	if (init_security) {
+		unsigned short idx = args->in_numargs;
+
+		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
+			err = -ENOMEM;
+			goto out_put_forget_req;
+		}
+
+		err = get_security_context(entry, mode, &security_ctx,
+					   &security_ctxlen);
+		if (err)
+			goto out_put_forget_req;
+
+		if (security_ctxlen > 0) {
+			args->in_args[idx].size = security_ctxlen;
+			args->in_args[idx].value = security_ctx;
+			args->in_numargs++;
+		}
+	}
+
 	err = fuse_simple_request(fm, args);
+	kfree(security_ctx);
 	if (err)
 		goto out_put_forget_req;
 
@@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
 	args.in_args[0].value = &inarg;
 	args.in_args[1].size = entry->d_name.len + 1;
 	args.in_args[1].value = entry->d_name.name;
-	return create_new_entry(fm, &args, dir, entry, mode);
+	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
 }
 
 static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
@@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 	args.in_args[0].value = &inarg;
 	args.in_args[1].size = entry->d_name.len + 1;
 	args.in_args[1].value = entry->d_name.name;
-	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
+	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
+				fm->fc->init_security);
 }
 
 static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
@@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
 	args.in_args[0].value = entry->d_name.name;
 	args.in_args[1].size = len;
 	args.in_args[1].value = link;
-	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
+	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
+				fm->fc->init_security);
 }
 
 void fuse_update_ctime(struct inode *inode)
@@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
 	args.in_args[0].value = &inarg;
 	args.in_args[1].size = newent->d_name.len + 1;
 	args.in_args[1].value = newent->d_name.name;
-	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
+	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
+			       false);
 	/* Contrary to "normal" filesystems it can happen that link
 	   makes two "logical" inodes point to the same "physical"
 	   inode.  We invalidate the attributes of the old one, so it
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 319596df5dc6..885f34f9967f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -765,6 +765,9 @@ struct fuse_conn {
 	/* Propagate syncfs() to server */
 	unsigned int sync_fs:1;
 
+	/* Initialize security xattrs when creating a new inode */
+	unsigned int init_security:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 36cd03114b6d..343bc9cfbd92 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 			}
 			if (arg->flags & FUSE_SETXATTR_EXT)
 				fc->setxattr_ext = 1;
+			if (arg->flags & FUSE_SECURITY_CTX)
+				fc->init_security = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
 		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
 		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
-		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
+		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		ia->in.flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 2fe54c80051a..777c773e143e 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -986,4 +986,15 @@ struct fuse_syncfs_in {
 	uint64_t	padding;
 };
 
+/*
+ * For each security context, send fuse_secctx with size of security context
+ * fuse_secctx will be followed by security context name and this in turn
+ * will be followed by actual context label.
+ * fuse_secctx, name, context
+ * */
+struct fuse_secctx {
+	uint32_t	size;
+	uint32_t	padding;
+};
+
 #endif /* _LINUX_FUSE_H */
-- 
2.31.1

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 12:24 PM, Vivek Goyal wrote:
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctx.
>   This contains total size of security context which follows this structure.
>
> - xattr name string.
>   This string represents name of xattr which should be used while setting
>   security context. As of now it is hardcoded to "security.selinux".

Why? It's not like "security.SMACK64' is a secret.

> - security context.
>   This is the actual security context whose size is specified in fuse_secctx
>   struct.

The possibility of multiple security contexts on a file is real
in the not too distant future. Also, a file can have multiple relevant
security attributes at creation. Smack, for example, may assign a
security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
interface cannot support either of these cases.

> This patch is modified version of patch from
> Chirantan Ekbote <chirantan@chromium.org>
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
>  fs/fuse/fuse_i.h          |   3 +
>  fs/fuse/inode.c           |   4 +-
>  include/uapi/linux/fuse.h |  11 ++++
>  4 files changed, 126 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index d9b977c0f38d..439bde1ea329 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -17,6 +17,9 @@
>  #include <linux/xattr.h>
>  #include <linux/iversion.h>
>  #include <linux/posix_acl.h>
> +#include <linux/security.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
>  
>  static void fuse_advise_use_readdirplus(struct inode *dir)
>  {
> @@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
>  	return ERR_PTR(err);
>  }
>  
> +static int get_security_context(struct dentry *entry, umode_t mode,
> +				void **security_ctx, u32 *security_ctxlen)
> +{
> +	struct fuse_secctx *fsecctx;
> +	void *ctx, *full_ctx;
> +	u32 ctxlen, full_ctxlen;
> +	int err = 0;
> +
> +	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
> +					    &ctxlen);
> +	if (err) {
> +		if (err != -EOPNOTSUPP)
> +			goto out_err;
> +		/* No LSM is supporting this security hook. Ignore error */
> +		err = 0;
> +		ctxlen = 0;
> +	}
> +
> +	if (ctxlen > 0) {
> +		/*
> +		 * security_dentry_init_security() does not return the name
> +		 * of lsm or xattr to which label belongs. As of now only
> +		 * selinux implements this. Hence, hardcoding the name to
> +		 * security.selinux.
> +		 */
> +		char *name = "security.selinux";
> +		void *ptr;
> +
> +		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> +		if (!full_ctx) {
> +			err = -ENOMEM;
> +			kfree(ctx);
> +			goto out_err;
> +		}
> +
> +		ptr = full_ctx;
> +		fsecctx = (struct fuse_secctx*) ptr;
> +		fsecctx->size = ctxlen;
> +		ptr += sizeof(*fsecctx);
> +		strcpy(ptr, name);
> +		ptr += strlen(name) + 1;
> +		memcpy(ptr, ctx, ctxlen);
> +		kfree(ctx);
> +	} else {
> +		full_ctxlen = sizeof(*fsecctx);
> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> +		if (!full_ctx) {
> +			err = -ENOMEM;
> +			goto out_err;
> +		}
> +	}
> +
> +	*security_ctxlen = full_ctxlen;
> +	*security_ctx = full_ctx;
> +out_err:
> +	return err;
> +}
> +
>  /*
>   * Atomic create+open operation
>   *
> @@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	struct fuse_entry_out outentry;
>  	struct fuse_inode *fi;
>  	struct fuse_file *ff;
> +	void *security_ctx = NULL;
> +	u32 security_ctxlen;
>  
>  	/* Userspace expects S_IFREG in create mode */
>  	BUG_ON((mode & S_IFMT) != S_IFREG);
> @@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	args.out_args[0].value = &outentry;
>  	args.out_args[1].size = sizeof(outopen);
>  	args.out_args[1].value = &outopen;
> +
> +	if (fm->fc->init_security) {
> +		err = get_security_context(entry, mode, &security_ctx,
> +					   &security_ctxlen);
> +		if (err)
> +			goto out_put_forget_req;
> +
> +		args.in_numargs = 3;
> +		args.in_args[2].size = security_ctxlen;
> +		args.in_args[2].value = security_ctx;
> +	}
> +
>  	err = fuse_simple_request(fm, &args);
>  	if (err)
>  		goto out_free_ff;
> @@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  
>  out_free_ff:
>  	fuse_file_free(ff);
> +	kfree(security_ctx);
>  out_put_forget_req:
>  	kfree(forget);
>  out_err:
> @@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>   */
>  static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>  			    struct inode *dir, struct dentry *entry,
> -			    umode_t mode)
> +			    umode_t mode, bool init_security)
>  {
>  	struct fuse_entry_out outarg;
>  	struct inode *inode;
>  	struct dentry *d;
>  	int err;
>  	struct fuse_forget_link *forget;
> +	void *security_ctx = NULL;
> +	u32 security_ctxlen = 0;
>  
>  	if (fuse_is_bad(dir))
>  		return -EIO;
> @@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>  	args->out_numargs = 1;
>  	args->out_args[0].size = sizeof(outarg);
>  	args->out_args[0].value = &outarg;
> +
> +	if (init_security) {
> +		unsigned short idx = args->in_numargs;
> +
> +		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> +			err = -ENOMEM;
> +			goto out_put_forget_req;
> +		}
> +
> +		err = get_security_context(entry, mode, &security_ctx,
> +					   &security_ctxlen);
> +		if (err)
> +			goto out_put_forget_req;
> +
> +		if (security_ctxlen > 0) {
> +			args->in_args[idx].size = security_ctxlen;
> +			args->in_args[idx].value = security_ctx;
> +			args->in_numargs++;
> +		}
> +	}
> +
>  	err = fuse_simple_request(fm, args);
> +	kfree(security_ctx);
>  	if (err)
>  		goto out_put_forget_req;
>  
> @@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>  	args.in_args[0].value = &inarg;
>  	args.in_args[1].size = entry->d_name.len + 1;
>  	args.in_args[1].value = entry->d_name.name;
> -	return create_new_entry(fm, &args, dir, entry, mode);
> +	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
>  }
>  
>  static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>  	args.in_args[0].value = &inarg;
>  	args.in_args[1].size = entry->d_name.len + 1;
>  	args.in_args[1].value = entry->d_name.name;
> -	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> +	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> +				fm->fc->init_security);
>  }
>  
>  static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>  	args.in_args[0].value = entry->d_name.name;
>  	args.in_args[1].size = len;
>  	args.in_args[1].value = link;
> -	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> +	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> +				fm->fc->init_security);
>  }
>  
>  void fuse_update_ctime(struct inode *inode)
> @@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
>  	args.in_args[0].value = &inarg;
>  	args.in_args[1].size = newent->d_name.len + 1;
>  	args.in_args[1].value = newent->d_name.name;
> -	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> +	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> +			       false);
>  	/* Contrary to "normal" filesystems it can happen that link
>  	   makes two "logical" inodes point to the same "physical"
>  	   inode.  We invalidate the attributes of the old one, so it
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 319596df5dc6..885f34f9967f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -765,6 +765,9 @@ struct fuse_conn {
>  	/* Propagate syncfs() to server */
>  	unsigned int sync_fs:1;
>  
> +	/* Initialize security xattrs when creating a new inode */
> +	unsigned int init_security:1;
> +
>  	/** The number of requests waiting for completion */
>  	atomic_t num_waiting;
>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 36cd03114b6d..343bc9cfbd92 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>  			}
>  			if (arg->flags & FUSE_SETXATTR_EXT)
>  				fc->setxattr_ext = 1;
> +			if (arg->flags & FUSE_SECURITY_CTX)
> +				fc->init_security = 1;
>  		} else {
>  			ra_pages = fc->max_read / PAGE_SIZE;
>  			fc->no_lock = 1;
> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
>  		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
>  		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
>  		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> -		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> +		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
>  #ifdef CONFIG_FUSE_DAX
>  	if (fm->fc->dax)
>  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 2fe54c80051a..777c773e143e 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -986,4 +986,15 @@ struct fuse_syncfs_in {
>  	uint64_t	padding;
>  };
>  
> +/*
> + * For each security context, send fuse_secctx with size of security context
> + * fuse_secctx will be followed by security context name and this in turn
> + * will be followed by actual context label.
> + * fuse_secctx, name, context
> + * */
> +struct fuse_secctx {
> +	uint32_t	size;
> +	uint32_t	padding;
> +};
> +
>  #endif /* _LINUX_FUSE_H */

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 12:24 PM, Vivek Goyal wrote:
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctx.
>   This contains total size of security context which follows this structure.
>
> - xattr name string.
>   This string represents name of xattr which should be used while setting
>   security context. As of now it is hardcoded to "security.selinux".

Why? It's not like "security.SMACK64' is a secret.

> - security context.
>   This is the actual security context whose size is specified in fuse_secctx
>   struct.

The possibility of multiple security contexts on a file is real
in the not too distant future. Also, a file can have multiple relevant
security attributes at creation. Smack, for example, may assign a
security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
interface cannot support either of these cases.

> This patch is modified version of patch from
> Chirantan Ekbote <chirantan@chromium.org>
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
>  fs/fuse/fuse_i.h          |   3 +
>  fs/fuse/inode.c           |   4 +-
>  include/uapi/linux/fuse.h |  11 ++++
>  4 files changed, 126 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index d9b977c0f38d..439bde1ea329 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -17,6 +17,9 @@
>  #include <linux/xattr.h>
>  #include <linux/iversion.h>
>  #include <linux/posix_acl.h>
> +#include <linux/security.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
>  
>  static void fuse_advise_use_readdirplus(struct inode *dir)
>  {
> @@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
>  	return ERR_PTR(err);
>  }
>  
> +static int get_security_context(struct dentry *entry, umode_t mode,
> +				void **security_ctx, u32 *security_ctxlen)
> +{
> +	struct fuse_secctx *fsecctx;
> +	void *ctx, *full_ctx;
> +	u32 ctxlen, full_ctxlen;
> +	int err = 0;
> +
> +	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
> +					    &ctxlen);
> +	if (err) {
> +		if (err != -EOPNOTSUPP)
> +			goto out_err;
> +		/* No LSM is supporting this security hook. Ignore error */
> +		err = 0;
> +		ctxlen = 0;
> +	}
> +
> +	if (ctxlen > 0) {
> +		/*
> +		 * security_dentry_init_security() does not return the name
> +		 * of lsm or xattr to which label belongs. As of now only
> +		 * selinux implements this. Hence, hardcoding the name to
> +		 * security.selinux.
> +		 */
> +		char *name = "security.selinux";
> +		void *ptr;
> +
> +		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> +		if (!full_ctx) {
> +			err = -ENOMEM;
> +			kfree(ctx);
> +			goto out_err;
> +		}
> +
> +		ptr = full_ctx;
> +		fsecctx = (struct fuse_secctx*) ptr;
> +		fsecctx->size = ctxlen;
> +		ptr += sizeof(*fsecctx);
> +		strcpy(ptr, name);
> +		ptr += strlen(name) + 1;
> +		memcpy(ptr, ctx, ctxlen);
> +		kfree(ctx);
> +	} else {
> +		full_ctxlen = sizeof(*fsecctx);
> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> +		if (!full_ctx) {
> +			err = -ENOMEM;
> +			goto out_err;
> +		}
> +	}
> +
> +	*security_ctxlen = full_ctxlen;
> +	*security_ctx = full_ctx;
> +out_err:
> +	return err;
> +}
> +
>  /*
>   * Atomic create+open operation
>   *
> @@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	struct fuse_entry_out outentry;
>  	struct fuse_inode *fi;
>  	struct fuse_file *ff;
> +	void *security_ctx = NULL;
> +	u32 security_ctxlen;
>  
>  	/* Userspace expects S_IFREG in create mode */
>  	BUG_ON((mode & S_IFMT) != S_IFREG);
> @@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	args.out_args[0].value = &outentry;
>  	args.out_args[1].size = sizeof(outopen);
>  	args.out_args[1].value = &outopen;
> +
> +	if (fm->fc->init_security) {
> +		err = get_security_context(entry, mode, &security_ctx,
> +					   &security_ctxlen);
> +		if (err)
> +			goto out_put_forget_req;
> +
> +		args.in_numargs = 3;
> +		args.in_args[2].size = security_ctxlen;
> +		args.in_args[2].value = security_ctx;
> +	}
> +
>  	err = fuse_simple_request(fm, &args);
>  	if (err)
>  		goto out_free_ff;
> @@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  
>  out_free_ff:
>  	fuse_file_free(ff);
> +	kfree(security_ctx);
>  out_put_forget_req:
>  	kfree(forget);
>  out_err:
> @@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>   */
>  static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>  			    struct inode *dir, struct dentry *entry,
> -			    umode_t mode)
> +			    umode_t mode, bool init_security)
>  {
>  	struct fuse_entry_out outarg;
>  	struct inode *inode;
>  	struct dentry *d;
>  	int err;
>  	struct fuse_forget_link *forget;
> +	void *security_ctx = NULL;
> +	u32 security_ctxlen = 0;
>  
>  	if (fuse_is_bad(dir))
>  		return -EIO;
> @@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>  	args->out_numargs = 1;
>  	args->out_args[0].size = sizeof(outarg);
>  	args->out_args[0].value = &outarg;
> +
> +	if (init_security) {
> +		unsigned short idx = args->in_numargs;
> +
> +		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> +			err = -ENOMEM;
> +			goto out_put_forget_req;
> +		}
> +
> +		err = get_security_context(entry, mode, &security_ctx,
> +					   &security_ctxlen);
> +		if (err)
> +			goto out_put_forget_req;
> +
> +		if (security_ctxlen > 0) {
> +			args->in_args[idx].size = security_ctxlen;
> +			args->in_args[idx].value = security_ctx;
> +			args->in_numargs++;
> +		}
> +	}
> +
>  	err = fuse_simple_request(fm, args);
> +	kfree(security_ctx);
>  	if (err)
>  		goto out_put_forget_req;
>  
> @@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>  	args.in_args[0].value = &inarg;
>  	args.in_args[1].size = entry->d_name.len + 1;
>  	args.in_args[1].value = entry->d_name.name;
> -	return create_new_entry(fm, &args, dir, entry, mode);
> +	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
>  }
>  
>  static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>  	args.in_args[0].value = &inarg;
>  	args.in_args[1].size = entry->d_name.len + 1;
>  	args.in_args[1].value = entry->d_name.name;
> -	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> +	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> +				fm->fc->init_security);
>  }
>  
>  static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>  	args.in_args[0].value = entry->d_name.name;
>  	args.in_args[1].size = len;
>  	args.in_args[1].value = link;
> -	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> +	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> +				fm->fc->init_security);
>  }
>  
>  void fuse_update_ctime(struct inode *inode)
> @@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
>  	args.in_args[0].value = &inarg;
>  	args.in_args[1].size = newent->d_name.len + 1;
>  	args.in_args[1].value = newent->d_name.name;
> -	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> +	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> +			       false);
>  	/* Contrary to "normal" filesystems it can happen that link
>  	   makes two "logical" inodes point to the same "physical"
>  	   inode.  We invalidate the attributes of the old one, so it
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 319596df5dc6..885f34f9967f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -765,6 +765,9 @@ struct fuse_conn {
>  	/* Propagate syncfs() to server */
>  	unsigned int sync_fs:1;
>  
> +	/* Initialize security xattrs when creating a new inode */
> +	unsigned int init_security:1;
> +
>  	/** The number of requests waiting for completion */
>  	atomic_t num_waiting;
>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 36cd03114b6d..343bc9cfbd92 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>  			}
>  			if (arg->flags & FUSE_SETXATTR_EXT)
>  				fc->setxattr_ext = 1;
> +			if (arg->flags & FUSE_SECURITY_CTX)
> +				fc->init_security = 1;
>  		} else {
>  			ra_pages = fc->max_read / PAGE_SIZE;
>  			fc->no_lock = 1;
> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
>  		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
>  		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
>  		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> -		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> +		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
>  #ifdef CONFIG_FUSE_DAX
>  	if (fm->fc->dax)
>  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 2fe54c80051a..777c773e143e 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -986,4 +986,15 @@ struct fuse_syncfs_in {
>  	uint64_t	padding;
>  };
>  
> +/*
> + * For each security context, send fuse_secctx with size of security context
> + * fuse_secctx will be followed by security context name and this in turn
> + * will be followed by actual context label.
> + * fuse_secctx, name, context
> + * */
> +struct fuse_secctx {
> +	uint32_t	size;
> +	uint32_t	padding;
> +};
> +
>  #endif /* _LINUX_FUSE_H */

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctx.
> >   This contains total size of security context which follows this structure.
> >
> > - xattr name string.
> >   This string represents name of xattr which should be used while setting
> >   security context. As of now it is hardcoded to "security.selinux".
> 
> Why? It's not like "security.SMACK64' is a secret.

Sorry, I don't understand what's the concern. Can you elaborate a bit
more.

I am hardcoding name to "security.selinux" because as of now only
SELinux implements this hook. And there is no way to know the name
of xattr, so I have had to hardcode it. But tomorrow if interface
changes so that name of xattr is also returned, we can easily get
rid of hardcoding.

If another LSM decides to implement this hook, then we can send
that name as well. Say "security.SMACK64".
> 
> > - security context.
> >   This is the actual security context whose size is specified in fuse_secctx
> >   struct.
> 
> The possibility of multiple security contexts on a file is real
> in the not too distant future. Also, a file can have multiple relevant
> security attributes at creation. Smack, for example, may assign a
> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
> interface cannot support either of these cases.

Right. As of now it does not support capability to support multiple
security context. But we should be easily able to extend the protocol
whenever that supports lands in kernel. Say a new option
FUSE_SECURITY_CTX_EXT which will allow sending multiple security
context labels (along with associated xattr names).

As of now there is no need to increase the complexity of current
implementation both in fuse as well as virtiofsd when kernel
does not even support multiple lables using security_dentry_init_security()
hook.

Thanks
Vivek

> 
> > This patch is modified version of patch from
> > Chirantan Ekbote <chirantan@chromium.org>
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
> >  fs/fuse/fuse_i.h          |   3 +
> >  fs/fuse/inode.c           |   4 +-
> >  include/uapi/linux/fuse.h |  11 ++++
> >  4 files changed, 126 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index d9b977c0f38d..439bde1ea329 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -17,6 +17,9 @@
> >  #include <linux/xattr.h>
> >  #include <linux/iversion.h>
> >  #include <linux/posix_acl.h>
> > +#include <linux/security.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> >  
> >  static void fuse_advise_use_readdirplus(struct inode *dir)
> >  {
> > @@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> >  	return ERR_PTR(err);
> >  }
> >  
> > +static int get_security_context(struct dentry *entry, umode_t mode,
> > +				void **security_ctx, u32 *security_ctxlen)
> > +{
> > +	struct fuse_secctx *fsecctx;
> > +	void *ctx, *full_ctx;
> > +	u32 ctxlen, full_ctxlen;
> > +	int err = 0;
> > +
> > +	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
> > +					    &ctxlen);
> > +	if (err) {
> > +		if (err != -EOPNOTSUPP)
> > +			goto out_err;
> > +		/* No LSM is supporting this security hook. Ignore error */
> > +		err = 0;
> > +		ctxlen = 0;
> > +	}
> > +
> > +	if (ctxlen > 0) {
> > +		/*
> > +		 * security_dentry_init_security() does not return the name
> > +		 * of lsm or xattr to which label belongs. As of now only
> > +		 * selinux implements this. Hence, hardcoding the name to
> > +		 * security.selinux.
> > +		 */
> > +		char *name = "security.selinux";
> > +		void *ptr;
> > +
> > +		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
> > +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > +		if (!full_ctx) {
> > +			err = -ENOMEM;
> > +			kfree(ctx);
> > +			goto out_err;
> > +		}
> > +
> > +		ptr = full_ctx;
> > +		fsecctx = (struct fuse_secctx*) ptr;
> > +		fsecctx->size = ctxlen;
> > +		ptr += sizeof(*fsecctx);
> > +		strcpy(ptr, name);
> > +		ptr += strlen(name) + 1;
> > +		memcpy(ptr, ctx, ctxlen);
> > +		kfree(ctx);
> > +	} else {
> > +		full_ctxlen = sizeof(*fsecctx);
> > +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > +		if (!full_ctx) {
> > +			err = -ENOMEM;
> > +			goto out_err;
> > +		}
> > +	}
> > +
> > +	*security_ctxlen = full_ctxlen;
> > +	*security_ctx = full_ctx;
> > +out_err:
> > +	return err;
> > +}
> > +
> >  /*
> >   * Atomic create+open operation
> >   *
> > @@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >  	struct fuse_entry_out outentry;
> >  	struct fuse_inode *fi;
> >  	struct fuse_file *ff;
> > +	void *security_ctx = NULL;
> > +	u32 security_ctxlen;
> >  
> >  	/* Userspace expects S_IFREG in create mode */
> >  	BUG_ON((mode & S_IFMT) != S_IFREG);
> > @@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >  	args.out_args[0].value = &outentry;
> >  	args.out_args[1].size = sizeof(outopen);
> >  	args.out_args[1].value = &outopen;
> > +
> > +	if (fm->fc->init_security) {
> > +		err = get_security_context(entry, mode, &security_ctx,
> > +					   &security_ctxlen);
> > +		if (err)
> > +			goto out_put_forget_req;
> > +
> > +		args.in_numargs = 3;
> > +		args.in_args[2].size = security_ctxlen;
> > +		args.in_args[2].value = security_ctx;
> > +	}
> > +
> >  	err = fuse_simple_request(fm, &args);
> >  	if (err)
> >  		goto out_free_ff;
> > @@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >  
> >  out_free_ff:
> >  	fuse_file_free(ff);
> > +	kfree(security_ctx);
> >  out_put_forget_req:
> >  	kfree(forget);
> >  out_err:
> > @@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> >   */
> >  static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> >  			    struct inode *dir, struct dentry *entry,
> > -			    umode_t mode)
> > +			    umode_t mode, bool init_security)
> >  {
> >  	struct fuse_entry_out outarg;
> >  	struct inode *inode;
> >  	struct dentry *d;
> >  	int err;
> >  	struct fuse_forget_link *forget;
> > +	void *security_ctx = NULL;
> > +	u32 security_ctxlen = 0;
> >  
> >  	if (fuse_is_bad(dir))
> >  		return -EIO;
> > @@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> >  	args->out_numargs = 1;
> >  	args->out_args[0].size = sizeof(outarg);
> >  	args->out_args[0].value = &outarg;
> > +
> > +	if (init_security) {
> > +		unsigned short idx = args->in_numargs;
> > +
> > +		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > +			err = -ENOMEM;
> > +			goto out_put_forget_req;
> > +		}
> > +
> > +		err = get_security_context(entry, mode, &security_ctx,
> > +					   &security_ctxlen);
> > +		if (err)
> > +			goto out_put_forget_req;
> > +
> > +		if (security_ctxlen > 0) {
> > +			args->in_args[idx].size = security_ctxlen;
> > +			args->in_args[idx].value = security_ctx;
> > +			args->in_numargs++;
> > +		}
> > +	}
> > +
> >  	err = fuse_simple_request(fm, args);
> > +	kfree(security_ctx);
> >  	if (err)
> >  		goto out_put_forget_req;
> >  
> > @@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> >  	args.in_args[0].value = &inarg;
> >  	args.in_args[1].size = entry->d_name.len + 1;
> >  	args.in_args[1].value = entry->d_name.name;
> > -	return create_new_entry(fm, &args, dir, entry, mode);
> > +	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> >  }
> >  
> >  static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> >  	args.in_args[0].value = &inarg;
> >  	args.in_args[1].size = entry->d_name.len + 1;
> >  	args.in_args[1].value = entry->d_name.name;
> > -	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> > +	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> > +				fm->fc->init_security);
> >  }
> >  
> >  static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> >  	args.in_args[0].value = entry->d_name.name;
> >  	args.in_args[1].size = len;
> >  	args.in_args[1].value = link;
> > -	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> > +	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> > +				fm->fc->init_security);
> >  }
> >  
> >  void fuse_update_ctime(struct inode *inode)
> > @@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> >  	args.in_args[0].value = &inarg;
> >  	args.in_args[1].size = newent->d_name.len + 1;
> >  	args.in_args[1].value = newent->d_name.name;
> > -	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> > +	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> > +			       false);
> >  	/* Contrary to "normal" filesystems it can happen that link
> >  	   makes two "logical" inodes point to the same "physical"
> >  	   inode.  We invalidate the attributes of the old one, so it
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 319596df5dc6..885f34f9967f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -765,6 +765,9 @@ struct fuse_conn {
> >  	/* Propagate syncfs() to server */
> >  	unsigned int sync_fs:1;
> >  
> > +	/* Initialize security xattrs when creating a new inode */
> > +	unsigned int init_security:1;
> > +
> >  	/** The number of requests waiting for completion */
> >  	atomic_t num_waiting;
> >  
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 36cd03114b6d..343bc9cfbd92 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> >  			}
> >  			if (arg->flags & FUSE_SETXATTR_EXT)
> >  				fc->setxattr_ext = 1;
> > +			if (arg->flags & FUSE_SECURITY_CTX)
> > +				fc->init_security = 1;
> >  		} else {
> >  			ra_pages = fc->max_read / PAGE_SIZE;
> >  			fc->no_lock = 1;
> > @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> >  		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> >  		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> >  		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> > -		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> > +		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> >  #ifdef CONFIG_FUSE_DAX
> >  	if (fm->fc->dax)
> >  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 2fe54c80051a..777c773e143e 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -986,4 +986,15 @@ struct fuse_syncfs_in {
> >  	uint64_t	padding;
> >  };
> >  
> > +/*
> > + * For each security context, send fuse_secctx with size of security context
> > + * fuse_secctx will be followed by security context name and this in turn
> > + * will be followed by actual context label.
> > + * fuse_secctx, name, context
> > + * */
> > +struct fuse_secctx {
> > +	uint32_t	size;
> > +	uint32_t	padding;
> > +};
> > +
> >  #endif /* _LINUX_FUSE_H */
> 

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctx.
> >   This contains total size of security context which follows this structure.
> >
> > - xattr name string.
> >   This string represents name of xattr which should be used while setting
> >   security context. As of now it is hardcoded to "security.selinux".
> 
> Why? It's not like "security.SMACK64' is a secret.

Sorry, I don't understand what's the concern. Can you elaborate a bit
more.

I am hardcoding name to "security.selinux" because as of now only
SELinux implements this hook. And there is no way to know the name
of xattr, so I have had to hardcode it. But tomorrow if interface
changes so that name of xattr is also returned, we can easily get
rid of hardcoding.

If another LSM decides to implement this hook, then we can send
that name as well. Say "security.SMACK64".
> 
> > - security context.
> >   This is the actual security context whose size is specified in fuse_secctx
> >   struct.
> 
> The possibility of multiple security contexts on a file is real
> in the not too distant future. Also, a file can have multiple relevant
> security attributes at creation. Smack, for example, may assign a
> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
> interface cannot support either of these cases.

Right. As of now it does not support capability to support multiple
security context. But we should be easily able to extend the protocol
whenever that supports lands in kernel. Say a new option
FUSE_SECURITY_CTX_EXT which will allow sending multiple security
context labels (along with associated xattr names).

As of now there is no need to increase the complexity of current
implementation both in fuse as well as virtiofsd when kernel
does not even support multiple lables using security_dentry_init_security()
hook.

Thanks
Vivek

> 
> > This patch is modified version of patch from
> > Chirantan Ekbote <chirantan@chromium.org>
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
> >  fs/fuse/fuse_i.h          |   3 +
> >  fs/fuse/inode.c           |   4 +-
> >  include/uapi/linux/fuse.h |  11 ++++
> >  4 files changed, 126 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index d9b977c0f38d..439bde1ea329 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -17,6 +17,9 @@
> >  #include <linux/xattr.h>
> >  #include <linux/iversion.h>
> >  #include <linux/posix_acl.h>
> > +#include <linux/security.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> >  
> >  static void fuse_advise_use_readdirplus(struct inode *dir)
> >  {
> > @@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> >  	return ERR_PTR(err);
> >  }
> >  
> > +static int get_security_context(struct dentry *entry, umode_t mode,
> > +				void **security_ctx, u32 *security_ctxlen)
> > +{
> > +	struct fuse_secctx *fsecctx;
> > +	void *ctx, *full_ctx;
> > +	u32 ctxlen, full_ctxlen;
> > +	int err = 0;
> > +
> > +	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
> > +					    &ctxlen);
> > +	if (err) {
> > +		if (err != -EOPNOTSUPP)
> > +			goto out_err;
> > +		/* No LSM is supporting this security hook. Ignore error */
> > +		err = 0;
> > +		ctxlen = 0;
> > +	}
> > +
> > +	if (ctxlen > 0) {
> > +		/*
> > +		 * security_dentry_init_security() does not return the name
> > +		 * of lsm or xattr to which label belongs. As of now only
> > +		 * selinux implements this. Hence, hardcoding the name to
> > +		 * security.selinux.
> > +		 */
> > +		char *name = "security.selinux";
> > +		void *ptr;
> > +
> > +		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
> > +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > +		if (!full_ctx) {
> > +			err = -ENOMEM;
> > +			kfree(ctx);
> > +			goto out_err;
> > +		}
> > +
> > +		ptr = full_ctx;
> > +		fsecctx = (struct fuse_secctx*) ptr;
> > +		fsecctx->size = ctxlen;
> > +		ptr += sizeof(*fsecctx);
> > +		strcpy(ptr, name);
> > +		ptr += strlen(name) + 1;
> > +		memcpy(ptr, ctx, ctxlen);
> > +		kfree(ctx);
> > +	} else {
> > +		full_ctxlen = sizeof(*fsecctx);
> > +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > +		if (!full_ctx) {
> > +			err = -ENOMEM;
> > +			goto out_err;
> > +		}
> > +	}
> > +
> > +	*security_ctxlen = full_ctxlen;
> > +	*security_ctx = full_ctx;
> > +out_err:
> > +	return err;
> > +}
> > +
> >  /*
> >   * Atomic create+open operation
> >   *
> > @@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >  	struct fuse_entry_out outentry;
> >  	struct fuse_inode *fi;
> >  	struct fuse_file *ff;
> > +	void *security_ctx = NULL;
> > +	u32 security_ctxlen;
> >  
> >  	/* Userspace expects S_IFREG in create mode */
> >  	BUG_ON((mode & S_IFMT) != S_IFREG);
> > @@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >  	args.out_args[0].value = &outentry;
> >  	args.out_args[1].size = sizeof(outopen);
> >  	args.out_args[1].value = &outopen;
> > +
> > +	if (fm->fc->init_security) {
> > +		err = get_security_context(entry, mode, &security_ctx,
> > +					   &security_ctxlen);
> > +		if (err)
> > +			goto out_put_forget_req;
> > +
> > +		args.in_numargs = 3;
> > +		args.in_args[2].size = security_ctxlen;
> > +		args.in_args[2].value = security_ctx;
> > +	}
> > +
> >  	err = fuse_simple_request(fm, &args);
> >  	if (err)
> >  		goto out_free_ff;
> > @@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >  
> >  out_free_ff:
> >  	fuse_file_free(ff);
> > +	kfree(security_ctx);
> >  out_put_forget_req:
> >  	kfree(forget);
> >  out_err:
> > @@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> >   */
> >  static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> >  			    struct inode *dir, struct dentry *entry,
> > -			    umode_t mode)
> > +			    umode_t mode, bool init_security)
> >  {
> >  	struct fuse_entry_out outarg;
> >  	struct inode *inode;
> >  	struct dentry *d;
> >  	int err;
> >  	struct fuse_forget_link *forget;
> > +	void *security_ctx = NULL;
> > +	u32 security_ctxlen = 0;
> >  
> >  	if (fuse_is_bad(dir))
> >  		return -EIO;
> > @@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> >  	args->out_numargs = 1;
> >  	args->out_args[0].size = sizeof(outarg);
> >  	args->out_args[0].value = &outarg;
> > +
> > +	if (init_security) {
> > +		unsigned short idx = args->in_numargs;
> > +
> > +		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > +			err = -ENOMEM;
> > +			goto out_put_forget_req;
> > +		}
> > +
> > +		err = get_security_context(entry, mode, &security_ctx,
> > +					   &security_ctxlen);
> > +		if (err)
> > +			goto out_put_forget_req;
> > +
> > +		if (security_ctxlen > 0) {
> > +			args->in_args[idx].size = security_ctxlen;
> > +			args->in_args[idx].value = security_ctx;
> > +			args->in_numargs++;
> > +		}
> > +	}
> > +
> >  	err = fuse_simple_request(fm, args);
> > +	kfree(security_ctx);
> >  	if (err)
> >  		goto out_put_forget_req;
> >  
> > @@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> >  	args.in_args[0].value = &inarg;
> >  	args.in_args[1].size = entry->d_name.len + 1;
> >  	args.in_args[1].value = entry->d_name.name;
> > -	return create_new_entry(fm, &args, dir, entry, mode);
> > +	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> >  }
> >  
> >  static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> >  	args.in_args[0].value = &inarg;
> >  	args.in_args[1].size = entry->d_name.len + 1;
> >  	args.in_args[1].value = entry->d_name.name;
> > -	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> > +	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> > +				fm->fc->init_security);
> >  }
> >  
> >  static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> >  	args.in_args[0].value = entry->d_name.name;
> >  	args.in_args[1].size = len;
> >  	args.in_args[1].value = link;
> > -	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> > +	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> > +				fm->fc->init_security);
> >  }
> >  
> >  void fuse_update_ctime(struct inode *inode)
> > @@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> >  	args.in_args[0].value = &inarg;
> >  	args.in_args[1].size = newent->d_name.len + 1;
> >  	args.in_args[1].value = newent->d_name.name;
> > -	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> > +	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> > +			       false);
> >  	/* Contrary to "normal" filesystems it can happen that link
> >  	   makes two "logical" inodes point to the same "physical"
> >  	   inode.  We invalidate the attributes of the old one, so it
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 319596df5dc6..885f34f9967f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -765,6 +765,9 @@ struct fuse_conn {
> >  	/* Propagate syncfs() to server */
> >  	unsigned int sync_fs:1;
> >  
> > +	/* Initialize security xattrs when creating a new inode */
> > +	unsigned int init_security:1;
> > +
> >  	/** The number of requests waiting for completion */
> >  	atomic_t num_waiting;
> >  
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 36cd03114b6d..343bc9cfbd92 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> >  			}
> >  			if (arg->flags & FUSE_SETXATTR_EXT)
> >  				fc->setxattr_ext = 1;
> > +			if (arg->flags & FUSE_SECURITY_CTX)
> > +				fc->init_security = 1;
> >  		} else {
> >  			ra_pages = fc->max_read / PAGE_SIZE;
> >  			fc->no_lock = 1;
> > @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> >  		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> >  		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> >  		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> > -		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> > +		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> >  #ifdef CONFIG_FUSE_DAX
> >  	if (fm->fc->dax)
> >  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 2fe54c80051a..777c773e143e 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -986,4 +986,15 @@ struct fuse_syncfs_in {
> >  	uint64_t	padding;
> >  };
> >  
> > +/*
> > + * For each security context, send fuse_secctx with size of security context
> > + * fuse_secctx will be followed by security context name and this in turn
> > + * will be followed by actual context label.
> > + * fuse_secctx, name, context
> > + * */
> > +struct fuse_secctx {
> > +	uint32_t	size;
> > +	uint32_t	padding;
> > +};
> > +
> >  #endif /* _LINUX_FUSE_H */
> 

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 1:18 PM, Vivek Goyal wrote:
> On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
>> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
>>> When a new inode is created, send its security context to server along
>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>> This gives server an opportunity to create new file and set security
>>> context (possibly atomically). In all the configurations it might not
>>> be possible to set context atomically.
>>>
>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>
>>> Following is the information sent to server.
>>>
>>> - struct fuse_secctx.
>>>   This contains total size of security context which follows this structure.
>>>
>>> - xattr name string.
>>>   This string represents name of xattr which should be used while setting
>>>   security context. As of now it is hardcoded to "security.selinux".
>> Why? It's not like "security.SMACK64' is a secret.
> Sorry, I don't understand what's the concern. Can you elaborate a bit
> more.

Sure. Interfaces that are designed as special case solutions for
SELinux tend to make my life miserable as the Smack maintainer and
for the efforts to complete LSM stacking. You make the change for
SELinux and leave the generalization as an exercise for some poor
sod like me to deal with later.

> I am hardcoding name to "security.selinux" because as of now only
> SELinux implements this hook.

Yes. A Smack hook implementation is on the todo list. If you hard code
this in fuse you're adding another thing that has to be done for
Smack support.

>  And there is no way to know the name
> of xattr, so I have had to hardcode it. But tomorrow if interface
> changes so that name of xattr is also returned, we can easily get
> rid of hardcoding.

So why not make the interface do that now?

> If another LSM decides to implement this hook, then we can send
> that name as well. Say "security.SMACK64".

Again, why not make it work that way now, and avoid having
to change the protocol later? Changing protocols and interfaces
is much harder than doing them generally in the first place.

>>> - security context.
>>>   This is the actual security context whose size is specified in fuse_secctx
>>>   struct.
>> The possibility of multiple security contexts on a file is real
>> in the not too distant future. Also, a file can have multiple relevant
>> security attributes at creation. Smack, for example, may assign a
>> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
>> interface cannot support either of these cases.
> Right. As of now it does not support capability to support multiple
> security context. But we should be easily able to extend the protocol
> whenever that supports lands in kernel.

No. Extending single data item protocols to support multiple
data items *hurts* most of the time. If it wasn't so much more
complicated you'd be doing it up front without fussing about it.

>  Say a new option
> FUSE_SECURITY_CTX_EXT which will allow sending multiple security
> context labels (along with associated xattr names).
>
> As of now there is no need to increase the complexity of current
> implementation both in fuse as well as virtiofsd when kernel
> does not even support multiple lables using security_dentry_init_security()
> hook.

You're 100% correct. For your purpose today there's no reason to
do anything else. It would be really handy if I didn't have yet
another thing that I don't have the time to rewrite.

>
> Thanks
> Vivek
>
>>> This patch is modified version of patch from
>>> Chirantan Ekbote <chirantan@chromium.org>
>>>
>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>> ---
>>>  fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
>>>  fs/fuse/fuse_i.h          |   3 +
>>>  fs/fuse/inode.c           |   4 +-
>>>  include/uapi/linux/fuse.h |  11 ++++
>>>  4 files changed, 126 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>> index d9b977c0f38d..439bde1ea329 100644
>>> --- a/fs/fuse/dir.c
>>> +++ b/fs/fuse/dir.c
>>> @@ -17,6 +17,9 @@
>>>  #include <linux/xattr.h>
>>>  #include <linux/iversion.h>
>>>  #include <linux/posix_acl.h>
>>> +#include <linux/security.h>
>>> +#include <linux/types.h>
>>> +#include <linux/kernel.h>
>>>  
>>>  static void fuse_advise_use_readdirplus(struct inode *dir)
>>>  {
>>> @@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
>>>  	return ERR_PTR(err);
>>>  }
>>>  
>>> +static int get_security_context(struct dentry *entry, umode_t mode,
>>> +				void **security_ctx, u32 *security_ctxlen)
>>> +{
>>> +	struct fuse_secctx *fsecctx;
>>> +	void *ctx, *full_ctx;
>>> +	u32 ctxlen, full_ctxlen;
>>> +	int err = 0;
>>> +
>>> +	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
>>> +					    &ctxlen);
>>> +	if (err) {
>>> +		if (err != -EOPNOTSUPP)
>>> +			goto out_err;
>>> +		/* No LSM is supporting this security hook. Ignore error */
>>> +		err = 0;
>>> +		ctxlen = 0;
>>> +	}
>>> +
>>> +	if (ctxlen > 0) {
>>> +		/*
>>> +		 * security_dentry_init_security() does not return the name
>>> +		 * of lsm or xattr to which label belongs. As of now only
>>> +		 * selinux implements this. Hence, hardcoding the name to
>>> +		 * security.selinux.
>>> +		 */
>>> +		char *name = "security.selinux";
>>> +		void *ptr;
>>> +
>>> +		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
>>> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> +		if (!full_ctx) {
>>> +			err = -ENOMEM;
>>> +			kfree(ctx);
>>> +			goto out_err;
>>> +		}
>>> +
>>> +		ptr = full_ctx;
>>> +		fsecctx = (struct fuse_secctx*) ptr;
>>> +		fsecctx->size = ctxlen;
>>> +		ptr += sizeof(*fsecctx);
>>> +		strcpy(ptr, name);
>>> +		ptr += strlen(name) + 1;
>>> +		memcpy(ptr, ctx, ctxlen);
>>> +		kfree(ctx);
>>> +	} else {
>>> +		full_ctxlen = sizeof(*fsecctx);
>>> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> +		if (!full_ctx) {
>>> +			err = -ENOMEM;
>>> +			goto out_err;
>>> +		}
>>> +	}
>>> +
>>> +	*security_ctxlen = full_ctxlen;
>>> +	*security_ctx = full_ctx;
>>> +out_err:
>>> +	return err;
>>> +}
>>> +
>>>  /*
>>>   * Atomic create+open operation
>>>   *
>>> @@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>  	struct fuse_entry_out outentry;
>>>  	struct fuse_inode *fi;
>>>  	struct fuse_file *ff;
>>> +	void *security_ctx = NULL;
>>> +	u32 security_ctxlen;
>>>  
>>>  	/* Userspace expects S_IFREG in create mode */
>>>  	BUG_ON((mode & S_IFMT) != S_IFREG);
>>> @@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>  	args.out_args[0].value = &outentry;
>>>  	args.out_args[1].size = sizeof(outopen);
>>>  	args.out_args[1].value = &outopen;
>>> +
>>> +	if (fm->fc->init_security) {
>>> +		err = get_security_context(entry, mode, &security_ctx,
>>> +					   &security_ctxlen);
>>> +		if (err)
>>> +			goto out_put_forget_req;
>>> +
>>> +		args.in_numargs = 3;
>>> +		args.in_args[2].size = security_ctxlen;
>>> +		args.in_args[2].value = security_ctx;
>>> +	}
>>> +
>>>  	err = fuse_simple_request(fm, &args);
>>>  	if (err)
>>>  		goto out_free_ff;
>>> @@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>  
>>>  out_free_ff:
>>>  	fuse_file_free(ff);
>>> +	kfree(security_ctx);
>>>  out_put_forget_req:
>>>  	kfree(forget);
>>>  out_err:
>>> @@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>>>   */
>>>  static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>>  			    struct inode *dir, struct dentry *entry,
>>> -			    umode_t mode)
>>> +			    umode_t mode, bool init_security)
>>>  {
>>>  	struct fuse_entry_out outarg;
>>>  	struct inode *inode;
>>>  	struct dentry *d;
>>>  	int err;
>>>  	struct fuse_forget_link *forget;
>>> +	void *security_ctx = NULL;
>>> +	u32 security_ctxlen = 0;
>>>  
>>>  	if (fuse_is_bad(dir))
>>>  		return -EIO;
>>> @@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>>  	args->out_numargs = 1;
>>>  	args->out_args[0].size = sizeof(outarg);
>>>  	args->out_args[0].value = &outarg;
>>> +
>>> +	if (init_security) {
>>> +		unsigned short idx = args->in_numargs;
>>> +
>>> +		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
>>> +			err = -ENOMEM;
>>> +			goto out_put_forget_req;
>>> +		}
>>> +
>>> +		err = get_security_context(entry, mode, &security_ctx,
>>> +					   &security_ctxlen);
>>> +		if (err)
>>> +			goto out_put_forget_req;
>>> +
>>> +		if (security_ctxlen > 0) {
>>> +			args->in_args[idx].size = security_ctxlen;
>>> +			args->in_args[idx].value = security_ctx;
>>> +			args->in_numargs++;
>>> +		}
>>> +	}
>>> +
>>>  	err = fuse_simple_request(fm, args);
>>> +	kfree(security_ctx);
>>>  	if (err)
>>>  		goto out_put_forget_req;
>>>  
>>> @@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>>>  	args.in_args[0].value = &inarg;
>>>  	args.in_args[1].size = entry->d_name.len + 1;
>>>  	args.in_args[1].value = entry->d_name.name;
>>> -	return create_new_entry(fm, &args, dir, entry, mode);
>>> +	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
>>>  }
>>>  
>>>  static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>  	args.in_args[0].value = &inarg;
>>>  	args.in_args[1].size = entry->d_name.len + 1;
>>>  	args.in_args[1].value = entry->d_name.name;
>>> -	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
>>> +	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
>>> +				fm->fc->init_security);
>>>  }
>>>  
>>>  static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>>  	args.in_args[0].value = entry->d_name.name;
>>>  	args.in_args[1].size = len;
>>>  	args.in_args[1].value = link;
>>> -	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
>>> +	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
>>> +				fm->fc->init_security);
>>>  }
>>>  
>>>  void fuse_update_ctime(struct inode *inode)
>>> @@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
>>>  	args.in_args[0].value = &inarg;
>>>  	args.in_args[1].size = newent->d_name.len + 1;
>>>  	args.in_args[1].value = newent->d_name.name;
>>> -	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
>>> +	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
>>> +			       false);
>>>  	/* Contrary to "normal" filesystems it can happen that link
>>>  	   makes two "logical" inodes point to the same "physical"
>>>  	   inode.  We invalidate the attributes of the old one, so it
>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>> index 319596df5dc6..885f34f9967f 100644
>>> --- a/fs/fuse/fuse_i.h
>>> +++ b/fs/fuse/fuse_i.h
>>> @@ -765,6 +765,9 @@ struct fuse_conn {
>>>  	/* Propagate syncfs() to server */
>>>  	unsigned int sync_fs:1;
>>>  
>>> +	/* Initialize security xattrs when creating a new inode */
>>> +	unsigned int init_security:1;
>>> +
>>>  	/** The number of requests waiting for completion */
>>>  	atomic_t num_waiting;
>>>  
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 36cd03114b6d..343bc9cfbd92 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>>>  			}
>>>  			if (arg->flags & FUSE_SETXATTR_EXT)
>>>  				fc->setxattr_ext = 1;
>>> +			if (arg->flags & FUSE_SECURITY_CTX)
>>> +				fc->init_security = 1;
>>>  		} else {
>>>  			ra_pages = fc->max_read / PAGE_SIZE;
>>>  			fc->no_lock = 1;
>>> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
>>>  		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
>>>  		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
>>>  		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
>>> -		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
>>> +		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
>>>  #ifdef CONFIG_FUSE_DAX
>>>  	if (fm->fc->dax)
>>>  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index 2fe54c80051a..777c773e143e 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -986,4 +986,15 @@ struct fuse_syncfs_in {
>>>  	uint64_t	padding;
>>>  };
>>>  
>>> +/*
>>> + * For each security context, send fuse_secctx with size of security context
>>> + * fuse_secctx will be followed by security context name and this in turn
>>> + * will be followed by actual context label.
>>> + * fuse_secctx, name, context
>>> + * */
>>> +struct fuse_secctx {
>>> +	uint32_t	size;
>>> +	uint32_t	padding;
>>> +};
>>> +
>>>  #endif /* _LINUX_FUSE_H */

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 1:18 PM, Vivek Goyal wrote:
> On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
>> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
>>> When a new inode is created, send its security context to server along
>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>> This gives server an opportunity to create new file and set security
>>> context (possibly atomically). In all the configurations it might not
>>> be possible to set context atomically.
>>>
>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>
>>> Following is the information sent to server.
>>>
>>> - struct fuse_secctx.
>>>   This contains total size of security context which follows this structure.
>>>
>>> - xattr name string.
>>>   This string represents name of xattr which should be used while setting
>>>   security context. As of now it is hardcoded to "security.selinux".
>> Why? It's not like "security.SMACK64' is a secret.
> Sorry, I don't understand what's the concern. Can you elaborate a bit
> more.

Sure. Interfaces that are designed as special case solutions for
SELinux tend to make my life miserable as the Smack maintainer and
for the efforts to complete LSM stacking. You make the change for
SELinux and leave the generalization as an exercise for some poor
sod like me to deal with later.

> I am hardcoding name to "security.selinux" because as of now only
> SELinux implements this hook.

Yes. A Smack hook implementation is on the todo list. If you hard code
this in fuse you're adding another thing that has to be done for
Smack support.

>  And there is no way to know the name
> of xattr, so I have had to hardcode it. But tomorrow if interface
> changes so that name of xattr is also returned, we can easily get
> rid of hardcoding.

So why not make the interface do that now?

> If another LSM decides to implement this hook, then we can send
> that name as well. Say "security.SMACK64".

Again, why not make it work that way now, and avoid having
to change the protocol later? Changing protocols and interfaces
is much harder than doing them generally in the first place.

>>> - security context.
>>>   This is the actual security context whose size is specified in fuse_secctx
>>>   struct.
>> The possibility of multiple security contexts on a file is real
>> in the not too distant future. Also, a file can have multiple relevant
>> security attributes at creation. Smack, for example, may assign a
>> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
>> interface cannot support either of these cases.
> Right. As of now it does not support capability to support multiple
> security context. But we should be easily able to extend the protocol
> whenever that supports lands in kernel.

No. Extending single data item protocols to support multiple
data items *hurts* most of the time. If it wasn't so much more
complicated you'd be doing it up front without fussing about it.

>  Say a new option
> FUSE_SECURITY_CTX_EXT which will allow sending multiple security
> context labels (along with associated xattr names).
>
> As of now there is no need to increase the complexity of current
> implementation both in fuse as well as virtiofsd when kernel
> does not even support multiple lables using security_dentry_init_security()
> hook.

You're 100% correct. For your purpose today there's no reason to
do anything else. It would be really handy if I didn't have yet
another thing that I don't have the time to rewrite.

>
> Thanks
> Vivek
>
>>> This patch is modified version of patch from
>>> Chirantan Ekbote <chirantan@chromium.org>
>>>
>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>> ---
>>>  fs/fuse/dir.c             | 114 ++++++++++++++++++++++++++++++++++++--
>>>  fs/fuse/fuse_i.h          |   3 +
>>>  fs/fuse/inode.c           |   4 +-
>>>  include/uapi/linux/fuse.h |  11 ++++
>>>  4 files changed, 126 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>> index d9b977c0f38d..439bde1ea329 100644
>>> --- a/fs/fuse/dir.c
>>> +++ b/fs/fuse/dir.c
>>> @@ -17,6 +17,9 @@
>>>  #include <linux/xattr.h>
>>>  #include <linux/iversion.h>
>>>  #include <linux/posix_acl.h>
>>> +#include <linux/security.h>
>>> +#include <linux/types.h>
>>> +#include <linux/kernel.h>
>>>  
>>>  static void fuse_advise_use_readdirplus(struct inode *dir)
>>>  {
>>> @@ -456,6 +459,65 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
>>>  	return ERR_PTR(err);
>>>  }
>>>  
>>> +static int get_security_context(struct dentry *entry, umode_t mode,
>>> +				void **security_ctx, u32 *security_ctxlen)
>>> +{
>>> +	struct fuse_secctx *fsecctx;
>>> +	void *ctx, *full_ctx;
>>> +	u32 ctxlen, full_ctxlen;
>>> +	int err = 0;
>>> +
>>> +	err = security_dentry_init_security(entry, mode, &entry->d_name, &ctx,
>>> +					    &ctxlen);
>>> +	if (err) {
>>> +		if (err != -EOPNOTSUPP)
>>> +			goto out_err;
>>> +		/* No LSM is supporting this security hook. Ignore error */
>>> +		err = 0;
>>> +		ctxlen = 0;
>>> +	}
>>> +
>>> +	if (ctxlen > 0) {
>>> +		/*
>>> +		 * security_dentry_init_security() does not return the name
>>> +		 * of lsm or xattr to which label belongs. As of now only
>>> +		 * selinux implements this. Hence, hardcoding the name to
>>> +		 * security.selinux.
>>> +		 */
>>> +		char *name = "security.selinux";
>>> +		void *ptr;
>>> +
>>> +		full_ctxlen = sizeof(*fsecctx) + strlen(name) + ctxlen + 1;
>>> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> +		if (!full_ctx) {
>>> +			err = -ENOMEM;
>>> +			kfree(ctx);
>>> +			goto out_err;
>>> +		}
>>> +
>>> +		ptr = full_ctx;
>>> +		fsecctx = (struct fuse_secctx*) ptr;
>>> +		fsecctx->size = ctxlen;
>>> +		ptr += sizeof(*fsecctx);
>>> +		strcpy(ptr, name);
>>> +		ptr += strlen(name) + 1;
>>> +		memcpy(ptr, ctx, ctxlen);
>>> +		kfree(ctx);
>>> +	} else {
>>> +		full_ctxlen = sizeof(*fsecctx);
>>> +		full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> +		if (!full_ctx) {
>>> +			err = -ENOMEM;
>>> +			goto out_err;
>>> +		}
>>> +	}
>>> +
>>> +	*security_ctxlen = full_ctxlen;
>>> +	*security_ctx = full_ctx;
>>> +out_err:
>>> +	return err;
>>> +}
>>> +
>>>  /*
>>>   * Atomic create+open operation
>>>   *
>>> @@ -476,6 +538,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>  	struct fuse_entry_out outentry;
>>>  	struct fuse_inode *fi;
>>>  	struct fuse_file *ff;
>>> +	void *security_ctx = NULL;
>>> +	u32 security_ctxlen;
>>>  
>>>  	/* Userspace expects S_IFREG in create mode */
>>>  	BUG_ON((mode & S_IFMT) != S_IFREG);
>>> @@ -517,6 +581,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>  	args.out_args[0].value = &outentry;
>>>  	args.out_args[1].size = sizeof(outopen);
>>>  	args.out_args[1].value = &outopen;
>>> +
>>> +	if (fm->fc->init_security) {
>>> +		err = get_security_context(entry, mode, &security_ctx,
>>> +					   &security_ctxlen);
>>> +		if (err)
>>> +			goto out_put_forget_req;
>>> +
>>> +		args.in_numargs = 3;
>>> +		args.in_args[2].size = security_ctxlen;
>>> +		args.in_args[2].value = security_ctx;
>>> +	}
>>> +
>>>  	err = fuse_simple_request(fm, &args);
>>>  	if (err)
>>>  		goto out_free_ff;
>>> @@ -554,6 +630,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>  
>>>  out_free_ff:
>>>  	fuse_file_free(ff);
>>> +	kfree(security_ctx);
>>>  out_put_forget_req:
>>>  	kfree(forget);
>>>  out_err:
>>> @@ -613,13 +690,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>>>   */
>>>  static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>>  			    struct inode *dir, struct dentry *entry,
>>> -			    umode_t mode)
>>> +			    umode_t mode, bool init_security)
>>>  {
>>>  	struct fuse_entry_out outarg;
>>>  	struct inode *inode;
>>>  	struct dentry *d;
>>>  	int err;
>>>  	struct fuse_forget_link *forget;
>>> +	void *security_ctx = NULL;
>>> +	u32 security_ctxlen = 0;
>>>  
>>>  	if (fuse_is_bad(dir))
>>>  		return -EIO;
>>> @@ -633,7 +712,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>>  	args->out_numargs = 1;
>>>  	args->out_args[0].size = sizeof(outarg);
>>>  	args->out_args[0].value = &outarg;
>>> +
>>> +	if (init_security) {
>>> +		unsigned short idx = args->in_numargs;
>>> +
>>> +		if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
>>> +			err = -ENOMEM;
>>> +			goto out_put_forget_req;
>>> +		}
>>> +
>>> +		err = get_security_context(entry, mode, &security_ctx,
>>> +					   &security_ctxlen);
>>> +		if (err)
>>> +			goto out_put_forget_req;
>>> +
>>> +		if (security_ctxlen > 0) {
>>> +			args->in_args[idx].size = security_ctxlen;
>>> +			args->in_args[idx].value = security_ctx;
>>> +			args->in_numargs++;
>>> +		}
>>> +	}
>>> +
>>>  	err = fuse_simple_request(fm, args);
>>> +	kfree(security_ctx);
>>>  	if (err)
>>>  		goto out_put_forget_req;
>>>  
>>> @@ -691,7 +792,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>>>  	args.in_args[0].value = &inarg;
>>>  	args.in_args[1].size = entry->d_name.len + 1;
>>>  	args.in_args[1].value = entry->d_name.name;
>>> -	return create_new_entry(fm, &args, dir, entry, mode);
>>> +	return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
>>>  }
>>>  
>>>  static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -719,7 +820,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>>  	args.in_args[0].value = &inarg;
>>>  	args.in_args[1].size = entry->d_name.len + 1;
>>>  	args.in_args[1].value = entry->d_name.name;
>>> -	return create_new_entry(fm, &args, dir, entry, S_IFDIR);
>>> +	return create_new_entry(fm, &args, dir, entry, S_IFDIR,
>>> +				fm->fc->init_security);
>>>  }
>>>  
>>>  static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -735,7 +837,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>>  	args.in_args[0].value = entry->d_name.name;
>>>  	args.in_args[1].size = len;
>>>  	args.in_args[1].value = link;
>>> -	return create_new_entry(fm, &args, dir, entry, S_IFLNK);
>>> +	return create_new_entry(fm, &args, dir, entry, S_IFLNK,
>>> +				fm->fc->init_security);
>>>  }
>>>  
>>>  void fuse_update_ctime(struct inode *inode)
>>> @@ -915,7 +1018,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
>>>  	args.in_args[0].value = &inarg;
>>>  	args.in_args[1].size = newent->d_name.len + 1;
>>>  	args.in_args[1].value = newent->d_name.name;
>>> -	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
>>> +	err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
>>> +			       false);
>>>  	/* Contrary to "normal" filesystems it can happen that link
>>>  	   makes two "logical" inodes point to the same "physical"
>>>  	   inode.  We invalidate the attributes of the old one, so it
>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>> index 319596df5dc6..885f34f9967f 100644
>>> --- a/fs/fuse/fuse_i.h
>>> +++ b/fs/fuse/fuse_i.h
>>> @@ -765,6 +765,9 @@ struct fuse_conn {
>>>  	/* Propagate syncfs() to server */
>>>  	unsigned int sync_fs:1;
>>>  
>>> +	/* Initialize security xattrs when creating a new inode */
>>> +	unsigned int init_security:1;
>>> +
>>>  	/** The number of requests waiting for completion */
>>>  	atomic_t num_waiting;
>>>  
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 36cd03114b6d..343bc9cfbd92 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>>>  			}
>>>  			if (arg->flags & FUSE_SETXATTR_EXT)
>>>  				fc->setxattr_ext = 1;
>>> +			if (arg->flags & FUSE_SECURITY_CTX)
>>> +				fc->init_security = 1;
>>>  		} else {
>>>  			ra_pages = fc->max_read / PAGE_SIZE;
>>>  			fc->no_lock = 1;
>>> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
>>>  		FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
>>>  		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
>>>  		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
>>> -		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
>>> +		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
>>>  #ifdef CONFIG_FUSE_DAX
>>>  	if (fm->fc->dax)
>>>  		ia->in.flags |= FUSE_MAP_ALIGNMENT;
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index 2fe54c80051a..777c773e143e 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -986,4 +986,15 @@ struct fuse_syncfs_in {
>>>  	uint64_t	padding;
>>>  };
>>>  
>>> +/*
>>> + * For each security context, send fuse_secctx with size of security context
>>> + * fuse_secctx will be followed by security context name and this in turn
>>> + * will be followed by actual context label.
>>> + * fuse_secctx, name, context
>>> + * */
>>> +struct fuse_secctx {
>>> +	uint32_t	size;
>>> +	uint32_t	padding;
>>> +};
>>> +
>>>  #endif /* _LINUX_FUSE_H */

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Fri, Sep 24, 2021 at 01:54:20PM -0700, Casey Schaufler wrote:
> On 9/24/2021 1:18 PM, Vivek Goyal wrote:
> > On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
> >> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
> >>> When a new inode is created, send its security context to server along
> >>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>> This gives server an opportunity to create new file and set security
> >>> context (possibly atomically). In all the configurations it might not
> >>> be possible to set context atomically.
> >>>
> >>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>
> >>> Following is the information sent to server.
> >>>
> >>> - struct fuse_secctx.
> >>>   This contains total size of security context which follows this structure.
> >>>
> >>> - xattr name string.
> >>>   This string represents name of xattr which should be used while setting
> >>>   security context. As of now it is hardcoded to "security.selinux".
> >> Why? It's not like "security.SMACK64' is a secret.
> > Sorry, I don't understand what's the concern. Can you elaborate a bit
> > more.
> 
> Sure. Interfaces that are designed as special case solutions for
> SELinux tend to make my life miserable as the Smack maintainer and
> for the efforts to complete LSM stacking. You make the change for
> SELinux and leave the generalization as an exercise for some poor
> sod like me to deal with later.

I am not expecting you do to fuse work. Once you add the new security
hook which can return multiple labels, I will gladly do fuse work
to send multiple labels.

> 
> > I am hardcoding name to "security.selinux" because as of now only
> > SELinux implements this hook.
> 
> Yes. A Smack hook implementation is on the todo list. If you hard code
> this in fuse you're adding another thing that has to be done for
> Smack support.
> 
> >  And there is no way to know the name
> > of xattr, so I have had to hardcode it. But tomorrow if interface
> > changes so that name of xattr is also returned, we can easily get
> > rid of hardcoding.
> 
> So why not make the interface do that now?

Because its unnecessary complexity for me. When multiple label support
is not even there, I need to write and test code to support multiple
labels when support is not even there.

> 
> > If another LSM decides to implement this hook, then we can send
> > that name as well. Say "security.SMACK64".
> 
> Again, why not make it work that way now, and avoid having
> to change the protocol later? Changing protocols and interfaces
> is much harder than doing them generally in the first place.

In case of fuse, it is not that complicated to change protocol and
add new options. Once you add support for smack and multiple labels,
I will gladly change fuse to be able to accomodate that.

> 
> >>> - security context.
> >>>   This is the actual security context whose size is specified in fuse_secctx
> >>>   struct.
> >> The possibility of multiple security contexts on a file is real
> >> in the not too distant future. Also, a file can have multiple relevant
> >> security attributes at creation. Smack, for example, may assign a
> >> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
> >> interface cannot support either of these cases.
> > Right. As of now it does not support capability to support multiple
> > security context. But we should be easily able to extend the protocol
> > whenever that supports lands in kernel.
> 
> No. Extending single data item protocols to support multiple
> data items *hurts* most of the time. If it wasn't so much more
> complicated you'd be doing it up front without fussing about it.

Its unnecessary work at this point of time. Once multiple labels
are supported, I can do this work.

I think we will need to send extra structure which tells how many
labels are going to follow. And then all the labels will follow
with same format I am using for single label.

struct fuse_secctx; xattr name string; actual label

> 
> >  Say a new option
> > FUSE_SECURITY_CTX_EXT which will allow sending multiple security
> > context labels (along with associated xattr names).
> >
> > As of now there is no need to increase the complexity of current
> > implementation both in fuse as well as virtiofsd when kernel
> > does not even support multiple lables using security_dentry_init_security()
> > hook.
> 
> You're 100% correct. For your purpose today there's no reason to
> do anything else. It would be really handy if I didn't have yet
> another thing that I don't have the time to rewrite.

I can help with adding fuse support once smack supports it. Right now
I can't even test it even if I sign up for extra complexity.

Vivek

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Fri, Sep 24, 2021 at 01:54:20PM -0700, Casey Schaufler wrote:
> On 9/24/2021 1:18 PM, Vivek Goyal wrote:
> > On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
> >> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
> >>> When a new inode is created, send its security context to server along
> >>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>> This gives server an opportunity to create new file and set security
> >>> context (possibly atomically). In all the configurations it might not
> >>> be possible to set context atomically.
> >>>
> >>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>
> >>> Following is the information sent to server.
> >>>
> >>> - struct fuse_secctx.
> >>>   This contains total size of security context which follows this structure.
> >>>
> >>> - xattr name string.
> >>>   This string represents name of xattr which should be used while setting
> >>>   security context. As of now it is hardcoded to "security.selinux".
> >> Why? It's not like "security.SMACK64' is a secret.
> > Sorry, I don't understand what's the concern. Can you elaborate a bit
> > more.
> 
> Sure. Interfaces that are designed as special case solutions for
> SELinux tend to make my life miserable as the Smack maintainer and
> for the efforts to complete LSM stacking. You make the change for
> SELinux and leave the generalization as an exercise for some poor
> sod like me to deal with later.

I am not expecting you do to fuse work. Once you add the new security
hook which can return multiple labels, I will gladly do fuse work
to send multiple labels.

> 
> > I am hardcoding name to "security.selinux" because as of now only
> > SELinux implements this hook.
> 
> Yes. A Smack hook implementation is on the todo list. If you hard code
> this in fuse you're adding another thing that has to be done for
> Smack support.
> 
> >  And there is no way to know the name
> > of xattr, so I have had to hardcode it. But tomorrow if interface
> > changes so that name of xattr is also returned, we can easily get
> > rid of hardcoding.
> 
> So why not make the interface do that now?

Because its unnecessary complexity for me. When multiple label support
is not even there, I need to write and test code to support multiple
labels when support is not even there.

> 
> > If another LSM decides to implement this hook, then we can send
> > that name as well. Say "security.SMACK64".
> 
> Again, why not make it work that way now, and avoid having
> to change the protocol later? Changing protocols and interfaces
> is much harder than doing them generally in the first place.

In case of fuse, it is not that complicated to change protocol and
add new options. Once you add support for smack and multiple labels,
I will gladly change fuse to be able to accomodate that.

> 
> >>> - security context.
> >>>   This is the actual security context whose size is specified in fuse_secctx
> >>>   struct.
> >> The possibility of multiple security contexts on a file is real
> >> in the not too distant future. Also, a file can have multiple relevant
> >> security attributes at creation. Smack, for example, may assign a
> >> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
> >> interface cannot support either of these cases.
> > Right. As of now it does not support capability to support multiple
> > security context. But we should be easily able to extend the protocol
> > whenever that supports lands in kernel.
> 
> No. Extending single data item protocols to support multiple
> data items *hurts* most of the time. If it wasn't so much more
> complicated you'd be doing it up front without fussing about it.

Its unnecessary work at this point of time. Once multiple labels
are supported, I can do this work.

I think we will need to send extra structure which tells how many
labels are going to follow. And then all the labels will follow
with same format I am using for single label.

struct fuse_secctx; xattr name string; actual label

> 
> >  Say a new option
> > FUSE_SECURITY_CTX_EXT which will allow sending multiple security
> > context labels (along with associated xattr names).
> >
> > As of now there is no need to increase the complexity of current
> > implementation both in fuse as well as virtiofsd when kernel
> > does not even support multiple lables using security_dentry_init_security()
> > hook.
> 
> You're 100% correct. For your purpose today there's no reason to
> do anything else. It would be really handy if I didn't have yet
> another thing that I don't have the time to rewrite.

I can help with adding fuse support once smack supports it. Right now
I can't even test it even if I sign up for extra complexity.

Vivek

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 2:16 PM, Vivek Goyal wrote:
> On Fri, Sep 24, 2021 at 01:54:20PM -0700, Casey Schaufler wrote:
>> On 9/24/2021 1:18 PM, Vivek Goyal wrote:
>>> On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
>>>> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
>>>>> When a new inode is created, send its security context to server along
>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>> This gives server an opportunity to create new file and set security
>>>>> context (possibly atomically). In all the configurations it might not
>>>>> be possible to set context atomically.
>>>>>
>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>
>>>>> Following is the information sent to server.
>>>>>
>>>>> - struct fuse_secctx.
>>>>>   This contains total size of security context which follows this structure.
>>>>>
>>>>> - xattr name string.
>>>>>   This string represents name of xattr which should be used while setting
>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>> Why? It's not like "security.SMACK64' is a secret.
>>> Sorry, I don't understand what's the concern. Can you elaborate a bit
>>> more.
>> Sure. Interfaces that are designed as special case solutions for
>> SELinux tend to make my life miserable as the Smack maintainer and
>> for the efforts to complete LSM stacking. You make the change for
>> SELinux and leave the generalization as an exercise for some poor
>> sod like me to deal with later.
> I am not expecting you do to fuse work. Once you add the new security
> hook which can return multiple labels, I will gladly do fuse work
> to send multiple labels.
>
>>> I am hardcoding name to "security.selinux" because as of now only
>>> SELinux implements this hook.
>> Yes. A Smack hook implementation is on the todo list. If you hard code
>> this in fuse you're adding another thing that has to be done for
>> Smack support.
>>
>>>  And there is no way to know the name
>>> of xattr, so I have had to hardcode it. But tomorrow if interface
>>> changes so that name of xattr is also returned, we can easily get
>>> rid of hardcoding.
>> So why not make the interface do that now?
> Because its unnecessary complexity for me. When multiple label support
> is not even there, I need to write and test code to support multiple
> labels when support is not even there.

Subsystems that treat labels as a special case (as opposed to supporting xattrs
properly) make me sad.


>>> If another LSM decides to implement this hook, then we can send
>>> that name as well. Say "security.SMACK64".
>> Again, why not make it work that way now, and avoid having
>> to change the protocol later? Changing protocols and interfaces
>> is much harder than doing them generally in the first place.
> In case of fuse, it is not that complicated to change protocol and
> add new options. Once you add support for smack and multiple labels,
> I will gladly change fuse to be able to accomodate that.

Cool. I'll hold you to that. Priority has been bumped up.



>>>>> - security context.
>>>>>   This is the actual security context whose size is specified in fuse_secctx
>>>>>   struct.
>>>> The possibility of multiple security contexts on a file is real
>>>> in the not too distant future. Also, a file can have multiple relevant
>>>> security attributes at creation. Smack, for example, may assign a
>>>> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
>>>> interface cannot support either of these cases.
>>> Right. As of now it does not support capability to support multiple
>>> security context. But we should be easily able to extend the protocol
>>> whenever that supports lands in kernel.
>> No. Extending single data item protocols to support multiple
>> data items *hurts* most of the time. If it wasn't so much more
>> complicated you'd be doing it up front without fussing about it.
> Its unnecessary work at this point of time. Once multiple labels
> are supported, I can do this work.
>
> I think we will need to send extra structure which tells how many
> labels are going to follow. And then all the labels will follow
> with same format I am using for single label.
>
> struct fuse_secctx; xattr name string; actual label
>
>>>  Say a new option
>>> FUSE_SECURITY_CTX_EXT which will allow sending multiple security
>>> context labels (along with associated xattr names).
>>>
>>> As of now there is no need to increase the complexity of current
>>> implementation both in fuse as well as virtiofsd when kernel
>>> does not even support multiple lables using security_dentry_init_security()
>>> hook.
>> You're 100% correct. For your purpose today there's no reason to
>> do anything else. It would be really handy if I didn't have yet
>> another thing that I don't have the time to rewrite.
> I can help with adding fuse support once smack supports it. Right now
> I can't even test it even if I sign up for extra complexity.
>
> Vivek
>

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 2:16 PM, Vivek Goyal wrote:
> On Fri, Sep 24, 2021 at 01:54:20PM -0700, Casey Schaufler wrote:
>> On 9/24/2021 1:18 PM, Vivek Goyal wrote:
>>> On Fri, Sep 24, 2021 at 12:58:28PM -0700, Casey Schaufler wrote:
>>>> On 9/24/2021 12:24 PM, Vivek Goyal wrote:
>>>>> When a new inode is created, send its security context to server along
>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>> This gives server an opportunity to create new file and set security
>>>>> context (possibly atomically). In all the configurations it might not
>>>>> be possible to set context atomically.
>>>>>
>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>
>>>>> Following is the information sent to server.
>>>>>
>>>>> - struct fuse_secctx.
>>>>>   This contains total size of security context which follows this structure.
>>>>>
>>>>> - xattr name string.
>>>>>   This string represents name of xattr which should be used while setting
>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>> Why? It's not like "security.SMACK64' is a secret.
>>> Sorry, I don't understand what's the concern. Can you elaborate a bit
>>> more.
>> Sure. Interfaces that are designed as special case solutions for
>> SELinux tend to make my life miserable as the Smack maintainer and
>> for the efforts to complete LSM stacking. You make the change for
>> SELinux and leave the generalization as an exercise for some poor
>> sod like me to deal with later.
> I am not expecting you do to fuse work. Once you add the new security
> hook which can return multiple labels, I will gladly do fuse work
> to send multiple labels.
>
>>> I am hardcoding name to "security.selinux" because as of now only
>>> SELinux implements this hook.
>> Yes. A Smack hook implementation is on the todo list. If you hard code
>> this in fuse you're adding another thing that has to be done for
>> Smack support.
>>
>>>  And there is no way to know the name
>>> of xattr, so I have had to hardcode it. But tomorrow if interface
>>> changes so that name of xattr is also returned, we can easily get
>>> rid of hardcoding.
>> So why not make the interface do that now?
> Because its unnecessary complexity for me. When multiple label support
> is not even there, I need to write and test code to support multiple
> labels when support is not even there.

Subsystems that treat labels as a special case (as opposed to supporting xattrs
properly) make me sad.


>>> If another LSM decides to implement this hook, then we can send
>>> that name as well. Say "security.SMACK64".
>> Again, why not make it work that way now, and avoid having
>> to change the protocol later? Changing protocols and interfaces
>> is much harder than doing them generally in the first place.
> In case of fuse, it is not that complicated to change protocol and
> add new options. Once you add support for smack and multiple labels,
> I will gladly change fuse to be able to accomodate that.

Cool. I'll hold you to that. Priority has been bumped up.



>>>>> - security context.
>>>>>   This is the actual security context whose size is specified in fuse_secctx
>>>>>   struct.
>>>> The possibility of multiple security contexts on a file is real
>>>> in the not too distant future. Also, a file can have multiple relevant
>>>> security attributes at creation. Smack, for example, may assign a
>>>> security.SMACK64 and a security.SMACK64TRANSMUTE attribute. Your
>>>> interface cannot support either of these cases.
>>> Right. As of now it does not support capability to support multiple
>>> security context. But we should be easily able to extend the protocol
>>> whenever that supports lands in kernel.
>> No. Extending single data item protocols to support multiple
>> data items *hurts* most of the time. If it wasn't so much more
>> complicated you'd be doing it up front without fussing about it.
> Its unnecessary work at this point of time. Once multiple labels
> are supported, I can do this work.
>
> I think we will need to send extra structure which tells how many
> labels are going to follow. And then all the labels will follow
> with same format I am using for single label.
>
> struct fuse_secctx; xattr name string; actual label
>
>>>  Say a new option
>>> FUSE_SECURITY_CTX_EXT which will allow sending multiple security
>>> context labels (along with associated xattr names).
>>>
>>> As of now there is no need to increase the complexity of current
>>> implementation both in fuse as well as virtiofsd when kernel
>>> does not even support multiple lables using security_dentry_init_security()
>>> hook.
>> You're 100% correct. For your purpose today there's no reason to
>> do anything else. It would be really handy if I didn't have yet
>> another thing that I don't have the time to rewrite.
> I can help with adding fuse support once smack supports it. Right now
> I can't even test it even if I sign up for extra complexity.
>
> Vivek
>

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Colin Walters]

On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctx.
>   This contains total size of security context which follows this structure.
>
> - xattr name string.
>   This string represents name of xattr which should be used while setting
>   security context. As of now it is hardcoded to "security.selinux".

Any reason not to just send all `security.*` xattrs found on the inode? 

(I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Colin Walters]

On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctx.
>   This contains total size of security context which follows this structure.
>
> - xattr name string.
>   This string represents name of xattr which should be used while setting
>   security context. As of now it is hardcoded to "security.selinux".

Any reason not to just send all `security.*` xattrs found on the inode? 

(I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> 
> 
> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctx.
> >   This contains total size of security context which follows this structure.
> >
> > - xattr name string.
> >   This string represents name of xattr which should be used while setting
> >   security context. As of now it is hardcoded to "security.selinux".
> 
> Any reason not to just send all `security.*` xattrs found on the inode? 
> 
> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)

So this inode is about to be created. There are no xattrs yet. And
filesystem is asking LSMs, what security labels should be set on this
inode before it is published. 

For local filesystems it is somewhat easy. They are the one creating
inode and can set all xattrs/labels before inode is added to inode
cache.

But for remote like filesystems, it is more tricky. Actual inode
creation first will happen on server and then client will instantiate
an inode based on information returned by server (Atleast that's
what fuse does).

So security_dentry_init_security() was created (I think by NFS folks)
so that they can query the label and send it along with create
request and server can take care of setting label (along with file
creation).

One limitation of security_dentry_init_security() is that it practically
supports only one label. And only SELinux has implemented. So for
all practical purposes this is a hook to obtain selinux label. NFS
and ceph already use it in that way.

Now there is a desire to be able to return more than one security
labels and support smack and possibly other LSMs. Sure, that great.
But I think for that we will have to implement a new hook which
can return multiple labels and filesystems like nfs, ceph and fuse
will have to be modified to cope with this new hook to support
multiple lables. 

And I am arguing that we can modify fuse when that hook has been
implemented. There is no point in adding that complexity in fuse
code as well all fuse-server implementations when there is nobody
generating multiple labels. We can't even test it.

Thanks
Vivek

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> 
> 
> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctx.
> >   This contains total size of security context which follows this structure.
> >
> > - xattr name string.
> >   This string represents name of xattr which should be used while setting
> >   security context. As of now it is hardcoded to "security.selinux".
> 
> Any reason not to just send all `security.*` xattrs found on the inode? 
> 
> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)

So this inode is about to be created. There are no xattrs yet. And
filesystem is asking LSMs, what security labels should be set on this
inode before it is published. 

For local filesystems it is somewhat easy. They are the one creating
inode and can set all xattrs/labels before inode is added to inode
cache.

But for remote like filesystems, it is more tricky. Actual inode
creation first will happen on server and then client will instantiate
an inode based on information returned by server (Atleast that's
what fuse does).

So security_dentry_init_security() was created (I think by NFS folks)
so that they can query the label and send it along with create
request and server can take care of setting label (along with file
creation).

One limitation of security_dentry_init_security() is that it practically
supports only one label. And only SELinux has implemented. So for
all practical purposes this is a hook to obtain selinux label. NFS
and ceph already use it in that way.

Now there is a desire to be able to return more than one security
labels and support smack and possibly other LSMs. Sure, that great.
But I think for that we will have to implement a new hook which
can return multiple labels and filesystems like nfs, ceph and fuse
will have to be modified to cope with this new hook to support
multiple lables. 

And I am arguing that we can modify fuse when that hook has been
implemented. There is no point in adding that complexity in fuse
code as well all fuse-server implementations when there is nobody
generating multiple labels. We can't even test it.

Thanks
Vivek

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>
>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>> When a new inode is created, send its security context to server along
>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>> This gives server an opportunity to create new file and set security
>>> context (possibly atomically). In all the configurations it might not
>>> be possible to set context atomically.
>>>
>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>
>>> Following is the information sent to server.
>>>
>>> - struct fuse_secctx.
>>>   This contains total size of security context which follows this structure.
>>>
>>> - xattr name string.
>>>   This string represents name of xattr which should be used while setting
>>>   security context. As of now it is hardcoded to "security.selinux".
>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>
>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> So this inode is about to be created. There are no xattrs yet. And
> filesystem is asking LSMs, what security labels should be set on this
> inode before it is published. 

No. That's imprecise. It's what SELinux does. An LSM can add any
number of attributes on inode creation, or none. These attributes
may or may not be "security labels". Assuming that they are is the
kind of thinking that leads people like Linus to conclude that the
LSM community is clueless.


>
> For local filesystems it is somewhat easy. They are the one creating
> inode and can set all xattrs/labels before inode is added to inode
> cache.
>
> But for remote like filesystems, it is more tricky. Actual inode
> creation first will happen on server and then client will instantiate
> an inode based on information returned by server (Atleast that's
> what fuse does).
>
> So security_dentry_init_security() was created (I think by NFS folks)
> so that they can query the label and send it along with create
> request and server can take care of setting label (along with file
> creation).
>
> One limitation of security_dentry_init_security() is that it practically
> supports only one label. And only SELinux has implemented. So for
> all practical purposes this is a hook to obtain selinux label. NFS
> and ceph already use it in that way.
>
> Now there is a desire to be able to return more than one security
> labels and support smack and possibly other LSMs. Sure, that great.
> But I think for that we will have to implement a new hook which
> can return multiple labels and filesystems like nfs, ceph and fuse
> will have to be modified to cope with this new hook to support
> multiple lables. 
>
> And I am arguing that we can modify fuse when that hook has been
> implemented. There is no point in adding that complexity in fuse
> code as well all fuse-server implementations when there is nobody
> generating multiple labels. We can't even test it.

There's a little bit of chicken-and-egg going on here.
There's no point in accommodating multiple labels in
this code because you can't have multiple labels. There's
no point in trying to support multiple labels because
you can't use them in virtiofs and a bunch of other
places.

>
> Thanks
> Vivek
>

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>
>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>> When a new inode is created, send its security context to server along
>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>> This gives server an opportunity to create new file and set security
>>> context (possibly atomically). In all the configurations it might not
>>> be possible to set context atomically.
>>>
>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>
>>> Following is the information sent to server.
>>>
>>> - struct fuse_secctx.
>>>   This contains total size of security context which follows this structure.
>>>
>>> - xattr name string.
>>>   This string represents name of xattr which should be used while setting
>>>   security context. As of now it is hardcoded to "security.selinux".
>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>
>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> So this inode is about to be created. There are no xattrs yet. And
> filesystem is asking LSMs, what security labels should be set on this
> inode before it is published. 

No. That's imprecise. It's what SELinux does. An LSM can add any
number of attributes on inode creation, or none. These attributes
may or may not be "security labels". Assuming that they are is the
kind of thinking that leads people like Linus to conclude that the
LSM community is clueless.


>
> For local filesystems it is somewhat easy. They are the one creating
> inode and can set all xattrs/labels before inode is added to inode
> cache.
>
> But for remote like filesystems, it is more tricky. Actual inode
> creation first will happen on server and then client will instantiate
> an inode based on information returned by server (Atleast that's
> what fuse does).
>
> So security_dentry_init_security() was created (I think by NFS folks)
> so that they can query the label and send it along with create
> request and server can take care of setting label (along with file
> creation).
>
> One limitation of security_dentry_init_security() is that it practically
> supports only one label. And only SELinux has implemented. So for
> all practical purposes this is a hook to obtain selinux label. NFS
> and ceph already use it in that way.
>
> Now there is a desire to be able to return more than one security
> labels and support smack and possibly other LSMs. Sure, that great.
> But I think for that we will have to implement a new hook which
> can return multiple labels and filesystems like nfs, ceph and fuse
> will have to be modified to cope with this new hook to support
> multiple lables. 
>
> And I am arguing that we can modify fuse when that hook has been
> implemented. There is no point in adding that complexity in fuse
> code as well all fuse-server implementations when there is nobody
> generating multiple labels. We can't even test it.

There's a little bit of chicken-and-egg going on here.
There's no point in accommodating multiple labels in
this code because you can't have multiple labels. There's
no point in trying to support multiple labels because
you can't use them in virtiofs and a bunch of other
places.

>
> Thanks
> Vivek
>

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> > On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>
> >> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>> When a new inode is created, send its security context to server along
> >>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>> This gives server an opportunity to create new file and set security
> >>> context (possibly atomically). In all the configurations it might not
> >>> be possible to set context atomically.
> >>>
> >>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>
> >>> Following is the information sent to server.
> >>>
> >>> - struct fuse_secctx.
> >>>   This contains total size of security context which follows this structure.
> >>>
> >>> - xattr name string.
> >>>   This string represents name of xattr which should be used while setting
> >>>   security context. As of now it is hardcoded to "security.selinux".
> >> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>
> >> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> > So this inode is about to be created. There are no xattrs yet. And
> > filesystem is asking LSMs, what security labels should be set on this
> > inode before it is published. 
> 
> No. That's imprecise. It's what SELinux does. An LSM can add any
> number of attributes on inode creation, or none. These attributes
> may or may not be "security labels". Assuming that they are is the
> kind of thinking that leads people like Linus to conclude that the
> LSM community is clueless.
> 
> 
> >
> > For local filesystems it is somewhat easy. They are the one creating
> > inode and can set all xattrs/labels before inode is added to inode
> > cache.
> >
> > But for remote like filesystems, it is more tricky. Actual inode
> > creation first will happen on server and then client will instantiate
> > an inode based on information returned by server (Atleast that's
> > what fuse does).
> >
> > So security_dentry_init_security() was created (I think by NFS folks)
> > so that they can query the label and send it along with create
> > request and server can take care of setting label (along with file
> > creation).
> >
> > One limitation of security_dentry_init_security() is that it practically
> > supports only one label. And only SELinux has implemented. So for
> > all practical purposes this is a hook to obtain selinux label. NFS
> > and ceph already use it in that way.
> >
> > Now there is a desire to be able to return more than one security
> > labels and support smack and possibly other LSMs. Sure, that great.
> > But I think for that we will have to implement a new hook which
> > can return multiple labels and filesystems like nfs, ceph and fuse
> > will have to be modified to cope with this new hook to support
> > multiple lables. 
> >
> > And I am arguing that we can modify fuse when that hook has been
> > implemented. There is no point in adding that complexity in fuse
> > code as well all fuse-server implementations when there is nobody
> > generating multiple labels. We can't even test it.
> 
> There's a little bit of chicken-and-egg going on here.
> There's no point in accommodating multiple labels in
> this code because you can't have multiple labels. There's
> no point in trying to support multiple labels because
> you can't use them in virtiofs and a bunch of other
> places.

Once security subsystem provides a hook to support multiple lables, then
atleast one filesystem will have to be converted to make use of this new
hook at the same time and rest of the filesystems can catch up later.

Vivek

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> > On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>
> >> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>> When a new inode is created, send its security context to server along
> >>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>> This gives server an opportunity to create new file and set security
> >>> context (possibly atomically). In all the configurations it might not
> >>> be possible to set context atomically.
> >>>
> >>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>
> >>> Following is the information sent to server.
> >>>
> >>> - struct fuse_secctx.
> >>>   This contains total size of security context which follows this structure.
> >>>
> >>> - xattr name string.
> >>>   This string represents name of xattr which should be used while setting
> >>>   security context. As of now it is hardcoded to "security.selinux".
> >> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>
> >> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> > So this inode is about to be created. There are no xattrs yet. And
> > filesystem is asking LSMs, what security labels should be set on this
> > inode before it is published. 
> 
> No. That's imprecise. It's what SELinux does. An LSM can add any
> number of attributes on inode creation, or none. These attributes
> may or may not be "security labels". Assuming that they are is the
> kind of thinking that leads people like Linus to conclude that the
> LSM community is clueless.
> 
> 
> >
> > For local filesystems it is somewhat easy. They are the one creating
> > inode and can set all xattrs/labels before inode is added to inode
> > cache.
> >
> > But for remote like filesystems, it is more tricky. Actual inode
> > creation first will happen on server and then client will instantiate
> > an inode based on information returned by server (Atleast that's
> > what fuse does).
> >
> > So security_dentry_init_security() was created (I think by NFS folks)
> > so that they can query the label and send it along with create
> > request and server can take care of setting label (along with file
> > creation).
> >
> > One limitation of security_dentry_init_security() is that it practically
> > supports only one label. And only SELinux has implemented. So for
> > all practical purposes this is a hook to obtain selinux label. NFS
> > and ceph already use it in that way.
> >
> > Now there is a desire to be able to return more than one security
> > labels and support smack and possibly other LSMs. Sure, that great.
> > But I think for that we will have to implement a new hook which
> > can return multiple labels and filesystems like nfs, ceph and fuse
> > will have to be modified to cope with this new hook to support
> > multiple lables. 
> >
> > And I am arguing that we can modify fuse when that hook has been
> > implemented. There is no point in adding that complexity in fuse
> > code as well all fuse-server implementations when there is nobody
> > generating multiple labels. We can't even test it.
> 
> There's a little bit of chicken-and-egg going on here.
> There's no point in accommodating multiple labels in
> this code because you can't have multiple labels. There's
> no point in trying to support multiple labels because
> you can't use them in virtiofs and a bunch of other
> places.

Once security subsystem provides a hook to support multiple lables, then
atleast one filesystem will have to be converted to make use of this new
hook at the same time and rest of the filesystems can catch up later.

Vivek

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>> When a new inode is created, send its security context to server along
>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>> This gives server an opportunity to create new file and set security
>>>>> context (possibly atomically). In all the configurations it might not
>>>>> be possible to set context atomically.
>>>>>
>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>
>>>>> Following is the information sent to server.
>>>>>
>>>>> - struct fuse_secctx.
>>>>>   This contains total size of security context which follows this structure.
>>>>>
>>>>> - xattr name string.
>>>>>   This string represents name of xattr which should be used while setting
>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>
>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>> So this inode is about to be created. There are no xattrs yet. And
>>> filesystem is asking LSMs, what security labels should be set on this
>>> inode before it is published. 
>> No. That's imprecise. It's what SELinux does. An LSM can add any
>> number of attributes on inode creation, or none. These attributes
>> may or may not be "security labels". Assuming that they are is the
>> kind of thinking that leads people like Linus to conclude that the
>> LSM community is clueless.
>>
>>
>>> For local filesystems it is somewhat easy. They are the one creating
>>> inode and can set all xattrs/labels before inode is added to inode
>>> cache.
>>>
>>> But for remote like filesystems, it is more tricky. Actual inode
>>> creation first will happen on server and then client will instantiate
>>> an inode based on information returned by server (Atleast that's
>>> what fuse does).
>>>
>>> So security_dentry_init_security() was created (I think by NFS folks)
>>> so that they can query the label and send it along with create
>>> request and server can take care of setting label (along with file
>>> creation).
>>>
>>> One limitation of security_dentry_init_security() is that it practically
>>> supports only one label. And only SELinux has implemented. So for
>>> all practical purposes this is a hook to obtain selinux label. NFS
>>> and ceph already use it in that way.
>>>
>>> Now there is a desire to be able to return more than one security
>>> labels and support smack and possibly other LSMs. Sure, that great.
>>> But I think for that we will have to implement a new hook which
>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>> will have to be modified to cope with this new hook to support
>>> multiple lables. 
>>>
>>> And I am arguing that we can modify fuse when that hook has been
>>> implemented. There is no point in adding that complexity in fuse
>>> code as well all fuse-server implementations when there is nobody
>>> generating multiple labels. We can't even test it.
>> There's a little bit of chicken-and-egg going on here.
>> There's no point in accommodating multiple labels in
>> this code because you can't have multiple labels. There's
>> no point in trying to support multiple labels because
>> you can't use them in virtiofs and a bunch of other
>> places.
> Once security subsystem provides a hook to support multiple lables, then
> atleast one filesystem will have to be converted to make use of this new
> hook at the same time and rest of the filesystems can catch up later.

Clearly you haven't been following the work I've been doing on
module stacking. That's completely understandable. There aren't
new hooks being added, or at least haven't been yet. Some of the
existing hooks are getting changed to provide the data required
for multiple security modules (e.g. secids become a set of secids).
Filesystems that support xattrs properly are unaffected because,
for all it's shortcomings, the LSM layer hides the details of
the security modules sufficiently. 

Which filesystem are you saying will have to "be converted"?
NFS is going to require some work, but that's because it was
done as a special case for "MAC labels". The NFS support for
security.* xattrs came much later. This is one of the reasons
why I'm concerned about the virtiofs implementation you're
proposing. We were never able to get the NFS "MAC label"
implementation to work properly with Smack, even though there is
no obvious reason it wouldn't.


>
> Vivek
>

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>> When a new inode is created, send its security context to server along
>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>> This gives server an opportunity to create new file and set security
>>>>> context (possibly atomically). In all the configurations it might not
>>>>> be possible to set context atomically.
>>>>>
>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>
>>>>> Following is the information sent to server.
>>>>>
>>>>> - struct fuse_secctx.
>>>>>   This contains total size of security context which follows this structure.
>>>>>
>>>>> - xattr name string.
>>>>>   This string represents name of xattr which should be used while setting
>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>
>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>> So this inode is about to be created. There are no xattrs yet. And
>>> filesystem is asking LSMs, what security labels should be set on this
>>> inode before it is published. 
>> No. That's imprecise. It's what SELinux does. An LSM can add any
>> number of attributes on inode creation, or none. These attributes
>> may or may not be "security labels". Assuming that they are is the
>> kind of thinking that leads people like Linus to conclude that the
>> LSM community is clueless.
>>
>>
>>> For local filesystems it is somewhat easy. They are the one creating
>>> inode and can set all xattrs/labels before inode is added to inode
>>> cache.
>>>
>>> But for remote like filesystems, it is more tricky. Actual inode
>>> creation first will happen on server and then client will instantiate
>>> an inode based on information returned by server (Atleast that's
>>> what fuse does).
>>>
>>> So security_dentry_init_security() was created (I think by NFS folks)
>>> so that they can query the label and send it along with create
>>> request and server can take care of setting label (along with file
>>> creation).
>>>
>>> One limitation of security_dentry_init_security() is that it practically
>>> supports only one label. And only SELinux has implemented. So for
>>> all practical purposes this is a hook to obtain selinux label. NFS
>>> and ceph already use it in that way.
>>>
>>> Now there is a desire to be able to return more than one security
>>> labels and support smack and possibly other LSMs. Sure, that great.
>>> But I think for that we will have to implement a new hook which
>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>> will have to be modified to cope with this new hook to support
>>> multiple lables. 
>>>
>>> And I am arguing that we can modify fuse when that hook has been
>>> implemented. There is no point in adding that complexity in fuse
>>> code as well all fuse-server implementations when there is nobody
>>> generating multiple labels. We can't even test it.
>> There's a little bit of chicken-and-egg going on here.
>> There's no point in accommodating multiple labels in
>> this code because you can't have multiple labels. There's
>> no point in trying to support multiple labels because
>> you can't use them in virtiofs and a bunch of other
>> places.
> Once security subsystem provides a hook to support multiple lables, then
> atleast one filesystem will have to be converted to make use of this new
> hook at the same time and rest of the filesystems can catch up later.

Clearly you haven't been following the work I've been doing on
module stacking. That's completely understandable. There aren't
new hooks being added, or at least haven't been yet. Some of the
existing hooks are getting changed to provide the data required
for multiple security modules (e.g. secids become a set of secids).
Filesystems that support xattrs properly are unaffected because,
for all it's shortcomings, the LSM layer hides the details of
the security modules sufficiently. 

Which filesystem are you saying will have to "be converted"?
NFS is going to require some work, but that's because it was
done as a special case for "MAC labels". The NFS support for
security.* xattrs came much later. This is one of the reasons
why I'm concerned about the virtiofs implementation you're
proposing. We were never able to get the NFS "MAC label"
implementation to work properly with Smack, even though there is
no obvious reason it wouldn't.


>
> Vivek
>

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> > On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> >> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> >>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>>>> When a new inode is created, send its security context to server along
> >>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>>>> This gives server an opportunity to create new file and set security
> >>>>> context (possibly atomically). In all the configurations it might not
> >>>>> be possible to set context atomically.
> >>>>>
> >>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>>>
> >>>>> Following is the information sent to server.
> >>>>>
> >>>>> - struct fuse_secctx.
> >>>>>   This contains total size of security context which follows this structure.
> >>>>>
> >>>>> - xattr name string.
> >>>>>   This string represents name of xattr which should be used while setting
> >>>>>   security context. As of now it is hardcoded to "security.selinux".
> >>>> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>>>
> >>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> >>> So this inode is about to be created. There are no xattrs yet. And
> >>> filesystem is asking LSMs, what security labels should be set on this
> >>> inode before it is published. 
> >> No. That's imprecise. It's what SELinux does. An LSM can add any
> >> number of attributes on inode creation, or none. These attributes
> >> may or may not be "security labels". Assuming that they are is the
> >> kind of thinking that leads people like Linus to conclude that the
> >> LSM community is clueless.
> >>
> >>
> >>> For local filesystems it is somewhat easy. They are the one creating
> >>> inode and can set all xattrs/labels before inode is added to inode
> >>> cache.
> >>>
> >>> But for remote like filesystems, it is more tricky. Actual inode
> >>> creation first will happen on server and then client will instantiate
> >>> an inode based on information returned by server (Atleast that's
> >>> what fuse does).
> >>>
> >>> So security_dentry_init_security() was created (I think by NFS folks)
> >>> so that they can query the label and send it along with create
> >>> request and server can take care of setting label (along with file
> >>> creation).
> >>>
> >>> One limitation of security_dentry_init_security() is that it practically
> >>> supports only one label. And only SELinux has implemented. So for
> >>> all practical purposes this is a hook to obtain selinux label. NFS
> >>> and ceph already use it in that way.
> >>>
> >>> Now there is a desire to be able to return more than one security
> >>> labels and support smack and possibly other LSMs. Sure, that great.
> >>> But I think for that we will have to implement a new hook which
> >>> can return multiple labels and filesystems like nfs, ceph and fuse
> >>> will have to be modified to cope with this new hook to support
> >>> multiple lables. 
> >>>
> >>> And I am arguing that we can modify fuse when that hook has been
> >>> implemented. There is no point in adding that complexity in fuse
> >>> code as well all fuse-server implementations when there is nobody
> >>> generating multiple labels. We can't even test it.
> >> There's a little bit of chicken-and-egg going on here.
> >> There's no point in accommodating multiple labels in
> >> this code because you can't have multiple labels. There's
> >> no point in trying to support multiple labels because
> >> you can't use them in virtiofs and a bunch of other
> >> places.
> > Once security subsystem provides a hook to support multiple lables, then
> > atleast one filesystem will have to be converted to make use of this new
> > hook at the same time and rest of the filesystems can catch up later.
> 
> Clearly you haven't been following the work I've been doing on
> module stacking. That's completely understandable. There aren't
> new hooks being added, or at least haven't been yet. Some of the
> existing hooks are getting changed to provide the data required
> for multiple security modules (e.g. secids become a set of secids).
> Filesystems that support xattrs properly are unaffected because,
> for all it's shortcomings, the LSM layer hides the details of
> the security modules sufficiently. 
> 
> Which filesystem are you saying will have to "be converted"?

When I grep for "security_dentry_init_security()" in current code,
I see two users, ceph and nfs.

fs/ceph/xattr.c
ceph_security_init_secctx()

fs/nfs/nfs4proc.c
nfs4_label_init_security()

So looks like these two file systems will have to be converted
(along with fuse).

Vivek

> NFS is going to require some work, but that's because it was
> done as a special case for "MAC labels". The NFS support for
> security.* xattrs came much later. This is one of the reasons
> why I'm concerned about the virtiofs implementation you're
> proposing. We were never able to get the NFS "MAC label"
> implementation to work properly with Smack, even though there is
> no obvious reason it wouldn't.
> 
> 
> >
> > Vivek
> >
> 

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> > On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> >> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> >>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>>>> When a new inode is created, send its security context to server along
> >>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>>>> This gives server an opportunity to create new file and set security
> >>>>> context (possibly atomically). In all the configurations it might not
> >>>>> be possible to set context atomically.
> >>>>>
> >>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>>>
> >>>>> Following is the information sent to server.
> >>>>>
> >>>>> - struct fuse_secctx.
> >>>>>   This contains total size of security context which follows this structure.
> >>>>>
> >>>>> - xattr name string.
> >>>>>   This string represents name of xattr which should be used while setting
> >>>>>   security context. As of now it is hardcoded to "security.selinux".
> >>>> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>>>
> >>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> >>> So this inode is about to be created. There are no xattrs yet. And
> >>> filesystem is asking LSMs, what security labels should be set on this
> >>> inode before it is published. 
> >> No. That's imprecise. It's what SELinux does. An LSM can add any
> >> number of attributes on inode creation, or none. These attributes
> >> may or may not be "security labels". Assuming that they are is the
> >> kind of thinking that leads people like Linus to conclude that the
> >> LSM community is clueless.
> >>
> >>
> >>> For local filesystems it is somewhat easy. They are the one creating
> >>> inode and can set all xattrs/labels before inode is added to inode
> >>> cache.
> >>>
> >>> But for remote like filesystems, it is more tricky. Actual inode
> >>> creation first will happen on server and then client will instantiate
> >>> an inode based on information returned by server (Atleast that's
> >>> what fuse does).
> >>>
> >>> So security_dentry_init_security() was created (I think by NFS folks)
> >>> so that they can query the label and send it along with create
> >>> request and server can take care of setting label (along with file
> >>> creation).
> >>>
> >>> One limitation of security_dentry_init_security() is that it practically
> >>> supports only one label. And only SELinux has implemented. So for
> >>> all practical purposes this is a hook to obtain selinux label. NFS
> >>> and ceph already use it in that way.
> >>>
> >>> Now there is a desire to be able to return more than one security
> >>> labels and support smack and possibly other LSMs. Sure, that great.
> >>> But I think for that we will have to implement a new hook which
> >>> can return multiple labels and filesystems like nfs, ceph and fuse
> >>> will have to be modified to cope with this new hook to support
> >>> multiple lables. 
> >>>
> >>> And I am arguing that we can modify fuse when that hook has been
> >>> implemented. There is no point in adding that complexity in fuse
> >>> code as well all fuse-server implementations when there is nobody
> >>> generating multiple labels. We can't even test it.
> >> There's a little bit of chicken-and-egg going on here.
> >> There's no point in accommodating multiple labels in
> >> this code because you can't have multiple labels. There's
> >> no point in trying to support multiple labels because
> >> you can't use them in virtiofs and a bunch of other
> >> places.
> > Once security subsystem provides a hook to support multiple lables, then
> > atleast one filesystem will have to be converted to make use of this new
> > hook at the same time and rest of the filesystems can catch up later.
> 
> Clearly you haven't been following the work I've been doing on
> module stacking. That's completely understandable. There aren't
> new hooks being added, or at least haven't been yet. Some of the
> existing hooks are getting changed to provide the data required
> for multiple security modules (e.g. secids become a set of secids).
> Filesystems that support xattrs properly are unaffected because,
> for all it's shortcomings, the LSM layer hides the details of
> the security modules sufficiently. 
> 
> Which filesystem are you saying will have to "be converted"?

When I grep for "security_dentry_init_security()" in current code,
I see two users, ceph and nfs.

fs/ceph/xattr.c
ceph_security_init_secctx()

fs/nfs/nfs4proc.c
nfs4_label_init_security()

So looks like these two file systems will have to be converted
(along with fuse).

Vivek

> NFS is going to require some work, but that's because it was
> done as a special case for "MAC labels". The NFS support for
> security.* xattrs came much later. This is one of the reasons
> why I'm concerned about the virtiofs implementation you're
> proposing. We were never able to get the NFS "MAC label"
> implementation to work properly with Smack, even though there is
> no obvious reason it wouldn't.
> 
> 
> >
> > Vivek
> >
> 

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 8:56 AM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>>>> When a new inode is created, send its security context to server along
>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>>>> This gives server an opportunity to create new file and set security
>>>>>>> context (possibly atomically). In all the configurations it might not
>>>>>>> be possible to set context atomically.
>>>>>>>
>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>>>
>>>>>>> Following is the information sent to server.
>>>>>>>
>>>>>>> - struct fuse_secctx.
>>>>>>>   This contains total size of security context which follows this structure.
>>>>>>>
>>>>>>> - xattr name string.
>>>>>>>   This string represents name of xattr which should be used while setting
>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>>>
>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>>>> So this inode is about to be created. There are no xattrs yet. And
>>>>> filesystem is asking LSMs, what security labels should be set on this
>>>>> inode before it is published. 
>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
>>>> number of attributes on inode creation, or none. These attributes
>>>> may or may not be "security labels". Assuming that they are is the
>>>> kind of thinking that leads people like Linus to conclude that the
>>>> LSM community is clueless.
>>>>
>>>>
>>>>> For local filesystems it is somewhat easy. They are the one creating
>>>>> inode and can set all xattrs/labels before inode is added to inode
>>>>> cache.
>>>>>
>>>>> But for remote like filesystems, it is more tricky. Actual inode
>>>>> creation first will happen on server and then client will instantiate
>>>>> an inode based on information returned by server (Atleast that's
>>>>> what fuse does).
>>>>>
>>>>> So security_dentry_init_security() was created (I think by NFS folks)
>>>>> so that they can query the label and send it along with create
>>>>> request and server can take care of setting label (along with file
>>>>> creation).
>>>>>
>>>>> One limitation of security_dentry_init_security() is that it practically
>>>>> supports only one label. And only SELinux has implemented. So for
>>>>> all practical purposes this is a hook to obtain selinux label. NFS
>>>>> and ceph already use it in that way.
>>>>>
>>>>> Now there is a desire to be able to return more than one security
>>>>> labels and support smack and possibly other LSMs. Sure, that great.
>>>>> But I think for that we will have to implement a new hook which
>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>>>> will have to be modified to cope with this new hook to support
>>>>> multiple lables. 
>>>>>
>>>>> And I am arguing that we can modify fuse when that hook has been
>>>>> implemented. There is no point in adding that complexity in fuse
>>>>> code as well all fuse-server implementations when there is nobody
>>>>> generating multiple labels. We can't even test it.
>>>> There's a little bit of chicken-and-egg going on here.
>>>> There's no point in accommodating multiple labels in
>>>> this code because you can't have multiple labels. There's
>>>> no point in trying to support multiple labels because
>>>> you can't use them in virtiofs and a bunch of other
>>>> places.
>>> Once security subsystem provides a hook to support multiple lables, then
>>> atleast one filesystem will have to be converted to make use of this new
>>> hook at the same time and rest of the filesystems can catch up later.
>> Clearly you haven't been following the work I've been doing on
>> module stacking. That's completely understandable. There aren't
>> new hooks being added, or at least haven't been yet. Some of the
>> existing hooks are getting changed to provide the data required
>> for multiple security modules (e.g. secids become a set of secids).
>> Filesystems that support xattrs properly are unaffected because,
>> for all it's shortcomings, the LSM layer hides the details of
>> the security modules sufficiently. 
>>
>> Which filesystem are you saying will have to "be converted"?
> When I grep for "security_dentry_init_security()" in current code,
> I see two users, ceph and nfs.

Neither of which support xattrs fully. Ceph can support them properly,
but does not by default. NFS is ancient and we've talked about it at
length. Also, the fact that they use security_dentry_init_security()
is a red herring. Really, this has no bearing on the issue of fuse.

>
> fs/ceph/xattr.c
> ceph_security_init_secctx()
>
> fs/nfs/nfs4proc.c
> nfs4_label_init_security()
>
> So looks like these two file systems will have to be converted
> (along with fuse).
>
> Vivek
>
>> NFS is going to require some work, but that's because it was
>> done as a special case for "MAC labels". The NFS support for
>> security.* xattrs came much later. This is one of the reasons
>> why I'm concerned about the virtiofs implementation you're
>> proposing. We were never able to get the NFS "MAC label"
>> implementation to work properly with Smack, even though there is
>> no obvious reason it wouldn't.
>>
>>
>>> Vivek
>>>

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 8:56 AM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>>>> When a new inode is created, send its security context to server along
>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>>>> This gives server an opportunity to create new file and set security
>>>>>>> context (possibly atomically). In all the configurations it might not
>>>>>>> be possible to set context atomically.
>>>>>>>
>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>>>
>>>>>>> Following is the information sent to server.
>>>>>>>
>>>>>>> - struct fuse_secctx.
>>>>>>>   This contains total size of security context which follows this structure.
>>>>>>>
>>>>>>> - xattr name string.
>>>>>>>   This string represents name of xattr which should be used while setting
>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>>>
>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>>>> So this inode is about to be created. There are no xattrs yet. And
>>>>> filesystem is asking LSMs, what security labels should be set on this
>>>>> inode before it is published. 
>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
>>>> number of attributes on inode creation, or none. These attributes
>>>> may or may not be "security labels". Assuming that they are is the
>>>> kind of thinking that leads people like Linus to conclude that the
>>>> LSM community is clueless.
>>>>
>>>>
>>>>> For local filesystems it is somewhat easy. They are the one creating
>>>>> inode and can set all xattrs/labels before inode is added to inode
>>>>> cache.
>>>>>
>>>>> But for remote like filesystems, it is more tricky. Actual inode
>>>>> creation first will happen on server and then client will instantiate
>>>>> an inode based on information returned by server (Atleast that's
>>>>> what fuse does).
>>>>>
>>>>> So security_dentry_init_security() was created (I think by NFS folks)
>>>>> so that they can query the label and send it along with create
>>>>> request and server can take care of setting label (along with file
>>>>> creation).
>>>>>
>>>>> One limitation of security_dentry_init_security() is that it practically
>>>>> supports only one label. And only SELinux has implemented. So for
>>>>> all practical purposes this is a hook to obtain selinux label. NFS
>>>>> and ceph already use it in that way.
>>>>>
>>>>> Now there is a desire to be able to return more than one security
>>>>> labels and support smack and possibly other LSMs. Sure, that great.
>>>>> But I think for that we will have to implement a new hook which
>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>>>> will have to be modified to cope with this new hook to support
>>>>> multiple lables. 
>>>>>
>>>>> And I am arguing that we can modify fuse when that hook has been
>>>>> implemented. There is no point in adding that complexity in fuse
>>>>> code as well all fuse-server implementations when there is nobody
>>>>> generating multiple labels. We can't even test it.
>>>> There's a little bit of chicken-and-egg going on here.
>>>> There's no point in accommodating multiple labels in
>>>> this code because you can't have multiple labels. There's
>>>> no point in trying to support multiple labels because
>>>> you can't use them in virtiofs and a bunch of other
>>>> places.
>>> Once security subsystem provides a hook to support multiple lables, then
>>> atleast one filesystem will have to be converted to make use of this new
>>> hook at the same time and rest of the filesystems can catch up later.
>> Clearly you haven't been following the work I've been doing on
>> module stacking. That's completely understandable. There aren't
>> new hooks being added, or at least haven't been yet. Some of the
>> existing hooks are getting changed to provide the data required
>> for multiple security modules (e.g. secids become a set of secids).
>> Filesystems that support xattrs properly are unaffected because,
>> for all it's shortcomings, the LSM layer hides the details of
>> the security modules sufficiently. 
>>
>> Which filesystem are you saying will have to "be converted"?
> When I grep for "security_dentry_init_security()" in current code,
> I see two users, ceph and nfs.

Neither of which support xattrs fully. Ceph can support them properly,
but does not by default. NFS is ancient and we've talked about it at
length. Also, the fact that they use security_dentry_init_security()
is a red herring. Really, this has no bearing on the issue of fuse.

>
> fs/ceph/xattr.c
> ceph_security_init_secctx()
>
> fs/nfs/nfs4proc.c
> nfs4_label_init_security()
>
> So looks like these two file systems will have to be converted
> (along with fuse).
>
> Vivek
>
>> NFS is going to require some work, but that's because it was
>> done as a special case for "MAC labels". The NFS support for
>> security.* xattrs came much later. This is one of the reasons
>> why I'm concerned about the virtiofs implementation you're
>> proposing. We were never able to get the NFS "MAC label"
>> implementation to work properly with Smack, even though there is
>> no obvious reason it wouldn't.
>>
>>
>>> Vivek
>>>

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
> > On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
> >> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> >>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> >>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> >>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>>>>>> When a new inode is created, send its security context to server along
> >>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>>>>>> This gives server an opportunity to create new file and set security
> >>>>>>> context (possibly atomically). In all the configurations it might not
> >>>>>>> be possible to set context atomically.
> >>>>>>>
> >>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>>>>>
> >>>>>>> Following is the information sent to server.
> >>>>>>>
> >>>>>>> - struct fuse_secctx.
> >>>>>>>   This contains total size of security context which follows this structure.
> >>>>>>>
> >>>>>>> - xattr name string.
> >>>>>>>   This string represents name of xattr which should be used while setting
> >>>>>>>   security context. As of now it is hardcoded to "security.selinux".
> >>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>>>>>
> >>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> >>>>> So this inode is about to be created. There are no xattrs yet. And
> >>>>> filesystem is asking LSMs, what security labels should be set on this
> >>>>> inode before it is published. 
> >>>> No. That's imprecise. It's what SELinux does. An LSM can add any
> >>>> number of attributes on inode creation, or none. These attributes
> >>>> may or may not be "security labels". Assuming that they are is the
> >>>> kind of thinking that leads people like Linus to conclude that the
> >>>> LSM community is clueless.
> >>>>
> >>>>
> >>>>> For local filesystems it is somewhat easy. They are the one creating
> >>>>> inode and can set all xattrs/labels before inode is added to inode
> >>>>> cache.
> >>>>>
> >>>>> But for remote like filesystems, it is more tricky. Actual inode
> >>>>> creation first will happen on server and then client will instantiate
> >>>>> an inode based on information returned by server (Atleast that's
> >>>>> what fuse does).
> >>>>>
> >>>>> So security_dentry_init_security() was created (I think by NFS folks)
> >>>>> so that they can query the label and send it along with create
> >>>>> request and server can take care of setting label (along with file
> >>>>> creation).
> >>>>>
> >>>>> One limitation of security_dentry_init_security() is that it practically
> >>>>> supports only one label. And only SELinux has implemented. So for
> >>>>> all practical purposes this is a hook to obtain selinux label. NFS
> >>>>> and ceph already use it in that way.
> >>>>>
> >>>>> Now there is a desire to be able to return more than one security
> >>>>> labels and support smack and possibly other LSMs. Sure, that great.
> >>>>> But I think for that we will have to implement a new hook which
> >>>>> can return multiple labels and filesystems like nfs, ceph and fuse
> >>>>> will have to be modified to cope with this new hook to support
> >>>>> multiple lables. 
> >>>>>
> >>>>> And I am arguing that we can modify fuse when that hook has been
> >>>>> implemented. There is no point in adding that complexity in fuse
> >>>>> code as well all fuse-server implementations when there is nobody
> >>>>> generating multiple labels. We can't even test it.
> >>>> There's a little bit of chicken-and-egg going on here.
> >>>> There's no point in accommodating multiple labels in
> >>>> this code because you can't have multiple labels. There's
> >>>> no point in trying to support multiple labels because
> >>>> you can't use them in virtiofs and a bunch of other
> >>>> places.
> >>> Once security subsystem provides a hook to support multiple lables, then
> >>> atleast one filesystem will have to be converted to make use of this new
> >>> hook at the same time and rest of the filesystems can catch up later.
> >> Clearly you haven't been following the work I've been doing on
> >> module stacking. That's completely understandable. There aren't
> >> new hooks being added, or at least haven't been yet. Some of the
> >> existing hooks are getting changed to provide the data required
> >> for multiple security modules (e.g. secids become a set of secids).
> >> Filesystems that support xattrs properly are unaffected because,
> >> for all it's shortcomings, the LSM layer hides the details of
> >> the security modules sufficiently. 
> >>
> >> Which filesystem are you saying will have to "be converted"?
> > When I grep for "security_dentry_init_security()" in current code,
> > I see two users, ceph and nfs.
> 
> Neither of which support xattrs fully. Ceph can support them properly,
> but does not by default. NFS is ancient and we've talked about it at
> length. Also, the fact that they use security_dentry_init_security()
> is a red herring. Really, this has no bearing on the issue of fuse.

Frankly speaking, I am now beginning to lose what's being asked for,
w.r.t this patch.

I see that NFS and ceph are supporting single security label at
the time of file creation and I added support for the same for
fuse.

You seem to want to have capability to send multiple "name,value,len"
tuples so that you can support multiple xattrs/labels down the
line.

Even if I do that, I am not sure what to do with those xattrs at
the other end. I am using /proc/thread-self/attr/fscreate to
set the security attribute of file.

https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html

How will this work with multiple labels. I think you will have to
extend fscreate or create new interface to be able to deal with it.

That's why I think that it seems premature that fuse interface be
written to deal with multiple labels when rest of the infrastructure
is not ready. It should be other way, instead. First rest of the
infrastructure should be written and then all the users make use
of new infra.

BTW, I am checking security_inode_init_security(). That seems to
return max 2 xattrs as of now?

#define MAX_LSM_EVM_XATTR       2
struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];

So we are allocating space for 3 xattrs. Last xattr is Null to signify
end of array. So, we seem to use on xattr for LSM and another for EVM.
Do I understand it correctly. Does that mean that smack stuff does
not work even with security_inode_init_security(). Or there is something
else going on.

Vivek

> 
> >
> > fs/ceph/xattr.c
> > ceph_security_init_secctx()
> >
> > fs/nfs/nfs4proc.c
> > nfs4_label_init_security()
> >
> > So looks like these two file systems will have to be converted
> > (along with fuse).
> >
> > Vivek
> >
> >> NFS is going to require some work, but that's because it was
> >> done as a special case for "MAC labels". The NFS support for
> >> security.* xattrs came much later. This is one of the reasons
> >> why I'm concerned about the virtiofs implementation you're
> >> proposing. We were never able to get the NFS "MAC label"
> >> implementation to work properly with Smack, even though there is
> >> no obvious reason it wouldn't.
> >>
> >>
> >>> Vivek
> >>>
> 

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
> > On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
> >> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> >>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> >>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> >>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>>>>>> When a new inode is created, send its security context to server along
> >>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>>>>>> This gives server an opportunity to create new file and set security
> >>>>>>> context (possibly atomically). In all the configurations it might not
> >>>>>>> be possible to set context atomically.
> >>>>>>>
> >>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>>>>>
> >>>>>>> Following is the information sent to server.
> >>>>>>>
> >>>>>>> - struct fuse_secctx.
> >>>>>>>   This contains total size of security context which follows this structure.
> >>>>>>>
> >>>>>>> - xattr name string.
> >>>>>>>   This string represents name of xattr which should be used while setting
> >>>>>>>   security context. As of now it is hardcoded to "security.selinux".
> >>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>>>>>
> >>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> >>>>> So this inode is about to be created. There are no xattrs yet. And
> >>>>> filesystem is asking LSMs, what security labels should be set on this
> >>>>> inode before it is published. 
> >>>> No. That's imprecise. It's what SELinux does. An LSM can add any
> >>>> number of attributes on inode creation, or none. These attributes
> >>>> may or may not be "security labels". Assuming that they are is the
> >>>> kind of thinking that leads people like Linus to conclude that the
> >>>> LSM community is clueless.
> >>>>
> >>>>
> >>>>> For local filesystems it is somewhat easy. They are the one creating
> >>>>> inode and can set all xattrs/labels before inode is added to inode
> >>>>> cache.
> >>>>>
> >>>>> But for remote like filesystems, it is more tricky. Actual inode
> >>>>> creation first will happen on server and then client will instantiate
> >>>>> an inode based on information returned by server (Atleast that's
> >>>>> what fuse does).
> >>>>>
> >>>>> So security_dentry_init_security() was created (I think by NFS folks)
> >>>>> so that they can query the label and send it along with create
> >>>>> request and server can take care of setting label (along with file
> >>>>> creation).
> >>>>>
> >>>>> One limitation of security_dentry_init_security() is that it practically
> >>>>> supports only one label. And only SELinux has implemented. So for
> >>>>> all practical purposes this is a hook to obtain selinux label. NFS
> >>>>> and ceph already use it in that way.
> >>>>>
> >>>>> Now there is a desire to be able to return more than one security
> >>>>> labels and support smack and possibly other LSMs. Sure, that great.
> >>>>> But I think for that we will have to implement a new hook which
> >>>>> can return multiple labels and filesystems like nfs, ceph and fuse
> >>>>> will have to be modified to cope with this new hook to support
> >>>>> multiple lables. 
> >>>>>
> >>>>> And I am arguing that we can modify fuse when that hook has been
> >>>>> implemented. There is no point in adding that complexity in fuse
> >>>>> code as well all fuse-server implementations when there is nobody
> >>>>> generating multiple labels. We can't even test it.
> >>>> There's a little bit of chicken-and-egg going on here.
> >>>> There's no point in accommodating multiple labels in
> >>>> this code because you can't have multiple labels. There's
> >>>> no point in trying to support multiple labels because
> >>>> you can't use them in virtiofs and a bunch of other
> >>>> places.
> >>> Once security subsystem provides a hook to support multiple lables, then
> >>> atleast one filesystem will have to be converted to make use of this new
> >>> hook at the same time and rest of the filesystems can catch up later.
> >> Clearly you haven't been following the work I've been doing on
> >> module stacking. That's completely understandable. There aren't
> >> new hooks being added, or at least haven't been yet. Some of the
> >> existing hooks are getting changed to provide the data required
> >> for multiple security modules (e.g. secids become a set of secids).
> >> Filesystems that support xattrs properly are unaffected because,
> >> for all it's shortcomings, the LSM layer hides the details of
> >> the security modules sufficiently. 
> >>
> >> Which filesystem are you saying will have to "be converted"?
> > When I grep for "security_dentry_init_security()" in current code,
> > I see two users, ceph and nfs.
> 
> Neither of which support xattrs fully. Ceph can support them properly,
> but does not by default. NFS is ancient and we've talked about it at
> length. Also, the fact that they use security_dentry_init_security()
> is a red herring. Really, this has no bearing on the issue of fuse.

Frankly speaking, I am now beginning to lose what's being asked for,
w.r.t this patch.

I see that NFS and ceph are supporting single security label at
the time of file creation and I added support for the same for
fuse.

You seem to want to have capability to send multiple "name,value,len"
tuples so that you can support multiple xattrs/labels down the
line.

Even if I do that, I am not sure what to do with those xattrs at
the other end. I am using /proc/thread-self/attr/fscreate to
set the security attribute of file.

https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html

How will this work with multiple labels. I think you will have to
extend fscreate or create new interface to be able to deal with it.

That's why I think that it seems premature that fuse interface be
written to deal with multiple labels when rest of the infrastructure
is not ready. It should be other way, instead. First rest of the
infrastructure should be written and then all the users make use
of new infra.

BTW, I am checking security_inode_init_security(). That seems to
return max 2 xattrs as of now?

#define MAX_LSM_EVM_XATTR       2
struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];

So we are allocating space for 3 xattrs. Last xattr is Null to signify
end of array. So, we seem to use on xattr for LSM and another for EVM.
Do I understand it correctly. Does that mean that smack stuff does
not work even with security_inode_init_security(). Or there is something
else going on.

Vivek

> 
> >
> > fs/ceph/xattr.c
> > ceph_security_init_secctx()
> >
> > fs/nfs/nfs4proc.c
> > nfs4_label_init_security()
> >
> > So looks like these two file systems will have to be converted
> > (along with fuse).
> >
> > Vivek
> >
> >> NFS is going to require some work, but that's because it was
> >> done as a special case for "MAC labels". The NFS support for
> >> security.* xattrs came much later. This is one of the reasons
> >> why I'm concerned about the virtiofs implementation you're
> >> proposing. We were never able to get the NFS "MAC label"
> >> implementation to work properly with Smack, even though there is
> >> no obvious reason it wouldn't.
> >>
> >>
> >>> Vivek
> >>>
> 

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 12:20 PM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
>> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
>>> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
>>>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
>>>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>>>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>>>>>> When a new inode is created, send its security context to server along
>>>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>>>>>> This gives server an opportunity to create new file and set security
>>>>>>>>> context (possibly atomically). In all the configurations it might not
>>>>>>>>> be possible to set context atomically.
>>>>>>>>>
>>>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>>>>>
>>>>>>>>> Following is the information sent to server.
>>>>>>>>>
>>>>>>>>> - struct fuse_secctx.
>>>>>>>>>   This contains total size of security context which follows this structure.
>>>>>>>>>
>>>>>>>>> - xattr name string.
>>>>>>>>>   This string represents name of xattr which should be used while setting
>>>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>>>>>
>>>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>>>>>> So this inode is about to be created. There are no xattrs yet. And
>>>>>>> filesystem is asking LSMs, what security labels should be set on this
>>>>>>> inode before it is published. 
>>>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
>>>>>> number of attributes on inode creation, or none. These attributes
>>>>>> may or may not be "security labels". Assuming that they are is the
>>>>>> kind of thinking that leads people like Linus to conclude that the
>>>>>> LSM community is clueless.
>>>>>>
>>>>>>
>>>>>>> For local filesystems it is somewhat easy. They are the one creating
>>>>>>> inode and can set all xattrs/labels before inode is added to inode
>>>>>>> cache.
>>>>>>>
>>>>>>> But for remote like filesystems, it is more tricky. Actual inode
>>>>>>> creation first will happen on server and then client will instantiate
>>>>>>> an inode based on information returned by server (Atleast that's
>>>>>>> what fuse does).
>>>>>>>
>>>>>>> So security_dentry_init_security() was created (I think by NFS folks)
>>>>>>> so that they can query the label and send it along with create
>>>>>>> request and server can take care of setting label (along with file
>>>>>>> creation).
>>>>>>>
>>>>>>> One limitation of security_dentry_init_security() is that it practically
>>>>>>> supports only one label. And only SELinux has implemented. So for
>>>>>>> all practical purposes this is a hook to obtain selinux label. NFS
>>>>>>> and ceph already use it in that way.
>>>>>>>
>>>>>>> Now there is a desire to be able to return more than one security
>>>>>>> labels and support smack and possibly other LSMs. Sure, that great.
>>>>>>> But I think for that we will have to implement a new hook which
>>>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>>>>>> will have to be modified to cope with this new hook to support
>>>>>>> multiple lables. 
>>>>>>>
>>>>>>> And I am arguing that we can modify fuse when that hook has been
>>>>>>> implemented. There is no point in adding that complexity in fuse
>>>>>>> code as well all fuse-server implementations when there is nobody
>>>>>>> generating multiple labels. We can't even test it.
>>>>>> There's a little bit of chicken-and-egg going on here.
>>>>>> There's no point in accommodating multiple labels in
>>>>>> this code because you can't have multiple labels. There's
>>>>>> no point in trying to support multiple labels because
>>>>>> you can't use them in virtiofs and a bunch of other
>>>>>> places.
>>>>> Once security subsystem provides a hook to support multiple lables, then
>>>>> atleast one filesystem will have to be converted to make use of this new
>>>>> hook at the same time and rest of the filesystems can catch up later.
>>>> Clearly you haven't been following the work I've been doing on
>>>> module stacking. That's completely understandable. There aren't
>>>> new hooks being added, or at least haven't been yet. Some of the
>>>> existing hooks are getting changed to provide the data required
>>>> for multiple security modules (e.g. secids become a set of secids).
>>>> Filesystems that support xattrs properly are unaffected because,
>>>> for all it's shortcomings, the LSM layer hides the details of
>>>> the security modules sufficiently. 
>>>>
>>>> Which filesystem are you saying will have to "be converted"?
>>> When I grep for "security_dentry_init_security()" in current code,
>>> I see two users, ceph and nfs.
>> Neither of which support xattrs fully. Ceph can support them properly,
>> but does not by default. NFS is ancient and we've talked about it at
>> length. Also, the fact that they use security_dentry_init_security()
>> is a red herring. Really, this has no bearing on the issue of fuse.
> Frankly speaking, I am now beginning to lose what's being asked for,
> w.r.t this patch.

1. Support for multiple concurrent security.* xattrs
2. Abandon mapping security.* attrs to user.* xattrs

> I see that NFS and ceph are supporting single security label at
> the time of file creation and I added support for the same for
> fuse.

NFS took that course because the IETF refused for a very long time
to accept a name+value pair in the protocol. The implementation
was a compromise.

>
> You seem to want to have capability to send multiple "name,value,len"
> tuples so that you can support multiple xattrs/labels down the
> line.

No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
	security.SMACK64		- the "security label"
	security.SMACK64EXEC		- the Smack label to run the program with
	security.SMACK64TRANSMUTE	- controls labeling on files created

There has been discussion about using additional attributes for things
like socket labeling.

This isn't hypothetical. It's real today. 

> Even if I do that, I am not sure what to do with those xattrs at
> the other end. I am using /proc/thread-self/attr/fscreate to
> set the security attribute of file.

Either you don't realize that attr/fscreate is SELinux specific, or
you don't care, or possibly (and sadly) both.

>
> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
>
> How will this work with multiple labels. I think you will have to
> extend fscreate or create new interface to be able to deal with it.

Yeah. That thread didn't go to the LSM mail list. It was essentially
kept within the RedHat SELinux community. It's no wonder everyone
involved thought that your approach is swell. No one who would get
goobsmacked by it was on the thread.

>
> That's why I think that it seems premature that fuse interface be
> written to deal with multiple labels when rest of the infrastructure
> is not ready. It should be other way, instead. First rest of the
> infrastructure should be written and then all the users make use
> of new infra.

Today the LSM infrastructure allows a security module to use as many
xattrs as it likes. Again, Smack uses multiple security.* xattrs today.

> BTW, I am checking security_inode_init_security(). That seems to
> return max 2 xattrs as of now?
>
> #define MAX_LSM_EVM_XATTR       2
> struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];

You're looking at the bowels of the EVM subsystem. That herring is red, too.

> So we are allocating space for 3 xattrs. Last xattr is Null to signify
> end of array. So, we seem to use on xattr for LSM and another for EVM.
> Do I understand it correctly. Does that mean that smack stuff does
> not work even with security_inode_init_security(). Or there is something
> else going on.

There's something else going on.

>
> Vivek
>
>>> fs/ceph/xattr.c
>>> ceph_security_init_secctx()
>>>
>>> fs/nfs/nfs4proc.c
>>> nfs4_label_init_security()
>>>
>>> So looks like these two file systems will have to be converted
>>> (along with fuse).
>>>
>>> Vivek
>>>
>>>> NFS is going to require some work, but that's because it was
>>>> done as a special case for "MAC labels". The NFS support for
>>>> security.* xattrs came much later. This is one of the reasons
>>>> why I'm concerned about the virtiofs implementation you're
>>>> proposing. We were never able to get the NFS "MAC label"
>>>> implementation to work properly with Smack, even though there is
>>>> no obvious reason it wouldn't.
>>>>
>>>>
>>>>> Vivek
>>>>>

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 12:20 PM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
>> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
>>> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
>>>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
>>>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>>>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>>>>>> When a new inode is created, send its security context to server along
>>>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>>>>>> This gives server an opportunity to create new file and set security
>>>>>>>>> context (possibly atomically). In all the configurations it might not
>>>>>>>>> be possible to set context atomically.
>>>>>>>>>
>>>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>>>>>
>>>>>>>>> Following is the information sent to server.
>>>>>>>>>
>>>>>>>>> - struct fuse_secctx.
>>>>>>>>>   This contains total size of security context which follows this structure.
>>>>>>>>>
>>>>>>>>> - xattr name string.
>>>>>>>>>   This string represents name of xattr which should be used while setting
>>>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>>>>>
>>>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>>>>>> So this inode is about to be created. There are no xattrs yet. And
>>>>>>> filesystem is asking LSMs, what security labels should be set on this
>>>>>>> inode before it is published. 
>>>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
>>>>>> number of attributes on inode creation, or none. These attributes
>>>>>> may or may not be "security labels". Assuming that they are is the
>>>>>> kind of thinking that leads people like Linus to conclude that the
>>>>>> LSM community is clueless.
>>>>>>
>>>>>>
>>>>>>> For local filesystems it is somewhat easy. They are the one creating
>>>>>>> inode and can set all xattrs/labels before inode is added to inode
>>>>>>> cache.
>>>>>>>
>>>>>>> But for remote like filesystems, it is more tricky. Actual inode
>>>>>>> creation first will happen on server and then client will instantiate
>>>>>>> an inode based on information returned by server (Atleast that's
>>>>>>> what fuse does).
>>>>>>>
>>>>>>> So security_dentry_init_security() was created (I think by NFS folks)
>>>>>>> so that they can query the label and send it along with create
>>>>>>> request and server can take care of setting label (along with file
>>>>>>> creation).
>>>>>>>
>>>>>>> One limitation of security_dentry_init_security() is that it practically
>>>>>>> supports only one label. And only SELinux has implemented. So for
>>>>>>> all practical purposes this is a hook to obtain selinux label. NFS
>>>>>>> and ceph already use it in that way.
>>>>>>>
>>>>>>> Now there is a desire to be able to return more than one security
>>>>>>> labels and support smack and possibly other LSMs. Sure, that great.
>>>>>>> But I think for that we will have to implement a new hook which
>>>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>>>>>> will have to be modified to cope with this new hook to support
>>>>>>> multiple lables. 
>>>>>>>
>>>>>>> And I am arguing that we can modify fuse when that hook has been
>>>>>>> implemented. There is no point in adding that complexity in fuse
>>>>>>> code as well all fuse-server implementations when there is nobody
>>>>>>> generating multiple labels. We can't even test it.
>>>>>> There's a little bit of chicken-and-egg going on here.
>>>>>> There's no point in accommodating multiple labels in
>>>>>> this code because you can't have multiple labels. There's
>>>>>> no point in trying to support multiple labels because
>>>>>> you can't use them in virtiofs and a bunch of other
>>>>>> places.
>>>>> Once security subsystem provides a hook to support multiple lables, then
>>>>> atleast one filesystem will have to be converted to make use of this new
>>>>> hook at the same time and rest of the filesystems can catch up later.
>>>> Clearly you haven't been following the work I've been doing on
>>>> module stacking. That's completely understandable. There aren't
>>>> new hooks being added, or at least haven't been yet. Some of the
>>>> existing hooks are getting changed to provide the data required
>>>> for multiple security modules (e.g. secids become a set of secids).
>>>> Filesystems that support xattrs properly are unaffected because,
>>>> for all it's shortcomings, the LSM layer hides the details of
>>>> the security modules sufficiently. 
>>>>
>>>> Which filesystem are you saying will have to "be converted"?
>>> When I grep for "security_dentry_init_security()" in current code,
>>> I see two users, ceph and nfs.
>> Neither of which support xattrs fully. Ceph can support them properly,
>> but does not by default. NFS is ancient and we've talked about it at
>> length. Also, the fact that they use security_dentry_init_security()
>> is a red herring. Really, this has no bearing on the issue of fuse.
> Frankly speaking, I am now beginning to lose what's being asked for,
> w.r.t this patch.

1. Support for multiple concurrent security.* xattrs
2. Abandon mapping security.* attrs to user.* xattrs

> I see that NFS and ceph are supporting single security label at
> the time of file creation and I added support for the same for
> fuse.

NFS took that course because the IETF refused for a very long time
to accept a name+value pair in the protocol. The implementation
was a compromise.

>
> You seem to want to have capability to send multiple "name,value,len"
> tuples so that you can support multiple xattrs/labels down the
> line.

No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
	security.SMACK64		- the "security label"
	security.SMACK64EXEC		- the Smack label to run the program with
	security.SMACK64TRANSMUTE	- controls labeling on files created

There has been discussion about using additional attributes for things
like socket labeling.

This isn't hypothetical. It's real today. 

> Even if I do that, I am not sure what to do with those xattrs at
> the other end. I am using /proc/thread-self/attr/fscreate to
> set the security attribute of file.

Either you don't realize that attr/fscreate is SELinux specific, or
you don't care, or possibly (and sadly) both.

>
> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
>
> How will this work with multiple labels. I think you will have to
> extend fscreate or create new interface to be able to deal with it.

Yeah. That thread didn't go to the LSM mail list. It was essentially
kept within the RedHat SELinux community. It's no wonder everyone
involved thought that your approach is swell. No one who would get
goobsmacked by it was on the thread.

>
> That's why I think that it seems premature that fuse interface be
> written to deal with multiple labels when rest of the infrastructure
> is not ready. It should be other way, instead. First rest of the
> infrastructure should be written and then all the users make use
> of new infra.

Today the LSM infrastructure allows a security module to use as many
xattrs as it likes. Again, Smack uses multiple security.* xattrs today.

> BTW, I am checking security_inode_init_security(). That seems to
> return max 2 xattrs as of now?
>
> #define MAX_LSM_EVM_XATTR       2
> struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];

You're looking at the bowels of the EVM subsystem. That herring is red, too.

> So we are allocating space for 3 xattrs. Last xattr is Null to signify
> end of array. So, we seem to use on xattr for LSM and another for EVM.
> Do I understand it correctly. Does that mean that smack stuff does
> not work even with security_inode_init_security(). Or there is something
> else going on.

There's something else going on.

>
> Vivek
>
>>> fs/ceph/xattr.c
>>> ceph_security_init_secctx()
>>>
>>> fs/nfs/nfs4proc.c
>>> nfs4_label_init_security()
>>>
>>> So looks like these two file systems will have to be converted
>>> (along with fuse).
>>>
>>> Vivek
>>>
>>>> NFS is going to require some work, but that's because it was
>>>> done as a special case for "MAC labels". The NFS support for
>>>> security.* xattrs came much later. This is one of the reasons
>>>> why I'm concerned about the virtiofs implementation you're
>>>> proposing. We were never able to get the NFS "MAC label"
>>>> implementation to work properly with Smack, even though there is
>>>> no obvious reason it wouldn't.
>>>>
>>>>
>>>>> Vivek
>>>>>

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 01:19:25PM -0700, Casey Schaufler wrote:
> On 9/27/2021 12:20 PM, Vivek Goyal wrote:
> > On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
> >> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
> >>> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
> >>>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> >>>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> >>>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> >>>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>>>>>>>> When a new inode is created, send its security context to server along
> >>>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>>>>>>>> This gives server an opportunity to create new file and set security
> >>>>>>>>> context (possibly atomically). In all the configurations it might not
> >>>>>>>>> be possible to set context atomically.
> >>>>>>>>>
> >>>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>>>>>>>
> >>>>>>>>> Following is the information sent to server.
> >>>>>>>>>
> >>>>>>>>> - struct fuse_secctx.
> >>>>>>>>>   This contains total size of security context which follows this structure.
> >>>>>>>>>
> >>>>>>>>> - xattr name string.
> >>>>>>>>>   This string represents name of xattr which should be used while setting
> >>>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
> >>>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>>>>>>>
> >>>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> >>>>>>> So this inode is about to be created. There are no xattrs yet. And
> >>>>>>> filesystem is asking LSMs, what security labels should be set on this
> >>>>>>> inode before it is published. 
> >>>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
> >>>>>> number of attributes on inode creation, or none. These attributes
> >>>>>> may or may not be "security labels". Assuming that they are is the
> >>>>>> kind of thinking that leads people like Linus to conclude that the
> >>>>>> LSM community is clueless.
> >>>>>>
> >>>>>>
> >>>>>>> For local filesystems it is somewhat easy. They are the one creating
> >>>>>>> inode and can set all xattrs/labels before inode is added to inode
> >>>>>>> cache.
> >>>>>>>
> >>>>>>> But for remote like filesystems, it is more tricky. Actual inode
> >>>>>>> creation first will happen on server and then client will instantiate
> >>>>>>> an inode based on information returned by server (Atleast that's
> >>>>>>> what fuse does).
> >>>>>>>
> >>>>>>> So security_dentry_init_security() was created (I think by NFS folks)
> >>>>>>> so that they can query the label and send it along with create
> >>>>>>> request and server can take care of setting label (along with file
> >>>>>>> creation).
> >>>>>>>
> >>>>>>> One limitation of security_dentry_init_security() is that it practically
> >>>>>>> supports only one label. And only SELinux has implemented. So for
> >>>>>>> all practical purposes this is a hook to obtain selinux label. NFS
> >>>>>>> and ceph already use it in that way.
> >>>>>>>
> >>>>>>> Now there is a desire to be able to return more than one security
> >>>>>>> labels and support smack and possibly other LSMs. Sure, that great.
> >>>>>>> But I think for that we will have to implement a new hook which
> >>>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
> >>>>>>> will have to be modified to cope with this new hook to support
> >>>>>>> multiple lables. 
> >>>>>>>
> >>>>>>> And I am arguing that we can modify fuse when that hook has been
> >>>>>>> implemented. There is no point in adding that complexity in fuse
> >>>>>>> code as well all fuse-server implementations when there is nobody
> >>>>>>> generating multiple labels. We can't even test it.
> >>>>>> There's a little bit of chicken-and-egg going on here.
> >>>>>> There's no point in accommodating multiple labels in
> >>>>>> this code because you can't have multiple labels. There's
> >>>>>> no point in trying to support multiple labels because
> >>>>>> you can't use them in virtiofs and a bunch of other
> >>>>>> places.
> >>>>> Once security subsystem provides a hook to support multiple lables, then
> >>>>> atleast one filesystem will have to be converted to make use of this new
> >>>>> hook at the same time and rest of the filesystems can catch up later.
> >>>> Clearly you haven't been following the work I've been doing on
> >>>> module stacking. That's completely understandable. There aren't
> >>>> new hooks being added, or at least haven't been yet. Some of the
> >>>> existing hooks are getting changed to provide the data required
> >>>> for multiple security modules (e.g. secids become a set of secids).
> >>>> Filesystems that support xattrs properly are unaffected because,
> >>>> for all it's shortcomings, the LSM layer hides the details of
> >>>> the security modules sufficiently. 
> >>>>
> >>>> Which filesystem are you saying will have to "be converted"?
> >>> When I grep for "security_dentry_init_security()" in current code,
> >>> I see two users, ceph and nfs.
> >> Neither of which support xattrs fully. Ceph can support them properly,
> >> but does not by default. NFS is ancient and we've talked about it at
> >> length. Also, the fact that they use security_dentry_init_security()
> >> is a red herring. Really, this has no bearing on the issue of fuse.
> > Frankly speaking, I am now beginning to lose what's being asked for,
> > w.r.t this patch.
> 
> 1. Support for multiple concurrent security.* xattrs

Supporting SMACK is not my priority right now. I am only interested
in SELinux at this point of time. I am willing to do some extra
work if SMACK can be easily incorporated in current framework. But
if current infrastructure does not support it properly, I am not
planning to write all that to support SMACK. That's a work for
somebody else who needs to support SMACK over fuse/virtiofs.

> 2. Abandon mapping security.* attrs to user.* xattrs

That I have moved away, for now. Planning to remap security.* xattrs
to trusted.* and will ask users to give CAP_SYS_ADMIN to daemon.

Once trusted xattrs are namespaced, this all should work very well.

> 
> > I see that NFS and ceph are supporting single security label at
> > the time of file creation and I added support for the same for
> > fuse.
> 
> NFS took that course because the IETF refused for a very long time
> to accept a name+value pair in the protocol. The implementation
> was a compromise.
> 
> >
> > You seem to want to have capability to send multiple "name,value,len"
> > tuples so that you can support multiple xattrs/labels down the
> > line.
> 
> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
> 	security.SMACK64		- the "security label"
> 	security.SMACK64EXEC		- the Smack label to run the program with
> 	security.SMACK64TRANSMUTE	- controls labeling on files created
> 
> There has been discussion about using additional attributes for things
> like socket labeling.
> 
> This isn't hypothetical. It's real today. 

It is real from SMACK point of view but it is not real from 
security_dentry_init_security() hook point of view. What's equivalent
of that hook to support SMACK and multiple labels?

> 
> > Even if I do that, I am not sure what to do with those xattrs at
> > the other end. I am using /proc/thread-self/attr/fscreate to
> > set the security attribute of file.
> 
> Either you don't realize that attr/fscreate is SELinux specific, or
> you don't care, or possibly (and sadly) both.

I do realize that it is SELinux specific and that's why I have raised
the concern that it does not work with SMACK.

What's the "fscreate" equivalent for SMACK so that I file server can
set it before creation of file and get correct context file?

> 
> >
> > https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
> >
> > How will this work with multiple labels. I think you will have to
> > extend fscreate or create new interface to be able to deal with it.
> 
> Yeah. That thread didn't go to the LSM mail list. It was essentially
> kept within the RedHat SELinux community. It's no wonder everyone
> involved thought that your approach is swell. No one who would get
> goobsmacked by it was on the thread.

My goal is to support SELinux at this point of time. If you goal is
to support SMACK, feel free to send patches on top to support that.

I sent kernel patches to LSM list to make it plenty clear that this
interface only supports single label which is SELinux. So there is
no hiding here. And when I am supporting only SELinux, making use
of fscreate makes perfect sense to me.

> 
> >
> > That's why I think that it seems premature that fuse interface be
> > written to deal with multiple labels when rest of the infrastructure
> > is not ready. It should be other way, instead. First rest of the
> > infrastructure should be written and then all the users make use
> > of new infra.
> 
> Today the LSM infrastructure allows a security module to use as many
> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.

security_dentry_init_security() can handle that? If not, what's the
equivalent.

> 
> > BTW, I am checking security_inode_init_security(). That seems to
> > return max 2 xattrs as of now?
> >
> > #define MAX_LSM_EVM_XATTR       2
> > struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
> 
> You're looking at the bowels of the EVM subsystem. That herring is red, too.
> 
> > So we are allocating space for 3 xattrs. Last xattr is Null to signify
> > end of array. So, we seem to use on xattr for LSM and another for EVM.
> > Do I understand it correctly. Does that mean that smack stuff does
> > not work even with security_inode_init_security(). Or there is something
> > else going on.
> 
> There's something else going on.

Help me understand what's going on. How are you returning multiple
xattrs from security_inode_init_security() when you have allocated
space for only one LSM xattr.

Vivek

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 01:19:25PM -0700, Casey Schaufler wrote:
> On 9/27/2021 12:20 PM, Vivek Goyal wrote:
> > On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
> >> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
> >>> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
> >>>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
> >>>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
> >>>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
> >>>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
> >>>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
> >>>>>>>>> When a new inode is created, send its security context to server along
> >>>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> >>>>>>>>> This gives server an opportunity to create new file and set security
> >>>>>>>>> context (possibly atomically). In all the configurations it might not
> >>>>>>>>> be possible to set context atomically.
> >>>>>>>>>
> >>>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
> >>>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
> >>>>>>>>>
> >>>>>>>>> Following is the information sent to server.
> >>>>>>>>>
> >>>>>>>>> - struct fuse_secctx.
> >>>>>>>>>   This contains total size of security context which follows this structure.
> >>>>>>>>>
> >>>>>>>>> - xattr name string.
> >>>>>>>>>   This string represents name of xattr which should be used while setting
> >>>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
> >>>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
> >>>>>>>>
> >>>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
> >>>>>>> So this inode is about to be created. There are no xattrs yet. And
> >>>>>>> filesystem is asking LSMs, what security labels should be set on this
> >>>>>>> inode before it is published. 
> >>>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
> >>>>>> number of attributes on inode creation, or none. These attributes
> >>>>>> may or may not be "security labels". Assuming that they are is the
> >>>>>> kind of thinking that leads people like Linus to conclude that the
> >>>>>> LSM community is clueless.
> >>>>>>
> >>>>>>
> >>>>>>> For local filesystems it is somewhat easy. They are the one creating
> >>>>>>> inode and can set all xattrs/labels before inode is added to inode
> >>>>>>> cache.
> >>>>>>>
> >>>>>>> But for remote like filesystems, it is more tricky. Actual inode
> >>>>>>> creation first will happen on server and then client will instantiate
> >>>>>>> an inode based on information returned by server (Atleast that's
> >>>>>>> what fuse does).
> >>>>>>>
> >>>>>>> So security_dentry_init_security() was created (I think by NFS folks)
> >>>>>>> so that they can query the label and send it along with create
> >>>>>>> request and server can take care of setting label (along with file
> >>>>>>> creation).
> >>>>>>>
> >>>>>>> One limitation of security_dentry_init_security() is that it practically
> >>>>>>> supports only one label. And only SELinux has implemented. So for
> >>>>>>> all practical purposes this is a hook to obtain selinux label. NFS
> >>>>>>> and ceph already use it in that way.
> >>>>>>>
> >>>>>>> Now there is a desire to be able to return more than one security
> >>>>>>> labels and support smack and possibly other LSMs. Sure, that great.
> >>>>>>> But I think for that we will have to implement a new hook which
> >>>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
> >>>>>>> will have to be modified to cope with this new hook to support
> >>>>>>> multiple lables. 
> >>>>>>>
> >>>>>>> And I am arguing that we can modify fuse when that hook has been
> >>>>>>> implemented. There is no point in adding that complexity in fuse
> >>>>>>> code as well all fuse-server implementations when there is nobody
> >>>>>>> generating multiple labels. We can't even test it.
> >>>>>> There's a little bit of chicken-and-egg going on here.
> >>>>>> There's no point in accommodating multiple labels in
> >>>>>> this code because you can't have multiple labels. There's
> >>>>>> no point in trying to support multiple labels because
> >>>>>> you can't use them in virtiofs and a bunch of other
> >>>>>> places.
> >>>>> Once security subsystem provides a hook to support multiple lables, then
> >>>>> atleast one filesystem will have to be converted to make use of this new
> >>>>> hook at the same time and rest of the filesystems can catch up later.
> >>>> Clearly you haven't been following the work I've been doing on
> >>>> module stacking. That's completely understandable. There aren't
> >>>> new hooks being added, or at least haven't been yet. Some of the
> >>>> existing hooks are getting changed to provide the data required
> >>>> for multiple security modules (e.g. secids become a set of secids).
> >>>> Filesystems that support xattrs properly are unaffected because,
> >>>> for all it's shortcomings, the LSM layer hides the details of
> >>>> the security modules sufficiently. 
> >>>>
> >>>> Which filesystem are you saying will have to "be converted"?
> >>> When I grep for "security_dentry_init_security()" in current code,
> >>> I see two users, ceph and nfs.
> >> Neither of which support xattrs fully. Ceph can support them properly,
> >> but does not by default. NFS is ancient and we've talked about it at
> >> length. Also, the fact that they use security_dentry_init_security()
> >> is a red herring. Really, this has no bearing on the issue of fuse.
> > Frankly speaking, I am now beginning to lose what's being asked for,
> > w.r.t this patch.
> 
> 1. Support for multiple concurrent security.* xattrs

Supporting SMACK is not my priority right now. I am only interested
in SELinux at this point of time. I am willing to do some extra
work if SMACK can be easily incorporated in current framework. But
if current infrastructure does not support it properly, I am not
planning to write all that to support SMACK. That's a work for
somebody else who needs to support SMACK over fuse/virtiofs.

> 2. Abandon mapping security.* attrs to user.* xattrs

That I have moved away, for now. Planning to remap security.* xattrs
to trusted.* and will ask users to give CAP_SYS_ADMIN to daemon.

Once trusted xattrs are namespaced, this all should work very well.

> 
> > I see that NFS and ceph are supporting single security label at
> > the time of file creation and I added support for the same for
> > fuse.
> 
> NFS took that course because the IETF refused for a very long time
> to accept a name+value pair in the protocol. The implementation
> was a compromise.
> 
> >
> > You seem to want to have capability to send multiple "name,value,len"
> > tuples so that you can support multiple xattrs/labels down the
> > line.
> 
> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
> 	security.SMACK64		- the "security label"
> 	security.SMACK64EXEC		- the Smack label to run the program with
> 	security.SMACK64TRANSMUTE	- controls labeling on files created
> 
> There has been discussion about using additional attributes for things
> like socket labeling.
> 
> This isn't hypothetical. It's real today. 

It is real from SMACK point of view but it is not real from 
security_dentry_init_security() hook point of view. What's equivalent
of that hook to support SMACK and multiple labels?

> 
> > Even if I do that, I am not sure what to do with those xattrs at
> > the other end. I am using /proc/thread-self/attr/fscreate to
> > set the security attribute of file.
> 
> Either you don't realize that attr/fscreate is SELinux specific, or
> you don't care, or possibly (and sadly) both.

I do realize that it is SELinux specific and that's why I have raised
the concern that it does not work with SMACK.

What's the "fscreate" equivalent for SMACK so that I file server can
set it before creation of file and get correct context file?

> 
> >
> > https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
> >
> > How will this work with multiple labels. I think you will have to
> > extend fscreate or create new interface to be able to deal with it.
> 
> Yeah. That thread didn't go to the LSM mail list. It was essentially
> kept within the RedHat SELinux community. It's no wonder everyone
> involved thought that your approach is swell. No one who would get
> goobsmacked by it was on the thread.

My goal is to support SELinux at this point of time. If you goal is
to support SMACK, feel free to send patches on top to support that.

I sent kernel patches to LSM list to make it plenty clear that this
interface only supports single label which is SELinux. So there is
no hiding here. And when I am supporting only SELinux, making use
of fscreate makes perfect sense to me.

> 
> >
> > That's why I think that it seems premature that fuse interface be
> > written to deal with multiple labels when rest of the infrastructure
> > is not ready. It should be other way, instead. First rest of the
> > infrastructure should be written and then all the users make use
> > of new infra.
> 
> Today the LSM infrastructure allows a security module to use as many
> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.

security_dentry_init_security() can handle that? If not, what's the
equivalent.

> 
> > BTW, I am checking security_inode_init_security(). That seems to
> > return max 2 xattrs as of now?
> >
> > #define MAX_LSM_EVM_XATTR       2
> > struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
> 
> You're looking at the bowels of the EVM subsystem. That herring is red, too.
> 
> > So we are allocating space for 3 xattrs. Last xattr is Null to signify
> > end of array. So, we seem to use on xattr for LSM and another for EVM.
> > Do I understand it correctly. Does that mean that smack stuff does
> > not work even with security_inode_init_security(). Or there is something
> > else going on.
> 
> There's something else going on.

Help me understand what's going on. How are you returning multiple
xattrs from security_inode_init_security() when you have allocated
space for only one LSM xattr.

Vivek

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 1:45 PM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 01:19:25PM -0700, Casey Schaufler wrote:
>> On 9/27/2021 12:20 PM, Vivek Goyal wrote:
>>> On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
>>>> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
>>>>> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
>>>>>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
>>>>>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>>>>>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>>>>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>>>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>>>>>>>> When a new inode is created, send its security context to server along
>>>>>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>>>>>>>> This gives server an opportunity to create new file and set security
>>>>>>>>>>> context (possibly atomically). In all the configurations it might not
>>>>>>>>>>> be possible to set context atomically.
>>>>>>>>>>>
>>>>>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>>>>>>>
>>>>>>>>>>> Following is the information sent to server.
>>>>>>>>>>>
>>>>>>>>>>> - struct fuse_secctx.
>>>>>>>>>>>   This contains total size of security context which follows this structure.
>>>>>>>>>>>
>>>>>>>>>>> - xattr name string.
>>>>>>>>>>>   This string represents name of xattr which should be used while setting
>>>>>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>>>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>>>>>>>
>>>>>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>>>>>>>> So this inode is about to be created. There are no xattrs yet. And
>>>>>>>>> filesystem is asking LSMs, what security labels should be set on this
>>>>>>>>> inode before it is published. 
>>>>>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
>>>>>>>> number of attributes on inode creation, or none. These attributes
>>>>>>>> may or may not be "security labels". Assuming that they are is the
>>>>>>>> kind of thinking that leads people like Linus to conclude that the
>>>>>>>> LSM community is clueless.
>>>>>>>>
>>>>>>>>
>>>>>>>>> For local filesystems it is somewhat easy. They are the one creating
>>>>>>>>> inode and can set all xattrs/labels before inode is added to inode
>>>>>>>>> cache.
>>>>>>>>>
>>>>>>>>> But for remote like filesystems, it is more tricky. Actual inode
>>>>>>>>> creation first will happen on server and then client will instantiate
>>>>>>>>> an inode based on information returned by server (Atleast that's
>>>>>>>>> what fuse does).
>>>>>>>>>
>>>>>>>>> So security_dentry_init_security() was created (I think by NFS folks)
>>>>>>>>> so that they can query the label and send it along with create
>>>>>>>>> request and server can take care of setting label (along with file
>>>>>>>>> creation).
>>>>>>>>>
>>>>>>>>> One limitation of security_dentry_init_security() is that it practically
>>>>>>>>> supports only one label. And only SELinux has implemented. So for
>>>>>>>>> all practical purposes this is a hook to obtain selinux label. NFS
>>>>>>>>> and ceph already use it in that way.
>>>>>>>>>
>>>>>>>>> Now there is a desire to be able to return more than one security
>>>>>>>>> labels and support smack and possibly other LSMs. Sure, that great.
>>>>>>>>> But I think for that we will have to implement a new hook which
>>>>>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>>>>>>>> will have to be modified to cope with this new hook to support
>>>>>>>>> multiple lables. 
>>>>>>>>>
>>>>>>>>> And I am arguing that we can modify fuse when that hook has been
>>>>>>>>> implemented. There is no point in adding that complexity in fuse
>>>>>>>>> code as well all fuse-server implementations when there is nobody
>>>>>>>>> generating multiple labels. We can't even test it.
>>>>>>>> There's a little bit of chicken-and-egg going on here.
>>>>>>>> There's no point in accommodating multiple labels in
>>>>>>>> this code because you can't have multiple labels. There's
>>>>>>>> no point in trying to support multiple labels because
>>>>>>>> you can't use them in virtiofs and a bunch of other
>>>>>>>> places.
>>>>>>> Once security subsystem provides a hook to support multiple lables, then
>>>>>>> atleast one filesystem will have to be converted to make use of this new
>>>>>>> hook at the same time and rest of the filesystems can catch up later.
>>>>>> Clearly you haven't been following the work I've been doing on
>>>>>> module stacking. That's completely understandable. There aren't
>>>>>> new hooks being added, or at least haven't been yet. Some of the
>>>>>> existing hooks are getting changed to provide the data required
>>>>>> for multiple security modules (e.g. secids become a set of secids).
>>>>>> Filesystems that support xattrs properly are unaffected because,
>>>>>> for all it's shortcomings, the LSM layer hides the details of
>>>>>> the security modules sufficiently. 
>>>>>>
>>>>>> Which filesystem are you saying will have to "be converted"?
>>>>> When I grep for "security_dentry_init_security()" in current code,
>>>>> I see two users, ceph and nfs.
>>>> Neither of which support xattrs fully. Ceph can support them properly,
>>>> but does not by default. NFS is ancient and we've talked about it at
>>>> length. Also, the fact that they use security_dentry_init_security()
>>>> is a red herring. Really, this has no bearing on the issue of fuse.
>>> Frankly speaking, I am now beginning to lose what's being asked for,
>>> w.r.t this patch.
>> 1. Support for multiple concurrent security.* xattrs
> Supporting SMACK is not my priority right now. I am only interested
> in SELinux at this point of time. I am willing to do some extra
> work if SMACK can be easily incorporated in current framework. But
> if current infrastructure does not support it properly, I am not
> planning to write all that to support SMACK. That's a work for
> somebody else who needs to support SMACK over fuse/virtiofs.

Nuts. I was just getting comfortable with the level of cooperation
I've had from the SELinux side recently.

>> 2. Abandon mapping security.* attrs to user.* xattrs
> That I have moved away, for now. Planning to remap security.* xattrs
> to trusted.* and will ask users to give CAP_SYS_ADMIN to daemon.
>
> Once trusted xattrs are namespaced, this all should work very well.

That's good to hear.


>>> I see that NFS and ceph are supporting single security label at
>>> the time of file creation and I added support for the same for
>>> fuse.
>> NFS took that course because the IETF refused for a very long time
>> to accept a name+value pair in the protocol. The implementation
>> was a compromise.
>>
>>> You seem to want to have capability to send multiple "name,value,len"
>>> tuples so that you can support multiple xattrs/labels down the
>>> line.
>> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
>> 	security.SMACK64		- the "security label"
>> 	security.SMACK64EXEC		- the Smack label to run the program with
>> 	security.SMACK64TRANSMUTE	- controls labeling on files created
>>
>> There has been discussion about using additional attributes for things
>> like socket labeling.
>>
>> This isn't hypothetical. It's real today. 
> It is real from SMACK point of view but it is not real from 
> security_dentry_init_security() hook point of view. What's equivalent
> of that hook to support SMACK and multiple labels?

When multiple security modules support this hook they will
each get called. So where today security_dentry_init_security()
calls selinux_dentry_init_security(), in the future it will
also call any other <lsm>_dentry_init_security() hook that
is registered. No LSM infrastructure change required.


>>> Even if I do that, I am not sure what to do with those xattrs at
>>> the other end. I am using /proc/thread-self/attr/fscreate to
>>> set the security attribute of file.
>> Either you don't realize that attr/fscreate is SELinux specific, or
>> you don't care, or possibly (and sadly) both.
> I do realize that it is SELinux specific and that's why I have raised
> the concern that it does not work with SMACK.
>
> What's the "fscreate" equivalent for SMACK so that I file server can
> set it before creation of file and get correct context file?

The Smack attribute will be inherited from the creating process.
There is no way to generally change the attribute of a file on
creation. The appropriateness of such a facility has been debated
long and loud over the years. SELinux, which implements so varied
a set of "security" controls opted for it. Smack, which sticks much
more closely to an access control model, considers it too dangerous.
You can change the Smack label with setxattr(1) if you have
CAP_MAC_ADMIN. If you really want the file created with a particular
Smack label you can change the process Smack label by writing to
/proc/self/attr/smack/current on newer kernels and /proc/self/attr/current
on older ones.


>>> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
>>>
>>> How will this work with multiple labels. I think you will have to
>>> extend fscreate or create new interface to be able to deal with it.
>> Yeah. That thread didn't go to the LSM mail list. It was essentially
>> kept within the RedHat SELinux community. It's no wonder everyone
>> involved thought that your approach is swell. No one who would get
>> goobsmacked by it was on the thread.
> My goal is to support SELinux at this point of time. If you goal is
> to support SMACK, feel free to send patches on top to support that.

It helps to know what's going on before it becomes a major overhaul.

> I sent kernel patches to LSM list to make it plenty clear that this
> interface only supports single label which is SELinux. So there is
> no hiding here. And when I am supporting only SELinux, making use
> of fscreate makes perfect sense to me.

I bet it does.

>>> That's why I think that it seems premature that fuse interface be
>>> written to deal with multiple labels when rest of the infrastructure
>>> is not ready. It should be other way, instead. First rest of the
>>> infrastructure should be written and then all the users make use
>>> of new infra.
>> Today the LSM infrastructure allows a security module to use as many
>> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.
> security_dentry_init_security() can handle that? If not, what's the
> equivalent.

Yes, it can.


>>> BTW, I am checking security_inode_init_security(). That seems to
>>> return max 2 xattrs as of now?
>>>
>>> #define MAX_LSM_EVM_XATTR       2
>>> struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>> You're looking at the bowels of the EVM subsystem. That herring is red, too.
>>
>>> So we are allocating space for 3 xattrs. Last xattr is Null to signify
>>> end of array. So, we seem to use on xattr for LSM and another for EVM.
>>> Do I understand it correctly. Does that mean that smack stuff does
>>> not work even with security_inode_init_security(). Or there is something
>>> else going on.
>> There's something else going on.
> Help me understand what's going on. How are you returning multiple
> xattrs from security_inode_init_security() when you have allocated
> space for only one LSM xattr.

Look into CONFIG_EVM_EXTRA_SMACK_XATTRS if you like. As I said,
you're digging deeply into EVM at this point. It's not the LSM
infrastructure.

>
> Vivek
>

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/27/2021 1:45 PM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 01:19:25PM -0700, Casey Schaufler wrote:
>> On 9/27/2021 12:20 PM, Vivek Goyal wrote:
>>> On Mon, Sep 27, 2021 at 10:56:59AM -0700, Casey Schaufler wrote:
>>>> On 9/27/2021 8:56 AM, Vivek Goyal wrote:
>>>>> On Mon, Sep 27, 2021 at 08:22:48AM -0700, Casey Schaufler wrote:
>>>>>> On 9/27/2021 7:05 AM, Vivek Goyal wrote:
>>>>>>> On Sun, Sep 26, 2021 at 05:53:11PM -0700, Casey Schaufler wrote:
>>>>>>>> On 9/24/2021 4:32 PM, Vivek Goyal wrote:
>>>>>>>>> On Fri, Sep 24, 2021 at 06:00:10PM -0400, Colin Walters wrote:
>>>>>>>>>> On Fri, Sep 24, 2021, at 3:24 PM, Vivek Goyal wrote:
>>>>>>>>>>> When a new inode is created, send its security context to server along
>>>>>>>>>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>>>>>>>>>> This gives server an opportunity to create new file and set security
>>>>>>>>>>> context (possibly atomically). In all the configurations it might not
>>>>>>>>>>> be possible to set context atomically.
>>>>>>>>>>>
>>>>>>>>>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>>>>>>>>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>>>>>>>>>
>>>>>>>>>>> Following is the information sent to server.
>>>>>>>>>>>
>>>>>>>>>>> - struct fuse_secctx.
>>>>>>>>>>>   This contains total size of security context which follows this structure.
>>>>>>>>>>>
>>>>>>>>>>> - xattr name string.
>>>>>>>>>>>   This string represents name of xattr which should be used while setting
>>>>>>>>>>>   security context. As of now it is hardcoded to "security.selinux".
>>>>>>>>>> Any reason not to just send all `security.*` xattrs found on the inode? 
>>>>>>>>>>
>>>>>>>>>> (I'm not super familiar with this code, it looks like we're going from the LSM-cached version attached to the inode, but presumably since we're sending bytes we can just ask the filesytem for the raw data instead)
>>>>>>>>> So this inode is about to be created. There are no xattrs yet. And
>>>>>>>>> filesystem is asking LSMs, what security labels should be set on this
>>>>>>>>> inode before it is published. 
>>>>>>>> No. That's imprecise. It's what SELinux does. An LSM can add any
>>>>>>>> number of attributes on inode creation, or none. These attributes
>>>>>>>> may or may not be "security labels". Assuming that they are is the
>>>>>>>> kind of thinking that leads people like Linus to conclude that the
>>>>>>>> LSM community is clueless.
>>>>>>>>
>>>>>>>>
>>>>>>>>> For local filesystems it is somewhat easy. They are the one creating
>>>>>>>>> inode and can set all xattrs/labels before inode is added to inode
>>>>>>>>> cache.
>>>>>>>>>
>>>>>>>>> But for remote like filesystems, it is more tricky. Actual inode
>>>>>>>>> creation first will happen on server and then client will instantiate
>>>>>>>>> an inode based on information returned by server (Atleast that's
>>>>>>>>> what fuse does).
>>>>>>>>>
>>>>>>>>> So security_dentry_init_security() was created (I think by NFS folks)
>>>>>>>>> so that they can query the label and send it along with create
>>>>>>>>> request and server can take care of setting label (along with file
>>>>>>>>> creation).
>>>>>>>>>
>>>>>>>>> One limitation of security_dentry_init_security() is that it practically
>>>>>>>>> supports only one label. And only SELinux has implemented. So for
>>>>>>>>> all practical purposes this is a hook to obtain selinux label. NFS
>>>>>>>>> and ceph already use it in that way.
>>>>>>>>>
>>>>>>>>> Now there is a desire to be able to return more than one security
>>>>>>>>> labels and support smack and possibly other LSMs. Sure, that great.
>>>>>>>>> But I think for that we will have to implement a new hook which
>>>>>>>>> can return multiple labels and filesystems like nfs, ceph and fuse
>>>>>>>>> will have to be modified to cope with this new hook to support
>>>>>>>>> multiple lables. 
>>>>>>>>>
>>>>>>>>> And I am arguing that we can modify fuse when that hook has been
>>>>>>>>> implemented. There is no point in adding that complexity in fuse
>>>>>>>>> code as well all fuse-server implementations when there is nobody
>>>>>>>>> generating multiple labels. We can't even test it.
>>>>>>>> There's a little bit of chicken-and-egg going on here.
>>>>>>>> There's no point in accommodating multiple labels in
>>>>>>>> this code because you can't have multiple labels. There's
>>>>>>>> no point in trying to support multiple labels because
>>>>>>>> you can't use them in virtiofs and a bunch of other
>>>>>>>> places.
>>>>>>> Once security subsystem provides a hook to support multiple lables, then
>>>>>>> atleast one filesystem will have to be converted to make use of this new
>>>>>>> hook at the same time and rest of the filesystems can catch up later.
>>>>>> Clearly you haven't been following the work I've been doing on
>>>>>> module stacking. That's completely understandable. There aren't
>>>>>> new hooks being added, or at least haven't been yet. Some of the
>>>>>> existing hooks are getting changed to provide the data required
>>>>>> for multiple security modules (e.g. secids become a set of secids).
>>>>>> Filesystems that support xattrs properly are unaffected because,
>>>>>> for all it's shortcomings, the LSM layer hides the details of
>>>>>> the security modules sufficiently. 
>>>>>>
>>>>>> Which filesystem are you saying will have to "be converted"?
>>>>> When I grep for "security_dentry_init_security()" in current code,
>>>>> I see two users, ceph and nfs.
>>>> Neither of which support xattrs fully. Ceph can support them properly,
>>>> but does not by default. NFS is ancient and we've talked about it at
>>>> length. Also, the fact that they use security_dentry_init_security()
>>>> is a red herring. Really, this has no bearing on the issue of fuse.
>>> Frankly speaking, I am now beginning to lose what's being asked for,
>>> w.r.t this patch.
>> 1. Support for multiple concurrent security.* xattrs
> Supporting SMACK is not my priority right now. I am only interested
> in SELinux at this point of time. I am willing to do some extra
> work if SMACK can be easily incorporated in current framework. But
> if current infrastructure does not support it properly, I am not
> planning to write all that to support SMACK. That's a work for
> somebody else who needs to support SMACK over fuse/virtiofs.

Nuts. I was just getting comfortable with the level of cooperation
I've had from the SELinux side recently.

>> 2. Abandon mapping security.* attrs to user.* xattrs
> That I have moved away, for now. Planning to remap security.* xattrs
> to trusted.* and will ask users to give CAP_SYS_ADMIN to daemon.
>
> Once trusted xattrs are namespaced, this all should work very well.

That's good to hear.


>>> I see that NFS and ceph are supporting single security label at
>>> the time of file creation and I added support for the same for
>>> fuse.
>> NFS took that course because the IETF refused for a very long time
>> to accept a name+value pair in the protocol. The implementation
>> was a compromise.
>>
>>> You seem to want to have capability to send multiple "name,value,len"
>>> tuples so that you can support multiple xattrs/labels down the
>>> line.
>> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
>> 	security.SMACK64		- the "security label"
>> 	security.SMACK64EXEC		- the Smack label to run the program with
>> 	security.SMACK64TRANSMUTE	- controls labeling on files created
>>
>> There has been discussion about using additional attributes for things
>> like socket labeling.
>>
>> This isn't hypothetical. It's real today. 
> It is real from SMACK point of view but it is not real from 
> security_dentry_init_security() hook point of view. What's equivalent
> of that hook to support SMACK and multiple labels?

When multiple security modules support this hook they will
each get called. So where today security_dentry_init_security()
calls selinux_dentry_init_security(), in the future it will
also call any other <lsm>_dentry_init_security() hook that
is registered. No LSM infrastructure change required.


>>> Even if I do that, I am not sure what to do with those xattrs at
>>> the other end. I am using /proc/thread-self/attr/fscreate to
>>> set the security attribute of file.
>> Either you don't realize that attr/fscreate is SELinux specific, or
>> you don't care, or possibly (and sadly) both.
> I do realize that it is SELinux specific and that's why I have raised
> the concern that it does not work with SMACK.
>
> What's the "fscreate" equivalent for SMACK so that I file server can
> set it before creation of file and get correct context file?

The Smack attribute will be inherited from the creating process.
There is no way to generally change the attribute of a file on
creation. The appropriateness of such a facility has been debated
long and loud over the years. SELinux, which implements so varied
a set of "security" controls opted for it. Smack, which sticks much
more closely to an access control model, considers it too dangerous.
You can change the Smack label with setxattr(1) if you have
CAP_MAC_ADMIN. If you really want the file created with a particular
Smack label you can change the process Smack label by writing to
/proc/self/attr/smack/current on newer kernels and /proc/self/attr/current
on older ones.


>>> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
>>>
>>> How will this work with multiple labels. I think you will have to
>>> extend fscreate or create new interface to be able to deal with it.
>> Yeah. That thread didn't go to the LSM mail list. It was essentially
>> kept within the RedHat SELinux community. It's no wonder everyone
>> involved thought that your approach is swell. No one who would get
>> goobsmacked by it was on the thread.
> My goal is to support SELinux at this point of time. If you goal is
> to support SMACK, feel free to send patches on top to support that.

It helps to know what's going on before it becomes a major overhaul.

> I sent kernel patches to LSM list to make it plenty clear that this
> interface only supports single label which is SELinux. So there is
> no hiding here. And when I am supporting only SELinux, making use
> of fscreate makes perfect sense to me.

I bet it does.

>>> That's why I think that it seems premature that fuse interface be
>>> written to deal with multiple labels when rest of the infrastructure
>>> is not ready. It should be other way, instead. First rest of the
>>> infrastructure should be written and then all the users make use
>>> of new infra.
>> Today the LSM infrastructure allows a security module to use as many
>> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.
> security_dentry_init_security() can handle that? If not, what's the
> equivalent.

Yes, it can.


>>> BTW, I am checking security_inode_init_security(). That seems to
>>> return max 2 xattrs as of now?
>>>
>>> #define MAX_LSM_EVM_XATTR       2
>>> struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
>> You're looking at the bowels of the EVM subsystem. That herring is red, too.
>>
>>> So we are allocating space for 3 xattrs. Last xattr is Null to signify
>>> end of array. So, we seem to use on xattr for LSM and another for EVM.
>>> Do I understand it correctly. Does that mean that smack stuff does
>>> not work even with security_inode_init_security(). Or there is something
>>> else going on.
>> There's something else going on.
> Help me understand what's going on. How are you returning multiple
> xattrs from security_inode_init_security() when you have allocated
> space for only one LSM xattr.

Look into CONFIG_EVM_EXTRA_SMACK_XATTRS if you like. As I said,
you're digging deeply into EVM at this point. It's not the LSM
infrastructure.

>
> Vivek
>

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 02:45:13PM -0700, Casey Schaufler wrote:
[..]
> >>> I see that NFS and ceph are supporting single security label at
> >>> the time of file creation and I added support for the same for
> >>> fuse.
> >> NFS took that course because the IETF refused for a very long time
> >> to accept a name+value pair in the protocol. The implementation
> >> was a compromise.
> >>
> >>> You seem to want to have capability to send multiple "name,value,len"
> >>> tuples so that you can support multiple xattrs/labels down the
> >>> line.
> >> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
> >> 	security.SMACK64		- the "security label"
> >> 	security.SMACK64EXEC		- the Smack label to run the program with
> >> 	security.SMACK64TRANSMUTE	- controls labeling on files created
> >>
> >> There has been discussion about using additional attributes for things
> >> like socket labeling.
> >>
> >> This isn't hypothetical. It's real today. 
> > It is real from SMACK point of view but it is not real from 
> > security_dentry_init_security() hook point of view. What's equivalent
> > of that hook to support SMACK and multiple labels?
> 
> When multiple security modules support this hook they will
> each get called. So where today security_dentry_init_security()
> calls selinux_dentry_init_security(), in the future it will
> also call any other <lsm>_dentry_init_security() hook that
> is registered. No LSM infrastructure change required.

I don't think security_dentry_init_security() can handle multiple
security labels without change.

int security_dentry_init_security(struct dentry *dentry, int mode,
                                        const struct qstr *name, void **ctx,
                                        u32 *ctxlen)
{
        return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
                                name, ctx, ctxlen);
}

It can reutrn only one security context. So most likely you will have
to add another hook to return multiple security context and slowly
deprecate this one.

IOW, as of today security_dentry_init_security() can't return multiple
security labels. In fact it does not even tell the caller what's the
name of the xattr. So caller has no idea if this security label came
from SELinux or some other LSM. So for all practical purposes this
is a hook for getting SELinux label and does not scale to support
other LSMs.

> 
> 
> >>> Even if I do that, I am not sure what to do with those xattrs at
> >>> the other end. I am using /proc/thread-self/attr/fscreate to
> >>> set the security attribute of file.
> >> Either you don't realize that attr/fscreate is SELinux specific, or
> >> you don't care, or possibly (and sadly) both.
> > I do realize that it is SELinux specific and that's why I have raised
> > the concern that it does not work with SMACK.
> >
> > What's the "fscreate" equivalent for SMACK so that I file server can
> > set it before creation of file and get correct context file?
> 
> The Smack attribute will be inherited from the creating process.
> There is no way to generally change the attribute of a file on
> creation. The appropriateness of such a facility has been debated
> long and loud over the years. SELinux, which implements so varied
> a set of "security" controls opted for it. Smack, which sticks much
> more closely to an access control model, considers it too dangerous.
> You can change the Smack label with setxattr(1) if you have
> CAP_MAC_ADMIN.

Ok, calling setxattr() after file creation will make the operation
non-atomic. Will be good if it continues to be atomic.

> If you really want the file created with a particular
> Smack label you can change the process Smack label by writing to
> /proc/self/attr/smack/current on newer kernels and /proc/self/attr/current
> on older ones.

I guess /proc/thread-self/attr/smack/current is the way to go in this
context, when one wants to support SMACK.

> 
> 
> >>> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
> >>>
> >>> How will this work with multiple labels. I think you will have to
> >>> extend fscreate or create new interface to be able to deal with it.
> >> Yeah. That thread didn't go to the LSM mail list. It was essentially
> >> kept within the RedHat SELinux community. It's no wonder everyone
> >> involved thought that your approach is swell. No one who would get
> >> goobsmacked by it was on the thread.
> > My goal is to support SELinux at this point of time. If you goal is
> > to support SMACK, feel free to send patches on top to support that.
> 
> It helps to know what's going on before it becomes a major overhaul.

Fair enough.

> 
> > I sent kernel patches to LSM list to make it plenty clear that this
> > interface only supports single label which is SELinux. So there is
> > no hiding here. And when I am supporting only SELinux, making use
> > of fscreate makes perfect sense to me.
> 
> I bet it does.
> 
> >>> That's why I think that it seems premature that fuse interface be
> >>> written to deal with multiple labels when rest of the infrastructure
> >>> is not ready. It should be other way, instead. First rest of the
> >>> infrastructure should be written and then all the users make use
> >>> of new infra.
> >> Today the LSM infrastructure allows a security module to use as many
> >> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.
> > security_dentry_init_security() can handle that? If not, what's the
> > equivalent.
> 
> Yes, it can.

How? How will security_dentry_init_security() return multiple lables?
It has parameters "u32 *ctxlen" and you can return only one. If you
try to return multiple labels and return total length in "ctxlen",
that does not help as you need to know length of individiual labels.
So you need to know the names of xattrs too. Without that its not
going to work.

So no, security_dentry_init_security() can not handle multiple 
security labels (and associated names). 

Vivek

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Vivek Goyal]

On Mon, Sep 27, 2021 at 02:45:13PM -0700, Casey Schaufler wrote:
[..]
> >>> I see that NFS and ceph are supporting single security label at
> >>> the time of file creation and I added support for the same for
> >>> fuse.
> >> NFS took that course because the IETF refused for a very long time
> >> to accept a name+value pair in the protocol. The implementation
> >> was a compromise.
> >>
> >>> You seem to want to have capability to send multiple "name,value,len"
> >>> tuples so that you can support multiple xattrs/labels down the
> >>> line.
> >> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
> >> 	security.SMACK64		- the "security label"
> >> 	security.SMACK64EXEC		- the Smack label to run the program with
> >> 	security.SMACK64TRANSMUTE	- controls labeling on files created
> >>
> >> There has been discussion about using additional attributes for things
> >> like socket labeling.
> >>
> >> This isn't hypothetical. It's real today. 
> > It is real from SMACK point of view but it is not real from 
> > security_dentry_init_security() hook point of view. What's equivalent
> > of that hook to support SMACK and multiple labels?
> 
> When multiple security modules support this hook they will
> each get called. So where today security_dentry_init_security()
> calls selinux_dentry_init_security(), in the future it will
> also call any other <lsm>_dentry_init_security() hook that
> is registered. No LSM infrastructure change required.

I don't think security_dentry_init_security() can handle multiple
security labels without change.

int security_dentry_init_security(struct dentry *dentry, int mode,
                                        const struct qstr *name, void **ctx,
                                        u32 *ctxlen)
{
        return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
                                name, ctx, ctxlen);
}

It can reutrn only one security context. So most likely you will have
to add another hook to return multiple security context and slowly
deprecate this one.

IOW, as of today security_dentry_init_security() can't return multiple
security labels. In fact it does not even tell the caller what's the
name of the xattr. So caller has no idea if this security label came
from SELinux or some other LSM. So for all practical purposes this
is a hook for getting SELinux label and does not scale to support
other LSMs.

> 
> 
> >>> Even if I do that, I am not sure what to do with those xattrs at
> >>> the other end. I am using /proc/thread-self/attr/fscreate to
> >>> set the security attribute of file.
> >> Either you don't realize that attr/fscreate is SELinux specific, or
> >> you don't care, or possibly (and sadly) both.
> > I do realize that it is SELinux specific and that's why I have raised
> > the concern that it does not work with SMACK.
> >
> > What's the "fscreate" equivalent for SMACK so that I file server can
> > set it before creation of file and get correct context file?
> 
> The Smack attribute will be inherited from the creating process.
> There is no way to generally change the attribute of a file on
> creation. The appropriateness of such a facility has been debated
> long and loud over the years. SELinux, which implements so varied
> a set of "security" controls opted for it. Smack, which sticks much
> more closely to an access control model, considers it too dangerous.
> You can change the Smack label with setxattr(1) if you have
> CAP_MAC_ADMIN.

Ok, calling setxattr() after file creation will make the operation
non-atomic. Will be good if it continues to be atomic.

> If you really want the file created with a particular
> Smack label you can change the process Smack label by writing to
> /proc/self/attr/smack/current on newer kernels and /proc/self/attr/current
> on older ones.

I guess /proc/thread-self/attr/smack/current is the way to go in this
context, when one wants to support SMACK.

> 
> 
> >>> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
> >>>
> >>> How will this work with multiple labels. I think you will have to
> >>> extend fscreate or create new interface to be able to deal with it.
> >> Yeah. That thread didn't go to the LSM mail list. It was essentially
> >> kept within the RedHat SELinux community. It's no wonder everyone
> >> involved thought that your approach is swell. No one who would get
> >> goobsmacked by it was on the thread.
> > My goal is to support SELinux at this point of time. If you goal is
> > to support SMACK, feel free to send patches on top to support that.
> 
> It helps to know what's going on before it becomes a major overhaul.

Fair enough.

> 
> > I sent kernel patches to LSM list to make it plenty clear that this
> > interface only supports single label which is SELinux. So there is
> > no hiding here. And when I am supporting only SELinux, making use
> > of fscreate makes perfect sense to me.
> 
> I bet it does.
> 
> >>> That's why I think that it seems premature that fuse interface be
> >>> written to deal with multiple labels when rest of the infrastructure
> >>> is not ready. It should be other way, instead. First rest of the
> >>> infrastructure should be written and then all the users make use
> >>> of new infra.
> >> Today the LSM infrastructure allows a security module to use as many
> >> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.
> > security_dentry_init_security() can handle that? If not, what's the
> > equivalent.
> 
> Yes, it can.

How? How will security_dentry_init_security() return multiple lables?
It has parameters "u32 *ctxlen" and you can return only one. If you
try to return multiple labels and return total length in "ctxlen",
that does not help as you need to know length of individiual labels.
So you need to know the names of xattrs too. Without that its not
going to work.

So no, security_dentry_init_security() can not handle multiple 
security labels (and associated names). 

Vivek

Re: [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/28/2021 5:49 AM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 02:45:13PM -0700, Casey Schaufler wrote:
> [..]
>>>>> I see that NFS and ceph are supporting single security label at
>>>>> the time of file creation and I added support for the same for
>>>>> fuse.
>>>> NFS took that course because the IETF refused for a very long time
>>>> to accept a name+value pair in the protocol. The implementation
>>>> was a compromise.
>>>>
>>>>> You seem to want to have capability to send multiple "name,value,len"
>>>>> tuples so that you can support multiple xattrs/labels down the
>>>>> line.
>>>> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
>>>> 	security.SMACK64		- the "security label"
>>>> 	security.SMACK64EXEC		- the Smack label to run the program with
>>>> 	security.SMACK64TRANSMUTE	- controls labeling on files created
>>>>
>>>> There has been discussion about using additional attributes for things
>>>> like socket labeling.
>>>>
>>>> This isn't hypothetical. It's real today. 
>>> It is real from SMACK point of view but it is not real from 
>>> security_dentry_init_security() hook point of view. What's equivalent
>>> of that hook to support SMACK and multiple labels?
>> When multiple security modules support this hook they will
>> each get called. So where today security_dentry_init_security()
>> calls selinux_dentry_init_security(), in the future it will
>> also call any other <lsm>_dentry_init_security() hook that
>> is registered. No LSM infrastructure change required.
> I don't think security_dentry_init_security() can handle multiple
> security labels without change.
>
> int security_dentry_init_security(struct dentry *dentry, int mode,
>                                         const struct qstr *name, void **ctx,
>                                         u32 *ctxlen)
> {
>         return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>                                 name, ctx, ctxlen);
> }
>
> It can reutrn only one security context. So most likely you will have
> to add another hook to return multiple security context and slowly
> deprecate this one.

That hasn't been the approach to date. Have a look at the stacking patches
I've been posting to see how the "interface_lsm" is being used.

> IOW, as of today security_dentry_init_security() can't return multiple
> security labels. In fact it does not even tell the caller what's the
> name of the xattr. So caller has no idea if this security label came
> from SELinux or some other LSM. So for all practical purposes this
> is a hook for getting SELinux label and does not scale to support
> other LSMs.

Yup. This is a case, like yours, where the developer from SELinux
could have created a general interface but instead decided that
it wasn't worth the bother. As a result I have to fix it for the
general case. Well, SELinux/RedHat doesn't care about stacking,
so I guess that's the way it goes.


>>>>> Even if I do that, I am not sure what to do with those xattrs at
>>>>> the other end. I am using /proc/thread-self/attr/fscreate to
>>>>> set the security attribute of file.
>>>> Either you don't realize that attr/fscreate is SELinux specific, or
>>>> you don't care, or possibly (and sadly) both.
>>> I do realize that it is SELinux specific and that's why I have raised
>>> the concern that it does not work with SMACK.
>>>
>>> What's the "fscreate" equivalent for SMACK so that I file server can
>>> set it before creation of file and get correct context file?
>> The Smack attribute will be inherited from the creating process.
>> There is no way to generally change the attribute of a file on
>> creation. The appropriateness of such a facility has been debated
>> long and loud over the years. SELinux, which implements so varied
>> a set of "security" controls opted for it. Smack, which sticks much
>> more closely to an access control model, considers it too dangerous.
>> You can change the Smack label with setxattr(1) if you have
>> CAP_MAC_ADMIN.
> Ok, calling setxattr() after file creation will make the operation
> non-atomic. Will be good if it continues to be atomic.

That's a known downside. If you run the daemon with a Smack label that
is generally not accessible (easy to do) to the public you can do it
safely.


>> If you really want the file created with a particular
>> Smack label you can change the process Smack label by writing to
>> /proc/self/attr/smack/current on newer kernels and /proc/self/attr/current
>> on older ones.
> I guess /proc/thread-self/attr/smack/current is the way to go in this
> context, when one wants to support SMACK.

Label flipping is pretty dangerous. I prefer the run-with-safe-label,
call setxattr() approach. It's explicit.


>>>>> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
>>>>>
>>>>> How will this work with multiple labels. I think you will have to
>>>>> extend fscreate or create new interface to be able to deal with it.
>>>> Yeah. That thread didn't go to the LSM mail list. It was essentially
>>>> kept within the RedHat SELinux community. It's no wonder everyone
>>>> involved thought that your approach is swell. No one who would get
>>>> goobsmacked by it was on the thread.
>>> My goal is to support SELinux at this point of time. If you goal is
>>> to support SMACK, feel free to send patches on top to support that.
>> It helps to know what's going on before it becomes a major overhaul.
> Fair enough.
>
>>> I sent kernel patches to LSM list to make it plenty clear that this
>>> interface only supports single label which is SELinux. So there is
>>> no hiding here. And when I am supporting only SELinux, making use
>>> of fscreate makes perfect sense to me.
>> I bet it does.
>>
>>>>> That's why I think that it seems premature that fuse interface be
>>>>> written to deal with multiple labels when rest of the infrastructure
>>>>> is not ready. It should be other way, instead. First rest of the
>>>>> infrastructure should be written and then all the users make use
>>>>> of new infra.
>>>> Today the LSM infrastructure allows a security module to use as many
>>>> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.
>>> security_dentry_init_security() can handle that? If not, what's the
>>> equivalent.
>> Yes, it can.
> How? How will security_dentry_init_security() return multiple lables?
> It has parameters "u32 *ctxlen" and you can return only one. If you
> try to return multiple labels and return total length in "ctxlen",
> that does not help as you need to know length of individiual labels.
> So you need to know the names of xattrs too. Without that its not
> going to work.
>
> So no, security_dentry_init_security() can not handle multiple 
> security labels (and associated names). 

As I mentioned before, this is an example of why I don't want to
see yet another special-for-SELinux case.

>
> Vivek
>

Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation

[Casey Schaufler]

On 9/28/2021 5:49 AM, Vivek Goyal wrote:
> On Mon, Sep 27, 2021 at 02:45:13PM -0700, Casey Schaufler wrote:
> [..]
>>>>> I see that NFS and ceph are supporting single security label at
>>>>> the time of file creation and I added support for the same for
>>>>> fuse.
>>>> NFS took that course because the IETF refused for a very long time
>>>> to accept a name+value pair in the protocol. The implementation
>>>> was a compromise.
>>>>
>>>>> You seem to want to have capability to send multiple "name,value,len"
>>>>> tuples so that you can support multiple xattrs/labels down the
>>>>> line.
>>>> No, so I can do it now. Smack keeps multiple xattrs on filesystem objects.
>>>> 	security.SMACK64		- the "security label"
>>>> 	security.SMACK64EXEC		- the Smack label to run the program with
>>>> 	security.SMACK64TRANSMUTE	- controls labeling on files created
>>>>
>>>> There has been discussion about using additional attributes for things
>>>> like socket labeling.
>>>>
>>>> This isn't hypothetical. It's real today. 
>>> It is real from SMACK point of view but it is not real from 
>>> security_dentry_init_security() hook point of view. What's equivalent
>>> of that hook to support SMACK and multiple labels?
>> When multiple security modules support this hook they will
>> each get called. So where today security_dentry_init_security()
>> calls selinux_dentry_init_security(), in the future it will
>> also call any other <lsm>_dentry_init_security() hook that
>> is registered. No LSM infrastructure change required.
> I don't think security_dentry_init_security() can handle multiple
> security labels without change.
>
> int security_dentry_init_security(struct dentry *dentry, int mode,
>                                         const struct qstr *name, void **ctx,
>                                         u32 *ctxlen)
> {
>         return call_int_hook(dentry_init_security, -EOPNOTSUPP, dentry, mode,
>                                 name, ctx, ctxlen);
> }
>
> It can reutrn only one security context. So most likely you will have
> to add another hook to return multiple security context and slowly
> deprecate this one.

That hasn't been the approach to date. Have a look at the stacking patches
I've been posting to see how the "interface_lsm" is being used.

> IOW, as of today security_dentry_init_security() can't return multiple
> security labels. In fact it does not even tell the caller what's the
> name of the xattr. So caller has no idea if this security label came
> from SELinux or some other LSM. So for all practical purposes this
> is a hook for getting SELinux label and does not scale to support
> other LSMs.

Yup. This is a case, like yours, where the developer from SELinux
could have created a general interface but instead decided that
it wasn't worth the bother. As a result I have to fix it for the
general case. Well, SELinux/RedHat doesn't care about stacking,
so I guess that's the way it goes.


>>>>> Even if I do that, I am not sure what to do with those xattrs at
>>>>> the other end. I am using /proc/thread-self/attr/fscreate to
>>>>> set the security attribute of file.
>>>> Either you don't realize that attr/fscreate is SELinux specific, or
>>>> you don't care, or possibly (and sadly) both.
>>> I do realize that it is SELinux specific and that's why I have raised
>>> the concern that it does not work with SMACK.
>>>
>>> What's the "fscreate" equivalent for SMACK so that I file server can
>>> set it before creation of file and get correct context file?
>> The Smack attribute will be inherited from the creating process.
>> There is no way to generally change the attribute of a file on
>> creation. The appropriateness of such a facility has been debated
>> long and loud over the years. SELinux, which implements so varied
>> a set of "security" controls opted for it. Smack, which sticks much
>> more closely to an access control model, considers it too dangerous.
>> You can change the Smack label with setxattr(1) if you have
>> CAP_MAC_ADMIN.
> Ok, calling setxattr() after file creation will make the operation
> non-atomic. Will be good if it continues to be atomic.

That's a known downside. If you run the daemon with a Smack label that
is generally not accessible (easy to do) to the public you can do it
safely.


>> If you really want the file created with a particular
>> Smack label you can change the process Smack label by writing to
>> /proc/self/attr/smack/current on newer kernels and /proc/self/attr/current
>> on older ones.
> I guess /proc/thread-self/attr/smack/current is the way to go in this
> context, when one wants to support SMACK.

Label flipping is pretty dangerous. I prefer the run-with-safe-label,
call setxattr() approach. It's explicit.


>>>>> https://listman.redhat.com/archives/virtio-fs/2021-September/msg00100.html
>>>>>
>>>>> How will this work with multiple labels. I think you will have to
>>>>> extend fscreate or create new interface to be able to deal with it.
>>>> Yeah. That thread didn't go to the LSM mail list. It was essentially
>>>> kept within the RedHat SELinux community. It's no wonder everyone
>>>> involved thought that your approach is swell. No one who would get
>>>> goobsmacked by it was on the thread.
>>> My goal is to support SELinux at this point of time. If you goal is
>>> to support SMACK, feel free to send patches on top to support that.
>> It helps to know what's going on before it becomes a major overhaul.
> Fair enough.
>
>>> I sent kernel patches to LSM list to make it plenty clear that this
>>> interface only supports single label which is SELinux. So there is
>>> no hiding here. And when I am supporting only SELinux, making use
>>> of fscreate makes perfect sense to me.
>> I bet it does.
>>
>>>>> That's why I think that it seems premature that fuse interface be
>>>>> written to deal with multiple labels when rest of the infrastructure
>>>>> is not ready. It should be other way, instead. First rest of the
>>>>> infrastructure should be written and then all the users make use
>>>>> of new infra.
>>>> Today the LSM infrastructure allows a security module to use as many
>>>> xattrs as it likes. Again, Smack uses multiple security.* xattrs today.
>>> security_dentry_init_security() can handle that? If not, what's the
>>> equivalent.
>> Yes, it can.
> How? How will security_dentry_init_security() return multiple lables?
> It has parameters "u32 *ctxlen" and you can return only one. If you
> try to return multiple labels and return total length in "ctxlen",
> that does not help as you need to know length of individiual labels.
> So you need to know the names of xattrs too. Without that its not
> going to work.
>
> So no, security_dentry_init_security() can not handle multiple 
> security labels (and associated names). 

As I mentioned before, this is an example of why I don't want to
see yet another special-for-SELinux case.

>
> Vivek
>