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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6f1d20dc-fb1e-58ed-a060-1537c19beed0@linux.vnet.ibm.com>
Date:   Sat, 19 Oct 2019 14:27:07 -0400
From:   Nayna <nayna@...ux.vnet.ibm.com>
To:     Michael Ellerman <mpe@...erman.id.au>,
        Nayna Jain <nayna@...ux.ibm.com>, linuxppc-dev@...abs.org,
        linux-efi@...r.kernel.org, linux-integrity@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Jeremy Kerr <jk@...abs.org>,
        Matthew Garret <matthew.garret@...ula.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Claudio Carvalho <cclaudio@...ux.ibm.com>,
        George Wilson <gcwilson@...ux.ibm.com>,
        Elaine Palmer <erpalmer@...ibm.com>,
        Eric Ricther <erichte@...ux.ibm.com>,
        "Oliver O'Halloran" <oohall@...il.com>
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 <nayna@...ux.ibm.com> writes:
>> This patch adds the measurement rules to the arch specific policies on
>> trusted boot enabled systems.
>>
>> Signed-off-by: Nayna Jain <nayna@...ux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@...ux.ibm.com>
>> ---
>>   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