Re: [PATCH v2 1/3] IMA: block writes of the security.ima xattr with weak hash algorithms

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

Hi Simon,

On Tue, 2021-07-20 at 09:25 +0000, THOBY Simon wrote:
> By default, writes of the extended attributes security.ima will be
> forbidden if the xattr value uses a hash algorithm not compiled in the
> kernel. Disabling weak hashes when building the kernel will thus prevent
> their use in IMA (these hashes will not only be blocked for setxattr,
> but they won't be allowed for measurement/appraisal either as the kernel
> will obviously not be able to measure files hashed with them). Note
> however that CONFIG_IMA depends on CONFIG_CRYPTO_MD5 and
> CONFIG_CRYPTO_SHA1, so this limits the security benefits of this
> measure.
> To bypass that limitation, if secure boot is enabled on the system,
> the allowed algorithms are further restricted: only writes performed
> with the algorithm specified in the ima_hash parameter (defined at
> build-time with CONFIG_IMA_DEFAULT_HASH or overwritten with a boot
> cmdline option) are allowed.

Although the intention of this patch is to prevent writing file
signatures based on weak hash algorithms, there are two logical
changes.  Each should be a separate patch.  For example, one patch
would only allow writing security.ima signatures based on configured
hash algorithms, while the other patch would limit writing security.ima
signatures based on the IMA default hash algorithm.

Basing the decision on whether to limit the security.ima signature to
the IMA default hash algorithm based on the secure boot flag, seems
rather arbitrary.   Instead perhaps base it on whether the IMA policy
contains any new policy rule "appraise_hash" options.  A policy without
the new "appraise_hash" option would permit both writing and verifying
signatures based on any configured hash algorithm.   A policy
containing "appraise_hash", would both limit the hash algorithms
writing the security.ima signatures and verifying them.

A new builtin policy could be defined based on the new "appraise_hash"
option or simply a flag (e.g. ima_policy=).


> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>  security/integrity/ima/ima_appraise.c | 54 +++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ef9dcfce45d4..e9a24acf25c6 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -575,6 +575,54 @@ static void ima_reset_appraise_flags(struct inode *inode, int digsig)
>  		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
>  }
>  
> +/**
> + * ima_setxattr_validate_hash_alg
> + *
> + * Called when the user tries to write the security.ima xattr.
> + * The xattr value maps to the hash algorithm hash_alg, and this function
> + * returns whether this setxattr should be allowed, emitting an audit
> + * message if necessary.
> + */
> +int ima_setxattr_validate_hash_alg(struct dentry *dentry,
> +				   const void *xattr_value,
> +				   size_t xattr_value_len)
> +{
> +	int res = -EACCES;
> +	char *path = NULL, *pathbuf = NULL;
> +	enum hash_algo hash_alg =
> +		ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
> +				  xattr_value_len);
> +
> +	/**

"/**" is used to indicate the start of kernel-doc comments.  Please use
the normal "/*"
comment here.

> +	 * When secure boot is enabled, only accept the IMA xattr if
> +	 * hashed with the same algorithm as defined in the ima_hash
> +	 * parameter (just like the 'ima_appraise' cmdline parameter
> +	 * is ignored if secure boot is enabled)
> +	 */
> +	if (arch_ima_get_secureboot() && hash_alg != ima_hash_algo)
> +		goto out_warn;
> +
> +	/* disallow xattr writes with algorithms not built in the kernel */
> +	if (hash_alg != ima_hash_algo
> +	    && !crypto_has_alg(hash_algo_name[hash_alg], 0, 0))
> +		goto out_warn;
> +
> +	return 0;
> +
> +out_warn:
> +	pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
> +	/* no memory available ? no file path for you */
> +	if (pathbuf)
> +		path = dentry_path(dentry, pathbuf, PATH_MAX);
> +
> +	integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
> +		path, "collect_data", "forbidden-hash-algorithm", res, 0);

This function is writing security xattrs, not collecting/calculating
the file hash.  Please update the audit message.  Instead of
"forbidden", perhaps use something a little less dramatic, like
"unsupported" or even "denied".

thanks,

Mimi

> +
> +	kfree(pathbuf);
> +
> +	return res;
> +}
> +
>  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		       const void *xattr_value, size_t xattr_value_len)
>  {
> @@ -592,6 +640,12 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG);
>  	}
>  	if (result == 1 || evm_revalidate_status(xattr_name)) {
> +		/* the user-supplied xattr must use an allowed hash algo */
> +		int rc = ima_setxattr_validate_hash_alg(dentry, xattr_value,
> +							xattr_value_len);
> +		if (rc != 0)
> +			return rc;
> +
>  		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
>  		if (result == 1)
>  			result = 0;