* [PATCH 0/2] fuse: Send file/inode security context during creation @ 2021-09-24 19:24 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 19:24 UTC (permalink / raw) To: linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: chirantan, vgoyal, miklos, stephen.smalley.work, dwalsh 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [Virtio-fs] [PATCH 0/2] fuse: Send file/inode security context during creation @ 2021-09-24 19:24 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 19:24 UTC (permalink / raw) To: linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: stephen.smalley.work, vgoyal, miklos 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] fuse: Add a flag FUSE_SECURITY_CTX 2021-09-24 19:24 ` [Virtio-fs] " Vivek Goyal @ 2021-09-24 19:24 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 19:24 UTC (permalink / raw) To: linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: chirantan, vgoyal, miklos, stephen.smalley.work, dwalsh 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Virtio-fs] [PATCH 1/2] fuse: Add a flag FUSE_SECURITY_CTX @ 2021-09-24 19:24 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 19:24 UTC (permalink / raw) To: linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: stephen.smalley.work, vgoyal, miklos 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 19:24 ` [Virtio-fs] " Vivek Goyal @ 2021-09-24 19:24 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 19:24 UTC (permalink / raw) To: linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: chirantan, vgoyal, miklos, stephen.smalley.work, dwalsh 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 19:24 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 19:24 UTC (permalink / raw) To: linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: stephen.smalley.work, vgoyal, miklos 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 19:24 ` [Virtio-fs] " Vivek Goyal @ 2021-09-24 19:58 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-24 19:58 UTC (permalink / raw) To: Vivek Goyal, linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: chirantan, miklos, stephen.smalley.work, dwalsh, 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 */ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 19:58 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-24 19:58 UTC (permalink / raw) To: Vivek Goyal, linux-fsdevel, virtio-fs, selinux, linux-security-module Cc: Casey Schaufler, stephen.smalley.work, miklos 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 */ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 19:58 ` [Virtio-fs] " Casey Schaufler @ 2021-09-24 20:18 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 20:18 UTC (permalink / raw) To: Casey Schaufler Cc: linux-fsdevel, virtio-fs, selinux, linux-security-module, chirantan, miklos, stephen.smalley.work, dwalsh 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 */ > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 20:18 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 20:18 UTC (permalink / raw) To: Casey Schaufler Cc: miklos, selinux, stephen.smalley.work, virtio-fs, linux-security-module, linux-fsdevel 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 */ > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 20:18 ` [Virtio-fs] " Vivek Goyal @ 2021-09-24 20:54 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-24 20:54 UTC (permalink / raw) To: Vivek Goyal Cc: linux-fsdevel, virtio-fs, selinux, linux-security-module, chirantan, miklos, stephen.smalley.work, dwalsh, 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 */ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 20:54 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-24 20:54 UTC (permalink / raw) To: Vivek Goyal Cc: miklos, selinux, Casey Schaufler, stephen.smalley.work, virtio-fs, linux-security-module, linux-fsdevel 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 */ ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 20:54 ` [Virtio-fs] " Casey Schaufler @ 2021-09-24 21:16 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 21:16 UTC (permalink / raw) To: Casey Schaufler Cc: linux-fsdevel, virtio-fs, selinux, linux-security-module, chirantan, miklos, stephen.smalley.work, dwalsh 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 21:16 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 21:16 UTC (permalink / raw) To: Casey Schaufler Cc: miklos, selinux, stephen.smalley.work, virtio-fs, linux-security-module, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 21:16 ` [Virtio-fs] " Vivek Goyal @ 2021-09-24 21:55 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-24 21:55 UTC (permalink / raw) To: Vivek Goyal Cc: linux-fsdevel, virtio-fs, selinux, linux-security-module, chirantan, miklos, stephen.smalley.work, dwalsh, 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 21:55 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-24 21:55 UTC (permalink / raw) To: Vivek Goyal Cc: miklos, selinux, Casey Schaufler, stephen.smalley.work, virtio-fs, linux-security-module, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 19:24 ` [Virtio-fs] " Vivek Goyal @ 2021-09-24 22:00 ` Colin Walters -1 siblings, 0 replies; 42+ messages in thread From: Colin Walters @ 2021-09-24 22:00 UTC (permalink / raw) To: Vivek Goyal, linux-fsdevel, virtio-fs, selinux, LSM List Cc: chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh 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) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 22:00 ` Colin Walters 0 siblings, 0 replies; 42+ messages in thread From: Colin Walters @ 2021-09-24 22:00 UTC (permalink / raw) To: Vivek Goyal, linux-fsdevel, virtio-fs, selinux, LSM List Cc: stephen.smalley.work, Miklos Szeredi 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) ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 22:00 ` [Virtio-fs] " Colin Walters @ 2021-09-24 23:32 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 23:32 UTC (permalink / raw) To: Colin Walters Cc: linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-24 23:32 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-24 23:32 UTC (permalink / raw) To: Colin Walters Cc: Miklos Szeredi, selinux, stephen.smalley.work, virtio-fs, LSM List, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-24 23:32 ` [Virtio-fs] " Vivek Goyal @ 2021-09-27 0:53 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 0:53 UTC (permalink / raw) To: Vivek Goyal, Colin Walters Cc: linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh, 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 0:53 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 0:53 UTC (permalink / raw) To: Vivek Goyal, Colin Walters Cc: Miklos Szeredi, selinux, Casey Schaufler, stephen.smalley.work, virtio-fs, LSM List, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 0:53 ` [Virtio-fs] " Casey Schaufler @ 2021-09-27 14:05 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 14:05 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 14:05 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 14:05 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 14:05 ` [Virtio-fs] " Vivek Goyal @ 2021-09-27 15:22 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 15:22 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh, 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 15:22 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 15:22 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, Casey Schaufler, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 15:22 ` [Virtio-fs] " Casey Schaufler @ 2021-09-27 15:56 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 15:56 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh 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 > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 15:56 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 15:56 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 15:56 ` [Virtio-fs] " Vivek Goyal @ 2021-09-27 17:56 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 17:56 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh, 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 >>> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 17:56 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 17:56 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, Casey Schaufler, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 >>> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 17:56 ` [Virtio-fs] " Casey Schaufler @ 2021-09-27 19:20 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 19:20 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh 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 > >>> > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 19:20 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 19:20 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 > >>> > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 19:20 ` [Virtio-fs] " Vivek Goyal @ 2021-09-27 20:19 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 20:19 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh, 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 >>>>> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 20:19 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 20:19 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, Casey Schaufler, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 >>>>> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 20:19 ` [Virtio-fs] " Casey Schaufler @ 2021-09-27 20:45 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 20:45 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 20:45 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-27 20:45 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 20:45 ` [Virtio-fs] " Vivek Goyal @ 2021-09-27 21:45 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 21:45 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh, 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-27 21:45 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-27 21:45 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, Casey Schaufler, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-27 21:45 ` [Virtio-fs] " Casey Schaufler @ 2021-09-28 12:49 ` Vivek Goyal -1 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-28 12:49 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-28 12:49 ` Vivek Goyal 0 siblings, 0 replies; 42+ messages in thread From: Vivek Goyal @ 2021-09-28 12:49 UTC (permalink / raw) To: Casey Schaufler Cc: Colin Walters, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] fuse: Send security context of inode on file creation 2021-09-28 12:49 ` [Virtio-fs] " Vivek Goyal @ 2021-09-28 14:25 ` Casey Schaufler -1 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-28 14:25 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, linux-fsdevel, virtio-fs, selinux, LSM List, chirantan, Miklos Szeredi, stephen.smalley.work, Daniel J Walsh, 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Virtio-fs] [PATCH 2/2] fuse: Send security context of inode on file creation @ 2021-09-28 14:25 ` Casey Schaufler 0 siblings, 0 replies; 42+ messages in thread From: Casey Schaufler @ 2021-09-28 14:25 UTC (permalink / raw) To: Vivek Goyal Cc: Colin Walters, Casey Schaufler, stephen.smalley.work, Miklos Szeredi, virtio-fs, selinux, LSM List, linux-fsdevel 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 > ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2021-09-28 14:25 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-24 19:24 [PATCH 0/2] fuse: Send file/inode security context during creation Vivek Goyal 2021-09-24 19:24 ` [Virtio-fs] " Vivek Goyal 2021-09-24 19:24 ` [PATCH 1/2] fuse: Add a flag FUSE_SECURITY_CTX Vivek Goyal 2021-09-24 19:24 ` [Virtio-fs] " Vivek Goyal 2021-09-24 19:24 ` [PATCH 2/2] fuse: Send security context of inode on file creation Vivek Goyal 2021-09-24 19:24 ` [Virtio-fs] " Vivek Goyal 2021-09-24 19:58 ` Casey Schaufler 2021-09-24 19:58 ` [Virtio-fs] " Casey Schaufler 2021-09-24 20:18 ` Vivek Goyal 2021-09-24 20:18 ` [Virtio-fs] " Vivek Goyal 2021-09-24 20:54 ` Casey Schaufler 2021-09-24 20:54 ` [Virtio-fs] " Casey Schaufler 2021-09-24 21:16 ` Vivek Goyal 2021-09-24 21:16 ` [Virtio-fs] " Vivek Goyal 2021-09-24 21:55 ` Casey Schaufler 2021-09-24 21:55 ` [Virtio-fs] " Casey Schaufler 2021-09-24 22:00 ` Colin Walters 2021-09-24 22:00 ` [Virtio-fs] " Colin Walters 2021-09-24 23:32 ` Vivek Goyal 2021-09-24 23:32 ` [Virtio-fs] " Vivek Goyal 2021-09-27 0:53 ` Casey Schaufler 2021-09-27 0:53 ` [Virtio-fs] " Casey Schaufler 2021-09-27 14:05 ` Vivek Goyal 2021-09-27 14:05 ` [Virtio-fs] " Vivek Goyal 2021-09-27 15:22 ` Casey Schaufler 2021-09-27 15:22 ` [Virtio-fs] " Casey Schaufler 2021-09-27 15:56 ` Vivek Goyal 2021-09-27 15:56 ` [Virtio-fs] " Vivek Goyal 2021-09-27 17:56 ` Casey Schaufler 2021-09-27 17:56 ` [Virtio-fs] " Casey Schaufler 2021-09-27 19:20 ` Vivek Goyal 2021-09-27 19:20 ` [Virtio-fs] " Vivek Goyal 2021-09-27 20:19 ` Casey Schaufler 2021-09-27 20:19 ` [Virtio-fs] " Casey Schaufler 2021-09-27 20:45 ` Vivek Goyal 2021-09-27 20:45 ` [Virtio-fs] " Vivek Goyal 2021-09-27 21:45 ` Casey Schaufler 2021-09-27 21:45 ` [Virtio-fs] " Casey Schaufler 2021-09-28 12:49 ` Vivek Goyal 2021-09-28 12:49 ` [Virtio-fs] " Vivek Goyal 2021-09-28 14:25 ` Casey Schaufler 2021-09-28 14:25 ` [Virtio-fs] " Casey Schaufler
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.