| Message ID | 20161221231506.19800-1-mic@digikod.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/21/2016 3:15 PM, Mickaël Salaün wrote: > Add a new LSM hook named inode_touch_atime which is needed to deny > indirect update of extended file attributes (i.e. access time) which are > not catched by the inode_setattr hook. By creating a new hook instead of > calling inode_setattr, we avoid to simulate a useless struct iattr. > > This hook allows to create read-only environments as with read-only > mount points. It can also take care of anonymous inodes. What security module would use this? > > Signed-off-by: Mickaël Salaün <mic@digikod.net> > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: Andy Lutomirski <luto@amacapital.net> > Cc: Casey Schaufler <casey@schaufler-ca.com> > Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> > Cc: Eric Paris <eparis@parisplace.org> > Cc: James Morris <james.l.morris@oracle.com> > Cc: John Johansen <john.johansen@canonical.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: Kentaro Takeda <takedakn@nttdata.co.jp> > Cc: Mimi Zohar <zohar@linux.vnet.ibm.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Serge E. Hallyn <serge@hallyn.com> > Cc: Stephen Smalley <sds@tycho.nsa.gov> > Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > fs/inode.c | 5 ++++- > include/linux/lsm_hooks.h | 8 ++++++++ > include/linux/security.h | 8 ++++++++ > security/security.c | 9 +++++++++ > 4 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/fs/inode.c b/fs/inode.c > index 88110fd0b282..8e7519196942 100644 > --- a/fs/inode.c > +++ b/fs/inode.c > @@ -1706,6 +1706,10 @@ void touch_atime(const struct path *path) > if (!__atime_needs_update(path, inode, false)) > return; > > + now = current_time(inode); > + if (security_inode_touch_atime(path, &now)) > + return; > + > if (!sb_start_write_trylock(inode->i_sb)) > return; > > @@ -1720,7 +1724,6 @@ void touch_atime(const struct path *path) > * We may also fail on filesystems that have the ability to make parts > * of the fs read only, e.g. subvolumes in Btrfs. > */ > - now = current_time(inode); > update_time(inode, &now, S_ATIME); > __mnt_drop_write(mnt); > skip_update: > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 558adfa5c8a8..e77051715e6b 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -428,6 +428,11 @@ > * security module does not know about attribute or a negative error code > * to abort the copy up. Note that the caller is responsible for reading > * and writing the xattrs as this hook is merely a filter. > + * @inode_touch_atime: > + * Check permission before updating access time. > + * @path contains the path structure for the file. > + * @ts contains the current time. > + * Return 0 if permission is granted. > * > * Security hooks for file operations > * > @@ -1458,6 +1463,8 @@ union security_list_options { > void (*inode_getsecid)(struct inode *inode, u32 *secid); > int (*inode_copy_up)(struct dentry *src, struct cred **new); > int (*inode_copy_up_xattr)(const char *name); > + int (*inode_touch_atime)(const struct path *path, > + const struct timespec *ts); > > int (*file_permission)(struct file *file, int mask); > int (*file_alloc_security)(struct file *file); > @@ -1731,6 +1738,7 @@ struct security_hook_heads { > struct list_head inode_getsecid; > struct list_head inode_copy_up; > struct list_head inode_copy_up_xattr; > + struct list_head inode_touch_atime; > struct list_head file_permission; > struct list_head file_alloc_security; > struct list_head file_free_security; > diff --git a/include/linux/security.h b/include/linux/security.h > index c2125e9093e8..619f44c290a5 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -30,6 +30,7 @@ > #include <linux/string.h> > #include <linux/mm.h> > #include <linux/fs.h> > +#include <linux/time.h> > > struct linux_binprm; > struct cred; > @@ -288,6 +289,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_touch_atime(const struct path *path, const struct timespec *ts); > int security_file_permission(struct file *file, int mask); > int security_file_alloc(struct file *file); > void security_file_free(struct file *file); > @@ -781,6 +783,12 @@ static inline int security_inode_copy_up_xattr(const char *name) > return -EOPNOTSUPP; > } > > +static inline int security_inode_touch_atime(const struct path *path, const > + struct timespec *ts) > +{ > + return 0; > +} > + > static inline int security_file_permission(struct file *file, int mask) > { > return 0; > diff --git a/security/security.c b/security/security.c > index f825304f04a7..cd093c4b4115 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -769,6 +769,13 @@ int security_inode_copy_up_xattr(const char *name) > } > EXPORT_SYMBOL(security_inode_copy_up_xattr); > > +int security_inode_touch_atime(const struct path *path, > + const struct timespec *ts) > +{ > + return call_int_hook(inode_touch_atime, 0, path, ts); > +} > +EXPORT_SYMBOL(security_inode_touch_atime); > + > int security_file_permission(struct file *file, int mask) > { > int ret; > @@ -1711,6 +1718,8 @@ struct security_hook_heads security_hook_heads = { > LIST_HEAD_INIT(security_hook_heads.inode_copy_up), > .inode_copy_up_xattr = > LIST_HEAD_INIT(security_hook_heads.inode_copy_up_xattr), > + .inode_touch_atime = > + LIST_HEAD_INIT(security_hook_heads.inode_touch_atime), > .file_permission = > LIST_HEAD_INIT(security_hook_heads.file_permission), > .file_alloc_security = -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/12/2016 00:33, Casey Schaufler wrote: > On 12/21/2016 3:15 PM, Mickaël Salaün wrote: >> Add a new LSM hook named inode_touch_atime which is needed to deny >> indirect update of extended file attributes (i.e. access time) which are >> not catched by the inode_setattr hook. By creating a new hook instead of >> calling inode_setattr, we avoid to simulate a useless struct iattr. >> >> This hook allows to create read-only environments as with read-only >> mount points. It can also take care of anonymous inodes. > > What security module would use this? SELinux should be interested. This is useful to create sandboxes so other LSM may be interested too I'm working on a new LSM and I would like this kind of hook to create a real read-only environment. Regards, Mickaël
On 12/21/2016 4:01 PM, Mickaël Salaün wrote: > On 22/12/2016 00:33, Casey Schaufler wrote: >> On 12/21/2016 3:15 PM, Mickaël Salaün wrote: >>> Add a new LSM hook named inode_touch_atime which is needed to deny >>> indirect update of extended file attributes (i.e. access time) which are >>> not catched by the inode_setattr hook. By creating a new hook instead of >>> calling inode_setattr, we avoid to simulate a useless struct iattr. >>> >>> This hook allows to create read-only environments as with read-only >>> mount points. It can also take care of anonymous inodes. >> What security module would use this? > SELinux should be interested. This is useful to create sandboxes so > other LSM may be interested too You'll have to provide more information than that. I don't see how this hook would help there. > I'm working on a new LSM and I would like this kind of hook to create a > real read-only environment. I'm curious about how this hook would be used to do that. And about your module. You probably want to propose the hook(s) you want to add at the same time you propose your module. I am looking forward to reviewing it. > Regards, > Mickaël -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 22, 2016 at 01:01:39AM +0100, Mickaël Salaün wrote: > SELinux should be interested. This is useful to create sandboxes so > other LSM may be interested too > > I'm working on a new LSM and I would like this kind of hook to create a > real read-only environment. What the...? Have you noticed if (!sb_start_write_trylock(inode->i_sb)) return; if (__mnt_want_write(mnt) != 0) goto skip_update; in touch_atime()? Just mount them read-only in your sandbox (on either level - both per-mountpoint and per-fs r/o will do) and be done with that; why bother with LSM when regular tools would suffice? -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 22, 2016 at 12:15:06AM +0100, Mickaël Salaün wrote: > Add a new LSM hook named inode_touch_atime which is needed to deny > indirect update of extended file attributes (i.e. access time) which are > not catched by the inode_setattr hook. By creating a new hook instead of > calling inode_setattr, we avoid to simulate a useless struct iattr. > > This hook allows to create read-only environments as with read-only > mount points. It can also take care of anonymous inodes. And LSM has absolutely no business doing that - that's what the mount code is for. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 22/12/2016 01:57, Al Viro wrote: > On Thu, Dec 22, 2016 at 01:01:39AM +0100, Mickaël Salaün wrote: > >> SELinux should be interested. This is useful to create sandboxes so >> other LSM may be interested too >> >> I'm working on a new LSM and I would like this kind of hook to create a >> real read-only environment. > > What the...? Have you noticed > if (!sb_start_write_trylock(inode->i_sb)) > return; > > if (__mnt_want_write(mnt) != 0) > goto skip_update; > in touch_atime()? Just mount them read-only in your sandbox (on either > level - both per-mountpoint and per-fs r/o will do) and be done > with that; why bother with LSM when regular tools would suffice? > Of course a read-only mount point can do the trick (except for anonymous inodes). However, a security policy (e.g. for SELinux) should not (and can't always) rely on mount options. For example, a security policy can come from a distro but they may not want to tie mount options with this policy. We may also not want a sandbox to being able to change mount options (even with user namespaces). Being able to write (meta-)data, whereas a security policy said that it's not allowed, seems like a flaw in this policy. Moreover, modifying access time is an easy way to create cover-channels without any LSM being able to notice it.
On Thu, Dec 22, 2016 at 09:58:42AM +0100, Mickaël Salaün wrote: > Of course a read-only mount point can do the trick (except for anonymous > inodes). However, a security policy (e.g. for SELinux) should not (and > can't always) rely on mount options. For example, a security policy can > come from a distro but they may not want to tie mount options with this > policy. We may also not want a sandbox to being able to change mount > options (even with user namespaces). > > Being able to write (meta-)data, whereas a security policy said that > it's not allowed, seems like a flaw in this policy. Moreover, modifying > access time is an easy way to create cover-channels without any LSM > being able to notice it. A security policy must not mess with the readonly state of a file system or mount, period. You're overstepping your boundaries. -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/inode.c b/fs/inode.c index 88110fd0b282..8e7519196942 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -1706,6 +1706,10 @@ void touch_atime(const struct path *path) if (!__atime_needs_update(path, inode, false)) return; + now = current_time(inode); + if (security_inode_touch_atime(path, &now)) + return; + if (!sb_start_write_trylock(inode->i_sb)) return; @@ -1720,7 +1724,6 @@ void touch_atime(const struct path *path) * We may also fail on filesystems that have the ability to make parts * of the fs read only, e.g. subvolumes in Btrfs. */ - now = current_time(inode); update_time(inode, &now, S_ATIME); __mnt_drop_write(mnt); skip_update: diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 558adfa5c8a8..e77051715e6b 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -428,6 +428,11 @@ * security module does not know about attribute or a negative error code * to abort the copy up. Note that the caller is responsible for reading * and writing the xattrs as this hook is merely a filter. + * @inode_touch_atime: + * Check permission before updating access time. + * @path contains the path structure for the file. + * @ts contains the current time. + * Return 0 if permission is granted. * * Security hooks for file operations * @@ -1458,6 +1463,8 @@ union security_list_options { void (*inode_getsecid)(struct inode *inode, u32 *secid); int (*inode_copy_up)(struct dentry *src, struct cred **new); int (*inode_copy_up_xattr)(const char *name); + int (*inode_touch_atime)(const struct path *path, + const struct timespec *ts); int (*file_permission)(struct file *file, int mask); int (*file_alloc_security)(struct file *file); @@ -1731,6 +1738,7 @@ struct security_hook_heads { struct list_head inode_getsecid; struct list_head inode_copy_up; struct list_head inode_copy_up_xattr; + struct list_head inode_touch_atime; struct list_head file_permission; struct list_head file_alloc_security; struct list_head file_free_security; diff --git a/include/linux/security.h b/include/linux/security.h index c2125e9093e8..619f44c290a5 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -30,6 +30,7 @@ #include <linux/string.h> #include <linux/mm.h> #include <linux/fs.h> +#include <linux/time.h> struct linux_binprm; struct cred; @@ -288,6 +289,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_touch_atime(const struct path *path, const struct timespec *ts); int security_file_permission(struct file *file, int mask); int security_file_alloc(struct file *file); void security_file_free(struct file *file); @@ -781,6 +783,12 @@ static inline int security_inode_copy_up_xattr(const char *name) return -EOPNOTSUPP; } +static inline int security_inode_touch_atime(const struct path *path, const + struct timespec *ts) +{ + return 0; +} + static inline int security_file_permission(struct file *file, int mask) { return 0; diff --git a/security/security.c b/security/security.c index f825304f04a7..cd093c4b4115 100644 --- a/security/security.c +++ b/security/security.c @@ -769,6 +769,13 @@ int security_inode_copy_up_xattr(const char *name) } EXPORT_SYMBOL(security_inode_copy_up_xattr); +int security_inode_touch_atime(const struct path *path, + const struct timespec *ts) +{ + return call_int_hook(inode_touch_atime, 0, path, ts); +} +EXPORT_SYMBOL(security_inode_touch_atime); + int security_file_permission(struct file *file, int mask) { int ret; @@ -1711,6 +1718,8 @@ struct security_hook_heads security_hook_heads = { LIST_HEAD_INIT(security_hook_heads.inode_copy_up), .inode_copy_up_xattr = LIST_HEAD_INIT(security_hook_heads.inode_copy_up_xattr), + .inode_touch_atime = + LIST_HEAD_INIT(security_hook_heads.inode_touch_atime), .file_permission = LIST_HEAD_INIT(security_hook_heads.file_permission), .file_alloc_security =
Add a new LSM hook named inode_touch_atime which is needed to deny indirect update of extended file attributes (i.e. access time) which are not catched by the inode_setattr hook. By creating a new hook instead of calling inode_setattr, we avoid to simulate a useless struct iattr. This hook allows to create read-only environments as with read-only mount points. It can also take care of anonymous inodes. Signed-off-by: Mickaël Salaün <mic@digikod.net> Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Andy Lutomirski <luto@amacapital.net> Cc: Casey Schaufler <casey@schaufler-ca.com> Cc: Dmitry Kasatkin <dmitry.kasatkin@gmail.com> Cc: Eric Paris <eparis@parisplace.org> Cc: James Morris <james.l.morris@oracle.com> Cc: John Johansen <john.johansen@canonical.com> Cc: Kees Cook <keescook@chromium.org> Cc: Kentaro Takeda <takedakn@nttdata.co.jp> Cc: Mimi Zohar <zohar@linux.vnet.ibm.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Serge E. Hallyn <serge@hallyn.com> Cc: Stephen Smalley <sds@tycho.nsa.gov> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- fs/inode.c | 5 ++++- include/linux/lsm_hooks.h | 8 ++++++++ include/linux/security.h | 8 ++++++++ security/security.c | 9 +++++++++ 4 files changed, 29 insertions(+), 1 deletion(-)