[PATCH v1] security: Add a new hook: inode_touch_atime

[PATCH v1] security: Add a new hook: inode_touch_atime

[Mickaël Salaün]

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(-)

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 =
-- 
2.11.0

Re: [PATCH v1] security: Add a new hook: inode_touch_atime

[Casey Schaufler]

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 =

Re: [PATCH v1] security: Add a new hook: inode_touch_atime

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 808 bytes --]


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] security: Add a new hook: inode_touch_atime

[Casey Schaufler]

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

Re: [PATCH v1] security: Add a new hook: inode_touch_atime

[Al Viro]

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?

Re: [PATCH v1] security: Add a new hook: inode_touch_atime

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 1390 bytes --]


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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1] security: Add a new hook: inode_touch_atime

[Christoph Hellwig]

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.

Re: [PATCH v1] security: Add a new hook: inode_touch_atime

[Christoph Hellwig]

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.