Re: [PATCH 07/11] evm: Set IMA_CHANGE_XATTR/ATTR bit if EVM_ALLOW_METADATA_WRITES is set

[Mimi Zohar]

On Thu, 2020-06-18 at 18:04 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
> 
> However, IMA is not notified about metadata changes and, after the first
> appraisal, always allows access to the files without checking metadata
> again.

^after the first successful appraisal
> 
> This patch checks in evm_reset_status() if EVM_ALLOW_METADATA WRITES is
> enabled and if it is, sets the IMA_CHANGE_XATTR/ATTR bits depending on the
> operation performed. At the next appraisal, metadata are revalidated.

EVM modifying IMA bits crosses the boundary between EVM and IMA.  There
is already an IMA post_setattr hook.  IMA could reset its own bit
there.  If necessary EVM could export as a function it's status info.

Mimi

> 
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
> 
> Cc: stable@xxxxxxxxxxxxxxx # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
>  security/integrity/evm/evm_main.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 41cc6a4aaaab..d4d918183094 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -478,13 +478,17 @@ int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>  	return evm_protect_xattr(dentry, xattr_name, NULL, 0);
>  }
>  
> -static void evm_reset_status(struct inode *inode)
> +static void evm_reset_status(struct inode *inode, int bit)
>  {
>  	struct integrity_iint_cache *iint;
>  
>  	iint = integrity_iint_find(inode);
> -	if (iint)
> +	if (iint) {
> +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> +			set_bit(bit, &iint->atomic_flags);
> +
>  		iint->evm_status = INTEGRITY_UNKNOWN;
> +	}
>  }
>  
>  /**:q
> @@ -507,7 +511,7 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
>  				  && !posix_xattr_acl(xattr_name)))
>  		return;
>  
> -	evm_reset_status(dentry->d_inode);
> +	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>  
>  	evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len);
>  }
> @@ -527,7 +531,7 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
>  	if (!evm_key_loaded() || !evm_protected_xattr(xattr_name))
>  		return;
>  
> -	evm_reset_status(dentry->d_inode);
> +	evm_reset_status(dentry->d_inode, IMA_CHANGE_XATTR);
>  
>  	evm_update_evmxattr(dentry, xattr_name, NULL, 0);
>  }
> @@ -600,6 +604,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>  	if (!evm_key_loaded())
>  		return;
>  
> +	evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR);
> +
>  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>  		evm_update_evmxattr(dentry, NULL, NULL, 0);
>  }