[v4,01/10] IMA: Defined an IMA hook to measure keys on key create or update
diff mbox series

Message ID 20191106190116.2578-2-nramas@linux.microsoft.com
State New
Headers show
Series
  • KEYS: Measure keys when they are created or updated
Related show

Commit Message

Lakshmi Ramasubramanian Nov. 6, 2019, 7:01 p.m. UTC
Asymmetric keys used for verifying file signatures or certificates
are currently not included in the IMA measurement list.

This patch defines a new IMA hook namely ima_post_key_create_or_update()
to measure asymmetric keys.

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

Comments

Mimi Zohar Nov. 6, 2019, 10:43 p.m. UTC | #1
On Wed, 2019-11-06 at 11:01 -0800, Lakshmi Ramasubramanian wrote:
> Asymmetric keys used for verifying file signatures or certificates
> are currently not included in the IMA measurement list.
> 
> This patch defines a new IMA hook namely ima_post_key_create_or_update()
> to measure asymmetric keys.
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  security/integrity/ima/ima_main.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index d7e987baf127..a0e233afe876 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -721,6 +721,22 @@ void ima_kexec_cmdline(const void *buf, int size)
>  					   KEXEC_CMDLINE, 0);
>  }
>  
> +/**
> + * ima_post_key_create_or_update - measure asymmetric keys
> + * @keyring: keyring to which the key is linked to
> + * @key: created or updated key
> + * @flags: key flags
> + * @create: flag indicating whether the key was created or updated
> + *
> + * Keys can only be measured, not appraised.
> + */
> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> +				   unsigned long flags, bool create)
> +{
> +	if ((keyring != NULL) && (key != NULL))
> +		return;

I would move the patch that defines the "keyring=" policy option prior
to this one.  Include the call to process_buffer_measurement() in this
patch.  A subsequent patch would add support to defer measuring the
key, by calling a function named something like
ima_queue_key_measurement().

Mimi

> +}
> +
>  static int __init init_ima(void)
>  {
>  	int error;
Lakshmi Ramasubramanian Nov. 7, 2019, 12:21 a.m. UTC | #2
On 11/6/2019 2:43 PM, Mimi Zohar wrote:

>> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>> +				   unsigned long flags, bool create)
>> +{
>> +	if ((keyring != NULL) && (key != NULL))
>> +		return;
> 
> I would move the patch that defines the "keyring=" policy option prior
> to this one.  Include the call to process_buffer_measurement() in this
> patch.  A subsequent patch would add support to defer measuring the
> key, by calling a function named something like
> ima_queue_key_measurement().
> 
> Mimi

As I'd stated in the other response, I wanted to isolate all key related 
code in a separate C file and build it if and only if all CONFIG 
dependencies are met.

I can do the following:

=> Define the IMA hook in ima_asymmetric_keys.c instead of ima_main.c

=> In include/linux/ima.h declare the IMA hook if 
CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled.
Else, NOP it.

#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
extern void ima_post_key_create_or_update(struct key *keyring,
					  struct key *key,
					  unsigned long flags,
                                           bool create);
#else
static inline void ima_post_key_create_or_update(struct key *keyring,
						 struct key *key,
						 unsigned long flags,
						 bool create) {}
#endif

Would that be acceptable?

thanks,
  -lakshmi
Mimi Zohar Nov. 7, 2019, 3:40 a.m. UTC | #3
On Wed, 2019-11-06 at 16:21 -0800, Lakshmi Ramasubramanian wrote:
> On 11/6/2019 2:43 PM, Mimi Zohar wrote:
> 
> >> +void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> >> +				   unsigned long flags, bool create)
> >> +{
> >> +	if ((keyring != NULL) && (key != NULL))
> >> +		return;
> > 
> > I would move the patch that defines the "keyring=" policy option prior
> > to this one.  Include the call to process_buffer_measurement() in this
> > patch.  A subsequent patch would add support to defer measuring the
> > key, by calling a function named something like
> > ima_queue_key_measurement().
> > 
> > Mimi
> 
> As I'd stated in the other response, I wanted to isolate all key related 
> code in a separate C file and build it if and only if all CONFIG 
> dependencies are met.

The basic measuring of keys shouldn't be any different than any other
policy rule, other than it is a key and not a file.  This is the
reason that I keep saying start out with the basics and then add
support to defer measuring keys on the trusted keyrings.

Only the queueing code needed for measuring keys on the trusted
keyrings would be in a separate file.

Mimi
Lakshmi Ramasubramanian Nov. 7, 2019, 6:42 p.m. UTC | #4
On 11/6/2019 7:40 PM, Mimi Zohar wrote:

>>> I would move the patch that defines the "keyring=" policy option prior
>>> to this one.  Include the call to process_buffer_measurement() in this
>>> patch.  A subsequent patch would add support to defer measuring the
>>> key, by calling a function named something like
>>> ima_queue_key_measurement().
>>>
>>> Mimi
>>
>> As I'd stated in the other response, I wanted to isolate all key related
>> code in a separate C file and build it if and only if all CONFIG
>> dependencies are met.
> 
> The basic measuring of keys shouldn't be any different than any other
> policy rule, other than it is a key and not a file.  This is the
> reason that I keep saying start out with the basics and then add
> support to defer measuring keys on the trusted keyrings.

I'll make the changes, rearrange the patches and send an updated set.

I do have a few questions since I am still not fully understanding the 
requirements you are targeting. Appreciate if you could please clarify.

As you already know, I am using the "public key" of the given asymmetric 
key as the "buffer" to measure in process_buffer_measurement().

The measurement decision is not based on whether the keyring is a 
trusted one or an untrusted one. As long as the IMA policy allows 
(through the "keyrings=" option) the key will be measured.

Do you want only trusted keyrings to be allowed in the measurement?
In my opinion, that decision should be deferred to whoever is setting up 
the IMA policy.

> Only the queueing code needed for measuring keys on the trusted
> keyrings would be in a separate file.
> 
> Mimi

The decision to process key immediately or defer (queue) is based on 
whether IMA has been initialized or not. Keyring is not used for this 
decision.

Could you please clarify how queuing is related to keyring's 
trustworthiness?

The check for whether the key is an asymmetric one or not, and 
extracting the "public key" if it is an asymmetric key needs to be in a 
separate file to handle the CONFIG dependencies in IMA.

thanks,
  -lakshmi
Mimi Zohar Nov. 7, 2019, 8:53 p.m. UTC | #5
On Thu, 2019-11-07 at 10:42 -0800, Lakshmi Ramasubramanian wrote:
> On 11/6/2019 7:40 PM, Mimi Zohar wrote:
> 
> >>> I would move the patch that defines the "keyring=" policy option prior
> >>> to this one.  Include the call to process_buffer_measurement() in this
> >>> patch.  A subsequent patch would add support to defer measuring the
> >>> key, by calling a function named something like
> >>> ima_queue_key_measurement().
> >>>
> >>
> >> As I'd stated in the other response, I wanted to isolate all key related
> >> code in a separate C file and build it if and only if all CONFIG
> >> dependencies are met.
> > 
> > The basic measuring of keys shouldn't be any different than any other
> > policy rule, other than it is a key and not a file.  This is the
> > reason that I keep saying start out with the basics and then add
> > support to defer measuring keys on the trusted keyrings.
> 
> I'll make the changes, rearrange the patches and send an updated set.
> 
> I do have a few questions since I am still not fully understanding the 
> requirements you are targeting. Appreciate if you could please clarify.
> 
> As you already know, I am using the "public key" of the given asymmetric 
> key as the "buffer" to measure in process_buffer_measurement().
> 
> The measurement decision is not based on whether the keyring is a 
> trusted one or an untrusted one. As long as the IMA policy allows 
> (through the "keyrings=" option) the key will be measured.

We should be able to measure all keys being loaded onto any keyring or
onto a specific "keyring=".   This shouldn't be any different than any
other policy rule.  Once you have this basic feature working, you
would address loading keys during early boot.

> 
> Do you want only trusted keyrings to be allowed in the measurement?
> In my opinion, that decision should be deferred to whoever is setting up 
> the IMA policy.

Right, but it shouldn't be limited to just "trusted" keyrings.  This
way you can first test loading keys onto any keyring.

> 
> > Only the queueing code needed for measuring keys on the trusted
> > keyrings would be in a separate file.
> > 
> 
> The decision to process key immediately or defer (queue) is based on 
> whether IMA has been initialized or not. Keyring is not used for this 
> decision.
> 
> Could you please clarify how queuing is related to keyring's 
> trustworthiness?
> 
> The check for whether the key is an asymmetric one or not, and 
> extracting the "public key" if it is an asymmetric key needs to be in a 
> separate file to handle the CONFIG dependencies in IMA.

Queuing the keys should be independent of measuring the keys.
 Initially you would start with just measuring the key.  From a high
level it would look like:

    ima_post_key_create_or_update(...)
    {
       "measure key based on
    policy(key, keyring, ...)"
    }

This requires the IMA "keyring=" policy option support be defined
first.

Subsequently you would add key queuing support, and then update
ima_post_key_create_or_update().  It would look like:

        ima_post_key_create_or_update(...)
        {
            if (custom policy is loaded)
               "measure key based on policy(key, keyring, ...)"
            else
                "queue key(key, keyring)"
        }

Mimi
Lakshmi Ramasubramanian Nov. 7, 2019, 9:12 p.m. UTC | #6
On 11/7/19 12:53 PM, Mimi Zohar wrote:

>>
>> The measurement decision is not based on whether the keyring is a
>> trusted one or an untrusted one. As long as the IMA policy allows
>> (through the "keyrings=" option) the key will be measured.
> 
> We should be able to measure all keys being loaded onto any keyring or
> onto a specific "keyring=".   This shouldn't be any different than any
> other policy rule.  Once you have this basic feature working, you
> would address loading keys during early boot.
Perfect - that's exactly how I have implemented it right now. Will 
continue to test it.

>> Do you want only trusted keyrings to be allowed in the measurement?
>> In my opinion, that decision should be deferred to whoever is setting up
>> the IMA policy.
> 
> Right, but it shouldn't be limited to just "trusted" keyrings.  This
> way you can first test loading keys onto any keyring.
Thank you.

> Queuing the keys should be independent of measuring the keys.
>   Initially you would start with just measuring the key.  From a high
> level it would look like:
> 
>      ima_post_key_create_or_update(...)
>      {
>         "measure key based on
>      policy(key, keyring, ...)"
>      }
> 
> This requires the IMA "keyring=" policy option support be defined
> first.
> 
> Subsequently you would add key queuing support, and then update
> ima_post_key_create_or_update().  It would look like:
> 
>          ima_post_key_create_or_update(...)
>          {
>              if (custom policy is loaded)
>                 "measure key based on policy(key, keyring, ...)"
>              else
>                  "queue key(key, keyring)"
>          }
> 
> Mimi

Yes - I have the above change working. Will continue testing.

thanks,
  -lakshmi

Patch
diff mbox series

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d7e987baf127..a0e233afe876 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -721,6 +721,22 @@  void ima_kexec_cmdline(const void *buf, int size)
 					   KEXEC_CMDLINE, 0);
 }
 
+/**
+ * ima_post_key_create_or_update - measure asymmetric keys
+ * @keyring: keyring to which the key is linked to
+ * @key: created or updated key
+ * @flags: key flags
+ * @create: flag indicating whether the key was created or updated
+ *
+ * Keys can only be measured, not appraised.
+ */
+void ima_post_key_create_or_update(struct key *keyring, struct key *key,
+				   unsigned long flags, bool create)
+{
+	if ((keyring != NULL) && (key != NULL))
+		return;
+}
+
 static int __init init_ima(void)
 {
 	int error;