| Message ID | 20200623202640.4936-3-bmeneg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: make appraisal state runtime dependent on secure boot | expand |
On Tue, 2020-06-23 at 17:26 -0300, Bruno Meneguele wrote: <snip> > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > index edde88dbe576..62dc11a5af01 100644 > --- a/security/integrity/ima/Kconfig > +++ b/security/integrity/ima/Kconfig > @@ -232,7 +232,7 @@ config IMA_APPRAISE_REQUIRE_POLICY_SIGS > > config IMA_APPRAISE_BOOTPARAM > bool "ima_appraise boot parameter" > - depends on IMA_APPRAISE && !IMA_ARCH_POLICY > + depends on IMA_APPRAISE Ok > default y > help > This option enables the different "ima_appraise=" modes > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > index e493063a3c34..6742f86b6c60 100644 > --- a/security/integrity/ima/ima_policy.c > +++ b/security/integrity/ima/ima_policy.c > @@ -732,12 +732,20 @@ void __init ima_init_policy(void) > * and custom policies, prior to other appraise rules. > * (Highest priority) > */ > - arch_entries = ima_init_arch_policy(); > - if (!arch_entries) > - pr_info("No architecture policies found\n"); > - else > - add_rules(arch_policy_entry, arch_entries, > - IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY); > + if (arch_ima_secure_or_trusted_boot()) { Today only "measure" and "appraise" rules are included in the arch specific policy, but someone might decide they want to include "audit" rules as well. I'm not if the "secure_boot" flag is available prior to calling default_appraise_setup(), but if it is, you could modify the test there to also check if the system is booted in secure boot mode (eg. IS_ENABLED(CONFIG_IMA_APPRAISE_BOOTPARAM) && !arch_ima_get_secureboot()) > + /* In secure and/or trusted boot the appraisal must be > + * enforced, regardless kernel parameters, preventing > + * runtime changes */ Only "appraise" rules are enforced. Mimi
On Fri, Jun 26, 2020 at 04:40:23PM -0400, Mimi Zohar wrote: > On Tue, 2020-06-23 at 17:26 -0300, Bruno Meneguele wrote: > <snip> > > > diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig > > index edde88dbe576..62dc11a5af01 100644 > > --- a/security/integrity/ima/Kconfig > > +++ b/security/integrity/ima/Kconfig > > @@ -232,7 +232,7 @@ config IMA_APPRAISE_REQUIRE_POLICY_SIGS > > > > config IMA_APPRAISE_BOOTPARAM > > bool "ima_appraise boot parameter" > > - depends on IMA_APPRAISE && !IMA_ARCH_POLICY > > + depends on IMA_APPRAISE > > Ok > > > default y > > help > > This option enables the different "ima_appraise=" modes > > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c > > index e493063a3c34..6742f86b6c60 100644 > > --- a/security/integrity/ima/ima_policy.c > > +++ b/security/integrity/ima/ima_policy.c > > @@ -732,12 +732,20 @@ void __init ima_init_policy(void) > > * and custom policies, prior to other appraise rules. > > * (Highest priority) > > */ > > - arch_entries = ima_init_arch_policy(); > > - if (!arch_entries) > > - pr_info("No architecture policies found\n"); > > - else > > - add_rules(arch_policy_entry, arch_entries, > > - IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY); > > + if (arch_ima_secure_or_trusted_boot()) { > > Today only "measure" and "appraise" rules are included in the arch > specific policy, but someone might decide they want to include "audit" > rules as well. > Right, but both arches (powerpc and x86) using specific arch policies only add it in case secure and/or trusted boot are enabled. That's why I considered enclosing the whole arch_policy loading in the secure/trusted boot checking there. I would say that a fine-grained check for which action the rules have can be added later, in a separate patchset. > I'm not if the "secure_boot" flag is available prior to calling > default_appraise_setup(), but if it is, you could modify the test > there to also check if the system is booted in secure boot mode (eg. > IS_ENABLED(CONFIG_IMA_APPRAISE_BOOTPARAM) && > !arch_ima_get_secureboot()) > Well pointed. I built a custom x86 kernel with some workaround to get this flag status within default_appraise_setup() and as a result the flag is was correctly available. Considering the nature of this flag (platform's firmware (in all arches?)) can we trust that every arch supporting secure/trusted boot will have it available in the __setup() call time? > > + /* In secure and/or trusted boot the appraisal must be > > + * enforced, regardless kernel parameters, preventing > > + * runtime changes */ > > Only "appraise" rules are enforced. > Hmm.. do you mean the comment wording is wrong/"could be better", pointing the "appraise" action explicitly?
On Mon, 2020-06-29 at 20:47 -0300, Bruno Meneguele wrote: > > > I'm not if the "secure_boot" flag is available prior to calling > > default_appraise_setup(), but if it is, you could modify the test > > there to also check if the system is booted in secure boot mode (eg. > > IS_ENABLED(CONFIG_IMA_APPRAISE_BOOTPARAM) && > > !arch_ima_get_secureboot()) > > > > Well pointed. I built a custom x86 kernel with some workaround to get > this flag status within default_appraise_setup() and as a result the > flag is was correctly available. > > Considering the nature of this flag (platform's firmware (in all > arches?)) can we trust that every arch supporting secure/trusted boot > will have it available in the __setup() call time? Calling default_appraise_setup() could be deferred. > > > > + /* In secure and/or trusted boot the appraisal must be > > > + * enforced, regardless kernel parameters, preventing > > > + * runtime changes */ > > > > Only "appraise" rules are enforced. > > > > Hmm.. do you mean the comment wording is wrong/"could be better", > pointing the "appraise" action explicitly? No, it's more than just the comment. Like "trusted boot", IMA- measurement only measures files, never enforces integrity. "ima_appraise" mode is only applicable to IMA-appraisal. Mimi
On Tue, Jun 30, 2020 at 07:00:48AM -0400, Mimi Zohar wrote: > On Mon, 2020-06-29 at 20:47 -0300, Bruno Meneguele wrote: > > > > > > I'm not if the "secure_boot" flag is available prior to calling > > > default_appraise_setup(), but if it is, you could modify the test > > > there to also check if the system is booted in secure boot mode (eg. > > > IS_ENABLED(CONFIG_IMA_APPRAISE_BOOTPARAM) && > > > !arch_ima_get_secureboot()) > > > > > > > Well pointed. I built a custom x86 kernel with some workaround to get > > this flag status within default_appraise_setup() and as a result the > > flag is was correctly available. > > > > Considering the nature of this flag (platform's firmware (in all > > arches?)) can we trust that every arch supporting secure/trusted boot > > will have it available in the __setup() call time? > > Calling default_appraise_setup() could be deferred. > Hmmm.. ok, I'm going to investigate it further. Didn't really know that. > > > > > > + /* In secure and/or trusted boot the appraisal must be > > > > + * enforced, regardless kernel parameters, preventing > > > > + * runtime changes */ > > > > > > Only "appraise" rules are enforced. > > > > > > > Hmm.. do you mean the comment wording is wrong/"could be better", > > pointing the "appraise" action explicitly? > > No, it's more than just the comment. Like "trusted boot", IMA- > measurement only measures files, never enforces integrity. > "ima_appraise" mode is only applicable to IMA-appraisal. ah! Ok, I see it now and in fact it shouldn't be part of the check alongside secureboot. Well, I'm going to rethink the approach entirely then. As you said, only deferring default_appraise_setup() may be probably enough. Thanks Mimi.
On Tue, Jun 30, 2020 at 02:00:43PM -0300, Bruno Meneguele wrote: > On Tue, Jun 30, 2020 at 07:00:48AM -0400, Mimi Zohar wrote: > > On Mon, 2020-06-29 at 20:47 -0300, Bruno Meneguele wrote: > > > > > > > > > I'm not if the "secure_boot" flag is available prior to calling > > > > default_appraise_setup(), but if it is, you could modify the test > > > > there to also check if the system is booted in secure boot mode (eg. > > > > IS_ENABLED(CONFIG_IMA_APPRAISE_BOOTPARAM) && > > > > !arch_ima_get_secureboot()) > > > > > > > > > > Well pointed. I built a custom x86 kernel with some workaround to get > > > this flag status within default_appraise_setup() and as a result the > > > flag is was correctly available. > > > > > > Considering the nature of this flag (platform's firmware (in all > > > arches?)) can we trust that every arch supporting secure/trusted boot > > > will have it available in the __setup() call time? > > > > Calling default_appraise_setup() could be deferred. > > > > Hmmm.. ok, I'm going to investigate it further. > Didn't really know that. > After some research on powerpc, x86 and s390 (the only users of arch policies) codes it's clear that, no matter what, the secure boot flag will be available even before the kernel cmdline is actually copied/saved in kernel's memory. Both powerpc and x86 populate it through setup_arch() call in init/main.c:kernel_start(), where some early_params are handled, but nothing about normal (non-early) __setup() params. s390 is a bit deeper where it gets the flag, right down its boot code, even before start_kernel(). With that said, it's safe checking it directly from default_appraise_setup(). I'm going to prepare a v4, test it and post it tomorrow.
diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c index 168393d399ba..78fb61b2e480 100644 --- a/arch/x86/kernel/ima_arch.c +++ b/arch/x86/kernel/ima_arch.c @@ -85,8 +85,7 @@ static const char * const sb_arch_rules[] = { const char * const *arch_get_ima_policy(void) { - if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && - arch_ima_secure_or_tusted_boot()) { + if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY)) { if (IS_ENABLED(CONFIG_MODULE_SIG)) set_module_sig_enforced(); return sb_arch_rules; diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig index edde88dbe576..62dc11a5af01 100644 --- a/security/integrity/ima/Kconfig +++ b/security/integrity/ima/Kconfig @@ -232,7 +232,7 @@ config IMA_APPRAISE_REQUIRE_POLICY_SIGS config IMA_APPRAISE_BOOTPARAM bool "ima_appraise boot parameter" - depends on IMA_APPRAISE && !IMA_ARCH_POLICY + depends on IMA_APPRAISE default y help This option enables the different "ima_appraise=" modes diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index e493063a3c34..6742f86b6c60 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -732,12 +732,20 @@ void __init ima_init_policy(void) * and custom policies, prior to other appraise rules. * (Highest priority) */ - arch_entries = ima_init_arch_policy(); - if (!arch_entries) - pr_info("No architecture policies found\n"); - else - add_rules(arch_policy_entry, arch_entries, - IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY); + if (arch_ima_secure_or_trusted_boot()) { + /* In secure and/or trusted boot the appraisal must be + * enforced, regardless kernel parameters, preventing + * runtime changes */ + pr_info("setting IMA appraisal to enforced\n"); + ima_appraise |= IMA_APPRAISE_ENFORCE; + + arch_entries = ima_init_arch_policy(); + if (!arch_entries) + pr_info("No architecture policies found\n"); + else + add_rules(arch_policy_entry, arch_entries, + IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY); + } /* * Insert the builtin "secure_boot" policy rules requiring file
IMA_APPRAISE_BOOTPARAM has been marked as dependent on !IMA_ARCH_POLICY in compile time, enforcing the appraisal whenever the kernel had the arch policy option enabled. However it breaks systems where the option is actually set but the system wasn't booted in a "secure boot" platform. In this scenario, anytime an appraisal policy (i.e. ima_policy=appraisal_tcb) is used it will be forced, giving no chance to the user set the 'fix' state (ima_appraise=fix) to actually measure system's files. This patch remove this compile time dependency and move it to a runtime decision: all architecture that supports it so far (powerpc, x86 and s390) only enable such specific policies if the secure/trusted boot is actually enabled in the platform, thus the IMA_APPRAISE_ENFORCE flag is set whenever the secure/trusted boot state is met, otherwise the kernel paramenter value passed is used. Signed-off-by: Bruno Meneguele <bmeneg@redhat.com> --- arch/x86/kernel/ima_arch.c | 3 +-- security/integrity/ima/Kconfig | 2 +- security/integrity/ima/ima_policy.c | 20 ++++++++++++++------ 3 files changed, 16 insertions(+), 9 deletions(-)