[RFC] ima: force the re-evaluation of files on untrusted file systems

Commit Message

Mimi Zohar Feb. 5, 2018, 1:40 p.m. UTC

On filesystems, such as fuse or remote filesystems, that we can not
detect or rely on the filesystem to tell us when a file has changed,
always re-measure, re-appraise, and/or re-audit the file.

Signed-of-by: Mimi Zohar <[e-mail]>

---
Hi Miklos,

Was something like this what you had in mind?

Mimi
---
 security/integrity/ima/ima_main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

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

On Mon, Feb 5, 2018 at 2:40 PM, Mimi Zohar <[e-mail]> wrote:
> On filesystems, such as fuse or remote filesystems, that we can not
> detect or rely on the filesystem to tell us when a file has changed,
> always re-measure, re-appraise, and/or re-audit the file.
>
> Signed-of-by: Mimi Zohar <[e-mail]>
>
> ---
> Hi Miklos,
>
> Was something like this what you had in mind?
>
> Mimi
> ---
>  security/integrity/ima/ima_main.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 6d78cb26784d..a428bd75232e 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -170,6 +170,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>                                int mask, enum ima_hooks func, int opened)
>  {
>         struct inode *inode = file_inode(file);
> +       struct dentry *dentry = file_dentry(file);
>         struct integrity_iint_cache *iint = NULL;
>         struct ima_template_desc *template_desc;
>         char *pathbuf = NULL;
> @@ -228,9 +229,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
>                                  IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
>                                  IMA_ACTION_FLAGS);
>
> -       if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags))
> -               /* reset all flags if ima_inode_setxattr was called */
> +       /*
> +        * Re-measure, re-appraise, and/or re-audit a file, if the security
> +        * xattrs changed or if the file is on an untrusted file system
> +        * (eg. FUSE, remote filesystems).
> +        */
> +       if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
> +           (dentry->d_op && dentry->d_op->d_revalidate)) {

It seems dangerous to rely implicitly on "d_revalidate != NULL". vfat
has a d_revalidate for handling 8.3 filenames but it's not a network
filesystem.

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

On Mon, 2018-02-05 at 15:24 +0100, Alban Crequy wrote:
> On Mon, Feb 5, 2018 at 2:40 PM, Mimi Zohar <[e-mail]> wrote:
> > On filesystems, such as fuse or remote filesystems, that we can not
> > detect or rely on the filesystem to tell us when a file has changed,
> > always re-measure, re-appraise, and/or re-audit the file.
> >
> > Signed-of-by: Mimi Zohar <[e-mail]>
> >
> > ---
> > Hi Miklos,
> >
> > Was something like this what you had in mind?
> >
> > Mimi
> > ---
> >  security/integrity/ima/ima_main.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 6d78cb26784d..a428bd75232e 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -170,6 +170,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> >                                int mask, enum ima_hooks func, int opened)
> >  {
> >         struct inode *inode = file_inode(file);
> > +       struct dentry *dentry = file_dentry(file);
> >         struct integrity_iint_cache *iint = NULL;
> >         struct ima_template_desc *template_desc;
> >         char *pathbuf = NULL;
> > @@ -228,9 +229,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
> >                                  IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
> >                                  IMA_ACTION_FLAGS);
> >
> > -       if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags))
> > -               /* reset all flags if ima_inode_setxattr was called */
> > +       /*
> > +        * Re-measure, re-appraise, and/or re-audit a file, if the security
> > +        * xattrs changed or if the file is on an untrusted file system
> > +        * (eg. FUSE, remote filesystems).
> > +        */
> > +       if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
> > +           (dentry->d_op && dentry->d_op->d_revalidate)) {
> 
> It seems dangerous to rely implicitly on "d_revalidate != NULL". vfat
> has a d_revalidate for handling 8.3 filenames but it's not a network
> filesystem.

Files might be unnecessarily re-evaluated, impacting performance, but
I'm not sure that it is dangerous.  For example, local OCFS2 files are
unnecessarily re-evaluated.

Mimi

James Bottomley Feb. 5, 2018, 3:50 p.m. UTC | #3

On Mon, 2018-02-05 at 08:40 -0500, Mimi Zohar wrote:
> On filesystems, such as fuse or remote filesystems, that we can not
> detect or rely on the filesystem to tell us when a file has changed,
> always re-measure, re-appraise, and/or re-audit the file.

Using the presence or absence of d_revalidate isn't definitive for
uncacheable appraisals: all stacked filesystems have to implement
d_revalidate just in case the underlying has it, but it doesn't mean
their appraisals can't be cached if they're fully built on top of
traditional filesystems (like they are in the Docker/OCI use case).  I
think the original flag approach is better.  The only thing stackable
filesystems argues for is that for them it should probably be a
superblock flag so it can be per-mount point (depending on overlay
composition).

d_revalidate() also strikes me as wrong from the semantic point of
view: it's about whether the path name to inode cache needs re-
evaluating not whether the underlying inode could change arbitrarily.
 These are definitely related but not necessarily equivalent concepts.

James

Miklos Szeredi Feb. 5, 2018, 4:12 p.m. UTC | #4

On Mon, Feb 5, 2018 at 4:50 PM, James Bottomley
<[e-mail]> wrote:
> On Mon, 2018-02-05 at 08:40 -0500, Mimi Zohar wrote:
>> On filesystems, such as fuse or remote filesystems, that we can not
>> detect or rely on the filesystem to tell us when a file has changed,
>> always re-measure, re-appraise, and/or re-audit the file.
>
> Using the presence or absence of d_revalidate isn't definitive for
> uncacheable appraisals: all stacked filesystems have to implement
> d_revalidate just in case the underlying has it, but it doesn't mean
> their appraisals can't be cached if they're fully built on top of
> traditional filesystems (like they are in the Docker/OCI use case).  I
> think the original flag approach is better.  The only thing stackable
> filesystems argues for is that for them it should probably be a
> superblock flag so it can be per-mount point (depending on overlay
> composition).
>
> d_revalidate() also strikes me as wrong from the semantic point of
> view: it's about whether the path name to inode cache needs re-
> evaluating not whether the underlying inode could change arbitrarily.
>  These are definitely related but not necessarily equivalent concepts.

True.  A more precise indication is whether cache pages have been
invalidated for a certain inode.   Can we used that?  I.e.
invalidate_inode_pages*() calls down into IMA or sets a flags or
whatever to indicate that the file contents might have changed.

Thanks,
Miklos

Mimi Zohar Feb. 5, 2018, 4:21 p.m. UTC | #5

On Mon, 2018-02-05 at 17:12 +0100, Miklos Szeredi wrote:
> On Mon, Feb 5, 2018 at 4:50 PM, James Bottomley
> <[e-mail]> wrote:
> > On Mon, 2018-02-05 at 08:40 -0500, Mimi Zohar wrote:
> >> On filesystems, such as fuse or remote filesystems, that we can not
> >> detect or rely on the filesystem to tell us when a file has changed,
> >> always re-measure, re-appraise, and/or re-audit the file.
> >
> > Using the presence or absence of d_revalidate isn't definitive for
> > uncacheable appraisals: all stacked filesystems have to implement
> > d_revalidate just in case the underlying has it, but it doesn't mean
> > their appraisals can't be cached if they're fully built on top of
> > traditional filesystems (like they are in the Docker/OCI use case).  I
> > think the original flag approach is better.  The only thing stackable
> > filesystems argues for is that for them it should probably be a
> > superblock flag so it can be per-mount point (depending on overlay
> > composition).
> >
> > d_revalidate() also strikes me as wrong from the semantic point of
> > view: it's about whether the path name to inode cache needs re-
> > evaluating not whether the underlying inode could change arbitrarily.
> >  These are definitely related but not necessarily equivalent concepts.
> 
> True.  A more precise indication is whether cache pages have been
> invalidated for a certain inode.   Can we used that?  I.e.
> invalidate_inode_pages*() calls down into IMA or sets a flags or
> whatever to indicate that the file contents might have changed.

I don't think that works for the FUSE use case.

Mimi

Miklos Szeredi Feb. 5, 2018, 4:30 p.m. UTC | #6

On Mon, Feb 5, 2018 at 5:21 PM, Mimi Zohar <[e-mail]> wrote:
> On Mon, 2018-02-05 at 17:12 +0100, Miklos Szeredi wrote:
>> On Mon, Feb 5, 2018 at 4:50 PM, James Bottomley
>> <[e-mail]> wrote:
>> > On Mon, 2018-02-05 at 08:40 -0500, Mimi Zohar wrote:
>> >> On filesystems, such as fuse or remote filesystems, that we can not
>> >> detect or rely on the filesystem to tell us when a file has changed,
>> >> always re-measure, re-appraise, and/or re-audit the file.
>> >
>> > Using the presence or absence of d_revalidate isn't definitive for
>> > uncacheable appraisals: all stacked filesystems have to implement
>> > d_revalidate just in case the underlying has it, but it doesn't mean
>> > their appraisals can't be cached if they're fully built on top of
>> > traditional filesystems (like they are in the Docker/OCI use case).  I
>> > think the original flag approach is better.  The only thing stackable
>> > filesystems argues for is that for them it should probably be a
>> > superblock flag so it can be per-mount point (depending on overlay
>> > composition).
>> >
>> > d_revalidate() also strikes me as wrong from the semantic point of
>> > view: it's about whether the path name to inode cache needs re-
>> > evaluating not whether the underlying inode could change arbitrarily.
>> >  These are definitely related but not necessarily equivalent concepts.
>>
>> True.  A more precise indication is whether cache pages have been
>> invalidated for a certain inode.   Can we used that?  I.e.
>> invalidate_inode_pages*() calls down into IMA or sets a flags or
>> whatever to indicate that the file contents might have changed.
>
> I don't think that works for the FUSE use case.

Okay, it's true that cache invalidation is just a hint about file
contents changing.  The file contents could change without cache
invalidation if userspace filesystem is buggy or malicious.

Thanks,
Miklos

Mimi Zohar Feb. 5, 2018, 4:37 p.m. UTC | #7

On Mon, 2018-02-05 at 17:30 +0100, Miklos Szeredi wrote:
> On Mon, Feb 5, 2018 at 5:21 PM, Mimi Zohar <[e-mail]> wrote:
> > On Mon, 2018-02-05 at 17:12 +0100, Miklos Szeredi wrote:
> >> On Mon, Feb 5, 2018 at 4:50 PM, James Bottomley
> >> <[e-mail]> wrote:
> >> > On Mon, 2018-02-05 at 08:40 -0500, Mimi Zohar wrote:
> >> >> On filesystems, such as fuse or remote filesystems, that we can not
> >> >> detect or rely on the filesystem to tell us when a file has changed,
> >> >> always re-measure, re-appraise, and/or re-audit the file.
> >> >
> >> > Using the presence or absence of d_revalidate isn't definitive for
> >> > uncacheable appraisals: all stacked filesystems have to implement
> >> > d_revalidate just in case the underlying has it, but it doesn't mean
> >> > their appraisals can't be cached if they're fully built on top of
> >> > traditional filesystems (like they are in the Docker/OCI use case).  I
> >> > think the original flag approach is better.  The only thing stackable
> >> > filesystems argues for is that for them it should probably be a
> >> > superblock flag so it can be per-mount point (depending on overlay
> >> > composition).
> >> >
> >> > d_revalidate() also strikes me as wrong from the semantic point of
> >> > view: it's about whether the path name to inode cache needs re-
> >> > evaluating not whether the underlying inode could change arbitrarily.
> >> >  These are definitely related but not necessarily equivalent concepts.
> >>
> >> True.  A more precise indication is whether cache pages have been
> >> invalidated for a certain inode.   Can we used that?  I.e.
> >> invalidate_inode_pages*() calls down into IMA or sets a flags or
> >> whatever to indicate that the file contents might have changed.
> >
> > I don't think that works for the FUSE use case.
> 
> Okay, it's true that cache invalidation is just a hint about file
> contents changing.  The file contents could change without cache
> invalidation if userspace filesystem is buggy or malicious.

Right, the untrusted, malicious userspace filesystem is the reason for
Alban's patches.  Can you review/ack those patches?

thanks,

Mimi

Patch

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6d78cb26784d..a428bd75232e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -170,6 +170,7 @@  static int process_measurement(struct file *file, char *buf, loff_t size,
 			       int mask, enum ima_hooks func, int opened)
 {
 	struct inode *inode = file_inode(file);
+	struct dentry *dentry = file_dentry(file);
 	struct integrity_iint_cache *iint = NULL;
 	struct ima_template_desc *template_desc;
 	char *pathbuf = NULL;
@@ -228,9 +229,16 @@  static int process_measurement(struct file *file, char *buf, loff_t size,
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
 				 IMA_ACTION_FLAGS);
 
-	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags))
-		/* reset all flags if ima_inode_setxattr was called */
+	/*
+	 * Re-measure, re-appraise, and/or re-audit a file, if the security
+	 * xattrs changed or if the file is on an untrusted file system
+	 * (eg. FUSE, remote filesystems).
+	 */
+	if (test_and_clear_bit(IMA_CHANGE_XATTR, &iint->atomic_flags) ||
+	    (dentry->d_op && dentry->d_op->d_revalidate)) {
 		iint->flags &= ~IMA_DONE_MASK;
+		iint->measured_pcrs = 0;
+	}
 
 	/* Determine if already appraised/measured based on bitmask
 	 * (IMA_MEASURE, IMA_MEASURED, IMA_XXXX_APPRAISE, IMA_XXXX_APPRAISED,