Re: [PATCH v3 2/4] IMA: add support to restrict the hash algorithms used for file appraisal

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

Hi Simon,

On Tue, 2021-07-27 at 10:23 +0000, THOBY Simon wrote:
> This patch plugs in support for restricting the hash algorithms
> accepted for protecting the security.ima xattr when appraising
> files.

The first sentence should provide the motivation for the patch.  For
example, start this paragraph by saying that the hash algorithms are
currently not restricted.  Then continue with "Restrict ..." or maybe
"Provide the plumbing to restrict ...".

> 
> Each ima policy rule can have a list of allowed hash algorithms,
> and a file matching the policy but whose IMA hash is
> not explicitly in the list will not pass appraisal.

Belongs in the patch associated with the IMA policy "appraise_hash"
rule option.
> 
> This do not apply only to IMA in hash mode, it also works with digital
> signatures, in which case it checks that the hash (which was then
> signed by the trusted private key) have been generated with one of
> the algortihms whitelisted for this specific rule.

Instead of phrasing this paragraph in the negative, the content could
be combined with the first paragraph in the positive.   For example,
"Restrict the permitted set of hash algorithms used for verifying file
signatures stored in security.ima xattr."

> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---

<snip>

> @@ -327,6 +329,20 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  
>  	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
>  
> +	/* Ensure that the digest was generated using an allowed algorithm */
> +	if (appraisal_allowed_hashes &&
> +	    !(appraisal_allowed_hashes & (1U << hash_algo))) {
> +		rc = -EACCES;
> +
> +		if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
> +			pathname = ima_d_path(&file->f_path, &pathbuf, filename);
> +
> +		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
> +			pathname, "collect_data", "forbidden-hash-algorithm", rc, 0);
> +
> +		goto out_locked;
> +	}
> +

This doesn't look like the right place for checking the hash algorithm.
IMA may be configured differently on different systems.  Some might
only enable measurement without appraisal, appraisal without
measurement, or both.  Only appraisal returns a failure and prevents
read, execute, mmap, etc.   The hash algorithm check probably should be
deferred to appraisal.  Placing the test here would skip measurements.

thanks,

Mimi

>  	rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
>  	if (rc != 0 && rc != -EBADF && rc != -EINVAL)
>  		goto out_locked;