[PATCH v3 0/2] ima: make appraisal state runtime dependent on secure boot

[PATCH v3 0/2] ima: make appraisal state runtime dependent on secure boot

[Bruno Meneguele]

To switch APPRAISE_BOOTPARAM and ARCH_POLICY dependency from compile time to
run time the secure boot checking code (specific to each arch) had to be
slightly modified to include, in the PowerPC arch, the Trusted Boot state,
which is also relevant to the arch policy choice and also required the
ima_appraise to be enforced. 

With that I changed the checking order: instead of first check the
arch_policy and then the secure/trusted boot state, now we first check the
boot state, set ima_appraise to be enforced and then the existence of arch
policy. In other words, whenever secure/trusted boot is enabled,
(ima_appraise & IMA_APPRAISE_ENFORCE) == true.

I've tested these patches in a x86_64 platform with and without secure boot
enabled and in a PowerPC without secure boot enabled:

1) with secure boot enabled (x86_64) and ima_policy=appraise_tcb, the
ima_appraise= options were completly ignored and the boot always failed with
"missing-hash" for /sbin/init, which is the expected result;

2) with secure boot enabled (x86_64), but no ima_policy:

[    1.396111] ima: Allocated hash algorithm: sha256
[    1.424025] ima: setting IMA appraisal to enforced
[    1.424039] audit: type=1807 audit(1592927955.557:2): action=measure func=KEXEC_KERNEL_CHECK res=1
[    1.424040] audit: type=1807 audit(1592927955.557:3): action=measure func=MODULE_CHECK res=1

3) with secure boot disabled (PowerPC and x86_64) and
"ima_policy=appraise_tcb ima_appraise=fix", audit messages were triggered
with "op=appraisal_data cause=missing-hash" but the system worked fine due
to "fix".

Bruno Meneguele (2):
  arch/ima: extend secure boot check to include trusted boot
  ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime

 arch/powerpc/kernel/ima_arch.c      |  5 +++--
 arch/s390/kernel/ima_arch.c         |  2 +-
 arch/x86/kernel/ima_arch.c          |  4 ++--
 include/linux/ima.h                 |  4 ++--
 security/integrity/ima/Kconfig      |  2 +-
 security/integrity/ima/ima_main.c   |  2 +-
 security/integrity/ima/ima_policy.c | 20 ++++++++++++++------
 7 files changed, 24 insertions(+), 15 deletions(-)

-- 
2.26.2

[PATCH v3 1/2] arch/ima: extend secure boot check to include trusted boot

[Bruno Meneguele]

ima_get_secureboot() has been used for checking platform's secure boot
state for enabling different arch specific IMA policies where available.
However, for powerpc there also is the concept of Trusted Boot, which is
also relevant to the check code.

This patch extend the code or'ing the Trusted Boot state in PowerPC arch
while leaving the other arches (x86 and s390) unchanged. The only changes
performed in the other arches is related to the function name change.

Signed-off-by: Bruno Meneguele <bmeneg@redhat.com>
---
 arch/powerpc/kernel/ima_arch.c    | 5 +++--
 arch/s390/kernel/ima_arch.c       | 2 +-
 arch/x86/kernel/ima_arch.c        | 5 +++--
 include/linux/ima.h               | 4 ++--
 security/integrity/ima/ima_main.c | 2 +-
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index 957abd592075..32b26b491c07 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -7,9 +7,10 @@
 #include <linux/ima.h>
 #include <asm/secure_boot.h>
 
-bool arch_ima_get_secureboot(void)
+bool arch_ima_secure_or_trusted_boot(void)
 {
-	return is_ppc_secureboot_enabled();
+	return (is_ppc_secureboot_enabled() ||
+		is_ppc_trustedboot_enabled());
 }
 
 /*
diff --git a/arch/s390/kernel/ima_arch.c b/arch/s390/kernel/ima_arch.c
index f3c3e6e1c5d3..9cf823cf2b79 100644
--- a/arch/s390/kernel/ima_arch.c
+++ b/arch/s390/kernel/ima_arch.c
@@ -3,7 +3,7 @@
 #include <linux/ima.h>
 #include <asm/boot_data.h>
 
-bool arch_ima_get_secureboot(void)
+bool arch_ima_secure_or_trusted_boot(void)
 {
 	return ipl_secure_flag;
 }
diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
index 7dfb1e808928..168393d399ba 100644
--- a/arch/x86/kernel/ima_arch.c
+++ b/arch/x86/kernel/ima_arch.c
@@ -51,7 +51,7 @@ static enum efi_secureboot_mode get_sb_mode(void)
 	return efi_secureboot_mode_enabled;
 }
 
-bool arch_ima_get_secureboot(void)
+bool arch_ima_secure_or_trusted_boot(void)
 {
 	static enum efi_secureboot_mode sb_mode;
 	static bool initialized;
@@ -85,7 +85,8 @@ static const char * const sb_arch_rules[] = {
 
 const char * const *arch_get_ima_policy(void)
 {
-	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot()) {
+	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) &&
+	    arch_ima_secure_or_tusted_boot()) {
 		if (IS_ENABLED(CONFIG_MODULE_SIG))
 			set_module_sig_enforced();
 		return sb_arch_rules;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..839b5c376ed6 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -32,10 +32,10 @@ extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
 #ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
-extern bool arch_ima_get_secureboot(void);
+extern bool arch_ima_secure_or_trusted_boot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else
-static inline bool arch_ima_get_secureboot(void)
+static inline bool arch_ima_secure_or_trusted_boot(void)
 {
 	return false;
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c1583d98c5e5..a760094e8f8d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -694,7 +694,7 @@ int ima_load_data(enum kernel_load_data_id id)
 	switch (id) {
 	case LOADING_KEXEC_IMAGE:
 		if (IS_ENABLED(CONFIG_KEXEC_SIG)
-		    && arch_ima_get_secureboot()) {
+		    && arch_ima_secure_or_trusted_boot()) {
 			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
 			return -EACCES;
 		}
-- 
2.26.2

[PATCH v3 2/2] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime

[Bruno Meneguele]

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

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
-- 
2.26.2

Re: [PATCH v3 0/2] ima: make appraisal state runtime dependent on secure boot

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 2760 bytes --]

Gentle ping for review.

I also forgot to add the changelog for the patch, please see below.

On Tue, Jun 23, 2020 at 05:26:38PM -0300, Bruno Meneguele wrote:
> To switch APPRAISE_BOOTPARAM and ARCH_POLICY dependency from compile time to
> run time the secure boot checking code (specific to each arch) had to be
> slightly modified to include, in the PowerPC arch, the Trusted Boot state,
> which is also relevant to the arch policy choice and also required the
> ima_appraise to be enforced. 
> 
> With that I changed the checking order: instead of first check the
> arch_policy and then the secure/trusted boot state, now we first check the
> boot state, set ima_appraise to be enforced and then the existence of arch
> policy. In other words, whenever secure/trusted boot is enabled,
> (ima_appraise & IMA_APPRAISE_ENFORCE) == true.
> 
> I've tested these patches in a x86_64 platform with and without secure boot
> enabled and in a PowerPC without secure boot enabled:
> 
> 1) with secure boot enabled (x86_64) and ima_policy=appraise_tcb, the
> ima_appraise= options were completly ignored and the boot always failed with
> "missing-hash" for /sbin/init, which is the expected result;
> 
> 2) with secure boot enabled (x86_64), but no ima_policy:
> 
> [    1.396111] ima: Allocated hash algorithm: sha256
> [    1.424025] ima: setting IMA appraisal to enforced
> [    1.424039] audit: type=1807 audit(1592927955.557:2): action=measure func=KEXEC_KERNEL_CHECK res=1
> [    1.424040] audit: type=1807 audit(1592927955.557:3): action=measure func=MODULE_CHECK res=1
> 
> 3) with secure boot disabled (PowerPC and x86_64) and
> "ima_policy=appraise_tcb ima_appraise=fix", audit messages were triggered
> with "op=appraisal_data cause=missing-hash" but the system worked fine due
> to "fix".

Changelog:

v2:
  - pr_info() message prefix correction
v3:
  - extend secure boot arch checker to also consider trusted boot
  - enforce IMA appraisal when secure boot is effectively enabled (Nayna)
  - fix ima_appraise flag assignment by or'ing it (Mimi)

> 
> Bruno Meneguele (2):
>   arch/ima: extend secure boot check to include trusted boot
>   ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime
> 
>  arch/powerpc/kernel/ima_arch.c      |  5 +++--
>  arch/s390/kernel/ima_arch.c         |  2 +-
>  arch/x86/kernel/ima_arch.c          |  4 ++--
>  include/linux/ima.h                 |  4 ++--
>  security/integrity/ima/Kconfig      |  2 +-
>  security/integrity/ima/ima_main.c   |  2 +-
>  security/integrity/ima/ima_policy.c | 20 ++++++++++++++------
>  7 files changed, 24 insertions(+), 15 deletions(-)
> 
> -- 
> 2.26.2
> 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] arch/ima: extend secure boot check to include trusted boot

[Mimi Zohar]

On Tue, 2020-06-23 at 17:26 -0300, Bruno Meneguele wrote:
<snip>

> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..a760094e8f8d 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -694,7 +694,7 @@ int ima_load_data(enum kernel_load_data_id id)
>  	switch (id) {
>  	case LOADING_KEXEC_IMAGE:
>  		if (IS_ENABLED(CONFIG_KEXEC_SIG)
> -		    && arch_ima_get_secureboot()) {
> +		    && arch_ima_secure_or_trusted_boot()) {
>  			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
>  			return -EACCES;
>  		}

Only IMA-appraisal enforces file integrity based on policy.

Mimi

Re: [PATCH v3 2/2] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime

[Mimi Zohar]

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

Re: [PATCH v3 2/2] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 2931 bytes --]

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?

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/2] arch/ima: extend secure boot check to include trusted boot

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1118 bytes --]

On Fri, Jun 26, 2020 at 04:23:12PM -0400, Mimi Zohar wrote:
> On Tue, 2020-06-23 at 17:26 -0300, Bruno Meneguele wrote:
> <snip>
> 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index c1583d98c5e5..a760094e8f8d 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -694,7 +694,7 @@ int ima_load_data(enum kernel_load_data_id id)
> >  	switch (id) {
> >  	case LOADING_KEXEC_IMAGE:
> >  		if (IS_ENABLED(CONFIG_KEXEC_SIG)
> > -		    && arch_ima_get_secureboot()) {
> > +		    && arch_ima_secure_or_trusted_boot()) {
> >  			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
> >  			return -EACCES;
> >  		}
> 
> Only IMA-appraisal enforces file integrity based on policy.
> 

Right, but I didn't get the relation to the code above: I basically
renamed the function: 

"arch_ima_get_secureboot" -> "arch_ima_secure_or_trusted_boot".  

Which doesn't change the ima_load_data logic.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime

[Mimi Zohar]

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

Re: [PATCH v3 2/2] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

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.

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/2] ima: move APPRAISE_BOOTPARAM dependency on ARCH_POLICY to runtime

[Bruno Meneguele]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

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. 

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]