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

Lakshmi Ramasubramanian
2021-08-04T20:51:03Z

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) {
>