Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
On 04/24/2018 11:22 AM, David Howells wrote: > Stephen Smalleywrote: > >> Neither fsopen() nor fscontext_fs_write() appear to perform any kind of >> up-front permission checking (DAC or MAC), although some security hooks may >> be ultimately called to allocate structures, parse security options, etc. >> Is there a reason not apply a may_mount() or similar check up front? > > may_mount() is called by fsmount() at the moment. It may make sense to move > this earlier to fsopen(). Note that there's also going to be something that > looks like: > > fd = fspick("/mnt"); > fsmount(fd, "/a", MNT_NOEXEC); // ie. bind mount > > or: > > fd = fspick("/mnt"); > write(fd, "o intr"); > write(fd, "x reconfigure"); // ie. something like remount > close(fd); > > I guess we'd want to call may_mount() in fspick() too. But there's also the > possibility of using this to create a query interfact too: > > fd = fspick("/mnt"); > write(fd, "q intr"); > read(fd, value_buffer); My concern was that fsopen()/fscontext_fs_write() may expose attack surface (e.g. mount option parsing code) that might not be normally accessible to unprivileged userspace (i.e. gated by may_mount() and security_sb_mount()) prior to your changes.
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
On 04/24/2018 11:22 AM, David Howells wrote: > Stephen Smalley wrote: > >> Neither fsopen() nor fscontext_fs_write() appear to perform any kind of >> up-front permission checking (DAC or MAC), although some security hooks may >> be ultimately called to allocate structures, parse security options, etc. >> Is there a reason not apply a may_mount() or similar check up front? > > may_mount() is called by fsmount() at the moment. It may make sense to move > this earlier to fsopen(). Note that there's also going to be something that > looks like: > > fd = fspick("/mnt"); > fsmount(fd, "/a", MNT_NOEXEC); // ie. bind mount > > or: > > fd = fspick("/mnt"); > write(fd, "o intr"); > write(fd, "x reconfigure"); // ie. something like remount > close(fd); > > I guess we'd want to call may_mount() in fspick() too. But there's also the > possibility of using this to create a query interfact too: > > fd = fspick("/mnt"); > write(fd, "q intr"); > read(fd, value_buffer); My concern was that fsopen()/fscontext_fs_write() may expose attack surface (e.g. mount option parsing code) that might not be normally accessible to unprivileged userspace (i.e. gated by may_mount() and security_sb_mount()) prior to your changes.
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
Stephen Smalleywrote: > Neither fsopen() nor fscontext_fs_write() appear to perform any kind of > up-front permission checking (DAC or MAC), although some security hooks may > be ultimately called to allocate structures, parse security options, etc. > Is there a reason not apply a may_mount() or similar check up front? may_mount() is called by fsmount() at the moment. It may make sense to move this earlier to fsopen(). Note that there's also going to be something that looks like: fd = fspick("/mnt"); fsmount(fd, "/a", MNT_NOEXEC); // ie. bind mount or: fd = fspick("/mnt"); write(fd, "o intr"); write(fd, "x reconfigure"); // ie. something like remount close(fd); I guess we'd want to call may_mount() in fspick() too. But there's also the possibility of using this to create a query interfact too: fd = fspick("/mnt"); write(fd, "q intr"); read(fd, value_buffer); David
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
Stephen Smalley wrote: > Neither fsopen() nor fscontext_fs_write() appear to perform any kind of > up-front permission checking (DAC or MAC), although some security hooks may > be ultimately called to allocate structures, parse security options, etc. > Is there a reason not apply a may_mount() or similar check up front? may_mount() is called by fsmount() at the moment. It may make sense to move this earlier to fsopen(). Note that there's also going to be something that looks like: fd = fspick("/mnt"); fsmount(fd, "/a", MNT_NOEXEC); // ie. bind mount or: fd = fspick("/mnt"); write(fd, "o intr"); write(fd, "x reconfigure"); // ie. something like remount close(fd); I guess we'd want to call may_mount() in fspick() too. But there's also the possibility of using this to create a query interfact too: fd = fspick("/mnt"); write(fd, "q intr"); read(fd, value_buffer); David
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
On 04/20/2018 11:35 AM, David Howells wrote: > Paul Moorewrote: > >> Adding the SELinux mailing list to the CC line; in the future please >> include the SELinux mailing list on patches like this. It would also >> be very helpful to include "selinux" somewhere in the subject line >> when the patch is predominately SELinux related (much like you did for >> the other LSMs in this patchset). > > I should probably evict the SELinux bits into their own patch since the point > of this patch is the LSM hooks, not specifically SELinux's implementation > thereof. > >> I can't say I've digested all of this yet, but what SELinux testing >> have you done with this patchset? > > Using the fsopen()/fsmount() syscalls, these hooks will be made use of, say > for NFS (which I haven't included in this list). Even sys_mount() will make > use of them a bit, so just booting the system does that. > > Note that for SELinux these hooks don't change very much except how the > parameters are handled. It doesn't actually change the checks that are made - > at least, not yet. There are some additional syscalls under consideration > (such as the ability to pick a live mounted filesystem into a context) that > might require additional permits. Neither fsopen() nor fscontext_fs_write() appear to perform any kind of up-front permission checking (DAC or MAC), although some security hooks may be ultimately called to allocate structures, parse security options, etc. Is there a reason not apply a may_mount() or similar check up front?
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
On 04/20/2018 11:35 AM, David Howells wrote: > Paul Moore wrote: > >> Adding the SELinux mailing list to the CC line; in the future please >> include the SELinux mailing list on patches like this. It would also >> be very helpful to include "selinux" somewhere in the subject line >> when the patch is predominately SELinux related (much like you did for >> the other LSMs in this patchset). > > I should probably evict the SELinux bits into their own patch since the point > of this patch is the LSM hooks, not specifically SELinux's implementation > thereof. > >> I can't say I've digested all of this yet, but what SELinux testing >> have you done with this patchset? > > Using the fsopen()/fsmount() syscalls, these hooks will be made use of, say > for NFS (which I haven't included in this list). Even sys_mount() will make > use of them a bit, so just booting the system does that. > > Note that for SELinux these hooks don't change very much except how the > parameters are handled. It doesn't actually change the checks that are made - > at least, not yet. There are some additional syscalls under consideration > (such as the ability to pick a live mounted filesystem into a context) that > might require additional permits. Neither fsopen() nor fscontext_fs_write() appear to perform any kind of up-front permission checking (DAC or MAC), although some security hooks may be ultimately called to allocate structures, parse security options, etc. Is there a reason not apply a may_mount() or similar check up front?
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
Paul Moorewrote: > Adding the SELinux mailing list to the CC line; in the future please > include the SELinux mailing list on patches like this. It would also > be very helpful to include "selinux" somewhere in the subject line > when the patch is predominately SELinux related (much like you did for > the other LSMs in this patchset). I should probably evict the SELinux bits into their own patch since the point of this patch is the LSM hooks, not specifically SELinux's implementation thereof. > I can't say I've digested all of this yet, but what SELinux testing > have you done with this patchset? Using the fsopen()/fsmount() syscalls, these hooks will be made use of, say for NFS (which I haven't included in this list). Even sys_mount() will make use of them a bit, so just booting the system does that. Note that for SELinux these hooks don't change very much except how the parameters are handled. It doesn't actually change the checks that are made - at least, not yet. There are some additional syscalls under consideration (such as the ability to pick a live mounted filesystem into a context) that might require additional permits. David
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
Paul Moore wrote: > Adding the SELinux mailing list to the CC line; in the future please > include the SELinux mailing list on patches like this. It would also > be very helpful to include "selinux" somewhere in the subject line > when the patch is predominately SELinux related (much like you did for > the other LSMs in this patchset). I should probably evict the SELinux bits into their own patch since the point of this patch is the LSM hooks, not specifically SELinux's implementation thereof. > I can't say I've digested all of this yet, but what SELinux testing > have you done with this patchset? Using the fsopen()/fsmount() syscalls, these hooks will be made use of, say for NFS (which I haven't included in this list). Even sys_mount() will make use of them a bit, so just booting the system does that. Note that for SELinux these hooks don't change very much except how the parameters are handled. It doesn't actually change the checks that are made - at least, not yet. There are some additional syscalls under consideration (such as the ability to pick a live mounted filesystem into a context) that might require additional permits. David
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
On Thu, Apr 19, 2018 at 9:31 AM, David Howellswrote: > Add LSM hooks for use by the filesystem context code. This includes: > > (1) Hooks to handle allocation, duplication and freeing of the security > record attached to a filesystem context. > > (2) A hook to snoop a mount options in key[=val] form. If the LSM decides > it wants to handle it, it can suppress the option being passed to the > filesystem. Note that 'val' may include commas and binary data with > the fsopen patch. > > (3) A hook to transfer the security from the context to a newly created > superblock. > > (4) A hook to rule on whether a path point can be used as a mountpoint. > > These are intended to replace: > > security_sb_copy_data > security_sb_kern_mount > security_sb_mount > security_sb_set_mnt_opts > security_sb_clone_mnt_opts > security_sb_parse_opts_str > > Signed-off-by: David Howells > cc: linux-security-mod...@vger.kernel.org > --- > > include/linux/lsm_hooks.h | 62 +++ > include/linux/security.h | 44 > security/security.c | 41 +++ > security/selinux/hooks.c | 262 > + > 4 files changed, 409 insertions(+) Adding the SELinux mailing list to the CC line; in the future please include the SELinux mailing list on patches like this. It would also be very helpful to include "selinux" somewhere in the subject line when the patch is predominately SELinux related (much like you did for the other LSMs in this patchset). I can't say I've digested all of this yet, but what SELinux testing have you done with this patchset? > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 9d0b286f3dba..da20f90d40bb 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -76,6 +76,50 @@ > * changes on the process such as clearing out non-inheritable signal > * state. This is called immediately after commit_creds(). > * > + * Security hooks for mount using fd context. > + * > + * @fs_context_alloc: > + * Allocate and attach a security structure to sc->security. This > pointer > + * is initialised to NULL by the caller. > + * @fc indicates the new filesystem context. > + * @src_sb indicates the source superblock of a submount. > + * @fs_context_dup: > + * Allocate and attach a security structure to sc->security. This > pointer > + * is initialised to NULL by the caller. > + * @fc indicates the new filesystem context. > + * @src_fc indicates the original filesystem context. > + * @fs_context_free: > + * Clean up a filesystem context. > + * @fc indicates the filesystem context. > + * @fs_context_parse_option: > + * Userspace provided an option to configure a superblock. The LSM may > + * reject it with an error and may use it for itself, in which case it > + * should return 1; otherwise it should return 0 to pass it on to the > + * filesystem. > + * @fc indicates the filesystem context. > + * @opt indicates the option in "key[=val]" form. It is NUL-terminated, > + * but val may be binary data. > + * @len indicates the size of the option. > + * @fs_context_validate: > + * Validate the filesystem context preparatory to applying it. This is > + * done after all the options have been parsed. > + * @fc indicates the filesystem context. > + * @sb_get_tree: > + * Assign the security to a newly created superblock. > + * @fc indicates the filesystem context. > + * @fc->root indicates the root that will be mounted. > + * @fc->root->d_sb points to the superblock. > + * @sb_reconfigure: > + * Apply reconfiguration to the security on a superblock. > + * @fc indicates the filesystem context. > + * @fc->root indicates a dentry in the mount. > + * @fc->root->d_sb points to the superblock. > + * @sb_mountpoint: > + * Equivalent of sb_mount, but with an fs_context. > + * @fc indicates the filesystem context. > + * @mountpoint indicates the path on which the mount will take place. > + * @mnt_flags indicates the MNT_* flags specified. > + * > * Security hooks for filesystem operations. > * > * @sb_alloc_security: > @@ -1450,6 +1494,16 @@ union security_list_options { > void (*bprm_committing_creds)(struct linux_binprm *bprm); > void (*bprm_committed_creds)(struct linux_binprm *bprm); > > + int (*fs_context_alloc)(struct fs_context *fc, struct super_block > *src_sb); > + int (*fs_context_dup)(struct fs_context *fc, struct fs_context > *src_sc); > + void (*fs_context_free)(struct fs_context *fc); > + int (*fs_context_parse_option)(struct fs_context *fc, char *opt, > size_t len); > + int (*fs_context_validate)(struct fs_context *fc); > + int (*sb_get_tree)(struct fs_context *fc); > + void
Re: [PATCH 04/24] VFS: Add LSM hooks for filesystem context [ver #7]
On Thu, Apr 19, 2018 at 9:31 AM, David Howells wrote: > Add LSM hooks for use by the filesystem context code. This includes: > > (1) Hooks to handle allocation, duplication and freeing of the security > record attached to a filesystem context. > > (2) A hook to snoop a mount options in key[=val] form. If the LSM decides > it wants to handle it, it can suppress the option being passed to the > filesystem. Note that 'val' may include commas and binary data with > the fsopen patch. > > (3) A hook to transfer the security from the context to a newly created > superblock. > > (4) A hook to rule on whether a path point can be used as a mountpoint. > > These are intended to replace: > > security_sb_copy_data > security_sb_kern_mount > security_sb_mount > security_sb_set_mnt_opts > security_sb_clone_mnt_opts > security_sb_parse_opts_str > > Signed-off-by: David Howells > cc: linux-security-mod...@vger.kernel.org > --- > > include/linux/lsm_hooks.h | 62 +++ > include/linux/security.h | 44 > security/security.c | 41 +++ > security/selinux/hooks.c | 262 > + > 4 files changed, 409 insertions(+) Adding the SELinux mailing list to the CC line; in the future please include the SELinux mailing list on patches like this. It would also be very helpful to include "selinux" somewhere in the subject line when the patch is predominately SELinux related (much like you did for the other LSMs in this patchset). I can't say I've digested all of this yet, but what SELinux testing have you done with this patchset? > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 9d0b286f3dba..da20f90d40bb 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -76,6 +76,50 @@ > * changes on the process such as clearing out non-inheritable signal > * state. This is called immediately after commit_creds(). > * > + * Security hooks for mount using fd context. > + * > + * @fs_context_alloc: > + * Allocate and attach a security structure to sc->security. This > pointer > + * is initialised to NULL by the caller. > + * @fc indicates the new filesystem context. > + * @src_sb indicates the source superblock of a submount. > + * @fs_context_dup: > + * Allocate and attach a security structure to sc->security. This > pointer > + * is initialised to NULL by the caller. > + * @fc indicates the new filesystem context. > + * @src_fc indicates the original filesystem context. > + * @fs_context_free: > + * Clean up a filesystem context. > + * @fc indicates the filesystem context. > + * @fs_context_parse_option: > + * Userspace provided an option to configure a superblock. The LSM may > + * reject it with an error and may use it for itself, in which case it > + * should return 1; otherwise it should return 0 to pass it on to the > + * filesystem. > + * @fc indicates the filesystem context. > + * @opt indicates the option in "key[=val]" form. It is NUL-terminated, > + * but val may be binary data. > + * @len indicates the size of the option. > + * @fs_context_validate: > + * Validate the filesystem context preparatory to applying it. This is > + * done after all the options have been parsed. > + * @fc indicates the filesystem context. > + * @sb_get_tree: > + * Assign the security to a newly created superblock. > + * @fc indicates the filesystem context. > + * @fc->root indicates the root that will be mounted. > + * @fc->root->d_sb points to the superblock. > + * @sb_reconfigure: > + * Apply reconfiguration to the security on a superblock. > + * @fc indicates the filesystem context. > + * @fc->root indicates a dentry in the mount. > + * @fc->root->d_sb points to the superblock. > + * @sb_mountpoint: > + * Equivalent of sb_mount, but with an fs_context. > + * @fc indicates the filesystem context. > + * @mountpoint indicates the path on which the mount will take place. > + * @mnt_flags indicates the MNT_* flags specified. > + * > * Security hooks for filesystem operations. > * > * @sb_alloc_security: > @@ -1450,6 +1494,16 @@ union security_list_options { > void (*bprm_committing_creds)(struct linux_binprm *bprm); > void (*bprm_committed_creds)(struct linux_binprm *bprm); > > + int (*fs_context_alloc)(struct fs_context *fc, struct super_block > *src_sb); > + int (*fs_context_dup)(struct fs_context *fc, struct fs_context > *src_sc); > + void (*fs_context_free)(struct fs_context *fc); > + int (*fs_context_parse_option)(struct fs_context *fc, char *opt, > size_t len); > + int (*fs_context_validate)(struct fs_context *fc); > + int (*sb_get_tree)(struct fs_context *fc); > + void (*sb_reconfigure)(struct fs_context *fc); > + int