Re: [PATCH v2 5/7] ima: Set security.ima on file close when ima_appraise=fix

[Mimi Zohar]

Please use "__fput()" rather than "file close".  Perhaps update the subject line to
something like "ima: Defer fixing security.ima to __fput()". 

On Thu, 2024-11-28 at 11:06 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> 
> IMA-Appraisal implements a fix mode, selectable from the kernel command
> line by specifying ima_appraise=fix.
> 
> The fix mode is meant to be used in a TOFU (trust on first use) model,
> where systems are supposed to work under controlled conditions before the
> real enforcement starts.
> 
> Since the systems are under controlled conditions, it is assumed that the
> files are not corrupted, and thus their current data digest can be trusted,
> and written to security.ima.
> 
> When IMA-Appraisal is switched to enforcing mode, the security.ima value
> collected during the fix mode is used as a reference value, and a mismatch
> with the current value cause the access request to be denied.
> 
> However, since fixing security.ima is placed in ima_appraise_measurement()
> during the integrity check, it requires the inode lock to be taken in
> process_measurement(), in addition to ima_update_xattr() invoked at file
> close.
> 
> Postpone the security.ima update to ima_check_last_writer(), by setting the
> new atomic flag IMA_UPDATE_XATTR_FIX in the inode integrity metadata, in
> ima_appraise_measurement(), if security.ima needs to be fixed. In this way,
> the inode lock can be removed from process_measurement(). Also, set the
> cause appropriately for the fix operation and for allowing access to new
> and empty signed files.
> 
> Finally, update security.ima when IMA_UPDATE_XATTR_FIX is set, and when
> there wasn't a previous security.ima update, which occurs if the process
> closing the file descriptor is the last writer.  
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>

Roberto, I really like the idea of removing the inode_lock in process_measurement()
needed for writing xattrs, but I'm concerned about the delay being introduced.  For
example, does it interfere with labeling the filesystem with file signatures
(with/without EVM enabled)?

> ---
>  security/integrity/ima/ima.h          |  1 +
>  security/integrity/ima/ima_appraise.c |  7 +++++--
>  security/integrity/ima/ima_main.c     | 18 +++++++++++-------
>  3 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index b4eeab48f08a..22c3b87cfcac 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -179,6 +179,7 @@ struct ima_kexec_hdr {
>  #define IMA_CHANGE_ATTR		2
>  #define IMA_DIGSIG		3
>  #define IMA_MUST_MEASURE	4
> +#define IMA_UPDATE_XATTR_FIX	5
>  
>  /* IMA integrity metadata associated with an inode */
>  struct ima_iint_cache {
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 656c709b974f..94401de8b805 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -576,8 +576,10 @@ int ima_appraise_measurement(enum ima_hooks func, struct
> ima_iint_cache *iint,
>  		if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
>  		    (!xattr_value ||
>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> -			if (!ima_fix_xattr(dentry, iint))
> -				status = INTEGRITY_PASS;
> +			/* Fix by setting security.ima on file close. */
> +			set_bit(IMA_UPDATE_XATTR_FIX, &iint->atomic_flags);
> +			status = INTEGRITY_PASS;
> +			cause = "fix";
>  		}
>  
>  		/*
> @@ -587,6 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct
> ima_iint_cache *iint,
>  		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
>  		    test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
>  			status = INTEGRITY_PASS;
> +			cause = "new-signed-file";
>  		}
>  
>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 1e474ff6a777..50b37420ea2c 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -158,13 +158,16 @@ static void ima_check_last_writer(struct ima_iint_cache
> *iint,
>  				  struct inode *inode, struct file *file)
>  {
>  	fmode_t mode = file->f_mode;
> -	bool update;
> +	bool update = false, update_fix;
>  
> -	if (!(mode & FMODE_WRITE))
> +	update_fix = test_and_clear_bit(IMA_UPDATE_XATTR_FIX,
> +					&iint->atomic_flags);
> +
> +	if (!(mode & FMODE_WRITE) && !update_fix)
>  		return;
>  
>  	ima_iint_lock(inode);
> -	if (atomic_read(&inode->i_writecount) == 1) {
> +	if (atomic_read(&inode->i_writecount) == 1 && (mode & FMODE_WRITE)) {

Probably better to reverse the "mode & FMODE_WRITE" and atomic_read() test order.

Mimi

>  		struct kstat stat;
>  
>  		update = test_and_clear_bit(IMA_UPDATE_XATTR,
> @@ -181,6 +184,10 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
>  				ima_update_xattr(iint, file);
>  		}
>  	}
> +
> +	if (!update && update_fix)
> +		ima_update_xattr(iint, file);
> +
>  	ima_iint_unlock(inode);
>  }
>  
> @@ -378,13 +385,10 @@ static int process_measurement(struct file *file, const
> struct cred *cred,
>  				      template_desc);
>  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
>  		rc = ima_check_blacklist(iint, modsig, pcr);
> -		if (rc != -EPERM) {
> -			inode_lock(inode);
> +		if (rc != -EPERM)
>  			rc = ima_appraise_measurement(func, iint, file,
>  						      pathname, xattr_value,
>  						      xattr_len, modsig);
> -			inode_unlock(inode);
> -		}
>  		if (!rc)
>  			rc = mmap_violation_check(func, file, &pathbuf,
>  						  &pathname, filename);