[RFC,v4,1/2] fuse: introduce new fs_type flag FS_IMA_NO_CACHE

Commit Message

Dongsu Park Jan. 30, 2018, 6:06 p.m. UTC

From: Alban Crequy <[e-mail]>

This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
re-appraised and re-audited each time. Cached integrity results should
not be used.

It is useful in FUSE because the userspace FUSE process can change the
underlying files at any time without notifying the kernel.

Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: Miklos Szeredi <[e-mail]>
Cc: Alexander Viro <[e-mail]>
Cc: Mimi Zohar <[e-mail]>
Cc: Dmitry Kasatkin <[e-mail]>
Cc: James Morris <[e-mail]>
Cc: Christoph Hellwig <[e-mail]>
Acked-by: "Serge E. Hallyn" <[e-mail]>
Acked-by: Seth Forshee <[e-mail]>
Tested-by: Dongsu Park <[e-mail]>
Signed-off-by: Alban Crequy <[e-mail]>
---
 fs/fuse/inode.c    | 2 +-
 include/linux/fs.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Mimi Zohar Feb. 2, 2018, 3:20 p.m. UTC | #1

Hi Miklos,

On Tue, 2018-01-30 at 19:06 +0100, Dongsu Park wrote:
> From: Alban Crequy <[e-mail]>
> 
> This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
> re-appraised and re-audited each time. Cached integrity results should
> not be used.
> 
> It is useful in FUSE because the userspace FUSE process can change the
> underlying files at any time without notifying the kernel.

Both IMA-measurement and IMA-appraisal cache the integrity results and
are dependent on the kernel to detect when a file changes in order to
clear the cached info and force the file to be re-evaluated.  This
detection was dependent on i_version changing.  For filesystems that
do not support i_version, remote or fuse filesystems, where the kernel
does not detect the file change, the file was measured and the
signature evaluated just once.

With commit a2a2c3c8580a ("ima: Use i_version only when filesystem
supports it"), which is being upstreamed in this open window,
i_version is considered an optimization.  If i_version is not enabled,
either because the local filesystem does not support it or the
filesystem wasn't mounted with i_version, the file will now always be
re-evaluated.

That patch does not address FUSE or remote filesystems, as the kernel
does not detect the change.  Further, even if the kernel could detect
the change, FUSE filesystems by definition are untrusted.

The original patches addressed FUSE filesystems, by defining a new IMA
policy option, forcing the file to be re-evaluated based on the
filesystem magic number.  All of the changes were in the IMA
subsystem.  These patches are the result of Christoph's comment on the
original patches saying, "ima has no business looking at either the
name _or_ the magic number."

Your help in resolving this problem is much appreciated!

Mimi

> 
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Miklos Szeredi <[e-mail]>
> Cc: Alexander Viro <[e-mail]>
> Cc: Mimi Zohar <[e-mail]>
> Cc: Dmitry Kasatkin <[e-mail]>
> Cc: James Morris <[e-mail]>
> Cc: Christoph Hellwig <[e-mail]>
> Acked-by: "Serge E. Hallyn" <[e-mail]>
> Acked-by: Seth Forshee <[e-mail]>
> Tested-by: Dongsu Park <[e-mail]>
> Signed-off-by: Alban Crequy <[e-mail]>
> ---
>  fs/fuse/inode.c    | 2 +-
>  include/linux/fs.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 624f18bb..0a9e5164 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1205,7 +1205,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
>  static struct file_system_type fuse_fs_type = {
>  	.owner		= THIS_MODULE,
>  	.name		= "fuse",
> -	.fs_flags	= FS_HAS_SUBTYPE,
> +	.fs_flags	= FS_HAS_SUBTYPE | FS_IMA_NO_CACHE,
>  	.mount		= fuse_mount,
>  	.kill_sb	= fuse_kill_sb_anon,
>  };
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 511fbaab..ced841ba 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2075,6 +2075,7 @@ struct file_system_type {
>  #define FS_BINARY_MOUNTDATA	2
>  #define FS_HAS_SUBTYPE		4
>  #define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
> +#define FS_IMA_NO_CACHE		16	/* Force IMA to re-measure, re-appraise, re-audit files */
>  #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
>  	struct dentry *(*mount) (struct file_system_type *, int,
>  		       const char *, void *);

Mimi Zohar Feb. 2, 2018, 3:33 p.m. UTC | #2

On Fri, 2018-02-02 at 10:20 -0500, Mimi Zohar wrote:
> Hi Miklos,
> 
> On Tue, 2018-01-30 at 19:06 +0100, Dongsu Park wrote:
> > From: Alban Crequy <[e-mail]>
> > 
> > This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
> > re-appraised and re-audited each time. Cached integrity results should
> > not be used.
> > 
> > It is useful in FUSE because the userspace FUSE process can change the
> > underlying files at any time without notifying the kernel.
> 
> Both IMA-measurement and IMA-appraisal cache the integrity results and
> are dependent on the kernel to detect when a file changes in order to
> clear the cached info and force the file to be re-evaluated.  This
> detection was dependent on i_version changing.  For filesystems that
> do not support i_version, remote or fuse filesystems, where the kernel
> does not detect the file change, the file was measured and the
> signature evaluated just once.
> 
> With commit a2a2c3c8580a ("ima: Use i_version only when filesystem
> supports it"), which is being upstreamed in this open window,
> i_version is considered an optimization.  If i_version is not enabled,
> either because the local filesystem does not support it or the
> filesystem wasn't mounted with i_version, the file will now always be
> re-evaluated.
> 
> That patch does not address FUSE or remote filesystems, as the kernel
> does not detect the change.  Further, even if the kernel could detect
> the change, FUSE filesystems by definition are untrusted.
> 
> The original patches addressed FUSE filesystems, by defining a new IMA
> policy option, forcing the file to be re-evaluated based on the
> filesystem magic number.  All of the changes were in the IMA
> subsystem.  These patches are the result of Christoph's comment on the
> original patches saying, "ima has no business looking at either the
> name _or_ the magic number."
> 
> Your help in resolving this problem is much appreciated!

Meaning, can you ack the fuse flag addition so we can take the series
through the IMA tree?

thanks,

Mimi

> 
> > 
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: Miklos Szeredi <[e-mail]>
> > Cc: Alexander Viro <[e-mail]>
> > Cc: Mimi Zohar <[e-mail]>
> > Cc: Dmitry Kasatkin <[e-mail]>
> > Cc: James Morris <[e-mail]>
> > Cc: Christoph Hellwig <[e-mail]>
> > Acked-by: "Serge E. Hallyn" <[e-mail]>
> > Acked-by: Seth Forshee <[e-mail]>
> > Tested-by: Dongsu Park <[e-mail]>
> > Signed-off-by: Alban Crequy <[e-mail]>
> > ---
> >  fs/fuse/inode.c    | 2 +-
> >  include/linux/fs.h | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 624f18bb..0a9e5164 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1205,7 +1205,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
> >  static struct file_system_type fuse_fs_type = {
> >  	.owner		= THIS_MODULE,
> >  	.name		= "fuse",
> > -	.fs_flags	= FS_HAS_SUBTYPE,
> > +	.fs_flags	= FS_HAS_SUBTYPE | FS_IMA_NO_CACHE,
> >  	.mount		= fuse_mount,
> >  	.kill_sb	= fuse_kill_sb_anon,
> >  };
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 511fbaab..ced841ba 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2075,6 +2075,7 @@ struct file_system_type {
> >  #define FS_BINARY_MOUNTDATA	2
> >  #define FS_HAS_SUBTYPE		4
> >  #define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
> > +#define FS_IMA_NO_CACHE		16	/* Force IMA to re-measure, re-appraise, re-audit files */
> >  #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
> >  	struct dentry *(*mount) (struct file_system_type *, int,
> >  		       const char *, void *);
>

Miklos Szeredi Feb. 2, 2018, 4:10 p.m. UTC | #3

On Fri, Feb 2, 2018 at 4:33 PM, Mimi Zohar <[e-mail]> wrote:
> On Fri, 2018-02-02 at 10:20 -0500, Mimi Zohar wrote:
>> Hi Miklos,
>>
>> On Tue, 2018-01-30 at 19:06 +0100, Dongsu Park wrote:
>> > From: Alban Crequy <[e-mail]>
>> >
>> > This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
>> > re-appraised and re-audited each time. Cached integrity results should
>> > not be used.
>> >
>> > It is useful in FUSE because the userspace FUSE process can change the
>> > underlying files at any time without notifying the kernel.

I don't really have an understanding what IMA is doing, I think the
same thing applies to any network filesystem (i.e. ones with
d_revalidate).

Isn't that the case?

Thanks,
Miklos

Mimi Zohar Feb. 2, 2018, 4:59 p.m. UTC | #4

On Fri, 2018-02-02 at 17:10 +0100, Miklos Szeredi wrote:
> On Fri, Feb 2, 2018 at 4:33 PM, Mimi Zohar <[e-mail]> wrote:
> > On Fri, 2018-02-02 at 10:20 -0500, Mimi Zohar wrote:
> >> Hi Miklos,
> >>
> >> On Tue, 2018-01-30 at 19:06 +0100, Dongsu Park wrote:
> >> > From: Alban Crequy <[e-mail]>
> >> >
> >> > This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
> >> > re-appraised and re-audited each time. Cached integrity results should
> >> > not be used.
> >> >
> >> > It is useful in FUSE because the userspace FUSE process can change the
> >> > underlying files at any time without notifying the kernel.
> 
> I don't really have an understanding what IMA is doing, I think the
> same thing applies to any network filesystem (i.e. ones with
> d_revalidate).
> 
> Isn't that the case?

IMA is calculating the file hash, for inclusion in the measurement
list, verifying the file signature stored in the xattr, or both.  For
the remote filesystem case, re-calculating the file hash would be
limited to inclusion in the measurement list.  For FUSE, the kernel
has access to the xattr, so re-calculating the file hash could also be
used to re-verify the file signature.

Mimi

Alban Crequy Feb. 5, 2018, 2:16 p.m. UTC | #5

On Fri, Feb 2, 2018 at 5:10 PM, Miklos Szeredi <[e-mail]> wrote:
> On Fri, Feb 2, 2018 at 4:33 PM, Mimi Zohar <[e-mail]> wrote:
>> On Fri, 2018-02-02 at 10:20 -0500, Mimi Zohar wrote:
>>> Hi Miklos,
>>>
>>> On Tue, 2018-01-30 at 19:06 +0100, Dongsu Park wrote:
>>> > From: Alban Crequy <[e-mail]>
>>> >
>>> > This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
>>> > re-appraised and re-audited each time. Cached integrity results should
>>> > not be used.
>>> >
>>> > It is useful in FUSE because the userspace FUSE process can change the
>>> > underlying files at any time without notifying the kernel.
>
> I don't really have an understanding what IMA is doing, I think the
> same thing applies to any network filesystem (i.e. ones with
> d_revalidate).
>
> Isn't that the case?

Hi Miklos,

From my limited understanding, network filesystems might need that
too, yes. I don't know if there are people interested in using both
IMA and network filesystems. If so, they would have to write that
patch and test it.

It is not a new issue, for neither network filesystems or FUSE. But I
am more interested in the FUSE use case because FUSE can be mounted by
unprivileged users either today with fusermount installed with setuid,
or soon with the coming patches to allow FUSE mounts in a non-init
user namespace. That makes the issue more visible than for network
filesystems where unprivileged users cannot mount.

Cheers,
Alban

Miklos Szeredi Feb. 7, 2018, 9:21 a.m. UTC | #6

On Tue, Jan 30, 2018 at 7:06 PM, Dongsu Park <[e-mail]> wrote:
> From: Alban Crequy <[e-mail]>
>
> This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
> re-appraised and re-audited each time. Cached integrity results should
> not be used.
>
> It is useful in FUSE because the userspace FUSE process can change the
> underlying files at any time without notifying the kernel.
>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: Miklos Szeredi <[e-mail]>
> Cc: Alexander Viro <[e-mail]>
> Cc: Mimi Zohar <[e-mail]>
> Cc: Dmitry Kasatkin <[e-mail]>
> Cc: James Morris <[e-mail]>
> Cc: Christoph Hellwig <[e-mail]>
> Acked-by: "Serge E. Hallyn" <[e-mail]>
> Acked-by: Seth Forshee <[e-mail]>
> Tested-by: Dongsu Park <[e-mail]>
> Signed-off-by: Alban Crequy <[e-mail]>
> ---
>  fs/fuse/inode.c    | 2 +-
>  include/linux/fs.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 624f18bb..0a9e5164 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1205,7 +1205,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
>  static struct file_system_type fuse_fs_type = {
>         .owner          = THIS_MODULE,
>         .name           = "fuse",
> -       .fs_flags       = FS_HAS_SUBTYPE,
> +       .fs_flags       = FS_HAS_SUBTYPE | FS_IMA_NO_CACHE,
>         .mount          = fuse_mount,
>         .kill_sb        = fuse_kill_sb_anon,
>  };
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 511fbaab..ced841ba 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2075,6 +2075,7 @@ struct file_system_type {
>  #define FS_BINARY_MOUNTDATA    2
>  #define FS_HAS_SUBTYPE         4
>  #define FS_USERNS_MOUNT                8       /* Can be mounted by userns root */
> +#define FS_IMA_NO_CACHE                16      /* Force IMA to re-measure, re-appraise, re-audit files */

I think it would be more logical to change the order of the patches
(i.e. first patch adds this constant and the code handling it, and
second patch just adds it to fuse's .fs_flags).

Otherwise

Acked-by:   Miklos Szeredi <[e-mail]>

Thanks,
Miklos


>  #define FS_RENAME_DOES_D_MOVE  32768   /* FS will handle d_move() during rename() internally. */
>         struct dentry *(*mount) (struct file_system_type *, int,
>                        const char *, void *);
> --
> 2.13.6
>

Mimi Zohar Feb. 7, 2018, 1:05 p.m. UTC | #7

On Wed, 2018-02-07 at 10:21 +0100, Miklos Szeredi wrote:
> On Tue, Jan 30, 2018 at 7:06 PM, Dongsu Park <[e-mail]> wrote:
> > From: Alban Crequy <[e-mail]>
> >
> > This new fs_type flag FS_IMA_NO_CACHE means files should be re-measured,
> > re-appraised and re-audited each time. Cached integrity results should
> > not be used.
> >
> > It is useful in FUSE because the userspace FUSE process can change the
> > underlying files at any time without notifying the kernel.
> >
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: Miklos Szeredi <[e-mail]>
> > Cc: Alexander Viro <[e-mail]>
> > Cc: Mimi Zohar <[e-mail]>
> > Cc: Dmitry Kasatkin <[e-mail]>
> > Cc: James Morris <[e-mail]>
> > Cc: Christoph Hellwig <[e-mail]>
> > Acked-by: "Serge E. Hallyn" <[e-mail]>
> > Acked-by: Seth Forshee <[e-mail]>
> > Tested-by: Dongsu Park <[e-mail]>
> > Signed-off-by: Alban Crequy <[e-mail]>
> > ---
> >  fs/fuse/inode.c    | 2 +-
> >  include/linux/fs.h | 1 +
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 624f18bb..0a9e5164 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1205,7 +1205,7 @@ static void fuse_kill_sb_anon(struct super_block *sb)
> >  static struct file_system_type fuse_fs_type = {
> >         .owner          = THIS_MODULE,
> >         .name           = "fuse",
> > -       .fs_flags       = FS_HAS_SUBTYPE,
> > +       .fs_flags       = FS_HAS_SUBTYPE | FS_IMA_NO_CACHE,
> >         .mount          = fuse_mount,
> >         .kill_sb        = fuse_kill_sb_anon,
> >  };
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 511fbaab..ced841ba 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2075,6 +2075,7 @@ struct file_system_type {
> >  #define FS_BINARY_MOUNTDATA    2
> >  #define FS_HAS_SUBTYPE         4
> >  #define FS_USERNS_MOUNT                8       /* Can be mounted by userns root */
> > +#define FS_IMA_NO_CACHE                16      /* Force IMA to re-measure, re-appraise, re-audit files */
> 
> I think it would be more logical to change the order of the patches
> (i.e. first patch adds this constant and the code handling it, and
> second patch just adds it to fuse's .fs_flags).
> 
> Otherwise
> 
> Acked-by:   Miklos Szeredi <[e-mail]>

Sure, thank you!

Mimi

Patch

diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 624f18bb..0a9e5164 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1205,7 +1205,7 @@  static void fuse_kill_sb_anon(struct super_block *sb)
 static struct file_system_type fuse_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "fuse",
-	.fs_flags	= FS_HAS_SUBTYPE,
+	.fs_flags	= FS_HAS_SUBTYPE | FS_IMA_NO_CACHE,
 	.mount		= fuse_mount,
 	.kill_sb	= fuse_kill_sb_anon,
 };
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 511fbaab..ced841ba 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2075,6 +2075,7 @@  struct file_system_type {
 #define FS_BINARY_MOUNTDATA	2
 #define FS_HAS_SUBTYPE		4
 #define FS_USERNS_MOUNT		8	/* Can be mounted by userns root */
+#define FS_IMA_NO_CACHE		16	/* Force IMA to re-measure, re-appraise, re-audit files */
 #define FS_RENAME_DOES_D_MOVE	32768	/* FS will handle d_move() during rename() internally. */
 	struct dentry *(*mount) (struct file_system_type *, int,
 		       const char *, void *);