[PATCH v2] integrity: track mtime in addition to i_version for assessment

[PATCH v2] integrity: track mtime in addition to i_version for assessment

[Jeff Layton]

From: Jeff Layton <jlayton@redhat.com>

The IMA assessment code tries to use the i_version counter to detect
when changes to a file have occurred. Many filesystems don't increment
it properly (or at all) so detecting changes with that is not always
reliable.

That check should be gated on IS_I_VERSION, as you can't rely on the
i_version field changing unless that returns true.

Have the code also track and check the mtime for the file. If the
IS_I_VERSION returns false, then use it to detect whether the file's
contents might have changed.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 security/integrity/ima/ima_api.c  |  4 +++-
 security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
 security/integrity/integrity.h    |  1 +
 3 files changed, 28 insertions(+), 9 deletions(-)

v2: switch to storing/checking mtime instead of ctime

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c2edba8de35e..b8d746bbc43d 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 	} hash;
 
 	if (!(iint->flags & IMA_COLLECTED)) {
-		u64 i_version = file_inode(file)->i_version;
+		u64 i_version = inode->i_version;
+		struct timespec i_mtime = inode->i_mtime;
 
 		if (file->f_flags & O_DIRECT) {
 			audit_cause = "failed(directio)";
@@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 				iint->ima_hash = tmpbuf;
 				memcpy(iint->ima_hash, &hash, length);
 				iint->version = i_version;
+				iint->mtime = i_mtime;
 				iint->flags |= IMA_COLLECTED;
 			} else
 				result = -ENOMEM;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2aebb7984437..8d12ef2d3ba2 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
 				  "invalid_pcr", "open_writers");
 }
 
+static bool ima_should_update_iint(struct integrity_iint_cache *iint,
+				struct inode *inode)
+{
+	if (atomic_read(&inode->i_writecount) != 1)
+		return false;
+	if (iint->flags & IMA_NEW_FILE)
+		return true;
+	if (IS_I_VERSION(inode)) {
+		if (iint->version != inode->i_version)
+			return true;
+	} else {
+		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
+			return true;
+		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
+			return true;
+	}
+	return false;
+}
+
 static void ima_check_last_writer(struct integrity_iint_cache *iint,
 				  struct inode *inode, struct file *file)
 {
@@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 		return;
 
 	inode_lock(inode);
-	if (atomic_read(&inode->i_writecount) == 1) {
-		if ((iint->version != inode->i_version) ||
-		    (iint->flags & IMA_NEW_FILE)) {
-			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
-			iint->measured_pcrs = 0;
-			if (iint->flags & IMA_APPRAISE)
-				ima_update_xattr(iint, file);
-		}
+	if (ima_should_update_iint(iint, inode)) {
+		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
+		iint->measured_pcrs = 0;
+		if (iint->flags & IMA_APPRAISE)
+			ima_update_xattr(iint, file);
 	}
 	inode_unlock(inode);
 }
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index a53e7e4ab06c..61fffa7583bf 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -102,6 +102,7 @@ struct integrity_iint_cache {
 	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
 	struct inode *inode;	/* back pointer to inode in question */
 	u64 version;		/* track inode changes */
+	struct timespec mtime;	/* track inode changes */
 	unsigned long flags;
 	unsigned long measured_pcrs;
 	enum integrity_status ima_file_status:4;
-- 
2.13.0

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Jeff Layton]

On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> From: Jeff Layton <jlayton@redhat.com>
> 
> The IMA assessment code tries to use the i_version counter to detect
> when changes to a file have occurred. Many filesystems don't increment
> it properly (or at all) so detecting changes with that is not always
> reliable.
> 
> That check should be gated on IS_I_VERSION, as you can't rely on the
> i_version field changing unless that returns true.
> 
> Have the code also track and check the mtime for the file. If the
> IS_I_VERSION returns false, then use it to detect whether the file's
> contents might have changed.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  security/integrity/ima/ima_api.c  |  4 +++-
>  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
>  security/integrity/integrity.h    |  1 +
>  3 files changed, 28 insertions(+), 9 deletions(-)
> 
> v2: switch to storing/checking mtime instead of ctime
> 

To be clear here, I don't have a large interest in IMA, but I am looking
at making changes to how the i_version counter is handled. IMA's use of
it is problematic for some of those changes (and somewhat sketchy).

I think you either want something like the patch below, or you need to
somehow ensure that you're not doing any of this on a superblock that
doesn't have MS_I_VERSION set on it.

I'm not that familiar with IMA in general though, so it's possible I'm
missing something. Is that already being done somehow?
 
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c2edba8de35e..b8d746bbc43d 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>  	} hash;
>  
>  	if (!(iint->flags & IMA_COLLECTED)) {
> -		u64 i_version = file_inode(file)->i_version;
> +		u64 i_version = inode->i_version;
> +		struct timespec i_mtime = inode->i_mtime;
>  
>  		if (file->f_flags & O_DIRECT) {
>  			audit_cause = "failed(directio)";
> @@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>  				iint->ima_hash = tmpbuf;
>  				memcpy(iint->ima_hash, &hash, length);
>  				iint->version = i_version;
> +				iint->mtime = i_mtime;
>  				iint->flags |= IMA_COLLECTED;
>  			} else
>  				result = -ENOMEM;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2aebb7984437..8d12ef2d3ba2 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
>  				  "invalid_pcr", "open_writers");
>  }
>  
> +static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> +				struct inode *inode)
> +{
> +	if (atomic_read(&inode->i_writecount) != 1)
> +		return false;
> +	if (iint->flags & IMA_NEW_FILE)
> +		return true;
> +	if (IS_I_VERSION(inode)) {
> +		if (iint->version != inode->i_version)
> +			return true;
> +	} else {
> +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> +			return true;
> +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static void ima_check_last_writer(struct integrity_iint_cache *iint,
>  				  struct inode *inode, struct file *file)
>  {
> @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>  		return;
>  
>  	inode_lock(inode);
> -	if (atomic_read(&inode->i_writecount) == 1) {
> -		if ((iint->version != inode->i_version) ||
> -		    (iint->flags & IMA_NEW_FILE)) {
> -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> -			iint->measured_pcrs = 0;
> -			if (iint->flags & IMA_APPRAISE)
> -				ima_update_xattr(iint, file);
> -		}
> +	if (ima_should_update_iint(iint, inode)) {
> +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> +		iint->measured_pcrs = 0;
> +		if (iint->flags & IMA_APPRAISE)
> +			ima_update_xattr(iint, file);
>  	}
>  	inode_unlock(inode);
>  }
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index a53e7e4ab06c..61fffa7583bf 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -102,6 +102,7 @@ struct integrity_iint_cache {
>  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
>  	struct inode *inode;	/* back pointer to inode in question */
>  	u64 version;		/* track inode changes */
> +	struct timespec mtime;	/* track inode changes */
>  	unsigned long flags;
>  	unsigned long measured_pcrs;
>  	enum integrity_status ima_file_status:4;

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Mimi Zohar]

On Fri, 2017-07-07 at 12:57 -0400, Jeff Layton wrote:
> On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > The IMA assessment code tries to use the i_version counter to detect
> > when changes to a file have occurred. Many filesystems don't increment
> > it properly (or at all) so detecting changes with that is not always
> > reliable.
> > 
> > That check should be gated on IS_I_VERSION, as you can't rely on the
> > i_version field changing unless that returns true.
> > 
> > Have the code also track and check the mtime for the file. If the
> > IS_I_VERSION returns false, then use it to detect whether the file's
> > contents might have changed.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  security/integrity/ima/ima_api.c  |  4 +++-
> >  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
> >  security/integrity/integrity.h    |  1 +
> >  3 files changed, 28 insertions(+), 9 deletions(-)
> > 
> > v2: switch to storing/checking mtime instead of ctime
> > 
> 
> To be clear here, I don't have a large interest in IMA, but I am looking
> at making changes to how the i_version counter is handled. IMA's use of
> it is problematic for some of those changes (and somewhat sketchy).
> 
> I think you either want something like the patch below, or you need to
> somehow ensure that you're not doing any of this on a superblock that
> doesn't have MS_I_VERSION set on it.
> 
> I'm not that familiar with IMA in general though, so it's possible I'm
> missing something. Is that already being done somehow?

Before reverting to using mtime, which wasn't fine grained enough at
the time, it would be helpful to first understand the type of changes
and the reasons for the changes you're looking to make to i_version.
 After all, i_version has been working all this time (~2009).

Mimi

> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index c2edba8de35e..b8d746bbc43d 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> >  	} hash;
> >  
> >  	if (!(iint->flags & IMA_COLLECTED)) {
> > -		u64 i_version = file_inode(file)->i_version;
> > +		u64 i_version = inode->i_version;
> > +		struct timespec i_mtime = inode->i_mtime;
> >  
> >  		if (file->f_flags & O_DIRECT) {
> >  			audit_cause = "failed(directio)";
> > @@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> >  				iint->ima_hash = tmpbuf;
> >  				memcpy(iint->ima_hash, &hash, length);
> >  				iint->version = i_version;
> > +				iint->mtime = i_mtime;
> >  				iint->flags |= IMA_COLLECTED;
> >  			} else
> >  				result = -ENOMEM;
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 2aebb7984437..8d12ef2d3ba2 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
> >  				  "invalid_pcr", "open_writers");
> >  }
> >  
> > +static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> > +				struct inode *inode)
> > +{
> > +	if (atomic_read(&inode->i_writecount) != 1)
> > +		return false;
> > +	if (iint->flags & IMA_NEW_FILE)
> > +		return true;
> > +	if (IS_I_VERSION(inode)) {
> > +		if (iint->version != inode->i_version)
> > +			return true;
> > +	} else {
> > +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> > +			return true;
> > +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> > +			return true;
> > +	}
> > +	return false;
> > +}
> > +
> >  static void ima_check_last_writer(struct integrity_iint_cache *iint,
> >  				  struct inode *inode, struct file *file)
> >  {
> > @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> >  		return;
> >  
> >  	inode_lock(inode);
> > -	if (atomic_read(&inode->i_writecount) == 1) {
> > -		if ((iint->version != inode->i_version) ||
> > -		    (iint->flags & IMA_NEW_FILE)) {
> > -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > -			iint->measured_pcrs = 0;
> > -			if (iint->flags & IMA_APPRAISE)
> > -				ima_update_xattr(iint, file);
> > -		}
> > +	if (ima_should_update_iint(iint, inode)) {
> > +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > +		iint->measured_pcrs = 0;
> > +		if (iint->flags & IMA_APPRAISE)
> > +			ima_update_xattr(iint, file);
> >  	}
> >  	inode_unlock(inode);
> >  }
> > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> > index a53e7e4ab06c..61fffa7583bf 100644
> > --- a/security/integrity/integrity.h
> > +++ b/security/integrity/integrity.h
> > @@ -102,6 +102,7 @@ struct integrity_iint_cache {
> >  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
> >  	struct inode *inode;	/* back pointer to inode in question */
> >  	u64 version;		/* track inode changes */
> > +	struct timespec mtime;	/* track inode changes */
> >  	unsigned long flags;
> >  	unsigned long measured_pcrs;
> >  	enum integrity_status ima_file_status:4;
> 

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Jeff Layton]

On Fri, 2017-07-07 at 13:24 -0400, Mimi Zohar wrote:
> On Fri, 2017-07-07 at 12:57 -0400, Jeff Layton wrote:
> > On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> > > From: Jeff Layton <jlayton@redhat.com>
> > > 
> > > The IMA assessment code tries to use the i_version counter to detect
> > > when changes to a file have occurred. Many filesystems don't increment
> > > it properly (or at all) so detecting changes with that is not always
> > > reliable.
> > > 
> > > That check should be gated on IS_I_VERSION, as you can't rely on the
> > > i_version field changing unless that returns true.
> > > 
> > > Have the code also track and check the mtime for the file. If the
> > > IS_I_VERSION returns false, then use it to detect whether the file's
> > > contents might have changed.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  security/integrity/ima/ima_api.c  |  4 +++-
> > >  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
> > >  security/integrity/integrity.h    |  1 +
> > >  3 files changed, 28 insertions(+), 9 deletions(-)
> > > 
> > > v2: switch to storing/checking mtime instead of ctime
> > > 
> > 
> > To be clear here, I don't have a large interest in IMA, but I am looking
> > at making changes to how the i_version counter is handled. IMA's use of
> > it is problematic for some of those changes (and somewhat sketchy).
> > 
> > I think you either want something like the patch below, or you need to
> > somehow ensure that you're not doing any of this on a superblock that
> > doesn't have MS_I_VERSION set on it.
> > 
> > I'm not that familiar with IMA in general though, so it's possible I'm
> > missing something. Is that already being done somehow?
> 
> Before reverting to using mtime, which wasn't fine grained enough at
> the time, it would be helpful to first understand the type of changes
> and the reasons for the changes you're looking to make to i_version.

Sure, I posted a patchset back in December, actually:

    [RFC PATCH v1 00/30] fs: inode->i_version rework and optimization

The basic idea is to improve performance on filesystems that implement
the i_version counter by allowing them to optimize away metadata updates
that are due solely to i_version counter change when no one is actually
using it. This will also pave the way to allow us to more reasonably
provide an i_version counter on network filesystems and the like that
might have weaker consistency guarantees than a local fs.

Your point about mtime not being granular enough is valid however. It's
certainly possible for extra writes to race in during the jiffy or so
window that represents the mtime resolution. It's just that that is
still more granular than you'll get on a filesystem that never actually
increments the i_version counter on a write.

>  After all, i_version has been working all this time (~2009).
> 

The i_version counter works just fine on filesystems that implement it
properly. That's a very short list: xfs, btrfs, and ext4 for local
filesystems. NFS and AFS would also likely be fine here, though they
don't set MS_I_VERSION.

The rest though do not support it consistently and IMA should not be
relying on it on them. This is why the kernel nfs server only relies on
the i_version field when IS_I_VERSION returns true.

I'll ask again -- is IMA somehow limited only being used only on that
subset of filesystems? My guess from a glance at the integrity_read
patchset is that it is not.

> > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > > index c2edba8de35e..b8d746bbc43d 100644
> > > --- a/security/integrity/ima/ima_api.c
> > > +++ b/security/integrity/ima/ima_api.c
> > > @@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > >  	} hash;
> > >  
> > >  	if (!(iint->flags & IMA_COLLECTED)) {
> > > -		u64 i_version = file_inode(file)->i_version;
> > > +		u64 i_version = inode->i_version;
> > > +		struct timespec i_mtime = inode->i_mtime;
> > >  
> > >  		if (file->f_flags & O_DIRECT) {
> > >  			audit_cause = "failed(directio)";
> > > @@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > >  				iint->ima_hash = tmpbuf;
> > >  				memcpy(iint->ima_hash, &hash, length);
> > >  				iint->version = i_version;
> > > +				iint->mtime = i_mtime;
> > >  				iint->flags |= IMA_COLLECTED;
> > >  			} else
> > >  				result = -ENOMEM;
> > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > index 2aebb7984437..8d12ef2d3ba2 100644
> > > --- a/security/integrity/ima/ima_main.c
> > > +++ b/security/integrity/ima/ima_main.c
> > > @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
> > >  				  "invalid_pcr", "open_writers");
> > >  }
> > >  
> > > +static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> > > +				struct inode *inode)
> > > +{
> > > +	if (atomic_read(&inode->i_writecount) != 1)
> > > +		return false;
> > > +	if (iint->flags & IMA_NEW_FILE)
> > > +		return true;
> > > +	if (IS_I_VERSION(inode)) {
> > > +		if (iint->version != inode->i_version)
> > > +			return true;
> > > +	} else {
> > > +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> > > +			return true;
> > > +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> > > +			return true;
> > > +	}
> > > +	return false;
> > > +}
> > > +
> > >  static void ima_check_last_writer(struct integrity_iint_cache *iint,
> > >  				  struct inode *inode, struct file *file)
> > >  {
> > > @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> > >  		return;
> > >  
> > >  	inode_lock(inode);
> > > -	if (atomic_read(&inode->i_writecount) == 1) {
> > > -		if ((iint->version != inode->i_version) ||
> > > -		    (iint->flags & IMA_NEW_FILE)) {
> > > -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > > -			iint->measured_pcrs = 0;
> > > -			if (iint->flags & IMA_APPRAISE)
> > > -				ima_update_xattr(iint, file);
> > > -		}
> > > +	if (ima_should_update_iint(iint, inode)) {
> > > +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > > +		iint->measured_pcrs = 0;
> > > +		if (iint->flags & IMA_APPRAISE)
> > > +			ima_update_xattr(iint, file);
> > >  	}
> > >  	inode_unlock(inode);
> > >  }
> > > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> > > index a53e7e4ab06c..61fffa7583bf 100644
> > > --- a/security/integrity/integrity.h
> > > +++ b/security/integrity/integrity.h
> > > @@ -102,6 +102,7 @@ struct integrity_iint_cache {
> > >  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
> > >  	struct inode *inode;	/* back pointer to inode in question */
> > >  	u64 version;		/* track inode changes */
> > > +	struct timespec mtime;	/* track inode changes */
> > >  	unsigned long flags;
> > >  	unsigned long measured_pcrs;
> > >  	enum integrity_status ima_file_status:4;
> 
> 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Mimi Zohar]

On Fri, 2017-07-07 at 13:49 -0400, Jeff Layton wrote:
> On Fri, 2017-07-07 at 13:24 -0400, Mimi Zohar wrote:
> > On Fri, 2017-07-07 at 12:57 -0400, Jeff Layton wrote:
> > > On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> > > > From: Jeff Layton <jlayton@redhat.com>
> > > > 
> > > > The IMA assessment code tries to use the i_version counter to detect
> > > > when changes to a file have occurred. Many filesystems don't increment
> > > > it properly (or at all) so detecting changes with that is not always
> > > > reliable.
> > > > 
> > > > That check should be gated on IS_I_VERSION, as you can't rely on the
> > > > i_version field changing unless that returns true.
> > > > 
> > > > Have the code also track and check the mtime for the file. If the
> > > > IS_I_VERSION returns false, then use it to detect whether the file's
> > > > contents might have changed.
> > > > 
> > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > ---
> > > >  security/integrity/ima/ima_api.c  |  4 +++-
> > > >  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
> > > >  security/integrity/integrity.h    |  1 +
> > > >  3 files changed, 28 insertions(+), 9 deletions(-)
> > > > 
> > > > v2: switch to storing/checking mtime instead of ctime
> > > > 
> > > 
> > > To be clear here, I don't have a large interest in IMA, but I am looking
> > > at making changes to how the i_version counter is handled. IMA's use of
> > > it is problematic for some of those changes (and somewhat sketchy).
> > > 
> > > I think you either want something like the patch below, or you need to
> > > somehow ensure that you're not doing any of this on a superblock that
> > > doesn't have MS_I_VERSION set on it.
> > > 
> > > I'm not that familiar with IMA in general though, so it's possible I'm
> > > missing something. Is that already being done somehow?
> > 
> > Before reverting to using mtime, which wasn't fine grained enough at
> > the time, it would be helpful to first understand the type of changes
> > and the reasons for the changes you're looking to make to i_version.
> 
> Sure, I posted a patchset back in December, actually:
> 
>     [RFC PATCH v1 00/30] fs: inode->i_version rework and optimization

Thanks you.

> The basic idea is to improve performance on filesystems that implement
> the i_version counter by allowing them to optimize away metadata updates
> that are due solely to i_version counter change when no one is actually
> using it. This will also pave the way to allow us to more reasonably
> provide an i_version counter on network filesystems and the like that
> might have weaker consistency guarantees than a local fs.

> Your point about mtime not being granular enough is valid however. It's
> certainly possible for extra writes to race in during the jiffy or so
> window that represents the mtime resolution. It's just that that is
> still more granular than you'll get on a filesystem that never actually
> increments the i_version counter on a write.

> >  After all, i_version has been working all this time (~2009).
> > 
> 
> The i_version counter works just fine on filesystems that implement it
> properly. That's a very short list: xfs, btrfs, and ext4 for local
> filesystems. NFS and AFS would also likely be fine here, though they
> don't set MS_I_VERSION.

> The rest though do not support it consistently and IMA should not be
> relying on it on them. This is why the kernel nfs server only relies on
> the i_version field when IS_I_VERSION returns true.
> 
> I'll ask again -- is IMA somehow limited only being used only on that
> subset of filesystems? My guess from a glance at the integrity_read
> patchset is that it is not.

Our main use case scenario is verifying the integrity of files,
updating the file hash for mutable files, and maintaining a
measurement list, including re-measurement of files that have changed,
on local file systems.  Without being in the file write path (or more
precisely __fput), there are no guarantees of file change
notification.

For file systems which do not support i_version, we are limited to an
initial file integrity verification and measurement.

Mimi

> > > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > > > index c2edba8de35e..b8d746bbc43d 100644
> > > > --- a/security/integrity/ima/ima_api.c
> > > > +++ b/security/integrity/ima/ima_api.c
> > > > @@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > > >  	} hash;
> > > >  
> > > >  	if (!(iint->flags & IMA_COLLECTED)) {
> > > > -		u64 i_version = file_inode(file)->i_version;
> > > > +		u64 i_version = inode->i_version;
> > > > +		struct timespec i_mtime = inode->i_mtime;
> > > >  
> > > >  		if (file->f_flags & O_DIRECT) {
> > > >  			audit_cause = "failed(directio)";
> > > > @@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > > >  				iint->ima_hash = tmpbuf;
> > > >  				memcpy(iint->ima_hash, &hash, length);
> > > >  				iint->version = i_version;
> > > > +				iint->mtime = i_mtime;
> > > >  				iint->flags |= IMA_COLLECTED;
> > > >  			} else
> > > >  				result = -ENOMEM;
> > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > index 2aebb7984437..8d12ef2d3ba2 100644
> > > > --- a/security/integrity/ima/ima_main.c
> > > > +++ b/security/integrity/ima/ima_main.c
> > > > @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
> > > >  				  "invalid_pcr", "open_writers");
> > > >  }
> > > >  
> > > > +static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> > > > +				struct inode *inode)
> > > > +{
> > > > +	if (atomic_read(&inode->i_writecount) != 1)
> > > > +		return false;
> > > > +	if (iint->flags & IMA_NEW_FILE)
> > > > +		return true;
> > > > +	if (IS_I_VERSION(inode)) {
> > > > +		if (iint->version != inode->i_version)
> > > > +			return true;
> > > > +	} else {
> > > > +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> > > > +			return true;
> > > > +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> > > > +			return true;
> > > > +	}
> > > > +	return false;
> > > > +}
> > > > +
> > > >  static void ima_check_last_writer(struct integrity_iint_cache *iint,
> > > >  				  struct inode *inode, struct file *file)
> > > >  {
> > > > @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> > > >  		return;
> > > >  
> > > >  	inode_lock(inode);
> > > > -	if (atomic_read(&inode->i_writecount) == 1) {
> > > > -		if ((iint->version != inode->i_version) ||
> > > > -		    (iint->flags & IMA_NEW_FILE)) {
> > > > -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > > > -			iint->measured_pcrs = 0;
> > > > -			if (iint->flags & IMA_APPRAISE)
> > > > -				ima_update_xattr(iint, file);
> > > > -		}
> > > > +	if (ima_should_update_iint(iint, inode)) {
> > > > +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > > > +		iint->measured_pcrs = 0;
> > > > +		if (iint->flags & IMA_APPRAISE)
> > > > +			ima_update_xattr(iint, file);
> > > >  	}
> > > >  	inode_unlock(inode);
> > > >  }
> > > > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> > > > index a53e7e4ab06c..61fffa7583bf 100644
> > > > --- a/security/integrity/integrity.h
> > > > +++ b/security/integrity/integrity.h
> > > > @@ -102,6 +102,7 @@ struct integrity_iint_cache {
> > > >  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
> > > >  	struct inode *inode;	/* back pointer to inode in question */
> > > >  	u64 version;		/* track inode changes */
> > > > +	struct timespec mtime;	/* track inode changes */
> > > >  	unsigned long flags;
> > > >  	unsigned long measured_pcrs;
> > > >  	enum integrity_status ima_file_status:4;
> > 
> > 
> 

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Jeff Layton]

On Fri, 2017-07-07 at 15:59 -0400, Mimi Zohar wrote:
> On Fri, 2017-07-07 at 13:49 -0400, Jeff Layton wrote:
> > On Fri, 2017-07-07 at 13:24 -0400, Mimi Zohar wrote:
> > > On Fri, 2017-07-07 at 12:57 -0400, Jeff Layton wrote:
> > > > On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> > > > > From: Jeff Layton <jlayton@redhat.com>
> > > > > 
> > > > > The IMA assessment code tries to use the i_version counter to detect
> > > > > when changes to a file have occurred. Many filesystems don't increment
> > > > > it properly (or at all) so detecting changes with that is not always
> > > > > reliable.
> > > > > 
> > > > > That check should be gated on IS_I_VERSION, as you can't rely on the
> > > > > i_version field changing unless that returns true.
> > > > > 
> > > > > Have the code also track and check the mtime for the file. If the
> > > > > IS_I_VERSION returns false, then use it to detect whether the file's
> > > > > contents might have changed.
> > > > > 
> > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > > ---
> > > > >  security/integrity/ima/ima_api.c  |  4 +++-
> > > > >  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
> > > > >  security/integrity/integrity.h    |  1 +
> > > > >  3 files changed, 28 insertions(+), 9 deletions(-)
> > > > > 
> > > > > v2: switch to storing/checking mtime instead of ctime
> > > > > 
> > > > 
> > > > To be clear here, I don't have a large interest in IMA, but I am looking
> > > > at making changes to how the i_version counter is handled. IMA's use of
> > > > it is problematic for some of those changes (and somewhat sketchy).
> > > > 
> > > > I think you either want something like the patch below, or you need to
> > > > somehow ensure that you're not doing any of this on a superblock that
> > > > doesn't have MS_I_VERSION set on it.
> > > > 
> > > > I'm not that familiar with IMA in general though, so it's possible I'm
> > > > missing something. Is that already being done somehow?
> > > 
> > > Before reverting to using mtime, which wasn't fine grained enough at
> > > the time, it would be helpful to first understand the type of changes
> > > and the reasons for the changes you're looking to make to i_version.
> > 
> > Sure, I posted a patchset back in December, actually:
> > 
> >     [RFC PATCH v1 00/30] fs: inode->i_version rework and optimization
> 
> Thanks you.
> 
> > The basic idea is to improve performance on filesystems that implement
> > the i_version counter by allowing them to optimize away metadata updates
> > that are due solely to i_version counter change when no one is actually
> > using it. This will also pave the way to allow us to more reasonably
> > provide an i_version counter on network filesystems and the like that
> > might have weaker consistency guarantees than a local fs.
> > Your point about mtime not being granular enough is valid however. It's
> > certainly possible for extra writes to race in during the jiffy or so
> > window that represents the mtime resolution. It's just that that is
> > still more granular than you'll get on a filesystem that never actually
> > increments the i_version counter on a write.
> > >  After all, i_version has been working all this time (~2009).
> > > 
> > 
> > The i_version counter works just fine on filesystems that implement it
> > properly. That's a very short list: xfs, btrfs, and ext4 for local
> > filesystems. NFS and AFS would also likely be fine here, though they
> > don't set MS_I_VERSION.
> > The rest though do not support it consistently and IMA should not be
> > relying on it on them. This is why the kernel nfs server only relies on
> > the i_version field when IS_I_VERSION returns true.
> > 
> > I'll ask again -- is IMA somehow limited only being used only on that
> > subset of filesystems? My guess from a glance at the integrity_read
> > patchset is that it is not.
> 
> Our main use case scenario is verifying the integrity of files,
> updating the file hash for mutable files, and maintaining a
> measurement list, including re-measurement of files that have changed,
> on local file systems.  Without being in the file write path (or more
> precisely __fput), there are no guarantees of file change
> notification.
> 
> For file systems which do not support i_version, we are limited to an
> initial file integrity verification and measurement.

How is your typical user to know whether to expect this guarantee from
the filesystem?

> Mimi
> 
> > > > > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > > > > index c2edba8de35e..b8d746bbc43d 100644
> > > > > --- a/security/integrity/ima/ima_api.c
> > > > > +++ b/security/integrity/ima/ima_api.c
> > > > > @@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > > > >  	} hash;
> > > > >  
> > > > >  	if (!(iint->flags & IMA_COLLECTED)) {
> > > > > -		u64 i_version = file_inode(file)->i_version;
> > > > > +		u64 i_version = inode->i_version;
> > > > > +		struct timespec i_mtime = inode->i_mtime;
> > > > >  
> > > > >  		if (file->f_flags & O_DIRECT) {
> > > > >  			audit_cause = "failed(directio)";
> > > > > @@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> > > > >  				iint->ima_hash = tmpbuf;
> > > > >  				memcpy(iint->ima_hash, &hash, length);
> > > > >  				iint->version = i_version;
> > > > > +				iint->mtime = i_mtime;
> > > > >  				iint->flags |= IMA_COLLECTED;
> > > > >  			} else
> > > > >  				result = -ENOMEM;
> > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > > > > index 2aebb7984437..8d12ef2d3ba2 100644
> > > > > --- a/security/integrity/ima/ima_main.c
> > > > > +++ b/security/integrity/ima/ima_main.c
> > > > > @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
> > > > >  				  "invalid_pcr", "open_writers");
> > > > >  }
> > > > >  
> > > > > +static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> > > > > +				struct inode *inode)
> > > > > +{
> > > > > +	if (atomic_read(&inode->i_writecount) != 1)
> > > > > +		return false;
> > > > > +	if (iint->flags & IMA_NEW_FILE)
> > > > > +		return true;
> > > > > +	if (IS_I_VERSION(inode)) {
> > > > > +		if (iint->version != inode->i_version)
> > > > > +			return true;
> > > > > +	} else {
> > > > > +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> > > > > +			return true;
> > > > > +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> > > > > +			return true;
> > > > > +	}
> > > > > +	return false;
> > > > > +}
> > > > > +
> > > > >  static void ima_check_last_writer(struct integrity_iint_cache *iint,
> > > > >  				  struct inode *inode, struct file *file)
> > > > >  {
> > > > > @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> > > > >  		return;
> > > > >  
> > > > >  	inode_lock(inode);
> > > > > -	if (atomic_read(&inode->i_writecount) == 1) {
> > > > > -		if ((iint->version != inode->i_version) ||
> > > > > -		    (iint->flags & IMA_NEW_FILE)) {
> > > > > -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > > > > -			iint->measured_pcrs = 0;
> > > > > -			if (iint->flags & IMA_APPRAISE)
> > > > > -				ima_update_xattr(iint, file);
> > > > > -		}
> > > > > +	if (ima_should_update_iint(iint, inode)) {
> > > > > +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > > > > +		iint->measured_pcrs = 0;
> > > > > +		if (iint->flags & IMA_APPRAISE)
> > > > > +			ima_update_xattr(iint, file);
> > > > >  	}
> > > > >  	inode_unlock(inode);
> > > > >  }
> > > > > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> > > > > index a53e7e4ab06c..61fffa7583bf 100644
> > > > > --- a/security/integrity/integrity.h
> > > > > +++ b/security/integrity/integrity.h
> > > > > @@ -102,6 +102,7 @@ struct integrity_iint_cache {
> > > > >  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
> > > > >  	struct inode *inode;	/* back pointer to inode in question */
> > > > >  	u64 version;		/* track inode changes */
> > > > > +	struct timespec mtime;	/* track inode changes */
> > > > >  	unsigned long flags;
> > > > >  	unsigned long measured_pcrs;
> > > > >  	enum integrity_status ima_file_status:4;
> > > 
> > > 
> 
> 

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Mimi Zohar]

On Fri, 2017-07-07 at 16:35 -0400, Jeff Layton wrote:
> On Fri, 2017-07-07 at 15:59 -0400, Mimi Zohar wrote:
> > On Fri, 2017-07-07 at 13:49 -0400, Jeff Layton wrote:
> > > On Fri, 2017-07-07 at 13:24 -0400, Mimi Zohar wrote:
> > > > On Fri, 2017-07-07 at 12:57 -0400, Jeff Layton wrote:
> > > > > On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> > > > > > From: Jeff Layton <jlayton@redhat.com>
> > > > > > 
> > > > > > The IMA assessment code tries to use the i_version counter to detect
> > > > > > when changes to a file have occurred. Many filesystems don't increment
> > > > > > it properly (or at all) so detecting changes with that is not always
> > > > > > reliable.
> > > > > > 
> > > > > > That check should be gated on IS_I_VERSION, as you can't rely on the
> > > > > > i_version field changing unless that returns true.
> > > > > > 
> > > > > > Have the code also track and check the mtime for the file. If the
> > > > > > IS_I_VERSION returns false, then use it to detect whether the file's
> > > > > > contents might have changed.
> > > > > > 
> > > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > > > ---
> > > > > >  security/integrity/ima/ima_api.c  |  4 +++-
> > > > > >  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
> > > > > >  security/integrity/integrity.h    |  1 +
> > > > > >  3 files changed, 28 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > v2: switch to storing/checking mtime instead of ctime
> > > > > > 
> > > > > 
> > > > > To be clear here, I don't have a large interest in IMA, but I am looking
> > > > > at making changes to how the i_version counter is handled. IMA's use of
> > > > > it is problematic for some of those changes (and somewhat sketchy).
> > > > > 
> > > > > I think you either want something like the patch below, or you need to
> > > > > somehow ensure that you're not doing any of this on a superblock that
> > > > > doesn't have MS_I_VERSION set on it.
> > > > > 
> > > > > I'm not that familiar with IMA in general though, so it's possible I'm
> > > > > missing something. Is that already being done somehow?
> > > > 
> > > > Before reverting to using mtime, which wasn't fine grained enough at
> > > > the time, it would be helpful to first understand the type of changes
> > > > and the reasons for the changes you're looking to make to i_version.
> > > 
> > > Sure, I posted a patchset back in December, actually:
> > > 
> > >     [RFC PATCH v1 00/30] fs: inode->i_version rework and optimization
> > 
> > Thanks you.
> > 
> > > The basic idea is to improve performance on filesystems that implement
> > > the i_version counter by allowing them to optimize away metadata updates
> > > that are due solely to i_version counter change when no one is actually
> > > using it. This will also pave the way to allow us to more reasonably
> > > provide an i_version counter on network filesystems and the like that
> > > might have weaker consistency guarantees than a local fs.
> > > Your point about mtime not being granular enough is valid however. It's
> > > certainly possible for extra writes to race in during the jiffy or so
> > > window that represents the mtime resolution. It's just that that is
> > > still more granular than you'll get on a filesystem that never actually
> > > increments the i_version counter on a write.
> > > >  After all, i_version has been working all this time (~2009).
> > > > 
> > > 
> > > The i_version counter works just fine on filesystems that implement it
> > > properly. That's a very short list: xfs, btrfs, and ext4 for local
> > > filesystems. NFS and AFS would also likely be fine here, though they
> > > don't set MS_I_VERSION.
> > > The rest though do not support it consistently and IMA should not be
> > > relying on it on them. This is why the kernel nfs server only relies on
> > > the i_version field when IS_I_VERSION returns true.
> > > 
> > > I'll ask again -- is IMA somehow limited only being used only on that
> > > subset of filesystems? My guess from a glance at the integrity_read
> > > patchset is that it is not.
> > 
> > Our main use case scenario is verifying the integrity of files,
> > updating the file hash for mutable files, and maintaining a
> > measurement list, including re-measurement of files that have changed,
> > on local file systems.  Without being in the file write path (or more
> > precisely __fput), there are no guarantees of file change
> > notification.
> > 
> > For file systems which do not support i_version, we are limited to an
> > initial file integrity verification and measurement.
> 
> How is your typical user to know whether to expect this guarantee from
> the filesystem?

IMA can be configured in a number of different ways.  There are three
main aspects - prevention (IMA-appraisal), detection (IMA-
measurement), and forensics/analytics/remediation (IMA-audit).  A lot
depends on the environment (eg. embedded, laptop, server, cloud) and
what is being protected.

For example, the large majority of files in the Trusted Computing
Base(TCB) are immutable files and should be signed.  File hashes only
need to be re-calculated and stored as extended attributes for mutable
files.

A "typical user" would first want all immutable files to come with a
file signature, or would need to sign them locally.  Files that are
modified locally (eg. scripts, configuration files) should be re-
signed.  Only then would a "typical user" be concerned with files that
change - the mutable files.  With EVM configured, these file hashes
would be protected from offline modification.  

A "typical user" should also enable IMA-measurement and IMA-audit,
just in case something happens...

Last year, my colleagues Stefan Berger and Mehmet Kayaalp gave a talk
at LPC titled "File signatures needed!".  I'm looking forward to
hearing Matthew Garrett's talk at the Open Source Summit titled
"Signing Linux Executables for Fun and Security".

Mimi

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[jlayton]

On Mon, 2017-07-10 at 08:10 -0400, Mimi Zohar wrote:
> On Fri, 2017-07-07 at 16:35 -0400, Jeff Layton wrote:
> > On Fri, 2017-07-07 at 15:59 -0400, Mimi Zohar wrote:
> > > On Fri, 2017-07-07 at 13:49 -0400, Jeff Layton wrote:
> > > > On Fri, 2017-07-07 at 13:24 -0400, Mimi Zohar wrote:
> > > > > On Fri, 2017-07-07 at 12:57 -0400, Jeff Layton wrote:
> > > > > > On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> > > > > > > From: Jeff Layton <jlayton@redhat.com>
> > > > > > > 
> > > > > > > The IMA assessment code tries to use the i_version
> > > > > > > counter to detect
> > > > > > > when changes to a file have occurred. Many filesystems
> > > > > > > don't increment
> > > > > > > it properly (or at all) so detecting changes with that is
> > > > > > > not always
> > > > > > > reliable.
> > > > > > > 
> > > > > > > That check should be gated on IS_I_VERSION, as you can't
> > > > > > > rely on the
> > > > > > > i_version field changing unless that returns true.
> > > > > > > 
> > > > > > > Have the code also track and check the mtime for the
> > > > > > > file. If the
> > > > > > > IS_I_VERSION returns false, then use it to detect whether
> > > > > > > the file's
> > > > > > > contents might have changed.
> > > > > > > 
> > > > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > > > > ---
> > > > > > >  security/integrity/ima/ima_api.c  |  4 +++-
> > > > > > >  security/integrity/ima/ima_main.c | 32
> > > > > > > ++++++++++++++++++++++++--------
> > > > > > >  security/integrity/integrity.h    |  1 +
> > > > > > >  3 files changed, 28 insertions(+), 9 deletions(-)
> > > > > > > 
> > > > > > > v2: switch to storing/checking mtime instead of ctime
> > > > > > > 
> > > > > > 
> > > > > > To be clear here, I don't have a large interest in IMA, but
> > > > > > I am looking
> > > > > > at making changes to how the i_version counter is handled.
> > > > > > IMA's use of
> > > > > > it is problematic for some of those changes (and somewhat
> > > > > > sketchy).
> > > > > > 
> > > > > > I think you either want something like the patch below, or
> > > > > > you need to
> > > > > > somehow ensure that you're not doing any of this on a
> > > > > > superblock that
> > > > > > doesn't have MS_I_VERSION set on it.
> > > > > > 
> > > > > > I'm not that familiar with IMA in general though, so it's
> > > > > > possible I'm
> > > > > > missing something. Is that already being done somehow?
> > > > > 
> > > > > Before reverting to using mtime, which wasn't fine grained
> > > > > enough at
> > > > > the time, it would be helpful to first understand the type of
> > > > > changes
> > > > > and the reasons for the changes you're looking to make to
> > > > > i_version.
> > > > 
> > > > Sure, I posted a patchset back in December, actually:
> > > > 
> > > >     [RFC PATCH v1 00/30] fs: inode->i_version rework and
> > > > optimization
> > > 
> > > Thanks you.
> > > 
> > > > The basic idea is to improve performance on filesystems that
> > > > implement
> > > > the i_version counter by allowing them to optimize away
> > > > metadata updates
> > > > that are due solely to i_version counter change when no one is
> > > > actually
> > > > using it. This will also pave the way to allow us to more
> > > > reasonably
> > > > provide an i_version counter on network filesystems and the
> > > > like that
> > > > might have weaker consistency guarantees than a local fs.
> > > > Your point about mtime not being granular enough is valid
> > > > however. It's
> > > > certainly possible for extra writes to race in during the jiffy
> > > > or so
> > > > window that represents the mtime resolution. It's just that
> > > > that is
> > > > still more granular than you'll get on a filesystem that never
> > > > actually
> > > > increments the i_version counter on a write.
> > > > >  After all, i_version has been working all this time (~2009).
> > > > > 
> > > > 
> > > > The i_version counter works just fine on filesystems that
> > > > implement it
> > > > properly. That's a very short list: xfs, btrfs, and ext4 for
> > > > local
> > > > filesystems. NFS and AFS would also likely be fine here, though
> > > > they
> > > > don't set MS_I_VERSION.
> > > > The rest though do not support it consistently and IMA should
> > > > not be
> > > > relying on it on them. This is why the kernel nfs server only
> > > > relies on
> > > > the i_version field when IS_I_VERSION returns true.
> > > > 
> > > > I'll ask again -- is IMA somehow limited only being used only
> > > > on that
> > > > subset of filesystems? My guess from a glance at the
> > > > integrity_read
> > > > patchset is that it is not.
> > > 
> > > Our main use case scenario is verifying the integrity of files,
> > > updating the file hash for mutable files, and maintaining a
> > > measurement list, including re-measurement of files that have
> > > changed,
> > > on local file systems.  Without being in the file write path (or
> > > more
> > > precisely __fput), there are no guarantees of file change
> > > notification.
> > > 
> > > For file systems which do not support i_version, we are limited
> > > to an
> > > initial file integrity verification and measurement.
> > 
> > How is your typical user to know whether to expect this guarantee
> > from
> > the filesystem?
> 
> IMA can be configured in a number of different ways.  There are three
> main aspects - prevention (IMA-appraisal), detection (IMA-
> measurement), and forensics/analytics/remediation (IMA-audit).  A lot
> depends on the environment (eg. embedded, laptop, server, cloud) and
> what is being protected.
> 
> For example, the large majority of files in the Trusted Computing
> Base(TCB) are immutable files and should be signed.  File hashes only
> need to be re-calculated and stored as extended attributes for
> mutable
> files.
> 
> A "typical user" would first want all immutable files to come with a
> file signature, or would need to sign them locally.  Files that are
> modified locally (eg. scripts, configuration files) should be re-
> signed.  Only then would a "typical user" be concerned with files
> that
> change - the mutable files.  With EVM configured, these file hashes
> would be protected from offline modification.  
> 
> A "typical user" should also enable IMA-measurement and IMA-audit,
> just in case something happens...
> 
> Last year, my colleagues Stefan Berger and Mehmet Kayaalp gave a talk
> at LPC titled "File signatures needed!".  I'm looking forward to
> hearing Matthew Garrett's talk at the Open Source Summit titled
> "Signing Linux Executables for Fun and Security".
> 

(cc'ing Bruce here too, since he's also done a lot of work with the
i_version counter...)

Thanks, but that doesn't really answer my question. Let me try again:

My main concern is whether IMA is supposed to be a filesystem-agnostic
feature.

Typically, when we accept features into the kernel we like for them to
work in a consistent way across different filesystems. Barring that, we
want to provide some way for userland to ascertain what sort of
semantics it can expect the fs to provide.

For instance, most filesystems support file locking, but some don't.
Those that don't typically return a particular error (-ENOLCK) when
someone attempts to set or query for a lock.

IMA, however does not have any sort of whitelist of supported
filesystems, AFAICT. It also doesn't properly check whether the
i_verison counter is properly supported by the filesystem before
attempting to use it.

If using the mtime is unacceptable due to insufficient granularity,
then it seems logical that relying on an i_version counter that is
never incremented is also bad.

So, my question is: Why is that not a problem here?

To put it another way... In an earlier reply you said:

> For file systems which do not support i_version, we are limited to an
> initial file integrity verification and measurement.

How is userland supposed to know which behavior to expect from the fs?

ext4 only provides a working i_version counter when you mount with "-o
i_version", so it's trivial to tell there. xfs and btrfs also have
functional i_version counter implementations, but there is no such
mount option for them (it's always on there). NFSv4 and AFS can provide
one too (as they're supplied by the server).

Suppose I want to use IMA on something else (say, ubifs). How do I know
whether I'm only going to get "initial file integrity verification and
measurement" or whether it'll be updated after being written?

Now, I happen to know that ubifs does _not_ support the i_version
counter because I can poke through the kernel sources and tell, but how
is Joe Random Linux User to know this?

Does that not matter for some reason? Is there a whitelist of
filesystems being maintained in some userland package?

Sorry if it seems like I'm being dense here, but I really just don't
understand how we can allow this code to be so cavalier about using the
i_version counter without taking steps to ensure that it actually does
a damned thing at all.

Thanks,
-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Mimi Zohar]

On Tue, 2017-07-11 at 21:17 -0400, jlayton@redhat.com wrote:
> On Mon, 2017-07-10 at 08:10 -0400, Mimi Zohar wrote:
> > On Fri, 2017-07-07 at 16:35 -0400, Jeff Layton wrote:
> > > On Fri, 2017-07-07 at 15:59 -0400, Mimi Zohar wrote:
> > > > On Fri, 2017-07-07 at 13:49 -0400, Jeff Layton wrote:
> > > > > On Fri, 2017-07-07 at 13:24 -0400, Mimi Zohar wrote:
> > > > > > On Fri, 2017-07-07 at 12:57 -0400, Jeff Layton wrote:
> > > > > > > On Fri, 2017-07-07 at 10:05 -0400, Jeff Layton wrote:
> > > > > > > > From: Jeff Layton <jlayton@redhat.com>
> > > > > > > > 
> > > > > > > > The IMA assessment code tries to use the i_version
> > > > > > > > counter to detect
> > > > > > > > when changes to a file have occurred. Many filesystems
> > > > > > > > don't increment
> > > > > > > > it properly (or at all) so detecting changes with that is
> > > > > > > > not always
> > > > > > > > reliable.
> > > > > > > > 
> > > > > > > > That check should be gated on IS_I_VERSION, as you can't
> > > > > > > > rely on the
> > > > > > > > i_version field changing unless that returns true.
> > > > > > > > 
> > > > > > > > Have the code also track and check the mtime for the
> > > > > > > > file. If the
> > > > > > > > IS_I_VERSION returns false, then use it to detect whether
> > > > > > > > the file's
> > > > > > > > contents might have changed.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > > > > > > ---
> > > > > > > >  security/integrity/ima/ima_api.c  |  4 +++-
> > > > > > > >  security/integrity/ima/ima_main.c | 32
> > > > > > > > ++++++++++++++++++++++++--------
> > > > > > > >  security/integrity/integrity.h    |  1 +
> > > > > > > >  3 files changed, 28 insertions(+), 9 deletions(-)
> > > > > > > > 
> > > > > > > > v2: switch to storing/checking mtime instead of ctime
> > > > > > > > 
> > > > > > > 
> > > > > > > To be clear here, I don't have a large interest in IMA, but
> > > > > > > I am looking
> > > > > > > at making changes to how the i_version counter is handled.
> > > > > > > IMA's use of
> > > > > > > it is problematic for some of those changes (and somewhat
> > > > > > > sketchy).
> > > > > > > 
> > > > > > > I think you either want something like the patch below, or
> > > > > > > you need to
> > > > > > > somehow ensure that you're not doing any of this on a
> > > > > > > superblock that
> > > > > > > doesn't have MS_I_VERSION set on it.
> > > > > > > 
> > > > > > > I'm not that familiar with IMA in general though, so it's
> > > > > > > possible I'm
> > > > > > > missing something. Is that already being done somehow?
> > > > > > 
> > > > > > Before reverting to using mtime, which wasn't fine grained
> > > > > > enough at
> > > > > > the time, it would be helpful to first understand the type of
> > > > > > changes
> > > > > > and the reasons for the changes you're looking to make to
> > > > > > i_version.
> > > > > 
> > > > > Sure, I posted a patchset back in December, actually:
> > > > > 
> > > > >     [RFC PATCH v1 00/30] fs: inode->i_version rework and
> > > > > optimization
> > > > 
> > > > Thanks you.
> > > > 
> > > > > The basic idea is to improve performance on filesystems that
> > > > > implement
> > > > > the i_version counter by allowing them to optimize away
> > > > > metadata updates
> > > > > that are due solely to i_version counter change when no one is
> > > > > actually
> > > > > using it. This will also pave the way to allow us to more
> > > > > reasonably
> > > > > provide an i_version counter on network filesystems and the
> > > > > like that
> > > > > might have weaker consistency guarantees than a local fs.
> > > > > Your point about mtime not being granular enough is valid
> > > > > however. It's
> > > > > certainly possible for extra writes to race in during the jiffy
> > > > > or so
> > > > > window that represents the mtime resolution. It's just that
> > > > > that is
> > > > > still more granular than you'll get on a filesystem that never
> > > > > actually
> > > > > increments the i_version counter on a write.
> > > > > >  After all, i_version has been working all this time (~2009).
> > > > > > 
> > > > > 
> > > > > The i_version counter works just fine on filesystems that
> > > > > implement it
> > > > > properly. That's a very short list: xfs, btrfs, and ext4 for
> > > > > local
> > > > > filesystems. NFS and AFS would also likely be fine here, though
> > > > > they
> > > > > don't set MS_I_VERSION.
> > > > > The rest though do not support it consistently and IMA should
> > > > > not be
> > > > > relying on it on them. This is why the kernel nfs server only
> > > > > relies on
> > > > > the i_version field when IS_I_VERSION returns true.
> > > > > 
> > > > > I'll ask again -- is IMA somehow limited only being used only
> > > > > on that
> > > > > subset of filesystems? My guess from a glance at the
> > > > > integrity_read
> > > > > patchset is that it is not.
> > > > 
> > > > Our main use case scenario is verifying the integrity of files,
> > > > updating the file hash for mutable files, and maintaining a
> > > > measurement list, including re-measurement of files that have
> > > > changed,
> > > > on local file systems.  Without being in the file write path (or
> > > > more
> > > > precisely __fput), there are no guarantees of file change
> > > > notification.
> > > > 
> > > > For file systems which do not support i_version, we are limited
> > > > to an
> > > > initial file integrity verification and measurement.
> > > 
> > > How is your typical user to know whether to expect this guarantee
> > > from
> > > the filesystem?
> > 
> > IMA can be configured in a number of different ways.  There are three
> > main aspects - prevention (IMA-appraisal), detection (IMA-
> > measurement), and forensics/analytics/remediation (IMA-audit).  A lot
> > depends on the environment (eg. embedded, laptop, server, cloud) and
> > what is being protected.
> > 
> > For example, the large majority of files in the Trusted Computing
> > Base(TCB) are immutable files and should be signed.  File hashes only
> > need to be re-calculated and stored as extended attributes for
> > mutable
> > files.
> > 
> > A "typical user" would first want all immutable files to come with a
> > file signature, or would need to sign them locally.  Files that are
> > modified locally (eg. scripts, configuration files) should be re-
> > signed.  Only then would a "typical user" be concerned with files
> > that
> > change - the mutable files.  With EVM configured, these file hashes
> > would be protected from offline modification.  
> > 
> > A "typical user" should also enable IMA-measurement and IMA-audit,
> > just in case something happens...
> > 
> > Last year, my colleagues Stefan Berger and Mehmet Kayaalp gave a talk
> > at LPC titled "File signatures needed!".  I'm looking forward to
> > hearing Matthew Garrett's talk at the Open Source Summit titled
> > "Signing Linux Executables for Fun and Security".
> > 
> 
> (cc'ing Bruce here too, since he's also done a lot of work with the
> i_version counter...)
> 
> Thanks, but that doesn't really answer my question. Let me try again:
> 
> My main concern is whether IMA is supposed to be a filesystem-agnostic
> feature.
> 
> Typically, when we accept features into the kernel we like for them to
> work in a consistent way across different filesystems. Barring that, we
> want to provide some way for userland to ascertain what sort of
> semantics it can expect the fs to provide.
> 
> For instance, most filesystems support file locking, but some don't.
> Those that don't typically return a particular error (-ENOLCK) when
> someone attempts to set or query for a lock.
> 
> IMA, however does not have any sort of whitelist of supported
> filesystems, AFAICT. It also doesn't properly check whether the
> i_verison counter is properly supported by the filesystem before
> attempting to use it.
> 
> If using the mtime is unacceptable due to insufficient granularity,
> then it seems logical that relying on an i_version counter that is
> never incremented is also bad.
> 
> So, my question is: Why is that not a problem here?
> 
> To put it another way... In an earlier reply you said:
> 
> > For file systems which do not support i_version, we are limited to an
> > initial file integrity verification and measurement.
> 
> How is userland supposed to know which behavior to expect from the fs?

Right, currently the only way of knowing is by looking at the IMA
measurement list to see if modified files are re-measured or, as you
said, by looking at the code.

I started working on adding logging/audit messages, but have not yet
posted them.  A very preliminary set of patches is available from
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.
git/next-log-iversion-experimental.

2745b7be961a ima: indicate possibly missing file measurements or verification
0c81a8c56153 security: define new LSM sb_post_new_mount hook

Mimi

> 
> ext4 only provides a working i_version counter when you mount with "-o
> i_version", so it's trivial to tell there. xfs and btrfs also have
> functional i_version counter implementations, but there is no such
> mount option for them (it's always on there). NFSv4 and AFS can provide
> one too (as they're supplied by the server).
> 
> Suppose I want to use IMA on something else (say, ubifs). How do I know
> whether I'm only going to get "initial file integrity verification and
> measurement" or whether it'll be updated after being written?
> 
> Now, I happen to know that ubifs does _not_ support the i_version
> counter because I can poke through the kernel sources and tell, but how
> is Joe Random Linux User to know this?
> 
> Does that not matter for some reason? Is there a whitelist of
> filesystems being maintained in some userland package?
> 
> Sorry if it seems like I'm being dense here, but I really just don't
> understand how we can allow this code to be so cavalier about using the
> i_version counter without taking steps to ensure that it actually does
> a damned thing at all.

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Bruce Fields]

On Wed, Jul 12, 2017 at 08:20:21AM -0400, Mimi Zohar wrote:
> Right, currently the only way of knowing is by looking at the IMA
> measurement list to see if modified files are re-measured or, as you
> said, by looking at the code.

Who's actually using this, and do they do any kind of checks, or
document the filesystem-specific limitations?

--b.

> 
> I started working on adding logging/audit messages, but have not yet
> posted them.  A very preliminary set of patches is available from
> https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.
> git/next-log-iversion-experimental.
> 
> 2745b7be961a ima: indicate possibly missing file measurements or verification
> 0c81a8c56153 security: define new LSM sb_post_new_mount hook
> 
> Mimi
> 
> > 
> > ext4 only provides a working i_version counter when you mount with "-o
> > i_version", so it's trivial to tell there. xfs and btrfs also have
> > functional i_version counter implementations, but there is no such
> > mount option for them (it's always on there). NFSv4 and AFS can provide
> > one too (as they're supplied by the server).
> > 
> > Suppose I want to use IMA on something else (say, ubifs). How do I know
> > whether I'm only going to get "initial file integrity verification and
> > measurement" or whether it'll be updated after being written?
> > 
> > Now, I happen to know that ubifs does _not_ support the i_version
> > counter because I can poke through the kernel sources and tell, but how
> > is Joe Random Linux User to know this?
> > 
> > Does that not matter for some reason? Is there a whitelist of
> > filesystems being maintained in some userland package?
> > 
> > Sorry if it seems like I'm being dense here, but I really just don't
> > understand how we can allow this code to be so cavalier about using the
> > i_version counter without taking steps to ensure that it actually does
> > a damned thing at all.
> 
> 

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Mimi Zohar]

On Wed, 2017-07-12 at 10:35 -0400, Bruce Fields wrote:
> On Wed, Jul 12, 2017 at 08:20:21AM -0400, Mimi Zohar wrote:
> > Right, currently the only way of knowing is by looking at the IMA
> > measurement list to see if modified files are re-measured or, as you
> > said, by looking at the code.
> 
> Who's actually using this, and do they do any kind of checks, or
> document the filesystem-specific limitations?

Knowing who is using it and how it is being used is the big question.
 I only hear about it when there are problems.

Over the years, there have been a number of Linux Security Summit
(LSS) talks, which have been mostly about embedded systems or locked
down systems, not so much for generic systems.

Examples include:

- Design and Implementation of a Security Architecture for Critical
Infrastructure Industrial Control Systems - David Safford, GE 2016

- IMA/EVM: Real Applications for Embedded Networking Systems - Petko
Manolov, Konsulko Group, and Mark Baushke, Juniper Networks 2015

- CC3: An Identity Attested Linux Security Supervisor Architecture
 - Greg Wettstein, IDfusion 2015

- The Linux Integrity Subsystem and TPM-based Network Endpoint
Assessment - Andreas Steffen, HSR University of Applied Sciences
Rapperswil, Switzerland 2012

Mimi

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Bruce Fields]

On Wed, Jul 12, 2017 at 01:56:50PM -0400, Mimi Zohar wrote:
> On Wed, 2017-07-12 at 10:35 -0400, Bruce Fields wrote:
> > On Wed, Jul 12, 2017 at 08:20:21AM -0400, Mimi Zohar wrote:
> > > Right, currently the only way of knowing is by looking at the IMA
> > > measurement list to see if modified files are re-measured or, as you
> > > said, by looking at the code.
> > 
> > Who's actually using this, and do they do any kind of checks, or
> > document the filesystem-specific limitations?
> 
> Knowing who is using it and how it is being used is the big question.
>  I only hear about it when there are problems.
> 
> Over the years, there have been a number of Linux Security Summit
> (LSS) talks, which have been mostly about embedded systems or locked
> down systems, not so much for generic systems.
> 
> Examples include:

Thanks, I skimmed a couple.  Hard to tell, but it sounds like they need
this to work.  I wonder if they're getting this right.  It'd be easy
enough to test for.

--b.

> 
> - Design and Implementation of a Security Architecture for Critical
> Infrastructure Industrial Control Systems - David Safford, GE 2016
> 
> - IMA/EVM: Real Applications for Embedded Networking Systems - Petko
> Manolov, Konsulko Group, and Mark Baushke, Juniper Networks 2015
> 
> - CC3: An Identity Attested Linux Security Supervisor Architecture
>  - Greg Wettstein, IDfusion 2015
> 
> - The Linux Integrity Subsystem and TPM-based Network Endpoint
> Assessment - Andreas Steffen, HSR University of Applied Sciences
> Rapperswil, Switzerland 2012
> 
> Mimi

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[J. Bruce Fields]

On Fri, Jul 07, 2017 at 10:05:30AM -0400, Jeff Layton wrote:
> From: Jeff Layton <jlayton@redhat.com>
> 
> The IMA assessment code tries to use the i_version counter to detect
> when changes to a file have occurred. Many filesystems don't increment
> it properly (or at all) so detecting changes with that is not always
> reliable.
> 
> That check should be gated on IS_I_VERSION, as you can't rely on the
> i_version field changing unless that returns true.
> 
> Have the code also track and check the mtime for the file. If the
> IS_I_VERSION returns false, then use it to detect whether the file's
> contents might have changed.

Do they care about attribute changes?  It's still better than nothing, I
suppose.

I also wonder whether they should be mixing in ctime as I plan to for
nfsd--the difference is whether they use it to check changes across
reboots.

--b.

> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  security/integrity/ima/ima_api.c  |  4 +++-
>  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
>  security/integrity/integrity.h    |  1 +
>  3 files changed, 28 insertions(+), 9 deletions(-)
> 
> v2: switch to storing/checking mtime instead of ctime
> 
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index c2edba8de35e..b8d746bbc43d 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>  	} hash;
>  
>  	if (!(iint->flags & IMA_COLLECTED)) {
> -		u64 i_version = file_inode(file)->i_version;
> +		u64 i_version = inode->i_version;
> +		struct timespec i_mtime = inode->i_mtime;
>  
>  		if (file->f_flags & O_DIRECT) {
>  			audit_cause = "failed(directio)";
> @@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>  				iint->ima_hash = tmpbuf;
>  				memcpy(iint->ima_hash, &hash, length);
>  				iint->version = i_version;
> +				iint->mtime = i_mtime;
>  				iint->flags |= IMA_COLLECTED;
>  			} else
>  				result = -ENOMEM;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 2aebb7984437..8d12ef2d3ba2 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
>  				  "invalid_pcr", "open_writers");
>  }
>  
> +static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> +				struct inode *inode)
> +{
> +	if (atomic_read(&inode->i_writecount) != 1)
> +		return false;
> +	if (iint->flags & IMA_NEW_FILE)
> +		return true;
> +	if (IS_I_VERSION(inode)) {
> +		if (iint->version != inode->i_version)
> +			return true;
> +	} else {
> +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> +			return true;
> +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  static void ima_check_last_writer(struct integrity_iint_cache *iint,
>  				  struct inode *inode, struct file *file)
>  {
> @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
>  		return;
>  
>  	inode_lock(inode);
> -	if (atomic_read(&inode->i_writecount) == 1) {
> -		if ((iint->version != inode->i_version) ||
> -		    (iint->flags & IMA_NEW_FILE)) {
> -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> -			iint->measured_pcrs = 0;
> -			if (iint->flags & IMA_APPRAISE)
> -				ima_update_xattr(iint, file);
> -		}
> +	if (ima_should_update_iint(iint, inode)) {
> +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> +		iint->measured_pcrs = 0;
> +		if (iint->flags & IMA_APPRAISE)
> +			ima_update_xattr(iint, file);
>  	}
>  	inode_unlock(inode);
>  }
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index a53e7e4ab06c..61fffa7583bf 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -102,6 +102,7 @@ struct integrity_iint_cache {
>  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
>  	struct inode *inode;	/* back pointer to inode in question */
>  	u64 version;		/* track inode changes */
> +	struct timespec mtime;	/* track inode changes */
>  	unsigned long flags;
>  	unsigned long measured_pcrs;
>  	enum integrity_status ima_file_status:4;
> -- 
> 2.13.0

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[Mimi Zohar]

On Tue, 2017-07-11 at 12:13 -0400, J. Bruce Fields wrote:
> On Fri, Jul 07, 2017 at 10:05:30AM -0400, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > The IMA assessment code tries to use the i_version counter to detect
> > when changes to a file have occurred. Many filesystems don't increment
> > it properly (or at all) so detecting changes with that is not always
> > reliable.
> > 
> > That check should be gated on IS_I_VERSION, as you can't rely on the
> > i_version field changing unless that returns true.
> > 
> > Have the code also track and check the mtime for the file. If the
> > IS_I_VERSION returns false, then use it to detect whether the file's
> > contents might have changed.
> 
> Do they care about attribute changes?  It's still better than nothing, I
> suppose.

IMA is only interested in file data changes, not file meta-data
changes.  EVM needs to recalculate the hmac stored as security.evm,
when file meta-data changes (eg. xattrs, uid, gid, mode,
i_generations), but this is dependent on being in the setxattr or
notify_change() paths, not i_version.

Mimi

> 
> I also wonder whether they should be mixing in ctime as I plan to for
> nfsd--the difference is whether they use it to check changes across
> reboots.
> 
> --b.
> 
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  security/integrity/ima/ima_api.c  |  4 +++-
> >  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++--------
> >  security/integrity/integrity.h    |  1 +
> >  3 files changed, 28 insertions(+), 9 deletions(-)
> > 
> > v2: switch to storing/checking mtime instead of ctime
> > 
> > diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> > index c2edba8de35e..b8d746bbc43d 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -205,7 +205,8 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> >  	} hash;
> >  
> >  	if (!(iint->flags & IMA_COLLECTED)) {
> > -		u64 i_version = file_inode(file)->i_version;
> > +		u64 i_version = inode->i_version;
> > +		struct timespec i_mtime = inode->i_mtime;
> >  
> >  		if (file->f_flags & O_DIRECT) {
> >  			audit_cause = "failed(directio)";
> > @@ -225,6 +226,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> >  				iint->ima_hash = tmpbuf;
> >  				memcpy(iint->ima_hash, &hash, length);
> >  				iint->version = i_version;
> > +				iint->mtime = i_mtime;
> >  				iint->flags |= IMA_COLLECTED;
> >  			} else
> >  				result = -ENOMEM;
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 2aebb7984437..8d12ef2d3ba2 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct file *file,
> >  				  "invalid_pcr", "open_writers");
> >  }
> >  
> > +static bool ima_should_update_iint(struct integrity_iint_cache *iint,
> > +				struct inode *inode)
> > +{
> > +	if (atomic_read(&inode->i_writecount) != 1)
> > +		return false;
> > +	if (iint->flags & IMA_NEW_FILE)
> > +		return true;
> > +	if (IS_I_VERSION(inode)) {
> > +		if (iint->version != inode->i_version)
> > +			return true;
> > +	} else {
> > +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> > +			return true;
> > +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> > +			return true;
> > +	}
> > +	return false;
> > +}
> > +
> >  static void ima_check_last_writer(struct integrity_iint_cache *iint,
> >  				  struct inode *inode, struct file *file)
> >  {
> > @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
> >  		return;
> >  
> >  	inode_lock(inode);
> > -	if (atomic_read(&inode->i_writecount) == 1) {
> > -		if ((iint->version != inode->i_version) ||
> > -		    (iint->flags & IMA_NEW_FILE)) {
> > -			iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > -			iint->measured_pcrs = 0;
> > -			if (iint->flags & IMA_APPRAISE)
> > -				ima_update_xattr(iint, file);
> > -		}
> > +	if (ima_should_update_iint(iint, inode)) {
> > +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > +		iint->measured_pcrs = 0;
> > +		if (iint->flags & IMA_APPRAISE)
> > +			ima_update_xattr(iint, file);
> >  	}
> >  	inode_unlock(inode);
> >  }
> > diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> > index a53e7e4ab06c..61fffa7583bf 100644
> > --- a/security/integrity/integrity.h
> > +++ b/security/integrity/integrity.h
> > @@ -102,6 +102,7 @@ struct integrity_iint_cache {
> >  	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
> >  	struct inode *inode;	/* back pointer to inode in question */
> >  	u64 version;		/* track inode changes */
> > +	struct timespec mtime;	/* track inode changes */
> >  	unsigned long flags;
> >  	unsigned long measured_pcrs;
> >  	enum integrity_status ima_file_status:4;
> > -- 
> > 2.13.0
> --
> 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
> 

Re: [PATCH v2] integrity: track mtime in addition to i_version for assessment

[jlayton]

On Tue, 2017-07-11 at 12:13 -0400, J. Bruce Fields wrote:
> On Fri, Jul 07, 2017 at 10:05:30AM -0400, Jeff Layton wrote:
> > From: Jeff Layton <jlayton@redhat.com>
> > 
> > The IMA assessment code tries to use the i_version counter to
> > detect
> > when changes to a file have occurred. Many filesystems don't
> > increment
> > it properly (or at all) so detecting changes with that is not
> > always
> > reliable.
> > 
> > That check should be gated on IS_I_VERSION, as you can't rely on
> > the
> > i_version field changing unless that returns true.
> > 
> > Have the code also track and check the mtime for the file. If the
> > IS_I_VERSION returns false, then use it to detect whether the
> > file's
> > contents might have changed.
> 
> Do they care about attribute changes?  It's still better than
> nothing, I
> suppose.
> 

Evidently not, which is why I switched from using ctime to mtime in
this patch.

> I also wonder whether they should be mixing in ctime as I plan to for
> nfsd--the difference is whether they use it to check changes across
> reboots.
> 

I don't think they record the i_version across reboots, so I don't
think there's any need.

> --b.
> 
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  security/integrity/ima/ima_api.c  |  4 +++-
> >  security/integrity/ima/ima_main.c | 32 ++++++++++++++++++++++++---
> > -----
> >  security/integrity/integrity.h    |  1 +
> >  3 files changed, 28 insertions(+), 9 deletions(-)
> > 
> > v2: switch to storing/checking mtime instead of ctime
> > 
> > diff --git a/security/integrity/ima/ima_api.c
> > b/security/integrity/ima/ima_api.c
> > index c2edba8de35e..b8d746bbc43d 100644
> > --- a/security/integrity/ima/ima_api.c
> > +++ b/security/integrity/ima/ima_api.c
> > @@ -205,7 +205,8 @@ int ima_collect_measurement(struct
> > integrity_iint_cache *iint,
> >  	} hash;
> >  
> >  	if (!(iint->flags & IMA_COLLECTED)) {
> > -		u64 i_version = file_inode(file)->i_version;
> > +		u64 i_version = inode->i_version;
> > +		struct timespec i_mtime = inode->i_mtime;
> >  
> >  		if (file->f_flags & O_DIRECT) {
> >  			audit_cause = "failed(directio)";
> > @@ -225,6 +226,7 @@ int ima_collect_measurement(struct
> > integrity_iint_cache *iint,
> >  				iint->ima_hash = tmpbuf;
> >  				memcpy(iint->ima_hash, &hash,
> > length);
> >  				iint->version = i_version;
> > +				iint->mtime = i_mtime;
> >  				iint->flags |= IMA_COLLECTED;
> >  			} else
> >  				result = -ENOMEM;
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 2aebb7984437..8d12ef2d3ba2 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -113,6 +113,25 @@ static void ima_rdwr_violation_check(struct
> > file *file,
> >  				  "invalid_pcr", "open_writers");
> >  }
> >  
> > +static bool ima_should_update_iint(struct integrity_iint_cache
> > *iint,
> > +				struct inode *inode)
> > +{
> > +	if (atomic_read(&inode->i_writecount) != 1)
> > +		return false;
> > +	if (iint->flags & IMA_NEW_FILE)
> > +		return true;
> > +	if (IS_I_VERSION(inode)) {
> > +		if (iint->version != inode->i_version)
> > +			return true;
> > +	} else {
> > +		if (iint->mtime.tv_sec != inode->i_mtime.tv_sec)
> > +			return true;
> > +		if (iint->mtime.tv_nsec != inode->i_mtime.tv_nsec)
> > +			return true;
> > +	}
> > +	return false;
> > +}
> > +
> >  static void ima_check_last_writer(struct integrity_iint_cache
> > *iint,
> >  				  struct inode *inode, struct file
> > *file)
> >  {
> > @@ -122,14 +141,11 @@ static void ima_check_last_writer(struct
> > integrity_iint_cache *iint,
> >  		return;
> >  
> >  	inode_lock(inode);
> > -	if (atomic_read(&inode->i_writecount) == 1) {
> > -		if ((iint->version != inode->i_version) ||
> > -		    (iint->flags & IMA_NEW_FILE)) {
> > -			iint->flags &= ~(IMA_DONE_MASK |
> > IMA_NEW_FILE);
> > -			iint->measured_pcrs = 0;
> > -			if (iint->flags & IMA_APPRAISE)
> > -				ima_update_xattr(iint, file);
> > -		}
> > +	if (ima_should_update_iint(iint, inode)) {
> > +		iint->flags &= ~(IMA_DONE_MASK | IMA_NEW_FILE);
> > +		iint->measured_pcrs = 0;
> > +		if (iint->flags & IMA_APPRAISE)
> > +			ima_update_xattr(iint, file);
> >  	}
> >  	inode_unlock(inode);
> >  }
> > diff --git a/security/integrity/integrity.h
> > b/security/integrity/integrity.h
> > index a53e7e4ab06c..61fffa7583bf 100644
> > --- a/security/integrity/integrity.h
> > +++ b/security/integrity/integrity.h
> > @@ -102,6 +102,7 @@ struct integrity_iint_cache {
> >  	struct rb_node rb_node;	/* rooted in
> > integrity_iint_tree */
> >  	struct inode *inode;	/* back pointer to inode in
> > question */
> >  	u64 version;		/* track inode changes */
> > +	struct timespec mtime;	/* track inode changes */
> >  	unsigned long flags;
> >  	unsigned long measured_pcrs;
> >  	enum integrity_status ima_file_status:4;
> > -- 
> > 2.13.0