[PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[Mimi Zohar]

If tmpfiles can be made persistent, then newly created tmpfiles need to
be treated like any other new files in policy.

This patch indicates which newly created tmpfiles are in policy, causing
the file hash to be calculated on __fput().

Reported-by: Ignaz Forster <ignaz.forster@gmx.de>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
Changelog:
- Fixed Subject line function name
- Addressed 0-day report: warning: 'return' with a value, in function returning void

 fs/namei.c                        |  2 ++
 include/linux/ima.h               |  6 ++++++
 security/integrity/ima/ima_main.c | 36 ++++++++++++++++++++++++++++++++++--
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 0cab6494978c..0911837979f6 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3495,6 +3495,8 @@ static int do_tmpfile(struct nameidata *nd, unsigned flags,
 		goto out2;
 	file->f_path.mnt = path.mnt;
 	error = finish_open(file, child, NULL);
+	if (!error)
+		ima_post_create_tmpfile(file);
 out2:
 	mnt_drop_write(path.mnt);
 out:
diff --git a/include/linux/ima.h b/include/linux/ima.h
index b5e16b8c50b7..e863c82bb258 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -18,6 +18,7 @@ struct linux_binprm;
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask);
+extern void ima_post_create_tmpfile(const struct file *file);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_load_data(enum kernel_load_data_id id);
@@ -56,6 +57,11 @@ static inline int ima_file_check(struct file *file, int mask)
 	return 0;
 }
 
+static inline void ima_post_create_tmpfile(const struct file *file)
+{
+	return;
+}
+
 static inline void ima_file_free(struct file *file)
 {
 	return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index bd9bd5f88206..388458c8ec29 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -395,6 +395,34 @@ int ima_file_check(struct file *file, int mask)
 EXPORT_SYMBOL_GPL(ima_file_check);
 
 /**
+ * ima_post_create_tmpfile - mark newly created tmpfile as new
+ * @file : newly created tmpfile
+ *
+ * No measuring, appraising or auditing of newly created tmpfiles is needed.
+ * Skip calling process_measurement(), but indicate which newly, created
+ * tmpfiles are in policy.
+ */
+void ima_post_create_tmpfile(const struct file *file)
+{
+	struct inode *inode = file_inode(file);
+	struct integrity_iint_cache *iint;
+	int must_appraise;
+
+	must_appraise = ima_must_appraise(inode, MAY_ACCESS, FILE_CHECK);
+	if (!must_appraise)
+		return;
+
+	/* Nothing to do if we can't allocate memory */
+	iint = integrity_inode_get(inode);
+	if (!iint)
+		return;
+
+	/* needed for writing the security xattrs */
+	set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
+	iint->ima_file_status = INTEGRITY_PASS;
+}
+
+/**
  * ima_post_path_mknod - mark as a new inode
  * @dentry: newly created dentry
  *
@@ -411,9 +439,13 @@ void ima_post_path_mknod(struct dentry *dentry)
 	if (!must_appraise)
 		return;
 
+	/* Nothing to do if we can't allocate memory */
 	iint = integrity_inode_get(inode);
-	if (iint)
-		iint->flags |= IMA_NEW_FILE;
+	if (!iint)
+		return;
+
+	/* needed for re-opening empty files */
+	iint->flags |= IMA_NEW_FILE;
 }
 
 /**
-- 
2.7.5

Re: [PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[Ignaz Forster]

Am 18.12.18 um 19:47 schrieb Mimi Zohar:
> If tmpfiles can be made persistent, then newly created tmpfiles need to
> be treated like any other new files in policy.
> 
> This patch indicates which newly created tmpfiles are in policy, causing
> the file hash to be calculated on __fput().
> 
> Reported-by: Ignaz Forster <ignaz.forster@gmx.de>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

Tested-by: Ignaz Forster <iforster@suse.com>

Thanks a lot, the patch is working as expected.

Ignaz

Re: [PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[Goldwyn Rodrigues]

On 13:47 18/12, Mimi Zohar wrote:
> If tmpfiles can be made persistent, then newly created tmpfiles need to
> be treated like any other new files in policy.
> 
> This patch indicates which newly created tmpfiles are in policy, causing
> the file hash to be calculated on __fput().

Discussed in overlayfs, this would be better if we use this on inode
and called from vfs_tmpfile() instead of do_tmpfile(). This will cover
the overlayfs case which uses tmpfiles while performing copy_up().
The patch is attached.

Here is the updated patch which works for my cases.
However, it is the the failure case after setting the IMA flags
I am concerned about, though I don't think that should be as harmful.


If tmpfiles can be made persistent, then newly created tmpfiles need to
be treated like any other new files in policy.

This patch indicates which newly created tmpfiles are in policy, causing
the file hash to be calculated on __fput().

Reported-by: Ignaz Forster <ignaz.forster@gmx.de>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
 fs/namei.c                        |  1 +
 include/linux/ima.h               |  6 ++++++
 security/integrity/ima/ima_main.c | 35 +++++++++++++++++++++++++++++++++--
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 914178cdbe94..373a7ec4b09d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3462,6 +3462,7 @@ struct dentry *vfs_tmpfile(struct dentry *dentry, umode_t mode, int open_flag)
 		inode->i_state |= I_LINKABLE;
 		spin_unlock(&inode->i_lock);
 	}
+	ima_post_create_tmpfile(inode);
 	return child;
 
 out_err:
diff --git a/include/linux/ima.h b/include/linux/ima.h
index b5e16b8c50b7..32b0c5bdcd99 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -18,6 +18,7 @@ struct linux_binprm;
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask);
+extern void ima_post_create_tmpfile(struct inode *inode);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
 extern int ima_load_data(enum kernel_load_data_id id);
@@ -56,6 +57,11 @@ static inline int ima_file_check(struct file *file, int mask)
 	return 0;
 }
 
+static inline void ima_post_create_tmpfile(struct inode *inode)
+{
+	return;
+}
+
 static inline void ima_file_free(struct file *file)
 {
 	return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 4ffac4f5c647..357edd140c09 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -396,6 +396,33 @@ int ima_file_check(struct file *file, int mask)
 }
 EXPORT_SYMBOL_GPL(ima_file_check);
 
+/**
+ * ima_post_create_tmpfile - mark newly created tmpfile as new
+ * @file : newly created tmpfile
+ *
+ * No measuring, appraising or auditing of newly created tmpfiles is needed.
+ * Skip calling process_measurement(), but indicate which newly, created
+ * tmpfiles are in policy.
+ */
+void ima_post_create_tmpfile(struct inode *inode)
+{
+	struct integrity_iint_cache *iint;
+	int must_appraise;
+
+	must_appraise = ima_must_appraise(inode, MAY_ACCESS, FILE_CHECK);
+	if (!must_appraise)
+		return;
+
+	/* Nothing to do if we can't allocate memory */
+	iint = integrity_inode_get(inode);
+	if (!iint)
+		return;
+
+	/* needed for writing the security xattrs */
+	set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
+	iint->ima_file_status = INTEGRITY_PASS;
+}
+
 /**
  * ima_post_path_mknod - mark as a new inode
  * @dentry: newly created dentry
@@ -413,9 +440,13 @@ void ima_post_path_mknod(struct dentry *dentry)
 	if (!must_appraise)
 		return;
 
+	/* Nothing to do if we can't allocate memory */
 	iint = integrity_inode_get(inode);
-	if (iint)
-		iint->flags |= IMA_NEW_FILE;
+	if (!iint)
+		return;
+
+	/* needed for re-opening empty files */
+	iint->flags |= IMA_NEW_FILE;
 }
 
 /**
-- 
2.16.4

Re: [PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[Mimi Zohar]

On Thu, 2019-01-17 at 15:34 -0600, Goldwyn Rodrigues wrote:
> On 13:47 18/12, Mimi Zohar wrote:
> > If tmpfiles can be made persistent, then newly created tmpfiles need to
> > be treated like any other new files in policy.
> > 
> > This patch indicates which newly created tmpfiles are in policy, causing
> > the file hash to be calculated on __fput().
> 
> Discussed in overlayfs, this would be better if we use this on inode
> and called from vfs_tmpfile() instead of do_tmpfile(). This will cover
> the overlayfs case which uses tmpfiles while performing copy_up().
> The patch is attached.
> 
> Here is the updated patch which works for my cases.
> However, it is the the failure case after setting the IMA flags
> I am concerned about, though I don't think that should be as harmful.

Right.  The new IMA hook allocates memory for storing the flags, which
needs to be cleaned up on failure.  For this reason, the IMA call is
deferred until after the transition from locally freeing memory on
failure to relying on __fput().  In "do_last", the call to IMA is
after "opened"; and in the original version of this patch the call to
IMA is after finish_open().

Mimi

> 
> 
> If tmpfiles can be made persistent, then newly created tmpfiles need to
> be treated like any other new files in policy.
> 
> This patch indicates which newly created tmpfiles are in policy, causing
> the file hash to be calculated on __fput().
> 
> Reported-by: Ignaz Forster <ignaz.forster@gmx.de>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/namei.c                        |  1 +
>  include/linux/ima.h               |  6 ++++++
>  security/integrity/ima/ima_main.c | 35 +++++++++++++++++++++++++++++++++--
>  3 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 914178cdbe94..373a7ec4b09d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3462,6 +3462,7 @@ struct dentry *vfs_tmpfile(struct dentry *dentry, umode_t mode, int open_flag)
>  		inode->i_state |= I_LINKABLE;
>  		spin_unlock(&inode->i_lock);
>  	}
> +	ima_post_create_tmpfile(inode);
>  	return child;
>  
>  out_err:
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index b5e16b8c50b7..32b0c5bdcd99 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -18,6 +18,7 @@ struct linux_binprm;
>  #ifdef CONFIG_IMA
>  extern int ima_bprm_check(struct linux_binprm *bprm);
>  extern int ima_file_check(struct file *file, int mask);
> +extern void ima_post_create_tmpfile(struct inode *inode);
>  extern void ima_file_free(struct file *file);
>  extern int ima_file_mmap(struct file *file, unsigned long prot);
>  extern int ima_load_data(enum kernel_load_data_id id);
> @@ -56,6 +57,11 @@ static inline int ima_file_check(struct file *file, int mask)
>  	return 0;
>  }
>  
> +static inline void ima_post_create_tmpfile(struct inode *inode)
> +{
> +	return;
> +}
> +
>  static inline void ima_file_free(struct file *file)
>  {
>  	return;
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 4ffac4f5c647..357edd140c09 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -396,6 +396,33 @@ int ima_file_check(struct file *file, int mask)
>  }
>  EXPORT_SYMBOL_GPL(ima_file_check);
>  
> +/**
> + * ima_post_create_tmpfile - mark newly created tmpfile as new
> + * @file : newly created tmpfile
> + *
> + * No measuring, appraising or auditing of newly created tmpfiles is needed.
> + * Skip calling process_measurement(), but indicate which newly, created
> + * tmpfiles are in policy.
> + */
> +void ima_post_create_tmpfile(struct inode *inode)
> +{
> +	struct integrity_iint_cache *iint;
> +	int must_appraise;
> +
> +	must_appraise = ima_must_appraise(inode, MAY_ACCESS, FILE_CHECK);
> +	if (!must_appraise)
> +		return;
> +
> +	/* Nothing to do if we can't allocate memory */
> +	iint = integrity_inode_get(inode);
> +	if (!iint)
> +		return;
> +
> +	/* needed for writing the security xattrs */
> +	set_bit(IMA_UPDATE_XATTR, &iint->atomic_flags);
> +	iint->ima_file_status = INTEGRITY_PASS;
> +}
> +
>  /**
>   * ima_post_path_mknod - mark as a new inode
>   * @dentry: newly created dentry
> @@ -413,9 +440,13 @@ void ima_post_path_mknod(struct dentry *dentry)
>  	if (!must_appraise)
>  		return;
>  
> +	/* Nothing to do if we can't allocate memory */
>  	iint = integrity_inode_get(inode);
> -	if (iint)
> -		iint->flags |= IMA_NEW_FILE;
> +	if (!iint)
> +		return;
> +
> +	/* needed for re-opening empty files */
> +	iint->flags |= IMA_NEW_FILE;
>  }
>  
>  /**

Re: [PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[Amir Goldstein]

On Mon, Jan 21, 2019 at 2:00 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Thu, 2019-01-17 at 15:34 -0600, Goldwyn Rodrigues wrote:
> > On 13:47 18/12, Mimi Zohar wrote:
> > > If tmpfiles can be made persistent, then newly created tmpfiles need to
> > > be treated like any other new files in policy.
> > >
> > > This patch indicates which newly created tmpfiles are in policy, causing
> > > the file hash to be calculated on __fput().
> >
> > Discussed in overlayfs, this would be better if we use this on inode
> > and called from vfs_tmpfile() instead of do_tmpfile(). This will cover
> > the overlayfs case which uses tmpfiles while performing copy_up().
> > The patch is attached.
> >
> > Here is the updated patch which works for my cases.
> > However, it is the the failure case after setting the IMA flags
> > I am concerned about, though I don't think that should be as harmful.
>
> Right.  The new IMA hook allocates memory for storing the flags, which
> needs to be cleaned up on failure.  For this reason, the IMA call is
> deferred until after the transition from locally freeing memory on
> failure to relying on __fput().  In "do_last", the call to IMA is
> after "opened"; and in the original version of this patch the call to
> IMA is after finish_open().
>

Not sure I understand the concern.
The integrity context is associated with the inode and will be freed
on destroy_inode() no matter which error path is taken.
Am I missing something?

Thanks,
Amir.

Re: [PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[Mimi Zohar]

On Mon, 2019-01-21 at 14:29 +0200, Amir Goldstein wrote:
> On Mon, Jan 21, 2019 at 2:00 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Thu, 2019-01-17 at 15:34 -0600, Goldwyn Rodrigues wrote:
> > > On 13:47 18/12, Mimi Zohar wrote:
> > > > If tmpfiles can be made persistent, then newly created tmpfiles need to
> > > > be treated like any other new files in policy.
> > > >
> > > > This patch indicates which newly created tmpfiles are in policy, causing
> > > > the file hash to be calculated on __fput().
> > >
> > > Discussed in overlayfs, this would be better if we use this on inode
> > > and called from vfs_tmpfile() instead of do_tmpfile(). This will cover
> > > the overlayfs case which uses tmpfiles while performing copy_up().
> > > The patch is attached.
> > >
> > > Here is the updated patch which works for my cases.
> > > However, it is the the failure case after setting the IMA flags
> > > I am concerned about, though I don't think that should be as harmful.
> >
> > Right.  The new IMA hook allocates memory for storing the flags, which
> > needs to be cleaned up on failure.  For this reason, the IMA call is
> > deferred until after the transition from locally freeing memory on
> > failure to relying on __fput().  In "do_last", the call to IMA is
> > after "opened"; and in the original version of this patch the call to
> > IMA is after finish_open().
> >
> 
> Not sure I understand the concern.
> The integrity context is associated with the inode and will be freed
> on destroy_inode() no matter which error path is taken.
> Am I missing something?

No, as long as destroy_inode() is called, it should be fine.

thanks,

Mimi

Re: [PATCH v2] ima: define ima_post_create_tmpfile() hook and add missing call

[Goldwyn Rodrigues]

On 10:43 22/01, Mimi Zohar wrote:
> On Mon, 2019-01-21 at 14:29 +0200, Amir Goldstein wrote:
> > On Mon, Jan 21, 2019 at 2:00 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > On Thu, 2019-01-17 at 15:34 -0600, Goldwyn Rodrigues wrote:
> > > > On 13:47 18/12, Mimi Zohar wrote:
> > > > > If tmpfiles can be made persistent, then newly created tmpfiles need to
> > > > > be treated like any other new files in policy.
> > > > >
> > > > > This patch indicates which newly created tmpfiles are in policy, causing
> > > > > the file hash to be calculated on __fput().
> > > >
> > > > Discussed in overlayfs, this would be better if we use this on inode
> > > > and called from vfs_tmpfile() instead of do_tmpfile(). This will cover
> > > > the overlayfs case which uses tmpfiles while performing copy_up().
> > > > The patch is attached.
> > > >
> > > > Here is the updated patch which works for my cases.
> > > > However, it is the the failure case after setting the IMA flags
> > > > I am concerned about, though I don't think that should be as harmful.
> > >
> > > Right.  The new IMA hook allocates memory for storing the flags, which
> > > needs to be cleaned up on failure.  For this reason, the IMA call is
> > > deferred until after the transition from locally freeing memory on
> > > failure to relying on __fput().  In "do_last", the call to IMA is
> > > after "opened"; and in the original version of this patch the call to
> > > IMA is after finish_open().
> > >
> > 
> > Not sure I understand the concern.
> > The integrity context is associated with the inode and will be freed
> > on destroy_inode() no matter which error path is taken.
> > Am I missing something?
> 
> No, as long as destroy_inode() is called, it should be fine.
> 

Excellent. I will resend the patch as v3.

Thanks!

-- 
Goldwyn