diff mbox series

[RFC,4/8] ima: Add digest_cache_measure and digest_cache_appraise boot-time policies

Message ID 20240214143525.2205481-5-roberto.sassu@huaweicloud.com (mailing list archive)
State Handled Elsewhere
Headers show
Series ima: Integrate with digest_cache LSM | expand

Commit Message

Roberto Sassu Feb. 14, 2024, 2:35 p.m. UTC 
From: Roberto Sassu <roberto.sassu@huawei.com>

Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
the kernel command line to add the following rule at the beginning of the
IMA policy, before other rules:

measure func=DIGEST_LIST_CHECK pcr=12

which will measure digest lists into PCR 12 (or the value in
CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).

'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
measure rules, if they have a compatible IMA hook. The PCR value still
comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.

Specify 'digest_cache_appraise' to add the following rule at the beginning,
before other rules:

appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig

which will appraise digest lists with IMA signatures or module-style
appended signatures.

'digest_cache_appraise' also adds 'digest_cache=content' to the other
appraise rules, if they have a compatible IMA hook.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 .../admin-guide/kernel-parameters.txt         | 15 ++++++-
 security/integrity/ima/Kconfig                | 10 +++++
 security/integrity/ima/ima_policy.c           | 45 +++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

Comments

Mimi Zohar March 7, 2024, 8:17 p.m. UTC | #1 
On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
> the kernel command line

The 'built-in' policies may be specified on the boot command line.  Please
update Subject line, to user the term "built-in" as well as here.

>  to add the following rule at the beginning of the
> IMA policy, before other rules:

Comments below...

> 
> measure func=DIGEST_LIST_CHECK pcr=12
> 
> which will measure digest lists into PCR 12 (or the value in
> CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).
> 
> 'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
> measure rules, if they have a compatible IMA hook. The PCR value still
> comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.
> 
> Specify 'digest_cache_appraise' to add the following rule at the beginning,
> before other rules:
> 
> appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
> 
> which will appraise digest lists with IMA signatures or module-style
> appended signatures.
> 
> 'digest_cache_appraise' also adds 'digest_cache=content' to the other
> appraise rules, if they have a compatible IMA hook.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  .../admin-guide/kernel-parameters.txt         | 15 ++++++-
>  security/integrity/ima/Kconfig                | 10 +++++
>  security/integrity/ima/ima_policy.c           | 45 +++++++++++++++++++
>  3 files changed, 69 insertions(+), 1 deletion(-)

[...]
 
> @@ -971,6 +1006,16 @@ void __init ima_init_policy(void)
>  {
>  	int build_appraise_entries, arch_entries;
>  
> +	/*
> +	 * We need to load digest cache rules at the beginning, to avoid dont_
> +	 * rules causing ours to not be reached.
> +	 */

"lockdown" trusts IMA to measure and appraise kernel modules, if the rule
exists.  Placing the digest_cache first breaks this trust.

From a trusted and secure boot perspective, the architecture specific policy
rules should not be ignored. Putting the digest_cache before any other rules
will limit others from being able to use digest_cache.

Instead of putting the digest_cache_{measure,appraise} built-in policies first,
skip loading the dont_measure_rules.


Mimi

> +	if (ima_digest_cache_measure)
> +		add_rules(&measure_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> +
> +	if (ima_digest_cache_appraise)
> +		add_rules(&appraise_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> +
>  	/* if !ima_policy, we load NO default rules */
>  	if (ima_policy)
>  		add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
Roberto Sassu March 8, 2024, 10:36 a.m. UTC | #2 
On Thu, 2024-03-07 at 15:17 -0500, Mimi Zohar wrote:
> On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
> > the kernel command line
> 
> The 'built-in' policies may be specified on the boot command line.  Please
> update Subject line, to user the term "built-in" as well as here.

Ok, will do.

> >  to add the following rule at the beginning of the
> > IMA policy, before other rules:
> 
> Comments below...
> 
> > 
> > measure func=DIGEST_LIST_CHECK pcr=12
> > 
> > which will measure digest lists into PCR 12 (or the value in
> > CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).
> > 
> > 'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
> > measure rules, if they have a compatible IMA hook. The PCR value still
> > comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.
> > 
> > Specify 'digest_cache_appraise' to add the following rule at the beginning,
> > before other rules:
> > 
> > appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
> > 
> > which will appraise digest lists with IMA signatures or module-style
> > appended signatures.
> > 
> > 'digest_cache_appraise' also adds 'digest_cache=content' to the other
> > appraise rules, if they have a compatible IMA hook.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  .../admin-guide/kernel-parameters.txt         | 15 ++++++-
> >  security/integrity/ima/Kconfig                | 10 +++++
> >  security/integrity/ima/ima_policy.c           | 45 +++++++++++++++++++
> >  3 files changed, 69 insertions(+), 1 deletion(-)
> 
> [...]
>  
> > @@ -971,6 +1006,16 @@ void __init ima_init_policy(void)
> >  {
> >  	int build_appraise_entries, arch_entries;
> >  
> > +	/*
> > +	 * We need to load digest cache rules at the beginning, to avoid dont_
> > +	 * rules causing ours to not be reached.
> > +	 */
> 
> "lockdown" trusts IMA to measure and appraise kernel modules, if the rule
> exists.  Placing the digest_cache first breaks this trust.

The new rules don't prevent other rules to be reached, since they are
'do' and not 'don_t' rules.

If the kernel reads a file with file ID READING_MODULE, that would
still be matched by rules with 'func=MODULE_CHECK', even if there are
rules with 'func=DIGEST_LIST_CHECK', which will be instead matched when
there is a kernel read with file ID READING_DIGEST_LIST.

We can talk about the rule modification. Speaking of appraising kernel
modules, setting 'ima_policy=digest_cache_appraise' in the kernel
command line would have the effect of changing:

appraise func=MODULE_CHECK appraise_type=imasig|modsig

to:

appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
appraise func=MODULE_CHECK appraise_type=imasig|modsig digest_cache=content

The effect of this would be that, if the kernel does not have
security.ima or an appended signature, appraisal will be still
successful by verifying the signature (in the xattr or appended) of the
digest list, and looking up the digest of the kernel module in that
digest list.

> From a trusted and secure boot perspective, the architecture specific policy
> rules should not be ignored.

I'm still missing how the architecture-specific policy would be
ignored.

> Putting the digest_cache before any other rules
> will limit others from being able to use digest_cache.

Sorry, didn't understand.

Let me just remark that measuring/appraising a digest list is a
necessary condition for using the digest cache built from that digest
list.

Not doing that has the same effect of a negative digest lookup, even if
that digest was in the digest list.

> Instead of putting the digest_cache_{measure,appraise} built-in policies first,
> skip loading the dont_measure_rules.

It does not seem a good idea. We still want to avoid
measurements/appraisal in the pseudo filesystems.

Roberto

> Mimi
> 
> > +	if (ima_digest_cache_measure)
> > +		add_rules(&measure_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> > +
> > +	if (ima_digest_cache_appraise)
> > +		add_rules(&appraise_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
> > +
> >  	/* if !ima_policy, we load NO default rules */
> >  	if (ima_policy)
> >  		add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),
Mimi Zohar March 8, 2024, 2:23 p.m. UTC | #3 
> > > @@ -971,6 +1006,16 @@ void __init ima_init_policy(void)
> > >  {
> > >  	int build_appraise_entries, arch_entries;
> > >  
> > > +	/*
> > > +	 * We need to load digest cache rules at the beginning, to avoid dont_
> > > +	 * rules causing ours to not be reached.
> > > +	 */
> > 
> > "lockdown" trusts IMA to measure and appraise kernel modules, if the rule
> > exists.  Placing the digest_cache first breaks this trust.
> 
> The new rules don't prevent other rules to be reached, since they are
> 'do' and not 'don_t' rules.

My mistake.  These are just the rules for measuring or appraising the digest
cache lists themselves, not the actual policy rules for using the digest_cache. 
This should be fine.

Perhaps update the comment to reflect initramfs usage.


thanks,

Mimi
Mimi Zohar March 11, 2024, 1:01 p.m. UTC | #4 
On Wed, 2024-02-14 at 15:35 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Specify the 'digest_cache_measure' boot-time policy with 'ima_policy=' in
> the kernel command line to add the following rule at the beginning of the
> IMA policy, before other rules:
> 
> measure func=DIGEST_LIST_CHECK pcr=12
> 
> which will measure digest lists into PCR 12 (or the value in
> CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX).
> 
> 'digest_cache_measure' also adds 'digest_cache=content pcr=12' to the other
> measure rules, if they have a compatible IMA hook. The PCR value still
> comes from CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX.
> 
> Specify 'digest_cache_appraise' to add the following rule at the beginning,
> before other rules:
> 
> appraise func=DIGEST_LIST_CHECK appraise_type=imasig|modsig
> 
> which will appraise digest lists with IMA signatures or module-style
> appended signatures.
> 
> 'digest_cache_appraise' also adds 'digest_cache=content' to the other
> appraise rules, if they have a compatible IMA hook.

Defining two new built-in policies - digest_cache_measure, digest_cache_appraise
- in a single patch would be acceptable, if there wasn't anything else going on.

Changing other policy rules should not be made in this patch.  A clear
explanation as to why other policy rules need to be modified is needed.  It
shouldn't be hidden here.

thanks,

Mimi

> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 31b3a25680d0..a79967fcba7d 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2011,7 +2011,8 @@ 
 	ima_policy=	[IMA]
 			The builtin policies to load during IMA setup.
 			Format: "tcb | appraise_tcb | secure_boot |
-				 fail_securely | critical_data"
+				 fail_securely | critical_data |
+				 digest_cache_measure | digest_cache_appraise"
 
 			The "tcb" policy measures all programs exec'd, files
 			mmap'd for exec, and all files opened with the read
@@ -2033,6 +2034,18 @@ 
 			The "critical_data" policy measures kernel integrity
 			critical data.
 
+			The "digest_cache_measure" policy measures digest lists
+			into PCR 12 (can be changed with kernel config), enables
+			the digest cache to be used for the other selected
+			measure rules (if compatible), and measures the files
+			with digest not found in the digest list into PCR 12
+			(changeable).
+
+			The "digest_cache_appraise" policy appraises digest
+			lists with IMA signatures or module-style appended
+			signatures, and enables the digest cache to be used for
+			the other selected appraise rules (if compatible).
+
 	ima_tcb		[IMA] Deprecated.  Use ima_policy= instead.
 			Load a policy which meets the needs of the Trusted
 			Computing Base.  This means IMA will measure all
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 475c32615006..6a481019fb6e 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -321,4 +321,14 @@  config IMA_DISABLE_HTABLE
 	help
 	   This option disables htable to allow measurement of duplicate records.
 
+config IMA_DIGEST_CACHE_MEASURE_PCR_IDX
+	int
+	range 8 14
+	default 12
+	help
+	  This option determines the TPM PCR register index that IMA uses to
+	  maintain the integrity aggregate of the measurement list, when the
+	  digest_cache LSM is used (different measurement style).  If unsure,
+	  use the default 12.
+
 endif
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 4ac83df8d255..04127f962ef4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -254,6 +254,21 @@  static struct ima_rule_entry critical_data_rules[] __ro_after_init = {
 	{.action = MEASURE, .func = CRITICAL_DATA, .flags = IMA_FUNC},
 };
 
+static struct ima_rule_entry measure_digest_cache_rule __ro_after_init = {
+#ifdef CONFIG_SECURITY_DIGEST_CACHE
+	.action = MEASURE, .func = DIGEST_LIST_CHECK,
+	.pcr = CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX,
+	.flags = IMA_FUNC | IMA_PCR
+#endif
+};
+
+static struct ima_rule_entry appraise_digest_cache_rule __ro_after_init = {
+#ifdef CONFIG_SECURITY_DIGEST_CACHE
+	.action = APPRAISE, .func = DIGEST_LIST_CHECK,
+	.flags = IMA_FUNC | IMA_DIGSIG_REQUIRED | IMA_MODSIG_ALLOWED,
+#endif
+};
+
 /* An array of architecture specific rules */
 static struct ima_rule_entry *arch_policy_entry __ro_after_init;
 
@@ -278,6 +293,8 @@  static bool ima_use_appraise_tcb __initdata;
 static bool ima_use_secure_boot __initdata;
 static bool ima_use_critical_data __initdata;
 static bool ima_fail_unverifiable_sigs __ro_after_init;
+static bool ima_digest_cache_measure __ro_after_init;
+static bool ima_digest_cache_appraise __ro_after_init;
 static int __init policy_setup(char *str)
 {
 	char *p;
@@ -295,6 +312,10 @@  static int __init policy_setup(char *str)
 			ima_use_critical_data = true;
 		else if (strcmp(p, "fail_securely") == 0)
 			ima_fail_unverifiable_sigs = true;
+		else if (strcmp(p, "digest_cache_measure") == 0)
+			ima_digest_cache_measure = true;
+		else if (strcmp(p, "digest_cache_appraise") == 0)
+			ima_digest_cache_appraise = true;
 		else
 			pr_err("policy \"%s\" not found", p);
 	}
@@ -897,6 +918,20 @@  static void add_rules(struct ima_rule_entry *entries, int count,
 	for (i = 0; i < count; i++) {
 		struct ima_rule_entry *entry;
 
+		if (IS_ENABLED(CONFIG_SECURITY_DIGEST_CACHE) &&
+		    entries[i].action == MEASURE && ima_digest_cache_measure &&
+		    ima_digest_cache_func_allowed(&entries[i])) {
+			entries[i].digest_cache_mask |= IMA_DIGEST_CACHE_MEASURE_CONTENT;
+			entries[i].pcr = CONFIG_IMA_DIGEST_CACHE_MEASURE_PCR_IDX;
+			entries[i].flags |= IMA_PCR;
+		}
+
+		if (IS_ENABLED(CONFIG_SECURITY_DIGEST_CACHE) &&
+		    entries[i].action == APPRAISE &&
+		    ima_digest_cache_appraise &&
+		    ima_digest_cache_func_allowed(&entries[i]))
+			entries[i].digest_cache_mask |= IMA_DIGEST_CACHE_APPRAISE_CONTENT;
+
 		if (policy_rule & IMA_DEFAULT_POLICY)
 			list_add_tail(&entries[i].list, &ima_default_rules);
 
@@ -971,6 +1006,16 @@  void __init ima_init_policy(void)
 {
 	int build_appraise_entries, arch_entries;
 
+	/*
+	 * We need to load digest cache rules at the beginning, to avoid dont_
+	 * rules causing ours to not be reached.
+	 */
+	if (ima_digest_cache_measure)
+		add_rules(&measure_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
+
+	if (ima_digest_cache_appraise)
+		add_rules(&appraise_digest_cache_rule, 1, IMA_DEFAULT_POLICY);
+
 	/* if !ima_policy, we load NO default rules */
 	if (ima_policy)
 		add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules),