| Message ID | 20190710133403.855-1-acgoide@tycho.nsa.gov (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] fanotify, inotify, dnotify, security: add security hook for fs notifications | expand |
On Wed, Jul 10, 2019 at 4:34 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. 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. > > 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 > both the inode and the mask of requested notification events. The mask has > 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. > > Fanotify further 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. > > The selinux_inode_notify hook implementation works by adding three new > file permissions: watch, watch_reads, and watch_with_perm (descriptions > about which will follow). The hook then decides which subset of these > permissions must be held by the requesting application based on the > contents of the provided mask. 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 permission is the baseline permission for setting a watch on an > object and is a requirement for any watch to be set whatsoever. It should > be noted that having either of the other two permissions (watch_reads and > watch_with_perm) does not imply the watch permission, though this could be > changed if need be. > > 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> > --- > fs/notify/dnotify/dnotify.c | 14 +++++++++++--- > fs/notify/fanotify/fanotify_user.c | 11 +++++++++-- > fs/notify/inotify/inotify_user.c | 12 ++++++++++-- > include/linux/lsm_hooks.h | 2 ++ > include/linux/security.h | 7 +++++++ > security/security.c | 5 +++++ > security/selinux/hooks.c | 22 ++++++++++++++++++++++ > security/selinux/include/classmap.h | 2 +- > 8 files changed, 67 insertions(+), 8 deletions(-) > > diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c > index 250369d6901d..e91ce092efb1 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); > + 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..c0d9fa998377 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -528,7 +528,7 @@ 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; > > @@ -567,8 +567,15 @@ 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; > + } > + > + ret = security_inode_notify(path->dentry->d_inode, mask); > if (ret) > path_put(path); > + > out: > return ret; > } > @@ -1014,7 +1021,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; > So the mark_type doesn't matter to SELinux? You have no need for mount_noitify and sb_notify hooks? A watch permission on the mount/sb root inode implies permission (as CAP_SYS_ADMIN) to watch all events in mount/sb? [...] > +static int selinux_inode_notify(struct inode *inode, u64 mask) > +{ > + u32 perm = FILE__WATCH; // basic permission, can a watch be set? > + > + struct common_audit_data ad; > + > + ad.type = LSM_AUDIT_DATA_INODE; > + ad.u.inode = inode; > + > + // check if the mask is requesting ability to set a blocking watch > + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM)) Better ALL_FSNOTIFY_PERM_EVENTS Thanks, Amir.
On 7/10/2019 6:34 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. That's a very model-specific viewpoint. It is true for a fine-grained model such as SELinux, but not necessarily for a model with more traditional object definitions. I'm not saying you're wrong, I'm saying that stating it as a given assumes your model. You can do that all you want within SELinux, but it doesn't hold when you're talking about the LSM infrastructure. Have you coordinated this with the work that David Howells is doing on generic notifications? > 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. You're not saying why this is an issue. > 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 > both the inode and the mask of requested notification events. The mask has > 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. A reasonable approach. It would be *nice* if you had a look at the other security modules to see what they might need from such a hook or hook set. > Fanotify further 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. How is this relevant? > The selinux_inode_notify hook implementation works by adding three new > file permissions: watch, watch_reads, and watch_with_perm (descriptions > about which will follow). The hook then decides which subset of these > permissions must be held by the requesting application based on the > contents of the provided mask. The selinux_file_open hook already checks > the requested file mode and therefore ensures that a watching process > cannot escalate its access through fanotify. Thereby increasing the granularity of control available. > The watch permission is the baseline permission for setting a watch on an > object and is a requirement for any watch to be set whatsoever. It should > be noted that having either of the other two permissions (watch_reads and > watch_with_perm) does not imply the watch permission, though this could be > changed if need be. > > 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> > --- > fs/notify/dnotify/dnotify.c | 14 +++++++++++--- > fs/notify/fanotify/fanotify_user.c | 11 +++++++++-- > fs/notify/inotify/inotify_user.c | 12 ++++++++++-- > include/linux/lsm_hooks.h | 2 ++ > include/linux/security.h | 7 +++++++ > security/security.c | 5 +++++ > security/selinux/hooks.c | 22 ++++++++++++++++++++++ > security/selinux/include/classmap.h | 2 +- > 8 files changed, 67 insertions(+), 8 deletions(-) > > diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c > index 250369d6901d..e91ce092efb1 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); > + 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..c0d9fa998377 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -528,7 +528,7 @@ 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; > > @@ -567,8 +567,15 @@ 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; > + } > + > + ret = security_inode_notify(path->dentry->d_inode, mask); > if (ret) > path_put(path); > + > out: > return ret; > } > @@ -1014,7 +1021,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..47b079f20aad 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,14 @@ 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); > if (error) > path_put(path); > + > return error; > } > > @@ -744,7 +752,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..ef6b74938dd8 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h Hook description comment is missing. > @@ -1571,6 +1571,7 @@ 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); > int (*inode_need_killpriv)(struct dentry *dentry); > int (*inode_killpriv)(struct dentry *dentry); > int (*inode_getsecurity)(struct inode *inode, const char *name, > @@ -1881,6 +1882,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..50106fb9eef9 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -301,6 +301,7 @@ 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); > int security_kernfs_init_security(struct kernfs_node *kn_dir, > struct kernfs_node *kn); > int security_file_permission(struct file *file, int mask); > @@ -392,6 +393,7 @@ 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); > int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); > + Please don't change whitespace unless it's directly adjacent to your code. > #else /* CONFIG_SECURITY */ > > static inline int call_lsm_notifier(enum lsm_event event, void *data) > @@ -776,6 +778,11 @@ 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) > +{ > + 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..57b2a96c1991 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) > +{ > + return call_int_hook(inode_notify, 0, inode, mask); > +} > + > 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..1a37966c2978 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -92,6 +92,7 @@ > #include <linux/kernfs.h> > #include <linux/stringhash.h> /* for hashlen_string() */ > #include <uapi/linux/mount.h> > +#include <linux/fsnotify.h> > > #include "avc.h" > #include "objsec.h" > @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) > return -EACCES; > } > > +static int selinux_inode_notify(struct inode *inode, u64 mask) > +{ > + u32 perm = FILE__WATCH; // basic permission, can a watch be set? We don't use // comments in the Linux kernel. > + > + struct common_audit_data ad; > + > + ad.type = LSM_AUDIT_DATA_INODE; > + ad.u.inode = inode; > + > + // check if the mask is requesting ability to set a blocking watch > + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM)) > + 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 +6818,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..0654dd2fbebf 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -7,7 +7,7 @@ > > #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ > "rename", "execute", "quotaon", "mounton", "audit_access", \ > - "open", "execmod" > + "open", "execmod", "watch", "watch_with_perm", "watch_reads" > > #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ > "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
On 7/10/19 9:38 AM, Casey Schaufler wrote: > On 7/10/2019 6:34 AM, Aaron Goidel wrote: >> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) >> return -EACCES; >> } >> >> +static int selinux_inode_notify(struct inode *inode, u64 mask) >> +{ >> + u32 perm = FILE__WATCH; // basic permission, can a watch be set? > > We don't use // comments in the Linux kernel. > I thought that we had recently moved into the 21st century on that issue, but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update. checkpatch allows C99 comments by default. Joe, do you recall about this? thanks.
On 7/10/2019 9:49 AM, Randy Dunlap wrote: > On 7/10/19 9:38 AM, Casey Schaufler wrote: >> On 7/10/2019 6:34 AM, Aaron Goidel wrote: >>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) >>> return -EACCES; >>> } >>> >>> +static int selinux_inode_notify(struct inode *inode, u64 mask) >>> +{ >>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set? >> We don't use // comments in the Linux kernel. >> > I thought that we had recently moved into the 21st century on that issue, > but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update. Really? Yuck. Next thing you know M4 macros will be allowed. > > checkpatch allows C99 comments by default. > Joe, do you recall about this? > > thanks.
On Wed, 2019-07-10 at 09:49 -0700, Randy Dunlap wrote: > On 7/10/19 9:38 AM, Casey Schaufler wrote: > > On 7/10/2019 6:34 AM, Aaron Goidel wrote: > > > @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) > > > return -EACCES; > > > } > > > > > > +static int selinux_inode_notify(struct inode *inode, u64 mask) > > > +{ > > > + u32 perm = FILE__WATCH; // basic permission, can a watch be set? > > > > We don't use // comments in the Linux kernel. > > > > I thought that we had recently moved into the 21st century on that issue, > but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update. > > checkpatch allows C99 comments by default. > Joe, do you recall about this? My recollection is it was something I thought was just simple and useful so I added it to checkpatch without going through the negative of the nominal approvals required by modifying CodingStyle.
On Wed, 2019-07-10 at 10:18 -0700, Joe Perches wrote: > On Wed, 2019-07-10 at 09:49 -0700, Randy Dunlap wrote: > > On 7/10/19 9:38 AM, Casey Schaufler wrote: > > > On 7/10/2019 6:34 AM, Aaron Goidel wrote: > > > > @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) > > > > return -EACCES; > > > > } > > > > > > > > +static int selinux_inode_notify(struct inode *inode, u64 mask) > > > > +{ > > > > + u32 perm = FILE__WATCH; // basic permission, can a watch be set? > > > > > > We don't use // comments in the Linux kernel. > > > > > > > I thought that we had recently moved into the 21st century on that issue, > > but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update. > > > > checkpatch allows C99 comments by default. > > Joe, do you recall about this? > > My recollection is it was something I thought was > just simple and useful so I added it to checkpatch > without going through the negative of the nominal > approvals required by modifying CodingStyle. https://lkml.org/lkml/2016/7/8/625
On 7/10/19 10:55 AM, Amir Goldstein wrote: > On Wed, Jul 10, 2019 at 4:34 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. 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. >> >> 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 >> both the inode and the mask of requested notification events. The mask has >> 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. >> >> Fanotify further 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. >> >> The selinux_inode_notify hook implementation works by adding three new >> file permissions: watch, watch_reads, and watch_with_perm (descriptions >> about which will follow). The hook then decides which subset of these >> permissions must be held by the requesting application based on the >> contents of the provided mask. 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 permission is the baseline permission for setting a watch on an >> object and is a requirement for any watch to be set whatsoever. It should >> be noted that having either of the other two permissions (watch_reads and >> watch_with_perm) does not imply the watch permission, though this could be >> changed if need be. >> >> 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> >> --- >> fs/notify/dnotify/dnotify.c | 14 +++++++++++--- >> fs/notify/fanotify/fanotify_user.c | 11 +++++++++-- >> fs/notify/inotify/inotify_user.c | 12 ++++++++++-- >> include/linux/lsm_hooks.h | 2 ++ >> include/linux/security.h | 7 +++++++ >> security/security.c | 5 +++++ >> security/selinux/hooks.c | 22 ++++++++++++++++++++++ >> security/selinux/include/classmap.h | 2 +- >> 8 files changed, 67 insertions(+), 8 deletions(-) >> >> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c >> index 250369d6901d..e91ce092efb1 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); >> + 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..c0d9fa998377 100644 >> --- a/fs/notify/fanotify/fanotify_user.c >> +++ b/fs/notify/fanotify/fanotify_user.c >> @@ -528,7 +528,7 @@ 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; >> >> @@ -567,8 +567,15 @@ 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; >> + } >> + >> + ret = security_inode_notify(path->dentry->d_inode, mask); >> if (ret) >> path_put(path); >> + >> out: >> return ret; >> } >> @@ -1014,7 +1021,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; >> > > So the mark_type doesn't matter to SELinux? Good catch regarding the mark_type, I overlooked that as an issue. The recursive setting of watches--for everything under the mount, in the case of the mount type, and for all the contents of the superblock--warrants further security checks. > You have no need for mount_noitify and sb_notify hooks? While I fully agree that there needs to be separate logic to handle security checks for inode, mount, and superblock notifications, I don't see the need for additional hooks. I believe that it would be sufficient to pass down the mark_type into the existing hook and create two new permissions--one for watching sb's and the other for mounts--which will be checked based on the type. > A watch permission on the mount/sb root inode implies permission > (as CAP_SYS_ADMIN) to watch all events in mount/sb? This would be true for the new watch_mount and watch_sb permissions. In the case of sb's we can also apply check against the sb security label. For mounts there is no other security information, so there's likely no reason to pass it to the hook. Hence, my reasoning for editing the existing hook in favor of creating new ones. > > [...] > >> +static int selinux_inode_notify(struct inode *inode, u64 mask) >> +{ >> + u32 perm = FILE__WATCH; // basic permission, can a watch be set? >> + >> + struct common_audit_data ad; >> + >> + ad.type = LSM_AUDIT_DATA_INODE; >> + ad.u.inode = inode; >> + >> + // check if the mask is requesting ability to set a blocking watch >> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM)) > > Better ALL_FSNOTIFY_PERM_EVENTS Thanks, you will see this in v2! > > Thanks, > Amir.
On 7/10/19 10:22 AM, Joe Perches wrote: > On Wed, 2019-07-10 at 10:18 -0700, Joe Perches wrote: >> On Wed, 2019-07-10 at 09:49 -0700, Randy Dunlap wrote: >>> On 7/10/19 9:38 AM, Casey Schaufler wrote: >>>> On 7/10/2019 6:34 AM, Aaron Goidel wrote: >>>>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) >>>>> return -EACCES; >>>>> } >>>>> >>>>> +static int selinux_inode_notify(struct inode *inode, u64 mask) >>>>> +{ >>>>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set? >>>> >>>> We don't use // comments in the Linux kernel. >>>> >>> >>> I thought that we had recently moved into the 21st century on that issue, >>> but I don't see it mentioned in coding-style.rst. Maybe we need a Doc update. >>> >>> checkpatch allows C99 comments by default. >>> Joe, do you recall about this? >> >> My recollection is it was something I thought was >> just simple and useful so I added it to checkpatch >> without going through the negative of the nominal >> approvals required by modifying CodingStyle. > > https://lkml.org/lkml/2016/7/8/625 > Aha, thanks, I don't recall seeing that one.
On 7/10/19 12:38 PM, Casey Schaufler wrote: > On 7/10/2019 6:34 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. > > That's a very model-specific viewpoint. It is true for > a fine-grained model such as SELinux, but not necessarily > for a model with more traditional object definitions. > I'm not saying you're wrong, I'm saying that stating it > as a given assumes your model. You can do that all you want > within SELinux, but it doesn't hold when you're talking > about the LSM infrastructure. I think you'll find that even for Smack, merely checking read access to the watched inode is insufficient for your purposes, because the watch permits more than just observing changes to the state of the inode. The absence of a hook is a gap in LSM coverage, regardless of security model. If you are just objecting to the wording choice, then I suppose that can be amended to "is insufficient for SELinux" or "is insufficient for some needs" or something. > Have you coordinated this with the work that David Howells > is doing on generic notifications? We're following that work but to date it hasn't appeared to address dnotify/inotify/fanotify IIUC. I think it is complementary; we are adding LSM control over an existing kernel notification mechanism while he is adding a new notification facility for other kinds of events along with corresponding LSM hooks. It is consistent in that it provides a way to control setting of watches based on the watched object. >> 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. > > You're not saying why this is an issue. It allows the watching application control over the process that is attempting the access. Are you just asking for that to be stated more explicitly? >> 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 >> both the inode and the mask of requested notification events. The mask has >> 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. > > A reasonable approach. It would be *nice* if you had > a look at the other security modules to see what they > might need from such a hook or hook set. > >> Fanotify further 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. > > How is this relevant? It is part of ensuring complete control over fanotify. Some existing security modules (like Smack, for example) currently do not perform this checking of the requested file mode and therefore are subject to this privilege escalation scenario through fanotify. A watcher that only has read access to the file can get a read-write descriptor to it in this manner. You may argue that this doesn't matter because fanotify requires CAP_SYS_ADMIN but even for Smack that isn't the same as CAP_MAC_OVERRIDE. > >> The selinux_inode_notify hook implementation works by adding three new >> file permissions: watch, watch_reads, and watch_with_perm (descriptions >> about which will follow). The hook then decides which subset of these >> permissions must be held by the requesting application based on the >> contents of the provided mask. The selinux_file_open hook already checks >> the requested file mode and therefore ensures that a watching process >> cannot escalate its access through fanotify. > > Thereby increasing the granularity of control available. It isn't merely a question of granularity but also completeness and preventing privilege escalation. >> The watch permission is the baseline permission for setting a watch on an >> object and is a requirement for any watch to be set whatsoever. It should >> be noted that having either of the other two permissions (watch_reads and >> watch_with_perm) does not imply the watch permission, though this could be >> changed if need be. >> >> 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> >> --- >> fs/notify/dnotify/dnotify.c | 14 +++++++++++--- >> fs/notify/fanotify/fanotify_user.c | 11 +++++++++-- >> fs/notify/inotify/inotify_user.c | 12 ++++++++++-- >> include/linux/lsm_hooks.h | 2 ++ >> include/linux/security.h | 7 +++++++ >> security/security.c | 5 +++++ >> security/selinux/hooks.c | 22 ++++++++++++++++++++++ >> security/selinux/include/classmap.h | 2 +- >> 8 files changed, 67 insertions(+), 8 deletions(-) >> >> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c >> index 250369d6901d..e91ce092efb1 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); >> + 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..c0d9fa998377 100644 >> --- a/fs/notify/fanotify/fanotify_user.c >> +++ b/fs/notify/fanotify/fanotify_user.c >> @@ -528,7 +528,7 @@ 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; >> >> @@ -567,8 +567,15 @@ 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; >> + } >> + >> + ret = security_inode_notify(path->dentry->d_inode, mask); >> if (ret) >> path_put(path); >> + >> out: >> return ret; >> } >> @@ -1014,7 +1021,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..47b079f20aad 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,14 @@ 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); >> if (error) >> path_put(path); >> + >> return error; >> } >> >> @@ -744,7 +752,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..ef6b74938dd8 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h > > Hook description comment is missing. > >> @@ -1571,6 +1571,7 @@ 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); >> int (*inode_need_killpriv)(struct dentry *dentry); >> int (*inode_killpriv)(struct dentry *dentry); >> int (*inode_getsecurity)(struct inode *inode, const char *name, >> @@ -1881,6 +1882,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..50106fb9eef9 100644 >> --- a/include/linux/security.h >> +++ b/include/linux/security.h >> @@ -301,6 +301,7 @@ 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); >> int security_kernfs_init_security(struct kernfs_node *kn_dir, >> struct kernfs_node *kn); >> int security_file_permission(struct file *file, int mask); >> @@ -392,6 +393,7 @@ 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); >> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); >> + > > Please don't change whitespace unless it's directly adjacent to your code. > >> #else /* CONFIG_SECURITY */ >> >> static inline int call_lsm_notifier(enum lsm_event event, void *data) >> @@ -776,6 +778,11 @@ 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) >> +{ >> + 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..57b2a96c1991 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) >> +{ >> + return call_int_hook(inode_notify, 0, inode, mask); >> +} >> + >> 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..1a37966c2978 100644 >> --- a/security/selinux/hooks.c >> +++ b/security/selinux/hooks.c >> @@ -92,6 +92,7 @@ >> #include <linux/kernfs.h> >> #include <linux/stringhash.h> /* for hashlen_string() */ >> #include <uapi/linux/mount.h> >> +#include <linux/fsnotify.h> >> >> #include "avc.h" >> #include "objsec.h" >> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) >> return -EACCES; >> } >> >> +static int selinux_inode_notify(struct inode *inode, u64 mask) >> +{ >> + u32 perm = FILE__WATCH; // basic permission, can a watch be set? > > We don't use // comments in the Linux kernel. > >> + >> + struct common_audit_data ad; >> + >> + ad.type = LSM_AUDIT_DATA_INODE; >> + ad.u.inode = inode; >> + >> + // check if the mask is requesting ability to set a blocking watch >> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM)) >> + 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 +6818,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..0654dd2fbebf 100644 >> --- a/security/selinux/include/classmap.h >> +++ b/security/selinux/include/classmap.h >> @@ -7,7 +7,7 @@ >> >> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ >> "rename", "execute", "quotaon", "mounton", "audit_access", \ >> - "open", "execmod" >> + "open", "execmod", "watch", "watch_with_perm", "watch_reads" >> >> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ >> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ >
On 7/10/2019 11:39 AM, Stephen Smalley wrote: > On 7/10/19 12:38 PM, Casey Schaufler wrote: >> On 7/10/2019 6:34 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. >> >> That's a very model-specific viewpoint. It is true for >> a fine-grained model such as SELinux, but not necessarily >> for a model with more traditional object definitions. >> I'm not saying you're wrong, I'm saying that stating it >> as a given assumes your model. You can do that all you want >> within SELinux, but it doesn't hold when you're talking >> about the LSM infrastructure. > > I think you'll find that even for Smack, merely checking read access to the watched inode is insufficient for your purposes, because the watch permits more than just observing changes to the state of the inode. The absence of a hook is a gap in LSM coverage, regardless of security model. If you are just objecting to the wording choice, then I suppose that can be amended to "is insufficient for SELinux" or "is insufficient for some needs" or something. More an objection to the assumption of model than anything else. There are enough differing viewpoints on what is necessary and/or sufficient that I wouldn't want the assumption to be a bone of contention later on. > >> Have you coordinated this with the work that David Howells >> is doing on generic notifications? > > We're following that work but to date it hasn't appeared to address dnotify/inotify/fanotify IIUC. I think it is complementary; we are adding LSM control over an existing kernel notification mechanism while he is adding a new notification facility for other kinds of events along with corresponding LSM hooks. It is consistent in that it provides a way to control setting of watches based on the watched object. All true. My hope is that LSM controls on notification mechanisms have some sort of coordination. I'd rather have one hook that's used in multiple places than yet another set of disparate hooks that do mostly the same thing. > >>> 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. >> >> You're not saying why this is an issue. > > It allows the watching application control over the process that is attempting the access. Are you just asking for that to be stated more explicitly? Yes, that would be good. > >>> 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 >>> both the inode and the mask of requested notification events. The mask has >>> 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. >> >> A reasonable approach. It would be *nice* if you had >> a look at the other security modules to see what they >> might need from such a hook or hook set. >> >>> Fanotify further 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. >> >> How is this relevant? > > It is part of ensuring complete control over fanotify. Some existing security modules (like Smack, for example) currently do not perform this checking of the requested file mode and therefore are subject to this privilege escalation scenario through fanotify. A watcher that only has read access to the file can get a read-write descriptor to it in this manner. You may argue that this doesn't matter because fanotify requires CAP_SYS_ADMIN but even for Smack that isn't the same as CAP_MAC_OVERRIDE. Yes, there's a difference in the assumptions security modules make about the privilege escalation. Again the point is that it isn't a good idea to include a single module's policy regarding that in the argument for the generic hook. It's enough to explain why SELinux needs it. > >> >>> The selinux_inode_notify hook implementation works by adding three new >>> file permissions: watch, watch_reads, and watch_with_perm (descriptions >>> about which will follow). The hook then decides which subset of these >>> permissions must be held by the requesting application based on the >>> contents of the provided mask. The selinux_file_open hook already checks >>> the requested file mode and therefore ensures that a watching process >>> cannot escalate its access through fanotify. >> >> Thereby increasing the granularity of control available. > > It isn't merely a question of granularity but also completeness and preventing privilege escalation. I was simply making an observation. > >>> The watch permission is the baseline permission for setting a watch on an >>> object and is a requirement for any watch to be set whatsoever. It should >>> be noted that having either of the other two permissions (watch_reads and >>> watch_with_perm) does not imply the watch permission, though this could be >>> changed if need be. >>> >>> 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> >>> --- >>> fs/notify/dnotify/dnotify.c | 14 +++++++++++--- >>> fs/notify/fanotify/fanotify_user.c | 11 +++++++++-- >>> fs/notify/inotify/inotify_user.c | 12 ++++++++++-- >>> include/linux/lsm_hooks.h | 2 ++ >>> include/linux/security.h | 7 +++++++ >>> security/security.c | 5 +++++ >>> security/selinux/hooks.c | 22 ++++++++++++++++++++++ >>> security/selinux/include/classmap.h | 2 +- >>> 8 files changed, 67 insertions(+), 8 deletions(-) >>> >>> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c >>> index 250369d6901d..e91ce092efb1 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); >>> + 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..c0d9fa998377 100644 >>> --- a/fs/notify/fanotify/fanotify_user.c >>> +++ b/fs/notify/fanotify/fanotify_user.c >>> @@ -528,7 +528,7 @@ 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; >>> @@ -567,8 +567,15 @@ 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; >>> + } >>> + >>> + ret = security_inode_notify(path->dentry->d_inode, mask); >>> if (ret) >>> path_put(path); >>> + >>> out: >>> return ret; >>> } >>> @@ -1014,7 +1021,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..47b079f20aad 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,14 @@ 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); >>> if (error) >>> path_put(path); >>> + >>> return error; >>> } >>> @@ -744,7 +752,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..ef6b74938dd8 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >> >> Hook description comment is missing. >> >>> @@ -1571,6 +1571,7 @@ 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); >>> int (*inode_need_killpriv)(struct dentry *dentry); >>> int (*inode_killpriv)(struct dentry *dentry); >>> int (*inode_getsecurity)(struct inode *inode, const char *name, >>> @@ -1881,6 +1882,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..50106fb9eef9 100644 >>> --- a/include/linux/security.h >>> +++ b/include/linux/security.h >>> @@ -301,6 +301,7 @@ 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); >>> int security_kernfs_init_security(struct kernfs_node *kn_dir, >>> struct kernfs_node *kn); >>> int security_file_permission(struct file *file, int mask); >>> @@ -392,6 +393,7 @@ 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); >>> int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); >>> + >> >> Please don't change whitespace unless it's directly adjacent to your code. >> >>> #else /* CONFIG_SECURITY */ >>> static inline int call_lsm_notifier(enum lsm_event event, void *data) >>> @@ -776,6 +778,11 @@ 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) >>> +{ >>> + 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..57b2a96c1991 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) >>> +{ >>> + return call_int_hook(inode_notify, 0, inode, mask); >>> +} >>> + >>> 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..1a37966c2978 100644 >>> --- a/security/selinux/hooks.c >>> +++ b/security/selinux/hooks.c >>> @@ -92,6 +92,7 @@ >>> #include <linux/kernfs.h> >>> #include <linux/stringhash.h> /* for hashlen_string() */ >>> #include <uapi/linux/mount.h> >>> +#include <linux/fsnotify.h> >>> #include "avc.h" >>> #include "objsec.h" >>> @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) >>> return -EACCES; >>> } >>> +static int selinux_inode_notify(struct inode *inode, u64 mask) >>> +{ >>> + u32 perm = FILE__WATCH; // basic permission, can a watch be set? >> >> We don't use // comments in the Linux kernel. >> >>> + >>> + struct common_audit_data ad; >>> + >>> + ad.type = LSM_AUDIT_DATA_INODE; >>> + ad.u.inode = inode; >>> + >>> + // check if the mask is requesting ability to set a blocking watch >>> + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM)) >>> + 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 +6818,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..0654dd2fbebf 100644 >>> --- a/security/selinux/include/classmap.h >>> +++ b/security/selinux/include/classmap.h >>> @@ -7,7 +7,7 @@ >>> #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ >>> "rename", "execute", "quotaon", "mounton", "audit_access", \ >>> - "open", "execmod" >>> + "open", "execmod", "watch", "watch_with_perm", "watch_reads" >>> #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ >>> "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ >> >
On Wed, 10 Jul 2019, Casey Schaufler wrote: > On 7/10/2019 6:34 AM, Aaron Goidel wrote: > > > 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. > > You're not saying why this is an issue. Also in the description, please explain the issues with read and write notifications and why a simple 'read' permission is not adequate.
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c index 250369d6901d..e91ce092efb1 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); + 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..c0d9fa998377 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -528,7 +528,7 @@ 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; @@ -567,8 +567,15 @@ 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; + } + + ret = security_inode_notify(path->dentry->d_inode, mask); if (ret) path_put(path); + out: return ret; } @@ -1014,7 +1021,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..47b079f20aad 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,14 @@ 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); if (error) path_put(path); + return error; } @@ -744,7 +752,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..ef6b74938dd8 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -1571,6 +1571,7 @@ 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); int (*inode_need_killpriv)(struct dentry *dentry); int (*inode_killpriv)(struct dentry *dentry); int (*inode_getsecurity)(struct inode *inode, const char *name, @@ -1881,6 +1882,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..50106fb9eef9 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -301,6 +301,7 @@ 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); int security_kernfs_init_security(struct kernfs_node *kn_dir, struct kernfs_node *kn); int security_file_permission(struct file *file, int mask); @@ -392,6 +393,7 @@ 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); int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); + #else /* CONFIG_SECURITY */ static inline int call_lsm_notifier(enum lsm_event event, void *data) @@ -776,6 +778,11 @@ 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) +{ + 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..57b2a96c1991 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) +{ + return call_int_hook(inode_notify, 0, inode, mask); +} + 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..1a37966c2978 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -92,6 +92,7 @@ #include <linux/kernfs.h> #include <linux/stringhash.h> /* for hashlen_string() */ #include <uapi/linux/mount.h> +#include <linux/fsnotify.h> #include "avc.h" #include "objsec.h" @@ -3261,6 +3262,26 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) return -EACCES; } +static int selinux_inode_notify(struct inode *inode, u64 mask) +{ + u32 perm = FILE__WATCH; // basic permission, can a watch be set? + + struct common_audit_data ad; + + ad.type = LSM_AUDIT_DATA_INODE; + ad.u.inode = inode; + + // check if the mask is requesting ability to set a blocking watch + if (mask & (FS_OPEN_PERM | FS_OPEN_EXEC_PERM | FS_ACCESS_PERM)) + 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 +6818,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..0654dd2fbebf 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -7,7 +7,7 @@ #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ "rename", "execute", "quotaon", "mounton", "audit_access", \ - "open", "execmod" + "open", "execmod", "watch", "watch_with_perm", "watch_reads" #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \
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. 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. 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 both the inode and the mask of requested notification events. The mask has 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. Fanotify further 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. The selinux_inode_notify hook implementation works by adding three new file permissions: watch, watch_reads, and watch_with_perm (descriptions about which will follow). The hook then decides which subset of these permissions must be held by the requesting application based on the contents of the provided mask. 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 permission is the baseline permission for setting a watch on an object and is a requirement for any watch to be set whatsoever. It should be noted that having either of the other two permissions (watch_reads and watch_with_perm) does not imply the watch permission, though this could be changed if need be. 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> --- fs/notify/dnotify/dnotify.c | 14 +++++++++++--- fs/notify/fanotify/fanotify_user.c | 11 +++++++++-- fs/notify/inotify/inotify_user.c | 12 ++++++++++-- include/linux/lsm_hooks.h | 2 ++ include/linux/security.h | 7 +++++++ security/security.c | 5 +++++ security/selinux/hooks.c | 22 ++++++++++++++++++++++ security/selinux/include/classmap.h | 2 +- 8 files changed, 67 insertions(+), 8 deletions(-)