Re: [PATCH v6 4/5] IMA: add a policy option to restrict xattr hash algorithms on appraisal

[Lakshmi Ramasubramanian <nramas@linux.microsoft.com>]

Hi Simon,

On 8/4/2021 2:20 AM, THOBY Simon wrote:

> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   Documentation/ABI/testing/ima_policy |  6 ++-
>   security/integrity/ima/ima_policy.c  | 75 ++++++++++++++++++++++++++--
>   2 files changed, 76 insertions(+), 5 deletions(-)
> 

>   
> +static unsigned int ima_parse_appraise_hash(char *arg)
> +{
> +	unsigned int res = 0;
> +	int idx;
> +	char *token;
> +
> +	while ((token = strsep(&arg, ",")) != NULL) {
> +		idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);
> +
> +		if (idx < 0) {
> +			pr_err("unknown hash algorithm \"%s\", ignoring",
> +			       token);
> +			continue;
Is it right to ignore an invalid digest algorithm given in the IMA 
policy rule? Should "invalid ima policy" error be reported instead?

Other changes look good.

Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

  -lakshmi

> +		}
> +
> +		/* Add the hash algorithm to the 'allowed' bitfield */
> +		res |= (1U << idx);
> +	}
> +
> +	return res;
> +}
> +
>   static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   {
>   	struct audit_buffer *ab;
> @@ -1522,6 +1546,26 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>   			else
>   				result = -EINVAL;
>   			break;
> +		case Opt_appraise_hash:
> +			ima_log_string(ab, "appraise_hash", args[0].from);
> +
> +			if (entry->allowed_hashes) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
> +			entry->allowed_hashes =
> +				ima_parse_appraise_hash(args[0].from);
> +
> +			/* invalid or empty list of algorithms */
> +			if (!entry->allowed_hashes) {
> +				result = -EINVAL;
> +				break;
> +			}
> +
> +			entry->flags |= IMA_VALIDATE_HASH;
> +
> +			break;
>   		case Opt_permit_directio:
>   			entry->flags |= IMA_PERMIT_DIRECTIO;
>   			break;
> @@ -1714,6 +1758,23 @@ static void ima_show_rule_opt_list(struct seq_file *m,
>   		seq_printf(m, "%s%s", i ? "|" : "", opt_list->items[i]);
>   }
>   
> +static void ima_policy_show_appraise_hash(struct seq_file *m,
> +					  unsigned int allowed_hashes)
> +{
> +	int idx, list_size = 0;
> +
> +	for (idx = 0; idx < HASH_ALGO__LAST; idx++) {
> +		if (!(allowed_hashes & (1U << idx)))
> +			continue;
> +
> +		/* only add commas if the list contains multiple entries */
> +		if (list_size++)
> +			seq_puts(m, ",");
> +
> +		seq_puts(m, hash_algo_name[idx]);
> +	}
> +}
> +
>   int ima_policy_show(struct seq_file *m, void *v)
>   {
>   	struct ima_rule_entry *entry = v;
> @@ -1825,6 +1886,12 @@ int ima_policy_show(struct seq_file *m, void *v)
>   		seq_puts(m, " ");
>   	}
>   
> +	if (entry->flags & IMA_VALIDATE_HASH) {
> +		seq_puts(m, "appraise_hash=");
> +		ima_policy_show_appraise_hash(m, entry->allowed_hashes);
> +		seq_puts(m, " ");
> +	}
> +
>   	for (i = 0; i < MAX_LSM_RULES; i++) {
>   		if (entry->lsm[i].rule) {
>   			switch (i) {
>