| Message ID | 20190718143042.11059-1-acgoide@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v2] fanotify, inotify, dnotify, security: add security hook for fs notifications | expand |
On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote: > > As of now, setting watches on filesystem objects has, at most, applied a > check for read access to the inode, and in the case of fanotify, requires > CAP_SYS_ADMIN. No specific security hook or permission check has been > provided to control the setting of watches. Using any of inotify, dnotify, > or fanotify, it is possible to observe, not only write-like operations, but > even read access to a file. Modeling the watch as being merely a read from > the file is insufficient for the needs of SELinux. This is due to the fact > that read access should not necessarily imply access to information about > when another process reads from a file. Furthermore, fanotify watches grant > more power to an application in the form of permission events. While > notification events are solely, unidirectional (i.e. they only pass > information to the receiving application), permission events are blocking. > Permission events make a request to the receiving application which will > then reply with a decision as to whether or not that action may be > completed. This causes the issue of the watching application having the > ability to exercise control over the triggering process. Without drawing a > distinction within the permission check, the ability to read would imply > the greater ability to control an application. Additionally, mount and > superblock watches apply to all files within the same mount or superblock. > Read access to one file should not necessarily imply the ability to watch > all files accessed within a given mount or superblock. > > In order to solve these issues, a new LSM hook is implemented and has been > placed within the system calls for marking filesystem objects with inotify, > fanotify, and dnotify watches. These calls to the hook are placed at the > point at which the target inode has been resolved and are provided with the > inode, the mask of requested notification events, and the type of object on > which the mark is being set (inode, superblock, or mount). The mask and > mark_type have already been translated into common FS_* values shared by > the entirety of the fs notification infrastructure. > > This only provides a hook at the point of setting a watch, and presumes > that permission to set a particular watch implies the ability to receive > all notification about that object which match the mask. This is all that > is required for SELinux. If other security modules require additional hooks > or infrastructure to control delivery of notification, these can be added > by them. It does not make sense for us to propose hooks for which we have > no implementation. The understanding that all notifications received by the > requesting application are all strictly of a type for which the application > has been granted permission shows that this implementation is sufficient in > its coverage. > > Security modules wishing to provide complete control over fanotify must > also implement a security_file_open hook that validates that the access > requested by the watching application is authorized. Fanotify has the issue > that it returns a file descriptor with the file mode specified during > fanotify_init() to the watching process on event. This is already covered > by the LSM security_file_open hook if the security module implements > checking of the requested file mode there. Otherwise, a watching process > can obtain escalated access to a file for which it has not been authorized. > > The selinux_inode_notify hook implementation works by adding five new file > permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm > (descriptions about which will follow), and one new filesystem permission: > watch (which is applied to superblock checks). The hook then decides which > subset of these permissions must be held by the requesting application > based on the contents of the provided mask and the mark_type. The > selinux_file_open hook already checks the requested file mode and therefore > ensures that a watching process cannot escalate its access through > fanotify. > > The watch, watch_mount, and watch_sb permissions are the baseline > permissions for setting a watch on an object and each are a requirement for > any watch to be set on a file, mount, or superblock respectively. It should > be noted that having either of the other two permissions (watch_reads and > watch_with_perm) does not imply the watch, watch_mount, or watch_sb > permission. Superblock watches further require the filesystem watch > permission to the superblock. As there is no labeled object in view for > mounts, there is no specific check for mount watches beyond watch_mount to > the inode. Such a check could be added in the future, if a suitable labeled > object existed representing the mount. > > The watch_reads permission is required to receive notifications from > read-exclusive events on filesystem objects. These events include accessing > a file for the purpose of reading and closing a file which has been opened > read-only. This distinction has been drawn in order to provide a direct > indication in the policy for this otherwise not obvious capability. Read > access to a file should not necessarily imply the ability to observe read > events on a file. > > Finally, watch_with_perm only applies to fanotify masks since it is the > only way to set a mask which allows for the blocking, permission event. > This permission is needed for any watch which is of this type. Though > fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit > trust to root, which we do not do, and does not support least privilege. > > Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov> > --- > v2: > - Adds support for mark_type > - Adds watch_sb and watch_mount file permissions > - Adds watch as new filesystem permission > - LSM hook now recieves mark_type argument > - Changed LSM hook logic to implement checks for corresponding mark_types > - Adds missing hook description comment > - Fixes extrainous whitespace > - Updates patch description based on feedback > > fs/notify/dnotify/dnotify.c | 14 +++++++-- > fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++-- > fs/notify/inotify/inotify_user.c | 13 ++++++-- > include/linux/lsm_hooks.h | 6 ++++ > include/linux/security.h | 9 +++++- > security/security.c | 5 +++ > security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++ > security/selinux/include/classmap.h | 5 +-- > 8 files changed, 116 insertions(+), 10 deletions(-) > > diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c > index 250369d6901d..4690d8a66035 100644 > --- a/fs/notify/dnotify/dnotify.c > +++ b/fs/notify/dnotify/dnotify.c > @@ -22,6 +22,7 @@ > #include <linux/sched/signal.h> > #include <linux/dnotify.h> > #include <linux/init.h> > +#include <linux/security.h> > #include <linux/spinlock.h> > #include <linux/slab.h> > #include <linux/fdtable.h> > @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) > goto out_err; > } > > + /* > + * convert the userspace DN_* "arg" to the internal FS_* > + * defined in fsnotify > + */ > + mask = convert_arg(arg); > + > + error = security_inode_notify(inode, mask, FSNOTIFY_OBJ_TYPE_INODE); > + if (error) > + goto out_err; > + > /* expect most fcntl to add new rather than augment old */ > dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL); > if (!dn) { > @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) > goto out_err; > } > > - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */ > - mask = convert_arg(arg); > - > /* set up the new_fsn_mark and new_dn_mark */ > new_fsn_mark = &new_dn_mark->fsn_mark; > fsnotify_init_mark(new_fsn_mark, dnotify_group); > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index a90bb19dcfa2..9e3137badb6b 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = { > }; > > static int fanotify_find_path(int dfd, const char __user *filename, > - struct path *path, unsigned int flags) > + struct path *path, unsigned int flags, __u64 mask) > { > int ret; > + unsigned int mark_type; > > pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__, > dfd, filename, flags); > @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename, > > /* you can only watch an inode if you have read permissions on it */ > ret = inode_permission(path->dentry->d_inode, MAY_READ); > + if (ret) { > + path_put(path); > + goto out; > + } > + > + switch (flags & FANOTIFY_MARK_TYPE_BITS) { > + case FAN_MARK_MOUNT: > + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT; > + break; > + case FAN_MARK_FILESYSTEM: > + mark_type = FSNOTIFY_OBJ_TYPE_SB; > + break; > + case FAN_MARK_INODE: > + mark_type = FSNOTIFY_OBJ_TYPE_INODE; > + break; > + default: > + ret = -EINVAL; > + goto out; > + } > + > + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type); If you prefer 3 hooks security_{inode,mount,sb}_notify() please place them in fanotify_add_{inode,mount,sb}_mark(). If you prefer single hook with path argument, please pass path down to fanotify_add_mark() and call security_path_notify() from there, where you already have the object type argument. > if (ret) > path_put(path); > + > out: > return ret; > } > @@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, > goto fput_and_out; > } > > - ret = fanotify_find_path(dfd, pathname, &path, flags); > + ret = fanotify_find_path(dfd, pathname, &path, flags, mask); > if (ret) > goto fput_and_out; > > diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c > index 7b53598c8804..73b321a30bbc 100644 > --- a/fs/notify/inotify/inotify_user.c > +++ b/fs/notify/inotify/inotify_user.c > @@ -39,6 +39,7 @@ > #include <linux/poll.h> > #include <linux/wait.h> > #include <linux/memcontrol.h> > +#include <linux/security.h> > > #include "inotify.h" > #include "../fdinfo.h" > @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = { > /* > * find_inode - resolve a user-given path to a specific inode > */ > -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags) > +static int inotify_find_inode(const char __user *dirname, struct path *path, > + unsigned int flags, __u64 mask) > { > int error; > > @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns > return error; > /* you can only watch an inode if you have read permissions on it */ > error = inode_permission(path->dentry->d_inode, MAY_READ); > + if (error) { > + path_put(path); > + return error; > + } > + error = security_inode_notify(path->dentry->d_inode, mask, > + FSNOTIFY_OBJ_TYPE_INODE); > if (error) > path_put(path); > + > return error; > } > > @@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname, > if (mask & IN_ONLYDIR) > flags |= LOOKUP_DIRECTORY; > > - ret = inotify_find_inode(pathname, &path, flags); > + ret = inotify_find_inode(pathname, &path, flags, mask); > if (ret) > goto fput_and_out; > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 47f58cfb6a19..9b3f5a5f3246 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -394,6 +394,9 @@ > * Check permission before removing the extended attribute > * identified by @name for @dentry. > * Return 0 if permission is granted. > + * @inode_notify: > + * Check permissions before setting a watch on events as defined by @mask, > + * on an object @inode, whose type is defined by @mark_type. This is misleading IMO. First of all "mark_type" is already use to describe the FAN_MARK_XXX flag value, so please choose another name. object_type if you wish. Secondly, it does not make sense to pass an inode object when enforcing a mount mark, because there is no reference from inode to mount. You should either pass in @path argument to the hook or have different hooks for different object types. What about adding watches by kernel/audit_fsnotify.c? Do LSMs police other LSMs??? Thanks, Amir.
On 7/18/2019 7:30 AM, Aaron Goidel wrote: > As of now, setting watches on filesystem objects has, at most, applied a > check for read access to the inode, and in the case of fanotify, requires > CAP_SYS_ADMIN. No specific security hook or permission check has been > provided to control the setting of watches. Using any of inotify, dnotify, > or fanotify, it is possible to observe, not only write-like operations, but > even read access to a file. Modeling the watch as being merely a read from > the file is insufficient for the needs of SELinux. This is due to the fact > that read access should not necessarily imply access to information about > when another process reads from a file. Furthermore, fanotify watches grant > more power to an application in the form of permission events. While > notification events are solely, unidirectional (i.e. they only pass > information to the receiving application), permission events are blocking. > Permission events make a request to the receiving application which will > then reply with a decision as to whether or not that action may be > completed. This causes the issue of the watching application having the > ability to exercise control over the triggering process. Without drawing a > distinction within the permission check, the ability to read would imply > the greater ability to control an application. Additionally, mount and > superblock watches apply to all files within the same mount or superblock. > Read access to one file should not necessarily imply the ability to watch > all files accessed within a given mount or superblock. > > In order to solve these issues, a new LSM hook is implemented and has been > placed within the system calls for marking filesystem objects with inotify, > fanotify, and dnotify watches. These calls to the hook are placed at the > point at which the target inode has been resolved and are provided with the > inode, the mask of requested notification events, and the type of object on > which the mark is being set (inode, superblock, or mount). The mask and > mark_type have already been translated into common FS_* values shared by > the entirety of the fs notification infrastructure. > > This only provides a hook at the point of setting a watch, and presumes > that permission to set a particular watch implies the ability to receive > all notification about that object which match the mask. This is all that > is required for SELinux. If other security modules require additional hooks > or infrastructure to control delivery of notification, these can be added > by them. It does not make sense for us to propose hooks for which we have > no implementation. The understanding that all notifications received by the > requesting application are all strictly of a type for which the application > has been granted permission shows that this implementation is sufficient in > its coverage. > > Security modules wishing to provide complete control over fanotify must > also implement a security_file_open hook that validates that the access > requested by the watching application is authorized. Fanotify has the issue > that it returns a file descriptor with the file mode specified during > fanotify_init() to the watching process on event. This is already covered > by the LSM security_file_open hook if the security module implements > checking of the requested file mode there. Otherwise, a watching process > can obtain escalated access to a file for which it has not been authorized. > > The selinux_inode_notify hook implementation works by adding five new file > permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm > (descriptions about which will follow), and one new filesystem permission: > watch (which is applied to superblock checks). The hook then decides which > subset of these permissions must be held by the requesting application > based on the contents of the provided mask and the mark_type. The > selinux_file_open hook already checks the requested file mode and therefore > ensures that a watching process cannot escalate its access through > fanotify. > > The watch, watch_mount, and watch_sb permissions are the baseline > permissions for setting a watch on an object and each are a requirement for > any watch to be set on a file, mount, or superblock respectively. It should > be noted that having either of the other two permissions (watch_reads and > watch_with_perm) does not imply the watch, watch_mount, or watch_sb > permission. Superblock watches further require the filesystem watch > permission to the superblock. As there is no labeled object in view for > mounts, there is no specific check for mount watches beyond watch_mount to > the inode. Such a check could be added in the future, if a suitable labeled > object existed representing the mount. > > The watch_reads permission is required to receive notifications from > read-exclusive events on filesystem objects. These events include accessing > a file for the purpose of reading and closing a file which has been opened > read-only. This distinction has been drawn in order to provide a direct > indication in the policy for this otherwise not obvious capability. Read > access to a file should not necessarily imply the ability to observe read > events on a file. > > Finally, watch_with_perm only applies to fanotify masks since it is the > only way to set a mask which allows for the blocking, permission event. > This permission is needed for any watch which is of this type. Though > fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit > trust to root, which we do not do, and does not support least privilege. It looks as if there may be overlap between this work and [RFC PATCH] security, capability: pass object information to security_capable https://www.spinics.net/lists/selinux/msg28312.html I say this because your patch looks an awful lot like what I suggested as the alternative to passing object information to security_capable(). It's possible that I've muddled the discussions in my brain, and that there isn't any way to use either to do both jobs. But it would be worth a look, and a single new hook or change to existing hook would be a Good Thing. > > Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov> > --- > v2: > - Adds support for mark_type > - Adds watch_sb and watch_mount file permissions > - Adds watch as new filesystem permission > - LSM hook now recieves mark_type argument > - Changed LSM hook logic to implement checks for corresponding mark_types > - Adds missing hook description comment > - Fixes extrainous whitespace > - Updates patch description based on feedback > > fs/notify/dnotify/dnotify.c | 14 +++++++-- > fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++-- > fs/notify/inotify/inotify_user.c | 13 ++++++-- > include/linux/lsm_hooks.h | 6 ++++ > include/linux/security.h | 9 +++++- > security/security.c | 5 +++ > security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++ > security/selinux/include/classmap.h | 5 +-- > 8 files changed, 116 insertions(+), 10 deletions(-) > > diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c > index 250369d6901d..4690d8a66035 100644 > --- a/fs/notify/dnotify/dnotify.c > +++ b/fs/notify/dnotify/dnotify.c > @@ -22,6 +22,7 @@ > #include <linux/sched/signal.h> > #include <linux/dnotify.h> > #include <linux/init.h> > +#include <linux/security.h> > #include <linux/spinlock.h> > #include <linux/slab.h> > #include <linux/fdtable.h> > @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) > goto out_err; > } > > + /* > + * convert the userspace DN_* "arg" to the internal FS_* > + * defined in fsnotify > + */ > + mask = convert_arg(arg); > + > + error = security_inode_notify(inode, mask, FSNOTIFY_OBJ_TYPE_INODE); > + if (error) > + goto out_err; > + > /* expect most fcntl to add new rather than augment old */ > dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL); > if (!dn) { > @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) > goto out_err; > } > > - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */ > - mask = convert_arg(arg); > - > /* set up the new_fsn_mark and new_dn_mark */ > new_fsn_mark = &new_dn_mark->fsn_mark; > fsnotify_init_mark(new_fsn_mark, dnotify_group); > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index a90bb19dcfa2..9e3137badb6b 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = { > }; > > static int fanotify_find_path(int dfd, const char __user *filename, > - struct path *path, unsigned int flags) > + struct path *path, unsigned int flags, __u64 mask) > { > int ret; > + unsigned int mark_type; > > pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__, > dfd, filename, flags); > @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename, > > /* you can only watch an inode if you have read permissions on it */ > ret = inode_permission(path->dentry->d_inode, MAY_READ); > + if (ret) { > + path_put(path); > + goto out; > + } > + > + switch (flags & FANOTIFY_MARK_TYPE_BITS) { > + case FAN_MARK_MOUNT: > + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT; > + break; > + case FAN_MARK_FILESYSTEM: > + mark_type = FSNOTIFY_OBJ_TYPE_SB; > + break; > + case FAN_MARK_INODE: > + mark_type = FSNOTIFY_OBJ_TYPE_INODE; > + break; > + default: > + ret = -EINVAL; > + goto out; > + } > + > + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type); > if (ret) > path_put(path); > + > out: > return ret; > } > @@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, > goto fput_and_out; > } > > - ret = fanotify_find_path(dfd, pathname, &path, flags); > + ret = fanotify_find_path(dfd, pathname, &path, flags, mask); > if (ret) > goto fput_and_out; > > diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c > index 7b53598c8804..73b321a30bbc 100644 > --- a/fs/notify/inotify/inotify_user.c > +++ b/fs/notify/inotify/inotify_user.c > @@ -39,6 +39,7 @@ > #include <linux/poll.h> > #include <linux/wait.h> > #include <linux/memcontrol.h> > +#include <linux/security.h> > > #include "inotify.h" > #include "../fdinfo.h" > @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = { > /* > * find_inode - resolve a user-given path to a specific inode > */ > -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags) > +static int inotify_find_inode(const char __user *dirname, struct path *path, > + unsigned int flags, __u64 mask) > { > int error; > > @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns > return error; > /* you can only watch an inode if you have read permissions on it */ > error = inode_permission(path->dentry->d_inode, MAY_READ); > + if (error) { > + path_put(path); > + return error; > + } > + error = security_inode_notify(path->dentry->d_inode, mask, > + FSNOTIFY_OBJ_TYPE_INODE); > if (error) > path_put(path); > + > return error; > } > > @@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname, > if (mask & IN_ONLYDIR) > flags |= LOOKUP_DIRECTORY; > > - ret = inotify_find_inode(pathname, &path, flags); > + ret = inotify_find_inode(pathname, &path, flags, mask); > if (ret) > goto fput_and_out; > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 47f58cfb6a19..9b3f5a5f3246 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -394,6 +394,9 @@ > * Check permission before removing the extended attribute > * identified by @name for @dentry. > * Return 0 if permission is granted. > + * @inode_notify: > + * Check permissions before setting a watch on events as defined by @mask, > + * on an object @inode, whose type is defined by @mark_type. > * @inode_getsecurity: > * Retrieve a copy of the extended attribute representation of the > * security label associated with @name for @inode via @buffer. Note that > @@ -1571,6 +1574,8 @@ union security_list_options { > int (*inode_getxattr)(struct dentry *dentry, const char *name); > int (*inode_listxattr)(struct dentry *dentry); > int (*inode_removexattr)(struct dentry *dentry, const char *name); > + int (*inode_notify)(struct inode *inode, u64 mask, > + unsigned int mark_type); > int (*inode_need_killpriv)(struct dentry *dentry); > int (*inode_killpriv)(struct dentry *dentry); > int (*inode_getsecurity)(struct inode *inode, const char *name, > @@ -1881,6 +1886,7 @@ struct security_hook_heads { > struct hlist_head inode_getxattr; > struct hlist_head inode_listxattr; > struct hlist_head inode_removexattr; > + struct hlist_head inode_notify; > struct hlist_head inode_need_killpriv; > struct hlist_head inode_killpriv; > struct hlist_head inode_getsecurity; > diff --git a/include/linux/security.h b/include/linux/security.h > index 659071c2e57c..b12666513138 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -301,6 +301,8 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer > void security_inode_getsecid(struct inode *inode, u32 *secid); > int security_inode_copy_up(struct dentry *src, struct cred **new); > int security_inode_copy_up_xattr(const char *name); > +int security_inode_notify(struct inode *inode, u64 mask, > + unsigned int mark_type); > int security_kernfs_init_security(struct kernfs_node *kn_dir, > struct kernfs_node *kn); > int security_file_permission(struct file *file, int mask); > @@ -387,7 +389,6 @@ int security_ismaclabel(const char *name); > int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); > int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid); > void security_release_secctx(char *secdata, u32 seclen); > - > void security_inode_invalidate_secctx(struct inode *inode); > int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); > int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen); > @@ -776,6 +777,12 @@ static inline int security_inode_removexattr(struct dentry *dentry, > return cap_inode_removexattr(dentry, name); > } > > +static inline int security_inode_notify(struct inode *inode, u64 mask, > + unsigned int mark_type) > +{ > + return 0; > +} > + > static inline int security_inode_need_killpriv(struct dentry *dentry) > { > return cap_inode_need_killpriv(dentry); > diff --git a/security/security.c b/security/security.c > index 613a5c00e602..bc30e201c137 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name) > return evm_inode_removexattr(dentry, name); > } > > +int security_inode_notify(struct inode *inode, u64 mask, unsigned int mark_type) > +{ > + return call_int_hook(inode_notify, 0, inode, mask, mark_type); > +} > + > int security_inode_need_killpriv(struct dentry *dentry) > { > return call_int_hook(inode_need_killpriv, 0, dentry); > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index c61787b15f27..c967e46a34ea 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -92,6 +92,8 @@ > #include <linux/kernfs.h> > #include <linux/stringhash.h> /* for hashlen_string() */ > #include <uapi/linux/mount.h> > +#include <linux/fsnotify.h> > +#include <linux/fanotify.h> > > #include "avc.h" > #include "objsec.h" > @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) > return -EACCES; > } > > +static int selinux_inode_notify(struct inode *inode, u64 mask, > + unsigned int mark_type) > +{ > + int ret; > + u32 perm; > + > + struct common_audit_data ad; > + > + ad.type = LSM_AUDIT_DATA_INODE; > + ad.u.inode = inode; > + > + /* > + * Set permission needed based on the type of mark being set. > + * Performs an additional check for sb watches. > + */ > + switch (mark_type) { > + case FSNOTIFY_OBJ_TYPE_VFSMOUNT: > + perm = FILE__WATCH_MOUNT; > + break; > + case FSNOTIFY_OBJ_TYPE_SB: > + perm = FILE__WATCH_SB; > + ret = superblock_has_perm(current_cred(), inode->i_sb, > + FILESYSTEM__WATCH, &ad); > + if (ret) > + return ret; > + break; > + case FSNOTIFY_OBJ_TYPE_INODE: > + perm = FILE__WATCH; > + break; > + default: > + return -EINVAL; > + } > + > + // check if the mask is requesting ability to set a blocking watch > + if (mask & (ALL_FSNOTIFY_PERM_EVENTS)) > + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission > + > + // is the mask asking to watch file reads? > + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE)) > + perm |= FILE__WATCH_READS; // check that permission as well > + > + return inode_has_perm(current_cred(), inode, perm, &ad); > +} > + > /* > * Copy the inode security context value to the user. > * > @@ -6797,6 +6843,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid), > LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up), > LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr), > + LSM_HOOK_INIT(inode_notify, selinux_inode_notify), > > LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security), > > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 201f7e588a29..32e9b03be3dd 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -7,7 +7,8 @@ > > #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ > "rename", "execute", "quotaon", "mounton", "audit_access", \ > - "open", "execmod" > + "open", "execmod", "watch", "watch_mount", "watch_sb", \ > + "watch_with_perm", "watch_reads" > > #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ > "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ > @@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = { > { "filesystem", > { "mount", "remount", "unmount", "getattr", > "relabelfrom", "relabelto", "associate", "quotamod", > - "quotaget", NULL } }, > + "quotaget", "watch", NULL } }, > { "file", > { COMMON_FILE_PERMS, > "execute_no_trans", "entrypoint", NULL } },
On 7/18/19 12:16 PM, Amir Goldstein wrote: > On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote: >> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c >> index a90bb19dcfa2..9e3137badb6b 100644 >> --- a/fs/notify/fanotify/fanotify_user.c >> +++ b/fs/notify/fanotify/fanotify_user.c >> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = { >> }; >> >> static int fanotify_find_path(int dfd, const char __user *filename, >> - struct path *path, unsigned int flags) >> + struct path *path, unsigned int flags, __u64 mask) >> { >> int ret; >> + unsigned int mark_type; >> >> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__, >> dfd, filename, flags); >> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename, >> >> /* you can only watch an inode if you have read permissions on it */ >> ret = inode_permission(path->dentry->d_inode, MAY_READ); >> + if (ret) { >> + path_put(path); >> + goto out; >> + } >> + >> + switch (flags & FANOTIFY_MARK_TYPE_BITS) { >> + case FAN_MARK_MOUNT: >> + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT; >> + break; >> + case FAN_MARK_FILESYSTEM: >> + mark_type = FSNOTIFY_OBJ_TYPE_SB; >> + break; >> + case FAN_MARK_INODE: >> + mark_type = FSNOTIFY_OBJ_TYPE_INODE; >> + break; >> + default: >> + ret = -EINVAL; >> + goto out; >> + } >> + >> + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type); > > If you prefer 3 hooks security_{inode,mount,sb}_notify() > please place them in fanotify_add_{inode,mount,sb}_mark(). > > If you prefer single hook with path argument, please pass path > down to fanotify_add_mark() and call security_path_notify() from there, > where you already have the object type argument. > I'm not clear on why you want me to move the hook call down to fanotify_add_mark(). I'd prefer to keep it adjacent to the existing inode_permission() call so that all the security checking occurs from one place. Moving it down requires adding a path arg to that entire call chain, even though it wouldn't otherwise be needed. And that raises the question of whether to continue passing the mnt_sb, mnt, or inode separately or just extract all those from the path inside of fanotify_add_*_mark(). It also seems to destroy the parallelism with fanotify_remove_*_mark(). I also don't see any real benefit in splitting into three separate hooks, especially as some security modules will want the path or inode even for the mount or superblock cases, since they may have no relevant security information for vfsmounts or superblocks.
On Tue, Jul 23, 2019 at 7:17 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote: > > On 7/18/19 12:16 PM, Amir Goldstein wrote: > > On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote: > >> > >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > >> index a90bb19dcfa2..9e3137badb6b 100644 > >> --- a/fs/notify/fanotify/fanotify_user.c > >> +++ b/fs/notify/fanotify/fanotify_user.c > >> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = { > >> }; > >> > >> static int fanotify_find_path(int dfd, const char __user *filename, > >> - struct path *path, unsigned int flags) > >> + struct path *path, unsigned int flags, __u64 mask) > >> { > >> int ret; > >> + unsigned int mark_type; > >> > >> pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__, > >> dfd, filename, flags); > >> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename, > >> > >> /* you can only watch an inode if you have read permissions on it */ > >> ret = inode_permission(path->dentry->d_inode, MAY_READ); > >> + if (ret) { > >> + path_put(path); > >> + goto out; > >> + } > >> + > >> + switch (flags & FANOTIFY_MARK_TYPE_BITS) { > >> + case FAN_MARK_MOUNT: > >> + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT; > >> + break; > >> + case FAN_MARK_FILESYSTEM: > >> + mark_type = FSNOTIFY_OBJ_TYPE_SB; > >> + break; > >> + case FAN_MARK_INODE: > >> + mark_type = FSNOTIFY_OBJ_TYPE_INODE; > >> + break; > >> + default: > >> + ret = -EINVAL; > >> + goto out; > >> + } > >> + > >> + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type); > > > > If you prefer 3 hooks security_{inode,mount,sb}_notify() > > please place them in fanotify_add_{inode,mount,sb}_mark(). > > > > If you prefer single hook with path argument, please pass path > > down to fanotify_add_mark() and call security_path_notify() from there, > > where you already have the object type argument. > > > I'm not clear on why you want me to move the hook call down to > fanotify_add_mark(). I'd prefer to keep it adjacent to the existing > inode_permission() call so that all the security checking occurs from > one place. Fine. > Moving it down requires adding a path arg to that entire call > chain, even though it wouldn't otherwise be needed. That doesn't matter. > And that raises the > question of whether to continue passing the mnt_sb, mnt, or inode > separately or just extract all those from the path inside of > fanotify_add_*_mark(). You lost me. The major issue I have is with passing @inode argument to hook for adding a mount watch. Makes no sense to me as @inode may be accessed from any mount and without passing @path to hook this information is lost. > > It also seems to destroy the parallelism with fanotify_remove_*_mark(). I don't know what that means. > I also don't see any real benefit in splitting into three separate > hooks, especially as some security modules will want the path or inode > even for the mount or superblock cases, since they may have no relevant > security information for vfsmounts or superblocks. OK. that is an argument for single hook with @path argument. That is fine by me. Thanks, Amir.
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index 250369d6901d..4690d8a66035 100644 --- a/fs/notify/dnotify/dnotify.c +++ b/fs/notify/dnotify/dnotify.c @@ -22,6 +22,7 @@ #include <linux/sched/signal.h> #include <linux/dnotify.h> #include <linux/init.h> +#include <linux/security.h> #include <linux/spinlock.h> #include <linux/slab.h> #include <linux/fdtable.h> @@ -288,6 +289,16 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) goto out_err; } + /* + * convert the userspace DN_* "arg" to the internal FS_* + * defined in fsnotify + */ + mask = convert_arg(arg); + + error = security_inode_notify(inode, mask, FSNOTIFY_OBJ_TYPE_INODE); + if (error) + goto out_err; + /* expect most fcntl to add new rather than augment old */ dn = kmem_cache_alloc(dnotify_struct_cache, GFP_KERNEL); if (!dn) { @@ -302,9 +313,6 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg) goto out_err; } - /* convert the userspace DN_* "arg" to the internal FS_* defines in fsnotify */ - mask = convert_arg(arg); - /* set up the new_fsn_mark and new_dn_mark */ new_fsn_mark = &new_dn_mark->fsn_mark; fsnotify_init_mark(new_fsn_mark, dnotify_group); diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index a90bb19dcfa2..9e3137badb6b 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = { }; static int fanotify_find_path(int dfd, const char __user *filename, - struct path *path, unsigned int flags) + struct path *path, unsigned int flags, __u64 mask) { int ret; + unsigned int mark_type; pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__, dfd, filename, flags); @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename, /* you can only watch an inode if you have read permissions on it */ ret = inode_permission(path->dentry->d_inode, MAY_READ); + if (ret) { + path_put(path); + goto out; + } + + switch (flags & FANOTIFY_MARK_TYPE_BITS) { + case FAN_MARK_MOUNT: + mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT; + break; + case FAN_MARK_FILESYSTEM: + mark_type = FSNOTIFY_OBJ_TYPE_SB; + break; + case FAN_MARK_INODE: + mark_type = FSNOTIFY_OBJ_TYPE_INODE; + break; + default: + ret = -EINVAL; + goto out; + } + + ret = security_inode_notify(path->dentry->d_inode, mask, mark_type); if (ret) path_put(path); + out: return ret; } @@ -1014,7 +1037,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, goto fput_and_out; } - ret = fanotify_find_path(dfd, pathname, &path, flags); + ret = fanotify_find_path(dfd, pathname, &path, flags, mask); if (ret) goto fput_and_out; diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c index 7b53598c8804..73b321a30bbc 100644 --- a/fs/notify/inotify/inotify_user.c +++ b/fs/notify/inotify/inotify_user.c @@ -39,6 +39,7 @@ #include <linux/poll.h> #include <linux/wait.h> #include <linux/memcontrol.h> +#include <linux/security.h> #include "inotify.h" #include "../fdinfo.h" @@ -342,7 +343,8 @@ static const struct file_operations inotify_fops = { /* * find_inode - resolve a user-given path to a specific inode */ -static int inotify_find_inode(const char __user *dirname, struct path *path, unsigned flags) +static int inotify_find_inode(const char __user *dirname, struct path *path, + unsigned int flags, __u64 mask) { int error; @@ -351,8 +353,15 @@ static int inotify_find_inode(const char __user *dirname, struct path *path, uns return error; /* you can only watch an inode if you have read permissions on it */ error = inode_permission(path->dentry->d_inode, MAY_READ); + if (error) { + path_put(path); + return error; + } + error = security_inode_notify(path->dentry->d_inode, mask, + FSNOTIFY_OBJ_TYPE_INODE); if (error) path_put(path); + return error; } @@ -744,7 +753,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int, fd, const char __user *, pathname, if (mask & IN_ONLYDIR) flags |= LOOKUP_DIRECTORY; - ret = inotify_find_inode(pathname, &path, flags); + ret = inotify_find_inode(pathname, &path, flags, mask); if (ret) goto fput_and_out; diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 47f58cfb6a19..9b3f5a5f3246 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -394,6 +394,9 @@ * Check permission before removing the extended attribute * identified by @name for @dentry. * Return 0 if permission is granted. + * @inode_notify: + * Check permissions before setting a watch on events as defined by @mask, + * on an object @inode, whose type is defined by @mark_type. * @inode_getsecurity: * Retrieve a copy of the extended attribute representation of the * security label associated with @name for @inode via @buffer. Note that @@ -1571,6 +1574,8 @@ union security_list_options { int (*inode_getxattr)(struct dentry *dentry, const char *name); int (*inode_listxattr)(struct dentry *dentry); int (*inode_removexattr)(struct dentry *dentry, const char *name); + int (*inode_notify)(struct inode *inode, u64 mask, + unsigned int mark_type); int (*inode_need_killpriv)(struct dentry *dentry); int (*inode_killpriv)(struct dentry *dentry); int (*inode_getsecurity)(struct inode *inode, const char *name, @@ -1881,6 +1886,7 @@ struct security_hook_heads { struct hlist_head inode_getxattr; struct hlist_head inode_listxattr; struct hlist_head inode_removexattr; + struct hlist_head inode_notify; struct hlist_head inode_need_killpriv; struct hlist_head inode_killpriv; struct hlist_head inode_getsecurity; diff --git a/include/linux/security.h b/include/linux/security.h index 659071c2e57c..b12666513138 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -301,6 +301,8 @@ int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer void security_inode_getsecid(struct inode *inode, u32 *secid); int security_inode_copy_up(struct dentry *src, struct cred **new); int security_inode_copy_up_xattr(const char *name); +int security_inode_notify(struct inode *inode, u64 mask, + unsigned int mark_type); int security_kernfs_init_security(struct kernfs_node *kn_dir, struct kernfs_node *kn); int security_file_permission(struct file *file, int mask); @@ -387,7 +389,6 @@ int security_ismaclabel(const char *name); int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid); void security_release_secctx(char *secdata, u32 seclen); - void security_inode_invalidate_secctx(struct inode *inode); int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen); int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen); @@ -776,6 +777,12 @@ static inline int security_inode_removexattr(struct dentry *dentry, return cap_inode_removexattr(dentry, name); } +static inline int security_inode_notify(struct inode *inode, u64 mask, + unsigned int mark_type) +{ + return 0; +} + static inline int security_inode_need_killpriv(struct dentry *dentry) { return cap_inode_need_killpriv(dentry); diff --git a/security/security.c b/security/security.c index 613a5c00e602..bc30e201c137 100644 --- a/security/security.c +++ b/security/security.c @@ -1251,6 +1251,11 @@ int security_inode_removexattr(struct dentry *dentry, const char *name) return evm_inode_removexattr(dentry, name); } +int security_inode_notify(struct inode *inode, u64 mask, unsigned int mark_type) +{ + return call_int_hook(inode_notify, 0, inode, mask, mark_type); +} + int security_inode_need_killpriv(struct dentry *dentry) { return call_int_hook(inode_need_killpriv, 0, dentry); diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index c61787b15f27..c967e46a34ea 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -92,6 +92,8 @@ #include <linux/kernfs.h> #include <linux/stringhash.h> /* for hashlen_string() */ #include <uapi/linux/mount.h> +#include <linux/fsnotify.h> +#include <linux/fanotify.h> #include "avc.h" #include "objsec.h" @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) return -EACCES; } +static int selinux_inode_notify(struct inode *inode, u64 mask, + unsigned int mark_type) +{ + int ret; + u32 perm; + + struct common_audit_data ad; + + ad.type = LSM_AUDIT_DATA_INODE; + ad.u.inode = inode; + + /* + * Set permission needed based on the type of mark being set. + * Performs an additional check for sb watches. + */ + switch (mark_type) { + case FSNOTIFY_OBJ_TYPE_VFSMOUNT: + perm = FILE__WATCH_MOUNT; + break; + case FSNOTIFY_OBJ_TYPE_SB: + perm = FILE__WATCH_SB; + ret = superblock_has_perm(current_cred(), inode->i_sb, + FILESYSTEM__WATCH, &ad); + if (ret) + return ret; + break; + case FSNOTIFY_OBJ_TYPE_INODE: + perm = FILE__WATCH; + break; + default: + return -EINVAL; + } + + // check if the mask is requesting ability to set a blocking watch + if (mask & (ALL_FSNOTIFY_PERM_EVENTS)) + perm |= FILE__WATCH_WITH_PERM; // if so, check that permission + + // is the mask asking to watch file reads? + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE)) + perm |= FILE__WATCH_READS; // check that permission as well + + return inode_has_perm(current_cred(), inode, perm, &ad); +} + /* * Copy the inode security context value to the user. * @@ -6797,6 +6843,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid), LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up), LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr), + LSM_HOOK_INIT(inode_notify, selinux_inode_notify), LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security), diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 201f7e588a29..32e9b03be3dd 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -7,7 +7,8 @@ #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ "rename", "execute", "quotaon", "mounton", "audit_access", \ - "open", "execmod" + "open", "execmod", "watch", "watch_mount", "watch_sb", \ + "watch_with_perm", "watch_reads" #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ @@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = { { "filesystem", { "mount", "remount", "unmount", "getattr", "relabelfrom", "relabelto", "associate", "quotamod", - "quotaget", NULL } }, + "quotaget", "watch", NULL } }, { "file", { COMMON_FILE_PERMS, "execute_no_trans", "entrypoint", NULL } },
As of now, setting watches on filesystem objects has, at most, applied a check for read access to the inode, and in the case of fanotify, requires CAP_SYS_ADMIN. No specific security hook or permission check has been provided to control the setting of watches. Using any of inotify, dnotify, or fanotify, it is possible to observe, not only write-like operations, but even read access to a file. Modeling the watch as being merely a read from the file is insufficient for the needs of SELinux. This is due to the fact that read access should not necessarily imply access to information about when another process reads from a file. Furthermore, fanotify watches grant more power to an application in the form of permission events. While notification events are solely, unidirectional (i.e. they only pass information to the receiving application), permission events are blocking. Permission events make a request to the receiving application which will then reply with a decision as to whether or not that action may be completed. This causes the issue of the watching application having the ability to exercise control over the triggering process. Without drawing a distinction within the permission check, the ability to read would imply the greater ability to control an application. Additionally, mount and superblock watches apply to all files within the same mount or superblock. Read access to one file should not necessarily imply the ability to watch all files accessed within a given mount or superblock. In order to solve these issues, a new LSM hook is implemented and has been placed within the system calls for marking filesystem objects with inotify, fanotify, and dnotify watches. These calls to the hook are placed at the point at which the target inode has been resolved and are provided with the inode, the mask of requested notification events, and the type of object on which the mark is being set (inode, superblock, or mount). The mask and mark_type have already been translated into common FS_* values shared by the entirety of the fs notification infrastructure. This only provides a hook at the point of setting a watch, and presumes that permission to set a particular watch implies the ability to receive all notification about that object which match the mask. This is all that is required for SELinux. If other security modules require additional hooks or infrastructure to control delivery of notification, these can be added by them. It does not make sense for us to propose hooks for which we have no implementation. The understanding that all notifications received by the requesting application are all strictly of a type for which the application has been granted permission shows that this implementation is sufficient in its coverage. Security modules wishing to provide complete control over fanotify must also implement a security_file_open hook that validates that the access requested by the watching application is authorized. Fanotify has the issue that it returns a file descriptor with the file mode specified during fanotify_init() to the watching process on event. This is already covered by the LSM security_file_open hook if the security module implements checking of the requested file mode there. Otherwise, a watching process can obtain escalated access to a file for which it has not been authorized. The selinux_inode_notify hook implementation works by adding five new file permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm (descriptions about which will follow), and one new filesystem permission: watch (which is applied to superblock checks). The hook then decides which subset of these permissions must be held by the requesting application based on the contents of the provided mask and the mark_type. The selinux_file_open hook already checks the requested file mode and therefore ensures that a watching process cannot escalate its access through fanotify. The watch, watch_mount, and watch_sb permissions are the baseline permissions for setting a watch on an object and each are a requirement for any watch to be set on a file, mount, or superblock respectively. It should be noted that having either of the other two permissions (watch_reads and watch_with_perm) does not imply the watch, watch_mount, or watch_sb permission. Superblock watches further require the filesystem watch permission to the superblock. As there is no labeled object in view for mounts, there is no specific check for mount watches beyond watch_mount to the inode. Such a check could be added in the future, if a suitable labeled object existed representing the mount. The watch_reads permission is required to receive notifications from read-exclusive events on filesystem objects. These events include accessing a file for the purpose of reading and closing a file which has been opened read-only. This distinction has been drawn in order to provide a direct indication in the policy for this otherwise not obvious capability. Read access to a file should not necessarily imply the ability to observe read events on a file. Finally, watch_with_perm only applies to fanotify masks since it is the only way to set a mask which allows for the blocking, permission event. This permission is needed for any watch which is of this type. Though fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit trust to root, which we do not do, and does not support least privilege. Signed-off-by: Aaron Goidel <acgoide@tycho.nsa.gov> --- v2: - Adds support for mark_type - Adds watch_sb and watch_mount file permissions - Adds watch as new filesystem permission - LSM hook now recieves mark_type argument - Changed LSM hook logic to implement checks for corresponding mark_types - Adds missing hook description comment - Fixes extrainous whitespace - Updates patch description based on feedback fs/notify/dnotify/dnotify.c | 14 +++++++-- fs/notify/fanotify/fanotify_user.c | 27 +++++++++++++++-- fs/notify/inotify/inotify_user.c | 13 ++++++-- include/linux/lsm_hooks.h | 6 ++++ include/linux/security.h | 9 +++++- security/security.c | 5 +++ security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++ security/selinux/include/classmap.h | 5 +-- 8 files changed, 116 insertions(+), 10 deletions(-)