by Nayna Jain

[permalink] [raw]

Subject: Re: [PATCH v7 4/8] powerpc/ima: add measurement rules to ima arch specific policy

Hi Michael,

On 10/15/2019 07:29 AM, Michael Ellerman wrote:
> Nayna Jain <[email protected]> writes:
>> This patch adds the measurement rules to the arch specific policies on
>> trusted boot enabled systems.
>>
>> Signed-off-by: Nayna Jain <[email protected]>
>> Reviewed-by: Mimi Zohar <[email protected]>
>> ---
>> arch/powerpc/kernel/ima_arch.c | 45 +++++++++++++++++++++++++++++++---
>> 1 file changed, 42 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>> index c22d82965eb4..88bfe4a1a9a5 100644
>> --- a/arch/powerpc/kernel/ima_arch.c
>> +++ b/arch/powerpc/kernel/ima_arch.c
>> @@ -12,8 +12,19 @@ bool arch_ima_get_secureboot(void)
>> return is_powerpc_os_secureboot_enabled();
>> }
>>
>> -/* Defines IMA appraise rules for secureboot */
>> +/*
>> + * The "arch_rules" contains both the securebot and trustedboot rules for adding
>> + * the kexec kernel image and kernel modules file hashes to the IMA measurement
>> + * list and verifying the file signatures against known good values.
>> + *
>> + * The "appraise_type=imasig|modsig" option allows the good signature to be
>> + * stored as an xattr or as an appended signature. The "template=ima-modsig"
>> + * option includes the appended signature, when available, in the IMA
>> + * measurement list.
>> + */
>> static const char *const arch_rules[] = {
>> + "measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
>> + "measure func=MODULE_CHECK template=ima-modsig",
>> "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
>> "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> @@ -22,12 +33,40 @@ static const char *const arch_rules[] = {
>> };
>>
>> /*
>> - * Returns the relevant IMA arch policies based on the system secureboot state.
>> + * The "measure_rules" are enabled only on "trustedboot" enabled systems.
>> + * These rules add the kexec kernel image and kernel modules file hashes to
>> + * the IMA measurement list.
>> + */
>> +static const char *const measure_rules[] = {
>> + "measure func=KEXEC_KERNEL_CHECK",
>> + "measure func=MODULE_CHECK",
> Why do these ones not have "template=ima-modsig" on the end?

ima-modsig template is applicable only when IMA "collects" the appended
signatures. IMA can then include it in the measurement list.

>
>> + NULL
>> +};
>> +
>> +/*
>> + * Returns the relevant IMA arch policies based on the system secureboot
>> + * and trustedboot state.
>> */
>> const char *const *arch_get_ima_policy(void)
>> {
>> - if (is_powerpc_os_secureboot_enabled())
>> + const char *const *rules;
>> + int offset = 0;
>> +
>> + for (rules = arch_rules; *rules != NULL; rules++) {
>> + if (strncmp(*rules, "appraise", 8) == 0)
>> + break;
>> + offset++;
>> + }
> This seems like kind of a hack, doesn't it? :)
>
> What we really want is three sets of rules isn't it? But some of them
> are shared between the different sets. But they just have to be flat
> arrays of strings.
>
> I think it would probably be cleaner to just use a #define for the
> shared part of the rules, eg something like:
>
> #ifdef CONFIG_MODULE_SIG_FORCE
> #define APPRAISE_MODULE
> #else
> #define APPRAISE_MODULE \
> "appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
> #endif
>
> #define APPRAISE_KERNEL \
> "appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig"
>
> #define MEASURE_KERNEL \
> "measure func=KEXEC_KERNEL_CHECK"
>
> #define MEASURE_MODULE \
> "measure func=MODULE_CHECK"
>
> #define APPEND_TEMPLATE_IMA_MODSIG \
> " template=ima-modsig"
>
> static const char *const secure_and_trusted_rules[] = {
> MEASURE_KERNEL APPEND_TEMPLATE_IMA_MODSIG,
> MEASURE_MODULE APPEND_TEMPLATE_IMA_MODSIG,
> APPRAISE_KERNEL,
> APPRAISE_MODULE
> NULL
> };
>
> static const char *const secure_rules[] = {
> APPRAISE_KERNEL,
> APPRAISE_MODULE
> NULL
> };
>
> static const char *const trusted_rules[] = {
> MEASURE_KERNEL,
> MEASURE_MODULE,
> NULL
> };

Yes, I agree it is sort of a hack to walk through the rules to find the
start of the appraise policy. While trying your suggestion, I realized
that defining three arrays, with some rule duplication, can fix the hack
without #defines. This also improves readability of the rules. I have
just now posted the new version with the changes. Please let me know if
this looks ok.

Thanks & Regards,
- Nayna

2019-10-19 18:35:44

by Nayna Jain

[permalink] [raw]

Subject: Re: [PATCH v7 7/8] ima: check against blacklisted hashes for files with modsig

Hi Mimi,

On 10/11/2019 09:19 AM, Mimi Zohar wrote:
> On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
>> Asymmetric private keys are used to sign multiple files. The kernel
>> currently support checking against the blacklisted keys. However, if the
>> public key is blacklisted, any file signed by the blacklisted key will
>> automatically fail signature verification. We might not want to blacklist
>> all the files signed by a particular key, but just a single file.
>> Blacklisting the public key is not fine enough granularity.
>>
>> This patch adds support for blacklisting binaries with appended signatures,
>> based on the IMA policy. Defined is a new policy option
>> "appraise_flag=check_blacklist".
> The blacklisted hash is not the same as the file hash, but is the file
> hash without the appended signature. Are there tools for calculating
> the blacklisted hash? Can you provide an example?

I have updated the patch description to specify that the blacklisted
hash is the file hash without the appended signature. I hope that makes
it clear now.

Thanks & Regards,
- Nayna