linux-kernel - Re: [PATCH v3 5/6] ima: Defer fixing security.ima to _

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <574ab3058a019c0536c29f54516c48fdae82af12.camel@linux.ibm.com>
Date: Fri, 31 Jan 2025 11:52:18 -0500
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Roberto Sassu <roberto.sassu@...weicloud.com>, corbet@....net,
        viro@...iv.linux.org.uk, brauner@...nel.org, jack@...e.cz,
        dmitry.kasatkin@...il.com, eric.snowberg@...cle.com,
        paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        Roberto Sassu
 <roberto.sassu@...wei.com>
Subject: Re: [PATCH v3 5/6] ima: Defer fixing security.ima to __fput()

On Wed, 2025-01-22 at 18:24 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@...wei.com>
> 
> 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.
> 
> Deferring fixing security.ima has a side effect: metadata of files with an
> invalid EVM HMAC cannot be updated until the file is close. In alternative
> to waiting, it is also recommended to add 'evm=fix' in the kernel command
> line to handle this case (recommendation added to kernel-parameters.txt as
> well).
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> ---

[ ... ]

> --- 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 ((mode & FMODE_WRITE) && atomic_read(&inode->i_writecount) == 1) {
>  		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);

In ima_appraise_measurement() IMA calls EVM to verify the file metadata
(evm_verifyxattr). One would think that since IMA is not "fixing" security.ima, EVM
would not require the inode lock to be taken by IMA.  However, in addition to
verifying the file metdata, EVM converts the original file metadata signature to an
HMAC.  This does require the inode lock. Perhaps the EVM conversion from a signature
to an HMAC needs to be deferred as well.

Mimi