Re: [PATCH v3] integrity: Always reference the blacklist keyring with apprasial

[Eric Snowberg]

> On Jul 14, 2023, at 1:22 AM, Roberto Sassu <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> 
> On Thu, 2023-07-13 at 21:11 -0400, Eric Snowberg wrote:
>> Commit 273df864cf746 ("ima: Check against blacklisted hashes for files with
>> modsig") introduced an appraise_flag option for referencing the blacklist
>> keyring.  Any matching binary found on this keyring fails signature
>> validation. This flag only works with module appended signatures.
>> 
>> An important part of a PKI infrastructure is to have the ability to do
>> revocation at a later time should a vulnerability be found.  Expand the
>> revocation flag usage to all appraisal functions. The flag is now
>> enabled by default. Setting the flag with an IMA policy has been
>> deprecated. Without a revocation capability like this in place, only
>> authenticity can be maintained. With this change, integrity can now be
>> achieved with digital signature based IMA appraisal.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
>> ---
>> v3 changes:
>>  No longer display appraise_flag=check_blacklist in the policy
>> v2 changes:
>>  Update the "case Opt_apprase_flag"
>>  Removed "appraise_flag=" in the powerpc arch specific policy rules
>> ---
>> Documentation/ABI/testing/ima_policy  |  6 +++---
>> arch/powerpc/kernel/ima_arch.c        |  8 ++++----
>> security/integrity/ima/ima_appraise.c | 12 +++++++-----
>> security/integrity/ima/ima_policy.c   | 17 +++++------------
>> 4 files changed, 19 insertions(+), 24 deletions(-)
>> 
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index 49db0ff288e5..a712c396f6e9 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -57,9 +57,9 @@ Description:
>> 				stored in security.ima xattr. Requires
>> 				specifying "digest_type=verity" first.)
>> 
>> -			appraise_flag:= [check_blacklist]
>> -			Currently, blacklist check is only for files signed with appended
>> -			signature.
>> +			appraise_flag:= [check_blacklist] (deprecated)
>> +			Setting the check_blacklist flag is no longer necessary.
>> +			All apprasial functions set it by default.
> 
> Typo.

I will fix that

>> 			digest_type:= verity
>> 			    Require fs-verity's file digest instead of the
>> 			    regular IMA file hash.
>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>> index 957abd592075..b7029beed847 100644
>> --- a/arch/powerpc/kernel/ima_arch.c
>> +++ b/arch/powerpc/kernel/ima_arch.c
>> @@ -23,9 +23,9 @@ bool arch_ima_get_secureboot(void)
>>  * is not enabled.
>>  */
>> static const char *const secure_rules[] = {
>> -	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> #ifndef CONFIG_MODULE_SIG
>> -	"appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> #endif
>> 	NULL
>> };
>> @@ -49,9 +49,9 @@ static const char *const trusted_rules[] = {
>> static const char *const secure_and_trusted_rules[] = {
>> 	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
>> 	"measure func=MODULE_CHECK template=ima-modsig",
>> -	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> #ifndef CONFIG_MODULE_SIG
>> -	"appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> #endif
>> 	NULL
>> };
>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> index 491c1aca0b1c..870dde67707b 100644
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -458,11 +458,13 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
>> 		ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
>> 
>> 		rc = is_binary_blacklisted(digest, digestsize);
>> -		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
>> -			process_buffer_measurement(&nop_mnt_idmap, NULL, digest, digestsize,
>> -						   "blacklisted-hash", NONE,
>> -						   pcr, NULL, false, NULL, 0);
>> -	}
>> +	} else if (iint->flags & IMA_DIGSIG_REQUIRED && iint->ima_hash)
>> +		rc = is_binary_blacklisted(iint->ima_hash->digest, iint->ima_hash->length);
> 
> Curiosity (I didn't read the previous discussions), if you are checking
> if binaries are blacklisted, why not doing for the BPRM_CHECK hook?

Yes, if the policy contains BPRM_CHECK, binaries are now checked to see
if they have been revoked.

>> +
>> +	if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> 
> Uhm, the measurement will be done only if you are also doing appraisal
> with digital signatures. But if you have only measure rules, you won't
> know. Shouldn't you run is_binary_blacklisted() also for measure rules?

This part of the patch is code that was moved down (see above where it used to be 
located) to allow for code reuse. Previously the measurement would only be done
when doing appraisal. With it being moved to this location, nothing changes.

>> +		process_buffer_measurement(&nop_mnt_idmap, NULL, digest, digestsize,
>> +					   "blacklisted-hash", NONE,
>> +					   pcr, NULL, false, NULL, 0);
>> 
>> 	return rc;
>> }
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index c9b3bd8f1bb9..69452b79686b 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -1280,7 +1280,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>> 				     IMA_FSNAME | IMA_GID | IMA_EGID |
>> 				     IMA_FGROUP | IMA_DIGSIG_REQUIRED |
>> 				     IMA_PERMIT_DIRECTIO | IMA_VALIDATE_ALGOS |
>> -				     IMA_VERITY_REQUIRED))
>> +				     IMA_CHECK_BLACKLIST | IMA_VERITY_REQUIRED))
>> 			return false;
>> 
>> 		break;
>> @@ -1355,7 +1355,7 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>> 
>> 	/* Ensure that combinations of flags are compatible with each other */
>> 	if (entry->flags & IMA_CHECK_BLACKLIST &&
>> -	    !(entry->flags & IMA_MODSIG_ALLOWED))
>> +	    !(entry->flags & IMA_DIGSIG_REQUIRED))
>> 		return false;
>> 
>> 	/*
>> @@ -1803,11 +1803,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>> 				if (entry->flags & IMA_VERITY_REQUIRED)
>> 					result = -EINVAL;
>> 				else
>> -					entry->flags |= IMA_DIGSIG_REQUIRED;
>> +					entry->flags |= IMA_DIGSIG_REQUIRED | IMA_CHECK_BLACKLIST;
>> 			} else if (strcmp(args[0].from, "sigv3") == 0) {
>> 				/* Only fsverity supports sigv3 for now */
>> 				if (entry->flags & IMA_VERITY_REQUIRED)
>> -					entry->flags |= IMA_DIGSIG_REQUIRED;
>> +					entry->flags |= IMA_DIGSIG_REQUIRED | IMA_CHECK_BLACKLIST;
>> 				else
>> 					result = -EINVAL;
>> 			} else if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
>> @@ -1816,18 +1816,13 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>> 					result = -EINVAL;
>> 				else
>> 					entry->flags |= IMA_DIGSIG_REQUIRED |
>> -						IMA_MODSIG_ALLOWED;
>> +						IMA_MODSIG_ALLOWED | IMA_CHECK_BLACKLIST;
>> 			} else {
>> 				result = -EINVAL;
>> 			}
>> 			break;
>> 		case Opt_appraise_flag:
>> 			ima_log_string(ab, "appraise_flag", args[0].from);
>> -			if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
>> -			    strstr(args[0].from, "blacklist"))
>> -				entry->flags |= IMA_CHECK_BLACKLIST;
>> -			else
>> -				result = -EINVAL;
>> 			break;
>> 		case Opt_appraise_algos:
>> 			ima_log_string(ab, "appraise_algos", args[0].from);
>> @@ -2271,8 +2266,6 @@ int ima_policy_show(struct seq_file *m, void *v)
>> 	}
>> 	if (entry->flags & IMA_VERITY_REQUIRED)
>> 		seq_puts(m, "digest_type=verity ");
>> -	if (entry->flags & IMA_CHECK_BLACKLIST)
>> -		seq_puts(m, "appraise_flag=check_blacklist ");
>> 	if (entry->flags & IMA_PERMIT_DIRECTIO)
>> 		seq_puts(m, "permit_directio ");
>> 	rcu_read_unlock();