[v2,3/4] KEYS: Added BUILTIN_TRUSTED_KEYS enum to measure keys added to builtin_trusted_keys keyring
diff mbox series

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

Commit Message

Lakshmi Ramasubramanian Oct. 23, 2019, 11:39 p.m. UTC
Added an ima policy hook BUILTIN_TRUSTED_KEYS to measure keys added
to builtin_trusted_keys keyring.

Added a helper function to check if the given keyring is
the builtin_trusted_keys keyring.

Defined a function to map the keyring to ima policy hook function
and use it when measuring the key.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy |  1 +
 certs/system_keyring.c               |  5 +++++
 include/keys/system_keyring.h        |  2 ++
 security/integrity/ima/ima.h         |  2 ++
 security/integrity/ima/ima_api.c     |  1 +
 security/integrity/ima/ima_main.c    | 25 +++++++++++++++++++++++--
 security/integrity/ima/ima_queue.c   |  2 +-
 7 files changed, 35 insertions(+), 3 deletions(-)

Comments

Mimi Zohar Oct. 27, 2019, 2:33 p.m. UTC | #1
On Wed, 2019-10-23 at 16:39 -0700, Lakshmi Ramasubramanian wrote:
> Added an ima policy hook BUILTIN_TRUSTED_KEYS to measure keys added
> to builtin_trusted_keys keyring.
> 
> Added a helper function to check if the given keyring is
> the builtin_trusted_keys keyring.
> 
> Defined a function to map the keyring to ima policy hook function
> and use it when measuring the key.
 
.builtin_trusted_keys is a trusted keyring, which is created by the
kernel.  It cannot be deleted or replaced by userspace, so it should
be possible to correlate a keyring name with a keyring number on
policy load.

Other examples of trusted keyrings are: .ima, .evm, .platform,
.blacklist, .builtin_regdb_keys.  Instead of defining a keyring
specific method of getting the keyring number, define a generic
method.  For example, the userspace command "keyctl describe
%keyring:.builtin_trusted_keys" searches /proc/keys, but the kernel
shouldn't need to access /proc/keys.

> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  Documentation/ABI/testing/ima_policy |  1 +
>  certs/system_keyring.c               |  5 +++++
>  include/keys/system_keyring.h        |  2 ++
>  security/integrity/ima/ima.h         |  2 ++
>  security/integrity/ima/ima_api.c     |  1 +
>  security/integrity/ima/ima_main.c    | 25 +++++++++++++++++++++++--
>  security/integrity/ima/ima_queue.c   |  2 +-
>  7 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index fc376a323908..25566c74e679 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -29,6 +29,7 @@ Description:
>  				[FIRMWARE_CHECK]
>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>  				[KEXEC_CMDLINE]
> +				[BUILTIN_TRUSTED_KEYS]

The .builtin_trusted_keys is the name of a keyring, not of an IMA
hook.  Define a new IMA policy "keyring=" option, where keyring is
optional.  Some IMA policy rules might look like:

# measure all keys
measure func=KEYRING_CHECK

# measure keys on the IMA keyring
measure func=KEYRING_CHECK keyring=".ima"

# measure keys on the BUILTIN and IMA keyrings into a different PCR
measure func=KEYRING_CHECK keyring=".builtin_trusted_keys|.ima" pcr=11


>  			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>  			       [[^]MAY_EXEC]
>  			fsmagic:= hex value
> 
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index bce430b3386e..986f80eead4d 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -605,6 +605,24 @@ int ima_load_data(enum kernel_load_data_id id)
>  	return 0;
>  }
>  
> +/*
> + * Maps the given keyring to a IMA Hook.
> + * @keyring: A keyring to which a key maybe linked to.
> + *
> + * This function currently handles only builtin_trusted_keys.
> + * To handle more keyrings, this function, ima hook and
> + * ima policy handler need to be updated.
> + */
> +static enum ima_hooks keyring_policy_map(struct key *keyring)
> +{
> +	enum ima_hooks func = NONE;
> +
> +	if (is_builtin_trusted_keyring(keyring))
> +		func = BUILTIN_TRUSTED_KEYS;
> +
> +	return func;
> +}
> +
>  /*
>   * process_buffer_measurement - Measure the buffer to ima log.
>   * @buf: pointer to the buffer that needs to be added to the log.
> @@ -706,19 +724,22 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>  				   unsigned long flags, bool create)
>  {
>  	const struct public_key *pk;
> +	enum ima_hooks func;
>  
>  	if (key->type != &key_type_asymmetric)
>  		return;
>  
> +	func = keyring_policy_map(keyring);
> +

"func", in this case, should be something like "KEYRING_CHECK".  No
mapping is necessary.

>  	if (!ima_initialized) {
> -		ima_queue_key_for_measurement(key, NONE);
> +		ima_queue_key_for_measurement(key, func);
>  		return;
>  	}
>  
>  	pk = key->payload.data[asym_crypto];
>  	process_buffer_measurement(pk->key, pk->keylen,
>  				   key->description,
> -				   NONE, 0);
> +				   func, 0);

Pass the "keyring" to process_buffer_measurement() and on to
ima_get_action(), so that ima_get_action() determines whether the
keyring is in policy.

Mimi

>  }
>
Lakshmi Ramasubramanian Oct. 28, 2019, 3:12 p.m. UTC | #2
On 10/27/19 7:33 AM, Mimi Zohar wrote:

> .builtin_trusted_keys is a trusted keyring, which is created by the
> kernel.  It cannot be deleted or replaced by userspace, so it should
> be possible to correlate a keyring name with a keyring number on
> policy load.

Yes - at policy load we can map a keyring name to a keyring number.

But at runtime we still need to know if the keyring parameter passed to 
the IMA hook function is configured to be measured.

void ima_post_key_create_or_update(struct key *keyring, struct key *key,
				   unsigned long flags, bool create);
{
    => Get the keyring number for the given "keyring".
    => Check if the keyring number is in the configured IMA policy.
    => If yes, measure the key.
    => Else, do nothing.
}

Did I misunderstand what you had stated?

>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index fc376a323908..25566c74e679 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -29,6 +29,7 @@ Description:
>>   				[FIRMWARE_CHECK]
>>   				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>>   				[KEXEC_CMDLINE]
>> +				[BUILTIN_TRUSTED_KEYS]
> 
> The .builtin_trusted_keys is the name of a keyring, not of an IMA
> hook.  Define a new IMA policy "keyring=" option, where keyring is
> optional.  Some IMA policy rules might look like:
> 
> # measure all keys
> measure func=KEYRING_CHECK
> 
> # measure keys on the IMA keyring
> measure func=KEYRING_CHECK keyring=".ima"
> 
> # measure keys on the BUILTIN and IMA keyrings into a different PCR
> measure func=KEYRING_CHECK keyring=".builtin_trusted_keys|.ima" pcr=11

I agree - I'll take a look at this.

> "func", in this case, should be something like "KEYRING_CHECK".  No
> mapping is necessary.
Agree.

> 
>>   	if (!ima_initialized) {
>> -		ima_queue_key_for_measurement(key, NONE);
>> +		ima_queue_key_for_measurement(key, func);
>>   		return;
>>   	}
>>   
>>   	pk = key->payload.data[asym_crypto];
>>   	process_buffer_measurement(pk->key, pk->keylen,
>>   				   key->description,
>> -				   NONE, 0);
>> +				   func, 0);
> 
> Pass the "keyring" to process_buffer_measurement() and on to
> ima_get_action(), so that ima_get_action() determines whether the
> keyring is in policy.

Agree.

thanks,
  -lakshmi
Lakshmi Ramasubramanian Oct. 28, 2019, 3:56 p.m. UTC | #3
On 10/27/19 7:33 AM, Mimi Zohar wrote:

> Other examples of trusted keyrings are: .ima, .evm, .platform,
> .blacklist, .builtin_regdb_keys.  Instead of defining a keyring
> specific method of getting the keyring number, define a generic
> method.  For example, the userspace command "keyctl describe
> %keyring:.builtin_trusted_keys" searches /proc/keys, but the kernel
> shouldn't need to access /proc/keys.

"description" field in "struct key" is set to ".builtin_trusted_keys" 
for Built-In Trusted Keys keyring.

Similarly, for other keyrings such as .ima, .evm, .blacklist, 
.builtin_regdb_keys, etc.

 > # measure keys on the BUILTIN and IMA keyrings into a different PCR
 > measure func=KEYRING_CHECK keyring=".builtin_trusted_keys|.ima" pcr=11

With IMA policy set like above, the keyring to keyring number mapping 
can be set at IMA policy load.

My earlier point about mapping the "keyring" to "keyring number" at 
runtime (when the IMA hook is called) still needs to be done to know if 
the given keyring is in the policy or not.

void ima_post_key_create_or_update(struct key *keyring, struct key *key,
				   unsigned long flags, bool create)

thanks,
  -lakshmi
Mimi Zohar Oct. 28, 2019, 5:08 p.m. UTC | #4
On Mon, 2019-10-28 at 08:12 -0700, Lakshmi Ramasubramanian wrote:
> On 10/27/19 7:33 AM, Mimi Zohar wrote:
> 
> > .builtin_trusted_keys is a trusted keyring, which is created by the
> > kernel.  It cannot be deleted or replaced by userspace, so it should
> > be possible to correlate a keyring name with a keyring number on
> > policy load.
> 
> Yes - at policy load we can map a keyring name to a keyring number.
> 
> But at runtime we still need to know if the keyring parameter passed to 
> the IMA hook function is configured to be measured.
> 
> void ima_post_key_create_or_update(struct key *keyring, struct key *key,
> 				   unsigned long flags, bool create);
> {
>     => Get the keyring number for the given "keyring".

There is no "getting" involved here.  Pass "keyring" to
process_buffer_measurement and on to ima_get_action().

>     => Check if the keyring number is in the configured IMA policy.

ima_get_action() should do a simple compare of the valued stored in
the IMA policy with the value returned by key_serial().

Mimi

>     => If yes, measure the key.
>     => Else, do nothing.
> }

> Did I misunderstand what you had stated?

Patch
diff mbox series

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index fc376a323908..25566c74e679 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,6 +29,7 @@  Description:
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 				[KEXEC_CMDLINE]
+				[BUILTIN_TRUSTED_KEYS]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 1eba08a1af82..5533c7f92fef 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -283,3 +283,8 @@  void __init set_platform_trusted_keys(struct key *keyring)
 	platform_trusted_keys = keyring;
 }
 #endif
+
+inline bool is_builtin_trusted_keyring(struct key *keyring)
+{
+	return (keyring == builtin_trusted_keys);
+}
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index c1a96fdf598b..2bc0aaa07f05 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -66,4 +66,6 @@  static inline void set_platform_trusted_keys(struct key *keyring)
 }
 #endif
 
+extern bool is_builtin_trusted_keyring(struct key *keyring);
+
 #endif /* _KEYS_SYSTEM_KEYRING_H */
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 38279707632a..92c25a6b4da7 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -23,6 +23,7 @@ 
 #include <crypto/hash_info.h>
 #include <crypto/public_key.h>
 #include <keys/asymmetric-type.h>
+#include <keys/system_keyring.h>
 
 #include "../integrity.h"
 
@@ -192,6 +193,7 @@  static inline unsigned long ima_hash_key(u8 *digest)
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
 	hook(KEXEC_CMDLINE)		\
+	hook(BUILTIN_TRUSTED_KEYS)	\
 	hook(MAX_CHECK)
 #define __ima_hook_enumify(ENUM)	ENUM,
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index f614e22bf39f..cc04706b7e7a 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -175,6 +175,7 @@  void ima_add_violation(struct file *file, const unsigned char *filename,
  *	subj,obj, and type: are LSM specific.
  *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
  *	| KEXEC_CMDLINE
+ *	| BUILTIN_TRUSTED_KEYS
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index bce430b3386e..986f80eead4d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -605,6 +605,24 @@  int ima_load_data(enum kernel_load_data_id id)
 	return 0;
 }
 
+/*
+ * Maps the given keyring to a IMA Hook.
+ * @keyring: A keyring to which a key maybe linked to.
+ *
+ * This function currently handles only builtin_trusted_keys.
+ * To handle more keyrings, this function, ima hook and
+ * ima policy handler need to be updated.
+ */
+static enum ima_hooks keyring_policy_map(struct key *keyring)
+{
+	enum ima_hooks func = NONE;
+
+	if (is_builtin_trusted_keyring(keyring))
+		func = BUILTIN_TRUSTED_KEYS;
+
+	return func;
+}
+
 /*
  * process_buffer_measurement - Measure the buffer to ima log.
  * @buf: pointer to the buffer that needs to be added to the log.
@@ -706,19 +724,22 @@  void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 				   unsigned long flags, bool create)
 {
 	const struct public_key *pk;
+	enum ima_hooks func;
 
 	if (key->type != &key_type_asymmetric)
 		return;
 
+	func = keyring_policy_map(keyring);
+
 	if (!ima_initialized) {
-		ima_queue_key_for_measurement(key, NONE);
+		ima_queue_key_for_measurement(key, func);
 		return;
 	}
 
 	pk = key->payload.data[asym_crypto];
 	process_buffer_measurement(pk->key, pk->keylen,
 				   key->description,
-				   NONE, 0);
+				   func, 0);
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index d42987022c12..ed77c4dc0520 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -319,7 +319,7 @@  void ima_measure_queued_keys(void)
 		process_buffer_measurement(entry->public_key,
 					   entry->public_key_len,
 					   entry->key_description,
-					   NONE, 0);
+					   entry->func, 0);
 		list_del(&entry->list);
 		ima_free_trusted_key_entry(entry);
 	}