Re: [PATCH v6 2/5] IMA: block writes of the security.ima xattr with unsupported algorithms

[Mimi Zohar <zohar@linux.ibm.com>]

On Wed, 2021-08-04 at 09:20 +0000, THOBY Simon wrote:
> By default, writes to the extended attributes security.ima will be
> allowed even if the hash algorithm used for the xattr is not compiled
> in the kernel (which does not make sense because the kernel would not
> be able to appraise that file as it lacks support for validating the
> hash).
> 
> Prevent writes to the security.ima xattr if the hash algorithm used is
> not available in the current kernel. Lo an audit message if such an
> operation is attempted.

Both "log" and "audit" can be used as verbs.   Perhaps update the first
line to be "Prevent and audit writes...", making the last line
unnecessary.

> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>  security/integrity/ima/ima.h          |  2 +-
>  security/integrity/ima/ima_appraise.c | 49 ++++++++++++++++++++++++++-
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 2f4c20b16ad7..829478dabeeb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -319,7 +319,7 @@ int ima_must_appraise(struct user_namespace *mnt_userns, struct inode *inode,
>  void ima_update_xattr(struct integrity_iint_cache *iint, struct file *file);
>  enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
>  					   enum ima_hooks func);
> -enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> +enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
>  				 int xattr_len);
>  int ima_read_xattr(struct dentry *dentry,
>  		   struct evm_ima_xattr_data **xattr_value);
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 63bec42c353f..ed1a98f6ee19 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -171,7 +171,7 @@ static void ima_cache_flags(struct integrity_iint_cache *iint,
>  	}
>  }
>  
> -enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
> +enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
>  				 int xattr_len)
>  {
>  	struct signature_v2_hdr *sig;
> @@ -575,6 +575,51 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
>  		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
>  }
>  
> +/**

Just a reminder, static functions don't require kernel-doc.

> + * validate_hash_algo() - Block setxattr with invalid digests

The digest might be valid, but the algorithm is unsupported.  Perhaps
"Block setxattr with unsupported hash algorithms" would be more
accurate.

> + * @dentry: object being setxattr()'ed

^object of the setxattr()

> + * @xattr_value: value supplied by userland for the xattr

userland supplied xattr value

> + * @xattr_value_len: length of xattr_value
> + *
> + * Context: called when the user tries to write the security.ima xattr.

Hm, "context" is probably unnecessary.

From here:
> + * The xattr value is mapped to some hash algorithm, and this algorithm
> + * must be built in the kernel for the setxattr to be allowed.
> + *
> + * Emit an audit message when the algorithm is invalid.
> + *

to here: is the longer summary, which would be before the "Context:" if
it is defined.

> + * Return: 0 on success, else an error.
> + */
> +static int validate_hash_algo(struct dentry *dentry,
> +				   const struct evm_ima_xattr_data *xattr_value,
> +				   size_t xattr_value_len)
> +{
> +	int result = 0;
> +	char *path = NULL, *pathbuf = NULL;
> +	enum hash_algo xattr_hash_algo;
> +
> +	xattr_hash_algo = ima_get_hash_algo(xattr_value, xattr_value_len);
> +
> +	if (likely(xattr_hash_algo == ima_hash_algo ||
> +		   crypto_has_alg(hash_algo_name[xattr_hash_algo], 0, 0)))
> +		return result;
> +
> +	result = -EACCES;

Unless there is a common function exit, I would just hard code the
return value.
> +
> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	if (!pathbuf)
> +		return result;
> +
> +	path = dentry_path(dentry, pathbuf, PATH_MAX);
> +
> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry), path,
> +			    "collect_data", "unavailable-hash-algorithm",
> +			    result, 0);
> +
> +	kfree(pathbuf);
> +
> +	return result;
> +}
> +
>  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		       const void *xattr_value, size_t xattr_value_len)
>  {
> @@ -595,6 +640,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
>  		if (result == 1)
>  			result = 0;
> +
> +		result = validate_hash_algo(dentry, xvalue, xattr_value_len);

On failure to validate the hash algorithm, the file will unnecessarily
be re-verified.  Only on success the appraise flags should be reset.

thanks,

Mimi

>  	}
>  	return result;
>  }