diff mbox series

[v5,6/7] IMA: add critical_data to the built-in policy rules

Message ID 20201101222626.6111-7-tusharsu@linux.microsoft.com (mailing list archive)
State Superseded
Delegated to: Paul Moore
Headers show
Series IMA: Infrastructure for measurement of critical kernel data | expand

Commit Message

Tushar Sugandhi Nov. 1, 2020, 10:26 p.m. UTC 
From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

The IMA hook to measure kernel critical data, namely
ima_measure_critical_data(), could be called before a custom IMA policy
is loaded. For example, SELinux calls ima_measure_critical_data() to
measure its state and policy when they are initialized. This occurs
before a custom IMA policy is loaded, and hence IMA hook will not
measure the data. A built-in policy is therefore needed to measure
critical data provided by callers before a custom IMA policy is loaded.

Add CRITICAL_DATA to built-in IMA rules if the kernel command line
contains "ima_policy=critical_data". Set the IMA template for this rule
to "ima-buf" since ima_measure_critical_data() measures a buffer.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_policy.c | 32 +++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Comments

Mimi Zohar Nov. 6, 2020, 3:24 p.m. UTC | #1 
Hi Lakshmi, Tushar,

This patch defines a new critical_data builtin policy.  Please update
the Subject line.

On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote:
> From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> 
> The IMA hook to measure kernel critical data, namely
> ima_measure_critical_data(), could be called before a custom IMA policy
> is loaded. For example, SELinux calls ima_measure_critical_data() to
> measure its state and policy when they are initialized. This occurs
> before a custom IMA policy is loaded, and hence IMA hook will not
> measure the data. A built-in policy is therefore needed to measure
> critical data provided by callers before a custom IMA policy is loaded.

^Define a new critical data builtin policy to allow measuring early
kernel integrity critical data before a custom IMA policy is loaded.

Either remove the references to SELinux or move this patch after the
subsequent patch which measures SELinux critical data.

> 
> Add CRITICAL_DATA to built-in IMA rules if the kernel command line
> contains "ima_policy=critical_data". Set the IMA template for this rule
> to "ima-buf" since ima_measure_critical_data() measures a buffer.
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

> ---
>  security/integrity/ima/ima_policy.c | 32 +++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index ec99e0bb6c6f..dc8fe969d3fe 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c

> @@ -875,6 +884,29 @@ void __init ima_init_policy(void)
>  			  ARRAY_SIZE(default_appraise_rules),
>  			  IMA_DEFAULT_POLICY);
>  
> +	if (ima_use_critical_data) {
> +		template = lookup_template_desc("ima-buf");
> +		if (!template) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +
> +		ret = template_desc_init_fields(template->fmt,
> +						&(template->fields),
> +						&(template->num_fields));

The default IMA template when measuring buffer data is "ima_buf".   Is
there a reason for allocating and initializing it here and not
deferring it until process_buffer_measurement()?

thanks,

Mimi

> +		if (ret)
> +			goto out;
> +
> +		critical_data_rules[0].template = template;
> +		add_rules(critical_data_rules,
> +			  ARRAY_SIZE(critical_data_rules),
> +			  IMA_DEFAULT_POLICY);
> +	}
> +
> +out:
> +	if (ret)
> +		pr_err("%s failed, result: %d\n", __func__, ret);
> +
>  	ima_update_policy_flag();
>  }
>
Lakshmi Ramasubramanian Nov. 6, 2020, 3:37 p.m. UTC | #2 
On 11/6/20 7:24 AM, Mimi Zohar wrote:

Hi Mimi,

Thanks for reviewing the patches.

> Hi Lakshmi, Tushar,
> 
> This patch defines a new critical_data builtin policy.  Please update
> the Subject line.
> 
> On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote:
>> From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>>
>> The IMA hook to measure kernel critical data, namely
>> ima_measure_critical_data(), could be called before a custom IMA policy
>> is loaded. For example, SELinux calls ima_measure_critical_data() to
>> measure its state and policy when they are initialized. This occurs
>> before a custom IMA policy is loaded, and hence IMA hook will not
>> measure the data. A built-in policy is therefore needed to measure
>> critical data provided by callers before a custom IMA policy is loaded.
> 
> ^Define a new critical data builtin policy to allow measuring early
> kernel integrity critical data before a custom IMA policy is loaded.

I will add the above line in the patch description.

> 
> Either remove the references to SELinux or move this patch after the
> subsequent patch which measures SELinux critical data.

I will remove the reference to SELinux.
I think it would be better to have this patch before the SELinux 
measurement patch.

> 
>>
>> Add CRITICAL_DATA to built-in IMA rules if the kernel command line
>> contains "ima_policy=critical_data". Set the IMA template for this rule
>> to "ima-buf" since ima_measure_critical_data() measures a buffer.
>>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> 
>> ---
>>   security/integrity/ima/ima_policy.c | 32 +++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index ec99e0bb6c6f..dc8fe969d3fe 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
> 
>> @@ -875,6 +884,29 @@ void __init ima_init_policy(void)
>>   			  ARRAY_SIZE(default_appraise_rules),
>>   			  IMA_DEFAULT_POLICY);
>>   
>> +	if (ima_use_critical_data) {
>> +		template = lookup_template_desc("ima-buf");
>> +		if (!template) {
>> +			ret = -EINVAL;
>> +			goto out;
>> +		}
>> +
>> +		ret = template_desc_init_fields(template->fmt,
>> +						&(template->fields),
>> +						&(template->num_fields));
> 
> The default IMA template when measuring buffer data is "ima_buf".   Is
> there a reason for allocating and initializing it here and not
> deferring it until process_buffer_measurement()?
> 

You are right - good catch.
I will remove the above and validate.

thanks,
  -lakshmi

> 
>> +		if (ret)
>> +			goto out;
>> +
>> +		critical_data_rules[0].template = template;
>> +		add_rules(critical_data_rules,
>> +			  ARRAY_SIZE(critical_data_rules),
>> +			  IMA_DEFAULT_POLICY);
>> +	}
>> +
>> +out:
>> +	if (ret)
>> +		pr_err("%s failed, result: %d\n", __func__, ret);
>> +
>>   	ima_update_policy_flag();
>>   }
>>   
>
Lakshmi Ramasubramanian Nov. 6, 2020, 11:51 p.m. UTC | #3 
On 11/6/20 7:37 AM, Lakshmi Ramasubramanian wrote:

Hi Mimi,

> 
>> Hi Lakshmi, Tushar,
>>
>> This patch defines a new critical_data builtin policy.  Please update
>> the Subject line.
>>
>> On Sun, 2020-11-01 at 14:26 -0800, Tushar Sugandhi wrote:
>>> From: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>>>
>>> The IMA hook to measure kernel critical data, namely
>>> ima_measure_critical_data(), could be called before a custom IMA policy
>>> is loaded. For example, SELinux calls ima_measure_critical_data() to
>>> measure its state and policy when they are initialized. This occurs
>>> before a custom IMA policy is loaded, and hence IMA hook will not
>>> measure the data. A built-in policy is therefore needed to measure
>>> critical data provided by callers before a custom IMA policy is loaded.
>>
>> ^Define a new critical data builtin policy to allow measuring early
>> kernel integrity critical data before a custom IMA policy is loaded.
> 
> I will add the above line in the patch description.
> 
>>
>> Either remove the references to SELinux or move this patch after the
>> subsequent patch which measures SELinux critical data.
> 
> I will remove the reference to SELinux.
> I think it would be better to have this patch before the SELinux 
> measurement patch.
> 
>>
>>>
>>> Add CRITICAL_DATA to built-in IMA rules if the kernel command line
>>> contains "ima_policy=critical_data". Set the IMA template for this rule
>>> to "ima-buf" since ima_measure_critical_data() measures a buffer.
>>>
>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>>
>>> ---
>>>   security/integrity/ima/ima_policy.c | 32 +++++++++++++++++++++++++++++
>>>   1 file changed, 32 insertions(+)
>>>
>>> diff --git a/security/integrity/ima/ima_policy.c 
>>> b/security/integrity/ima/ima_policy.c
>>> index ec99e0bb6c6f..dc8fe969d3fe 100644
>>> --- a/security/integrity/ima/ima_policy.c
>>> +++ b/security/integrity/ima/ima_policy.c
>>
>>> @@ -875,6 +884,29 @@ void __init ima_init_policy(void)
>>>                 ARRAY_SIZE(default_appraise_rules),
>>>                 IMA_DEFAULT_POLICY);
>>> +    if (ima_use_critical_data) {
>>> +        template = lookup_template_desc("ima-buf");
>>> +        if (!template) {
>>> +            ret = -EINVAL;
>>> +            goto out;
>>> +        }
>>> +
>>> +        ret = template_desc_init_fields(template->fmt,
>>> +                        &(template->fields),
>>> +                        &(template->num_fields));
>>
>> The default IMA template when measuring buffer data is "ima_buf".   Is
>> there a reason for allocating and initializing it here and not
>> deferring it until process_buffer_measurement()?
>>
> 
> You are right - good catch.
> I will remove the above and validate.
> 

process_buffer_measurement() allocates and initializes "ima-buf" 
template only when the parameter "func" is NONE. Currently, only 
ima_check_blacklist() passes NONE for func when calling 
process_buffer_measurement().

If "func" is anything other than NONE, ima_match_policy() picks
the default IMA template if the IMA policy rule does not specify a template.

We need to add "ima-buf" in the built-in policy for critical_data so 
that the default template is not used for buffer measurement.

Please let me know if I am missing something.

thanks,
  -lakshmi

>>
>>> +        if (ret)
>>> +            goto out;
>>> +
>>> +        critical_data_rules[0].template = template;
>>> +        add_rules(critical_data_rules,
>>> +              ARRAY_SIZE(critical_data_rules),
>>> +              IMA_DEFAULT_POLICY);
>>> +    }
>>> +
>>> +out:
>>> +    if (ret)
>>> +        pr_err("%s failed, result: %d\n", __func__, ret);
>>> +
>>>       ima_update_policy_flag();
>>>   }
>>
>
Mimi Zohar Nov. 8, 2020, 3:46 p.m. UTC | #4 
Hi Lakshmi,

On Fri, 2020-11-06 at 15:51 -0800, Lakshmi Ramasubramanian wrote:
> 
> >>> diff --git a/security/integrity/ima/ima_policy.c 
> >>> b/security/integrity/ima/ima_policy.c
> >>> index ec99e0bb6c6f..dc8fe969d3fe 100644
> >>> --- a/security/integrity/ima/ima_policy.c
> >>> +++ b/security/integrity/ima/ima_policy.c
> >>
> >>> @@ -875,6 +884,29 @@ void __init ima_init_policy(void)
> >>>                 ARRAY_SIZE(default_appraise_rules),
> >>>                 IMA_DEFAULT_POLICY);
> >>> +    if (ima_use_critical_data) {
> >>> +        template = lookup_template_desc("ima-buf");
> >>> +        if (!template) {
> >>> +            ret = -EINVAL;
> >>> +            goto out;
> >>> +        }
> >>> +
> >>> +        ret = template_desc_init_fields(template->fmt,
> >>> +                        &(template->fields),
> >>> +                        &(template->num_fields));
> >>
> >> The default IMA template when measuring buffer data is "ima_buf".   Is
> >> there a reason for allocating and initializing it here and not
> >> deferring it until process_buffer_measurement()?
> >>
> > 
> > You are right - good catch.
> > I will remove the above and validate.
> > 
> 
> process_buffer_measurement() allocates and initializes "ima-buf" 
> template only when the parameter "func" is NONE. Currently, only 
> ima_check_blacklist() passes NONE for func when calling 
> process_buffer_measurement().
> 
> If "func" is anything other than NONE, ima_match_policy() picks
> the default IMA template if the IMA policy rule does not specify a template.
> 
> We need to add "ima-buf" in the built-in policy for critical_data so 
> that the default template is not used for buffer measurement.
> 
> Please let me know if I am missing something.
> 

Let's explain a bit further what is happening and why.   As you said
ima_get_action() returns the template format, which may be the default
IMA template or the specific IMA policy rule template format.  This
works properly for both the arch specific and custom policies, but not
for builtin policies, because the policy rules may contain a rule
specific .template field.   When the rules don't contain a rule
specific template field, they default to the IMA default template.  In
the case of builtin policies, the policy rules cannot contain the
.template field.

The default template field for process_buffer_measurement() should
always be "ima-buf", not the default IMA template format.   Let's fix
this prior to this patch.

Probably something like this:
- In addition to initializing the default IMA template, initialize the
"ima-buf" template.  Maybe something similiar to
ima_template_desc_current().
- Set the default in process_buffer_measurement() to "ima-buf", before
calling ima_get_action().
- modify ima_match_policy() so that the default policy isn't reset when
already specified.

thanks,

Mimi


> >>
> >>> +        if (ret)
> >>> +            goto out;
> >>> +
> >>> +        critical_data_rules[0].template = template;
> >>> +        add_rules(critical_data_rules,
> >>> +              ARRAY_SIZE(critical_data_rules),
> >>> +              IMA_DEFAULT_POLICY);
> >>> +    }
> >>> +
> >>> +out:
> >>> +    if (ret)
> >>> +        pr_err("%s failed, result: %d\n", __func__, ret);
> >>> +
> >>>       ima_update_policy_flag();
> >>>   }
> >>
> > 
>
Lakshmi Ramasubramanian Nov. 9, 2020, 5:24 p.m. UTC | #5 
On 11/8/20 7:46 AM, Mimi Zohar wrote:
> Hi Lakshmi,
> 
> On Fri, 2020-11-06 at 15:51 -0800, Lakshmi Ramasubramanian wrote:
>>
>>>>> diff --git a/security/integrity/ima/ima_policy.c
>>>>> b/security/integrity/ima/ima_policy.c
>>>>> index ec99e0bb6c6f..dc8fe969d3fe 100644
>>>>> --- a/security/integrity/ima/ima_policy.c
>>>>> +++ b/security/integrity/ima/ima_policy.c
>>>>
>>>>> @@ -875,6 +884,29 @@ void __init ima_init_policy(void)
>>>>>                  ARRAY_SIZE(default_appraise_rules),
>>>>>                  IMA_DEFAULT_POLICY);
>>>>> +    if (ima_use_critical_data) {
>>>>> +        template = lookup_template_desc("ima-buf");
>>>>> +        if (!template) {
>>>>> +            ret = -EINVAL;
>>>>> +            goto out;
>>>>> +        }
>>>>> +
>>>>> +        ret = template_desc_init_fields(template->fmt,
>>>>> +                        &(template->fields),
>>>>> +                        &(template->num_fields));
>>>>
>>>> The default IMA template when measuring buffer data is "ima_buf".   Is
>>>> there a reason for allocating and initializing it here and not
>>>> deferring it until process_buffer_measurement()?
>>>>
>>>
>>> You are right - good catch.
>>> I will remove the above and validate.
>>>
>>
>> process_buffer_measurement() allocates and initializes "ima-buf"
>> template only when the parameter "func" is NONE. Currently, only
>> ima_check_blacklist() passes NONE for func when calling
>> process_buffer_measurement().
>>
>> If "func" is anything other than NONE, ima_match_policy() picks
>> the default IMA template if the IMA policy rule does not specify a template.
>>
>> We need to add "ima-buf" in the built-in policy for critical_data so
>> that the default template is not used for buffer measurement.
>>
>> Please let me know if I am missing something.
>>
> 
> Let's explain a bit further what is happening and why.   As you said
> ima_get_action() returns the template format, which may be the default
> IMA template or the specific IMA policy rule template format.  This
> works properly for both the arch specific and custom policies, but not
> for builtin policies, because the policy rules may contain a rule
> specific .template field.   When the rules don't contain a rule
> specific template field, they default to the IMA default template.  In
> the case of builtin policies, the policy rules cannot contain the
> .template field.
> 
> The default template field for process_buffer_measurement() should
> always be "ima-buf", not the default IMA template format.   Let's fix
> this prior to this patch.
> 
> Probably something like this:
> - In addition to initializing the default IMA template, initialize the
> "ima-buf" template.  Maybe something similiar to
> ima_template_desc_current().
> - Set the default in process_buffer_measurement() to "ima-buf", before
> calling ima_get_action().
> - modify ima_match_policy() so that the default policy isn't reset when
> already specified.
> 

Sure Mimi - I will try this out and update.

thanks,
  -lakshmi

> 
> 
>>>>
>>>>> +        if (ret)
>>>>> +            goto out;
>>>>> +
>>>>> +        critical_data_rules[0].template = template;
>>>>> +        add_rules(critical_data_rules,
>>>>> +              ARRAY_SIZE(critical_data_rules),
>>>>> +              IMA_DEFAULT_POLICY);
>>>>> +    }
>>>>> +
>>>>> +out:
>>>>> +    if (ret)
>>>>> +        pr_err("%s failed, result: %d\n", __func__, ret);
>>>>> +
>>>>>        ima_update_policy_flag();
>>>>>    }
>>>>
>>>
>>
diff mbox series

Patch

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ec99e0bb6c6f..dc8fe969d3fe 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -206,6 +206,10 @@  static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
 	 .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
 };
 
+static struct ima_rule_entry critical_data_rules[] __ro_after_init = {
+	{.action = MEASURE, .func = CRITICAL_DATA, .flags = IMA_FUNC},
+};
+
 /* An array of architecture specific rules */
 static struct ima_rule_entry *arch_policy_entry __ro_after_init;
 
@@ -228,6 +232,7 @@  __setup("ima_tcb", default_measure_policy_setup);
 
 static bool ima_use_appraise_tcb __initdata;
 static bool ima_use_secure_boot __initdata;
+static bool ima_use_critical_data __ro_after_init;
 static bool ima_fail_unverifiable_sigs __ro_after_init;
 static int __init policy_setup(char *str)
 {
@@ -242,6 +247,8 @@  static int __init policy_setup(char *str)
 			ima_use_appraise_tcb = true;
 		else if (strcmp(p, "secure_boot") == 0)
 			ima_use_secure_boot = true;
+		else if (strcmp(p, "critical_data") == 0)
+			ima_use_critical_data = true;
 		else if (strcmp(p, "fail_securely") == 0)
 			ima_fail_unverifiable_sigs = true;
 		else
@@ -813,6 +820,8 @@  static int __init ima_init_arch_policy(void)
 void __init ima_init_policy(void)
 {
 	int build_appraise_entries, arch_entries;
+	struct ima_template_desc *template = NULL;
+	int ret = 0;
 
 	/* if !ima_policy, we load NO default rules */
 	if (ima_policy)
@@ -875,6 +884,29 @@  void __init ima_init_policy(void)
 			  ARRAY_SIZE(default_appraise_rules),
 			  IMA_DEFAULT_POLICY);
 
+	if (ima_use_critical_data) {
+		template = lookup_template_desc("ima-buf");
+		if (!template) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		ret = template_desc_init_fields(template->fmt,
+						&(template->fields),
+						&(template->num_fields));
+		if (ret)
+			goto out;
+
+		critical_data_rules[0].template = template;
+		add_rules(critical_data_rules,
+			  ARRAY_SIZE(critical_data_rules),
+			  IMA_DEFAULT_POLICY);
+	}
+
+out:
+	if (ret)
+		pr_err("%s failed, result: %d\n", __func__, ret);
+
 	ima_update_policy_flag();
 }