[PATCH 0/1] KEYS: Measure keys in trusted keyring

[PATCH 0/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

Created using linux v5.3.0-rc5

Motive:

Motive behind this patch set is to measure the public keys in
the trusted keyring. If CONFIG_SECONDARY_TRUSTED_KEYRING is
enabled then the trusted keys keyring is secondary_trusted_keys.
Otherwise, the trusted keys keyring is builtin_trusted_keys.

Measurement of the trusted keys is an addition to
the existing IMA measurements and not a replacement for it.

The measurement is enabled through the configuration value
CONFIG_IMA_MEASURE_TRUSTED_KEYS. This configuration
is turned OFF by default and have to opted in by the kernel
builder.

Background:

Currently IMA measures file hashes and .ima signatures. IMA signatures
are validated against keys in ".ima" keyring. If the kernel is built with
CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY enabled,
then all keys in ".ima" keyring must be signed by a key in
".builtin_trusted_keys" or ".secondary_trusted_keys" keyrings.

On systems with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
enabled, measuring keys in the  trusted keyring provides a mechanism
to attest that the client's system binaries are indeed signed by signers
that chain to known trusted keys.

Without this patch set, to attest the clients one needs to maintain
an "allowed list" of file hashes of all versions of all client binaries
that are deployed on the clients in the enterprise. That is a huge
operational challenge in a large scale environment of clients with
heterogenous builds. This also limits scalability and agility of
rolling out frequent client binary updates.

Current patch:

This patch set to measure the public keys in the trusted keys
keyring is disabled by default and can be enabled with
CONFIG_IMA_MEASURE_TRUSTED_KEYS. When this configuration is
enabled, during boot IMA enumerates keys in the trusted keys
keyring and measures them in the IMA log.

Questions and concerns raised by reviewers on this patch set:

Question 1:
Is "Signed with a trusted key" equal to "Trusted file"?
Doesn't the service need the hashes of the system files to determine
whether a file is trusted or not?

"Signed with a trusted key" does not equal "Trusted"

Answer:
Agree "Signed with a trusted key" may not equal "Trusted".
To address this, the attesting service can maintain a small
manageable set of bad hashes (a "Blocked list") and a list of
trusted keys expected in client's trusted keys keyring.
Using this data, the service can detect the presence of
"Disallowed (untrusted) version of client binaries".

Question 2:
Providing more data to the service (such as the keys in trusted keyring)
empowers the service to deny access to clients (block clients).
IMA walks a fine line in enforcing and measuring file integrity.
This patchset breaches that fine line and in doing so brings back
the fears of trusted computing.

Answer:
Any new measurement we add in IMA will provide more data to service
and can enable it to deny access to clients. It is not clear why
this patch set would breach the fine line between measuring
and enforcing.

Since this patch set is disabled by default and enabled through
CONFIG_IMA_MEASURE_TRUSTED_KEYS, only those enterprises that
require this new measurement can opt-in for it. Since it is disabled
by default, it does not restrict the autonomy of independent users
who are unaffected by attestation.

Question 3:
IMA log already contains a pointer to the IMA keys used for signature
verification. Why does the service need to care what keys were used
to sign (install) the IMA keys? What is gained by measuring the keys
in the trusted keyring?

Answer:
To attest the clients using the current IMA log, service needs to maintain
hashes of all the deployed versions of all the system binaries for their
enterprise. This will introduce a very high operational overhead in
a large scale environment of clients with heterogenous builds.
This limits scalability and agility of rolling out frequent client
binary updates.

On the other hand, with the current patch set, we will have IMA
validate the file signature on the clients and the service validate
that the IMA keys were installed using trusted keys.

This provides a chain of trust:
    => IMA Key validates file signature on the client
    => Key in the trusted keyring attests IMA key on the client
    => Attestation service attests the trusted keys
       reported by the client in the IMA log

This approach, therefore, would require the service to maintain
a manageble set of trusted keys that it receives from a trusted source.
And, verify if the clients only have keys from that set of trusted keys.

Question 4:
Where will the attestation service receive the keys to validate against?

Answer:
Attestation service will receive the keys from a trusted source such as
the enterprise build services that provides the client builds.
The service will use this set of keys to verify that the keys reported by
the clients in the IMA log contains only keys from this trusted list.

Question 5:
What is changing in the IMA log through this patch set?

Answer:
This patch set does not remove any data that is currently included
in the IMA log. It only adds more data to the IMA log - the data on
keys in the trusted keyring

Lakshmi Ramasubramanian (1):
  KEYS: Measure keys in trusted keyring

 certs/system_keyring.c            | 15 ++++++
 include/keys/system_keyring.h     |  4 ++
 include/linux/key.h               | 21 ++++++++
 security/integrity/ima/Kconfig    | 14 ++++++
 security/integrity/ima/ima_init.c | 84 +++++++++++++++++++++++++++++++
 security/keys/keyring.c           | 63 +++++++++++++++++++++++
 6 files changed, 201 insertions(+)

-- 
2.17.1

[PATCH 1/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

Measure keys in the trusted keyring. If CONFIG_SECONDARY_TRUSTED_KEYRING
is enabled then the trusted keys keyring is secondary_trusted_keys.
Otherwise, the trusted keys keyring is builtin_trusted_keys.

This measurement is in addition to IMA measuring module\file
signature. It adds more information for attestation service
to validate the client has known good keys in the trusted
keyring.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 certs/system_keyring.c            | 15 ++++++
 include/keys/system_keyring.h     |  4 ++
 include/linux/key.h               | 21 ++++++++
 security/integrity/ima/Kconfig    | 14 ++++++
 security/integrity/ima/ima_init.c | 84 +++++++++++++++++++++++++++++++
 security/keys/keyring.c           | 63 +++++++++++++++++++++++
 6 files changed, 201 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 1eba08a1af82..221eabee70b4 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -283,3 +283,18 @@ void __init set_platform_trusted_keys(struct key *keyring)
 	platform_trusted_keys = keyring;
 }
 #endif
+
+#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
+struct key *get_trusted_keys(void)
+{
+	struct key *trusted_keys;
+
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+	trusted_keys = secondary_trusted_keys;
+#else
+	trusted_keys = builtin_trusted_keys;
+#endif
+
+	return trusted_keys;
+}
+#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index c1a96fdf598b..789782a1d5a9 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -66,4 +66,8 @@ static inline void set_platform_trusted_keys(struct key *keyring)
 }
 #endif
 
+#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
+extern struct key *get_trusted_keys(void);
+#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
+
 #endif /* _KEYS_SYSTEM_KEYRING_H */
diff --git a/include/linux/key.h b/include/linux/key.h
index 50028338a4cc..843198b94677 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -408,6 +408,27 @@ extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
 				 key_perm_t perm);
 extern void key_free_user_ns(struct user_namespace *);
 
+typedef int (*key_iterator_func)(void *key, u32 keylen,
+				const char *description);
+
+/*
+ * Context data used to iterate through the keys in a keyring.
+ *
+ *  size  - Total number of keys in the keyring
+ *  enumerated - Number of keys that have been enumerated so far
+ *  iterator - Pointer to the function called for each key
+ */
+struct keyring_iterator {
+	size_t size;
+	size_t enumerated;
+	key_iterator_func iterator;
+};
+
+#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
+extern long keyring_read_trusted_keys(
+	struct keyring_iterator *key_iterator);
+#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
+
 /*
  * The permissions required on a key that we're looking up.
  */
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 2ced99dde694..f33237da0add 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -297,3 +297,17 @@ config IMA_APPRAISE_SIGNED_INIT
 	default n
 	help
 	   This option requires user-space init to be signed.
+
+config IMA_MEASURE_TRUSTED_KEYS
+	bool "Measure the keys in the Trusted Keys keyring"
+	depends on IMA
+	default n
+	help
+	   This option enables measurement of the public key of
+	   the keys in the Trusted Keys keyring during
+	   IMA initialization. Depending on the kernel configuration
+	   the trusted keyring could be one of the following:
+	    if CONFIG_SECONDARY_TRUSTED_KEYRING is enabled then
+	        secondary_trusted_keys
+	    else
+	        builtin_trusted_keys
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index 5d55ade5f3b9..987939f6c0f6 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -89,6 +89,83 @@ static int __init ima_add_boot_aggregate(void)
 	return result;
 }
 
+#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
+int __init store_trusted_keyring_key(void *key,
+		u32 keylen, const char *key_description)
+{
+	static const char op[] = "store_trusted_keyring_key";
+	const char *audit_cause = "ENOMEM";
+	struct ima_template_entry *entry;
+	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
+	struct ima_event_data event_data = {iint, NULL, key_description,
+					    NULL, 0, NULL};
+	int result = -ENOMEM;
+	int violation = 0;
+	struct {
+		struct ima_digest_data hdr;
+		char digest[IMA_MAX_DIGEST_SIZE];
+	} hash;
+
+	if (key == NULL || keylen == 0)
+		return 0;
+
+	memset(iint, 0, sizeof(*iint));
+	memset(&hash, 0, sizeof(hash));
+	iint->ima_hash = &hash.hdr;
+	iint->ima_hash->algo = HASH_ALGO_SHA1;
+	iint->ima_hash->length = SHA1_DIGEST_SIZE;
+
+	result = ima_calc_buffer_hash(key, keylen, &hash.hdr);
+	if (result < 0) {
+		audit_cause = "hashing_error";
+		goto err_out;
+	}
+
+	result = ima_alloc_init_template(&event_data, &entry, NULL);
+	if (result < 0) {
+		audit_cause = "alloc_entry";
+		goto err_out;
+	}
+
+	result = ima_store_template(entry, violation, NULL,
+					key_description,
+					CONFIG_IMA_MEASURE_PCR_IDX);
+	if (result < 0) {
+		ima_free_template_entry(entry);
+		audit_cause = "store_entry";
+		goto err_out;
+	}
+	return 0;
+err_out:
+	integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL,
+				key_description, op,
+				audit_cause, result, 0);
+	return result;
+}
+
+int __init ima_add_trusted_keyring_keys(int (*store_trusted_key)(
+			void *key,
+			u32 keylen,
+			const char *key_description))
+{
+	struct keyring_iterator key_iterator;
+	int rc = 0;
+
+	/* Retrieve the information on keys in
+	 * the Built-In Trusted Keys keyring.
+	 */
+	key_iterator.size = 0;
+	key_iterator.enumerated = 0;
+	key_iterator.iterator = store_trusted_key;
+	rc = keyring_read_trusted_keys(&key_iterator);
+	if (rc < 0)
+		pr_err("Failed %d to read keys in trusted_keys\n", rc);
+
+	return rc;
+}
+
+#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
+
 #ifdef CONFIG_IMA_LOAD_X509
 void __init ima_load_x509(void)
 {
@@ -129,6 +206,13 @@ int __init ima_init(void)
 	if (rc != 0)
 		return rc;
 
+#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
+	/* Measure keys from builtin trusted keys keyring. */
+	rc = ima_add_trusted_keyring_keys(store_trusted_keyring_key);
+	if (rc != 0)
+		return rc;
+#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
+
 	ima_init_policy();
 
 	return ima_fs_init();
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index febf36c6ddc5..fe84923b3c1c 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -16,6 +16,9 @@
 #include <linux/nsproxy.h>
 #include <keys/keyring-type.h>
 #include <keys/user-type.h>
+#include <keys/asymmetric-type.h>
+#include <keys/system_keyring.h>
+#include <crypto/public_key.h>
 #include <linux/assoc_array_priv.h>
 #include <linux/uaccess.h>
 #include <net/net_namespace.h>
@@ -1790,3 +1793,63 @@ void keyring_restriction_gc(struct key *keyring, struct key_type *dead_type)
 
 	kleave(" [restriction gc]");
 }
+
+#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
+static int keyring_keys_iterator(const void *object, void *data)
+{
+	struct keyring_iterator *key_iterator = data;
+	const struct key *key = keyring_ptr_to_key(object);
+	const struct public_key *pk;
+	int rc = 0;
+
+	if (key_iterator->enumerated < key_iterator->size) {
+		key_iterator->enumerated++;
+		pk = key->payload.data[asym_crypto];
+		if ((pk != NULL) &&
+			(pk->keylen > 0) &&
+			(key->description != NULL)) {
+			rc = key_iterator->iterator(pk->key,
+					pk->keylen,
+					key->description);
+		}
+	}
+
+	return rc;
+}
+
+/*
+ * Read a list of keys from the given keyring.
+ *  keyring - Keyring to read the list of keys from
+ *  key_iterator - Keyring iterator
+ */
+long keyring_read_keys(
+	const struct key *keyring,
+	struct keyring_iterator *key_iterator)
+{
+	long ret = 0;
+
+	kenter("{%d}", key_serial(keyring));
+
+	key_iterator->size = keyring->keys.nr_leaves_on_tree;
+	key_iterator->enumerated = 0;
+	ret = assoc_array_iterate(&keyring->keys,
+				keyring_keys_iterator,
+				key_iterator);
+	if (ret == 0)
+		kleave(" = %ld [ok]", ret);
+	else
+		kleave(" = %ld [error]", ret);
+
+	return ret;
+}
+
+/*
+ * Read a list of keys from the trusted_keys keyring.
+ *  key_iterator - Keyring iterator
+ */
+long keyring_read_trusted_keys(
+	struct keyring_iterator *key_iterator)
+{
+	return keyring_read_keys(get_trusted_keys(), key_iterator);
+}
+#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
-- 
2.17.1

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

[Cc'ing Matthew Garrett]

On Tue, 2019-08-27 at 17:27 -0700, Lakshmi Ramasubramanian wrote:
> Created using linux v5.3.0-rc5
> 
> Motive:
> 
> Motive behind this patch set is to measure the public keys in
> the trusted keyring. If CONFIG_SECONDARY_TRUSTED_KEYRING is
> enabled then the trusted keys keyring is secondary_trusted_keys.
> Otherwise, the trusted keys keyring is builtin_trusted_keys.
> 
> Measurement of the trusted keys is an addition to
> the existing IMA measurements and not a replacement for it.
> 
> The measurement is enabled through the configuration value
> CONFIG_IMA_MEASURE_TRUSTED_KEYS. This configuration
> is turned OFF by default and have to opted in by the kernel
> builder.
> 
> Background:
> 
> Currently IMA measures file hashes and .ima signatures. IMA signatures
> are validated against keys in ".ima" keyring. If the kernel is built with
> CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY enabled,
> then all keys in ".ima" keyring must be signed by a key in
> ".builtin_trusted_keys" or ".secondary_trusted_keys" keyrings.
> 
> On systems with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> enabled, measuring keys in the  trusted keyring provides a mechanism
> to attest that the client's system binaries are indeed signed by signers
> that chain to known trusted keys.
> 
> Without this patch set, to attest the clients one needs to maintain
> an "allowed list" of file hashes of all versions of all client binaries
> that are deployed on the clients in the enterprise.  That is a huge
> operational challenge in a large scale environment of clients with
> heterogenous builds. This also limits scalability and agility of
> rolling out frequent client binary updates.

The purpose of the ima-sig template, which includes the file signature
and header containing the keyid, is to avoid needing to maintain a
white list as you described.
 
> 
> Current patch:
> 
> This patch set to measure the public keys in the trusted keys
> keyring is disabled by default and can be enabled with
> CONFIG_IMA_MEASURE_TRUSTED_KEYS. When this configuration is
> enabled, during boot IMA enumerates keys in the trusted keys
> keyring and measures them in the IMA log.
> 
> Questions and concerns raised by reviewers on this patch set:
> 
> Question 1:
> Is "Signed with a trusted key" equal to "Trusted file"?
> Doesn't the service need the hashes of the system files to determine
> whether a file is trusted or not?
> 
> "Signed with a trusted key" does not equal "Trusted"
> 
> Answer:
> Agree "Signed with a trusted key" may not equal "Trusted".
> To address this, the attesting service can maintain a small
> manageable set of bad hashes (a "Blocked list") and a list of
> trusted keys expected in client's trusted keys keyring.
> Using this data, the service can detect the presence of
> "Disallowed (untrusted) version of client binaries".
> 
> Question 2:
> Providing more data to the service (such as the keys in trusted keyring)
> empowers the service to deny access to clients (block clients).
> IMA walks a fine line in enforcing and measuring file integrity.
> This patchset breaches that fine line and in doing so brings back
> the fears of trusted computing.
> 
> Answer:
> Any new measurement we add in IMA will provide more data to service
> and can enable it to deny access to clients. It is not clear why
> this patch set would breach the fine line between measuring
> and enforcing.
> 
> Since this patch set is disabled by default and enabled through
> CONFIG_IMA_MEASURE_TRUSTED_KEYS, only those enterprises that
> require this new measurement can opt-in for it. Since it is disabled
> by default, it does not restrict the autonomy of independent users
> who are unaffected by attestation.

The concern isn't on the client side, but the server side.  Once the
ability of including measurements of keys on the builtin and/or
secondary keyrings on the client side exists, the attestation servers
can start requiring it.  Providing a means of disabling it on the
client side doesn't address this problem.

> 
> Question 3:
> IMA log already contains a pointer to the IMA keys used for signature
> verification. Why does the service need to care what keys were used
> to sign (install) the IMA keys? What is gained by measuring the keys
> in the trusted keyring?
> 
> Answer:
> To attest the clients using the current IMA log, service needs to maintain
> hashes of all the deployed versions of all the system binaries for their
> enterprise. This will introduce a very high operational overhead in
> a large scale environment of clients with heterogenous builds.
> This limits scalability and agility of rolling out frequent client
> binary updates.

No, there is no need for maintaining a binary hash white list.  The
attestation server requires a set of trusted keys used to sign
software.

The only reason for measuring the keys on the builtin and/or secondary
keyrings is to prevent system owners from signing and running
applications on their own systems.

Since you obviously disagree, I'd really like to hear other people's thoughts.

Mimi

> 
> On the other hand, with the current patch set, we will have IMA
> validate the file signature on the clients and the service validate
> that the IMA keys were installed using trusted keys.
> 
> This provides a chain of trust:
>     => IMA Key validates file signature on the client
>     => Key in the trusted keyring attests IMA key on the client
>     => Attestation service attests the trusted keys
>        reported by the client in the IMA log
> 
> This approach, therefore, would require the service to maintain
> a manageble set of trusted keys that it receives from a trusted source.
> And, verify if the clients only have keys from that set of trusted keys.
> 
> Question 4:
> Where will the attestation service receive the keys to validate against?
> 
> Answer:
> Attestation service will receive the keys from a trusted source such as
> the enterprise build services that provides the client builds.
> The service will use this set of keys to verify that the keys reported by
> the clients in the IMA log contains only keys from this trusted list.
> 
> Question 5:
> What is changing in the IMA log through this patch set?
> 
> Answer:
> This patch set does not remove any data that is currently included
> in the IMA log. It only adds more data to the IMA log - the data on
> keys in the trusted keyring
> 
> Lakshmi Ramasubramanian (1):
>   KEYS: Measure keys in trusted keyring
> 
>  certs/system_keyring.c            | 15 ++++++
>  include/keys/system_keyring.h     |  4 ++
>  include/linux/key.h               | 21 ++++++++
>  security/integrity/ima/Kconfig    | 14 ++++++
>  security/integrity/ima/ima_init.c | 84 +++++++++++++++++++++++++++++++
>  security/keys/keyring.c           | 63 +++++++++++++++++++++++
>  6 files changed, 201 insertions(+)
> 

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

>> Without this patch set, to attest the clients one needs to maintain
>> an "allowed list" of file hashes of all versions of all client binaries
>> that are deployed on the clients in the enterprise.  That is a huge
>> operational challenge in a large scale environment of clients with
>> heterogenous builds. This also limits scalability and agility of
>> rolling out frequent client binary updates.
> 
> The purpose of the ima-sig template, which includes the file signature
> and header containing the keyid, is to avoid needing to maintain a
> white list as you described.

If the service were to validate the signature in the ima-sig template, 
it needs to have the hash of the file. Using the keyid in ima-sig pick 
the key, calculate the signed hash and compare it with the signed hash 
in the ima-sig template. Correct?

Or, it has to maintain the signed hash of the file and compare it with 
the signed hash in the ima-sig template.

In both the cases, the service needs to have the hash or signed hash for 
all the client files (for all versions of that file). This the 
maintenance overhead we are trying to avoid.

> The concern isn't on the client side, but the server side.  Once the
> ability of including measurements of keys on the builtin and/or
> secondary keyrings on the client side exists, the attestation servers
> can start requiring it.  Providing a means of disabling it on the
> client side doesn't address this problem.

But, wouldn't this problem exist for any new measure we add on the 
client side? Why is it particularly an issue for measuring trusted keys?

> 
> No, there is no need for maintaining a binary hash white list.  The
> attestation server requires a set of trusted keys used to sign
> software.
> 
> The only reason for measuring the keys on the builtin and/or secondary
> keyrings is to prevent system owners from signing and running
> applications on their own systems.
> 
> Since you obviously disagree, I'd really like to hear other people's thoughts.

Actually I am agreeing with you - the reason we want to measure the keys 
in the trusted keyring is to ensure that the system binaries running on 
the client are signed by trusted keys only. Please see below:

We let IMA verify the integrity of the system files on the client using 
IMA key(s). The IMA key(s) are themselves signed by "Trusted Key(s)" - 
unsigned IMA key or IMA key signed by keys not in the trusted keyring 
are not even allowed to be added to the IMA keyring.

And, on the server we validate the "Trusted Keyring" contains only 
known\trusted keys.

Through the above process - the server does not need to know the signed 
file hash. It only needs to keep a list of trusted keys and verify if 
the keys reported by the client is in that trusted keys set.

Please let me know if that answers your questions.

Thanks,
  -lakshmi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

[Cc'ing Monty, Roberto]

On Thu, 2019-08-29 at 19:43 -0700, Lakshmi Ramasubramanian wrote:
> >> Without this patch set, to attest the clients one needs to maintain
> >> an "allowed list" of file hashes of all versions of all client binaries
> >> that are deployed on the clients in the enterprise.  That is a huge
> >> operational challenge in a large scale environment of clients with
> >> heterogenous builds. This also limits scalability and agility of
> >> rolling out frequent client binary updates.
> > 
> > The purpose of the ima-sig template, which includes the file signature
> > and header containing the keyid, is to avoid needing to maintain a
> > white list as you described.
> 
> If the service were to validate the signature in the ima-sig template, 
> it needs to have the hash of the file. Using the keyid in ima-sig pick 
> the key, calculate the signed hash and compare it with the signed hash 
> in the ima-sig template. Correct?
> 
> Or, it has to maintain the signed hash of the file and compare it with 
> the signed hash in the ima-sig template.
> 
> In both the cases, the service needs to have the hash or signed hash for 
> all the client files (for all versions of that file). This the 
> maintenance overhead we are trying to avoid.

No, the measurement list ima-sig template record contains both the
file hash and signature.  There's no need to maintain a white list of
either the file hashes or signed hashes.  All that is needed is the
set of permitted public keys (eg. keys on the trusted IMA keyring).

> 
> > The concern isn't on the client side, but the server side.  Once the
> > ability of including measurements of keys on the builtin and/or
> > secondary keyrings on the client side exists, the attestation servers
> > can start requiring it.  Providing a means of disabling it on the
> > client side doesn't address this problem.
> 
> But, wouldn't this problem exist for any new measure we add on the 
> client side? Why is it particularly an issue for measuring trusted keys?
> 
> > 
> > No, there is no need for maintaining a binary hash white list.  The
> > attestation server requires a set of trusted keys used to sign
> > software.
> > 
> > The only reason for measuring the keys on the builtin and/or secondary
> > keyrings is to prevent system owners from signing and running
> > applications on their own systems.
> > 
> > Since you obviously disagree, I'd really like to hear other people's thoughts.
> 
> Actually I am agreeing with you - the reason we want to measure the keys 
> in the trusted keyring is to ensure that the system binaries running on 
> the client are signed by trusted keys only.

The .builtin, .secondary, .ima, and .evm keyrings are all trusted
keyrings, based on a signature chain of trust rooted in the signed
Linux kernel.

Even though on the local system, files signed by the system owner
would be permitted, the attestation server would be able to control
access to whatever service.  For example, Trusted Network Connect
(TNC) could control network access.  By measuring the keys on the
builtin/secondary keyrings, that control is not based on who signed
the software package, but based on who signed the certificate of the
key that signed the software package.  My concern is how this level of
indirection could be abused.

> Please see below:
> 
> We let IMA verify the integrity of the system files on the client using 
> IMA key(s). The IMA key(s) are themselves signed by "Trusted Key(s)" - 
> unsigned IMA key or IMA key signed by keys not in the trusted keyring 
> are not even allowed to be added to the IMA keyring.
> 
> And, on the server we validate the "Trusted Keyring" contains only 
> known\trusted keys.
> 
> Through the above process - the server does not need to know the signed 
> file hash. It only needs to keep a list of trusted keys and verify if 
> the keys reported by the client is in that trusted keys set.
> 
> Please let me know if that answers your questions.

All of this would still be true, if you measured the keys on the
trusted IMA keyring, but without the level of indirection described
above.  Depending on your use case scenario, the problem with this
approach is maintaining a list of all the certificates that have been
signed by keys on the builtin, and if enabled, the secondary keyrings.

In the last LSS-NA BoF, Monty suggested, for a different use case, one
that needs to seal keys, measuring keys and extending a separate PCR.

Mimi

Re: [PATCH 1/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

On Tue, 2019-08-27 at 17:27 -0700, Lakshmi Ramasubramanian wrote:
> Measure keys in the trusted keyring. If CONFIG_SECONDARY_TRUSTED_KEYRING
> is enabled then the trusted keys keyring is secondary_trusted_keys.
> Otherwise, the trusted keys keyring is builtin_trusted_keys.
> 
> This measurement is in addition to IMA measuring module\file
> signature. It adds more information for attestation service
> to validate the client has known good keys in the trusted
> keyring.
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>

IMA measures, appraises, and audits files based on policy[1].  If
you're going to measure keys, all of the code should be within the IMA
subdirectory.  The only code outside of the IMA subdirectory is either
an LSM or IMA hook.  If an LSM hook already exists, use it.  If an LSM
hook doesn't exist and the location is generic that other LSMs would
be interested, define a new LSM hook, otherwise define a new IMA hook.

For example, to measure /boot/cmdline, the rule is "measure
func=KEXEC_CMDLINE template=ima-buf".  A similar rule for measuring
keys would look something like "measure func=KEYS template=ima-buf
pcr=<number>".

Remember "ifdef's" don't belong in C code[2].  Normally a stub
function is defined in an include file to avoid ifdefs.

Mimi

[1] Documentation/ABI/testing/ima_policy
[2] Refer to Documentation/process/coding-style.rst section "21)
Conditional Compilation".


> ---
>  certs/system_keyring.c            | 15 ++++++
>  include/keys/system_keyring.h     |  4 ++
>  include/linux/key.h               | 21 ++++++++
>  security/integrity/ima/Kconfig    | 14 ++++++
>  security/integrity/ima/ima_init.c | 84 +++++++++++++++++++++++++++++++
>  security/keys/keyring.c           | 63 +++++++++++++++++++++++
>  6 files changed, 201 insertions(+)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 1eba08a1af82..221eabee70b4 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -283,3 +283,18 @@ void __init set_platform_trusted_keys(struct key *keyring)
>  	platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
> +struct key *get_trusted_keys(void)
> +{
> +	struct key *trusted_keys;
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +	trusted_keys = secondary_trusted_keys;
> +#else
> +	trusted_keys = builtin_trusted_keys;
> +#endif
> +
> +	return trusted_keys;
> +}
> +#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index c1a96fdf598b..789782a1d5a9 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -66,4 +66,8 @@ static inline void set_platform_trusted_keys(struct key *keyring)
>  }
>  #endif
>  
> +#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
> +extern struct key *get_trusted_keys(void);
> +#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
> +
>  #endif /* _KEYS_SYSTEM_KEYRING_H */
> diff --git a/include/linux/key.h b/include/linux/key.h
> index 50028338a4cc..843198b94677 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -408,6 +408,27 @@ extern key_ref_t lookup_user_key(key_serial_t id, unsigned long flags,
>  				 key_perm_t perm);
>  extern void key_free_user_ns(struct user_namespace *);
>  
> +typedef int (*key_iterator_func)(void *key, u32 keylen,
> +				const char *description);
> +
> +/*
> + * Context data used to iterate through the keys in a keyring.
> + *
> + *  size  - Total number of keys in the keyring
> + *  enumerated - Number of keys that have been enumerated so far
> + *  iterator - Pointer to the function called for each key
> + */
> +struct keyring_iterator {
> +	size_t size;
> +	size_t enumerated;
> +	key_iterator_func iterator;
> +};
> +
> +#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
> +extern long keyring_read_trusted_keys(
> +	struct keyring_iterator *key_iterator);
> +#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
> +
>  /*
>   * The permissions required on a key that we're looking up.
>   */
> diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
> index 2ced99dde694..f33237da0add 100644
> --- a/security/integrity/ima/Kconfig
> +++ b/security/integrity/ima/Kconfig
> @@ -297,3 +297,17 @@ config IMA_APPRAISE_SIGNED_INIT
>  	default n
>  	help
>  	   This option requires user-space init to be signed.
> +
> +config IMA_MEASURE_TRUSTED_KEYS
> +	bool "Measure the keys in the Trusted Keys keyring"
> +	depends on IMA
> +	default n
> +	help
> +	   This option enables measurement of the public key of
> +	   the keys in the Trusted Keys keyring during
> +	   IMA initialization. Depending on the kernel configuration
> +	   the trusted keyring could be one of the following:
> +	    if CONFIG_SECONDARY_TRUSTED_KEYRING is enabled then
> +	        secondary_trusted_keys
> +	    else
> +	        builtin_trusted_keys
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 5d55ade5f3b9..987939f6c0f6 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -89,6 +89,83 @@ static int __init ima_add_boot_aggregate(void)
>  	return result;
>  }
>  
> +#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
> +int __init store_trusted_keyring_key(void *key,
> +		u32 keylen, const char *key_description)
> +{
> +	static const char op[] = "store_trusted_keyring_key";
> +	const char *audit_cause = "ENOMEM";
> +	struct ima_template_entry *entry;
> +	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
> +	struct ima_event_data event_data = {iint, NULL, key_description,
> +					    NULL, 0, NULL};
> +	int result = -ENOMEM;
> +	int violation = 0;
> +	struct {
> +		struct ima_digest_data hdr;
> +		char digest[IMA_MAX_DIGEST_SIZE];
> +	} hash;
> +
> +	if (key == NULL || keylen == 0)
> +		return 0;
> +
> +	memset(iint, 0, sizeof(*iint));
> +	memset(&hash, 0, sizeof(hash));
> +	iint->ima_hash = &hash.hdr;
> +	iint->ima_hash->algo = HASH_ALGO_SHA1;
> +	iint->ima_hash->length = SHA1_DIGEST_SIZE;
> +
> +	result = ima_calc_buffer_hash(key, keylen, &hash.hdr);
> +	if (result < 0) {
> +		audit_cause = "hashing_error";
> +		goto err_out;
> +	}
> +
> +	result = ima_alloc_init_template(&event_data, &entry, NULL);
> +	if (result < 0) {
> +		audit_cause = "alloc_entry";
> +		goto err_out;
> +	}
> +
> +	result = ima_store_template(entry, violation, NULL,
> +					key_description,
> +					CONFIG_IMA_MEASURE_PCR_IDX);
> +	if (result < 0) {
> +		ima_free_template_entry(entry);
> +		audit_cause = "store_entry";
> +		goto err_out;
> +	}
> +	return 0;
> +err_out:
> +	integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL,
> +				key_description, op,
> +				audit_cause, result, 0);
> +	return result;
> +}
> +
> +int __init ima_add_trusted_keyring_keys(int (*store_trusted_key)(
> +			void *key,
> +			u32 keylen,
> +			const char *key_description))
> +{
> +	struct keyring_iterator key_iterator;
> +	int rc = 0;
> +
> +	/* Retrieve the information on keys in
> +	 * the Built-In Trusted Keys keyring.
> +	 */
> +	key_iterator.size = 0;
> +	key_iterator.enumerated = 0;
> +	key_iterator.iterator = store_trusted_key;
> +	rc = keyring_read_trusted_keys(&key_iterator);
> +	if (rc < 0)
> +		pr_err("Failed %d to read keys in trusted_keys\n", rc);
> +
> +	return rc;
> +}
> +
> +#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
> +
>  #ifdef CONFIG_IMA_LOAD_X509
>  void __init ima_load_x509(void)
>  {
> @@ -129,6 +206,13 @@ int __init ima_init(void)
>  	if (rc != 0)
>  		return rc;
>  
> +#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
> +	/* Measure keys from builtin trusted keys keyring. */
> +	rc = ima_add_trusted_keyring_keys(store_trusted_keyring_key);
> +	if (rc != 0)
> +		return rc;
> +#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */
> +
>  	ima_init_policy();
>  
>  	return ima_fs_init();
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index febf36c6ddc5..fe84923b3c1c 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -16,6 +16,9 @@
>  #include <linux/nsproxy.h>
>  #include <keys/keyring-type.h>
>  #include <keys/user-type.h>
> +#include <keys/asymmetric-type.h>
> +#include <keys/system_keyring.h>
> +#include <crypto/public_key.h>
>  #include <linux/assoc_array_priv.h>
>  #include <linux/uaccess.h>
>  #include <net/net_namespace.h>
> @@ -1790,3 +1793,63 @@ void keyring_restriction_gc(struct key *keyring, struct key_type *dead_type)
>  
>  	kleave(" [restriction gc]");
>  }
> +
> +#ifdef CONFIG_IMA_MEASURE_TRUSTED_KEYS
> +static int keyring_keys_iterator(const void *object, void *data)
> +{
> +	struct keyring_iterator *key_iterator = data;
> +	const struct key *key = keyring_ptr_to_key(object);
> +	const struct public_key *pk;
> +	int rc = 0;
> +
> +	if (key_iterator->enumerated < key_iterator->size) {
> +		key_iterator->enumerated++;
> +		pk = key->payload.data[asym_crypto];
> +		if ((pk != NULL) &&
> +			(pk->keylen > 0) &&
> +			(key->description != NULL)) {
> +			rc = key_iterator->iterator(pk->key,
> +					pk->keylen,
> +					key->description);
> +		}
> +	}
> +
> +	return rc;
> +}
> +
> +/*
> + * Read a list of keys from the given keyring.
> + *  keyring - Keyring to read the list of keys from
> + *  key_iterator - Keyring iterator
> + */
> +long keyring_read_keys(
> +	const struct key *keyring,
> +	struct keyring_iterator *key_iterator)
> +{
> +	long ret = 0;
> +
> +	kenter("{%d}", key_serial(keyring));
> +
> +	key_iterator->size = keyring->keys.nr_leaves_on_tree;
> +	key_iterator->enumerated = 0;
> +	ret = assoc_array_iterate(&keyring->keys,
> +				keyring_keys_iterator,
> +				key_iterator);
> +	if (ret == 0)
> +		kleave(" = %ld [ok]", ret);
> +	else
> +		kleave(" = %ld [error]", ret);
> +
> +	return ret;
> +}
> +
> +/*
> + * Read a list of keys from the trusted_keys keyring.
> + *  key_iterator - Keyring iterator
> + */
> +long keyring_read_trusted_keys(
> +	struct keyring_iterator *key_iterator)
> +{
> +	return keyring_read_keys(get_trusted_keys(), key_iterator);
> +}
> +#endif /* CONFIG_IMA_MEASURE_TRUSTED_KEYS */

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

On 8/30/19 11:41 AM, Mimi Zohar wrote:

> No, the measurement list ima-sig template record contains both the
> file hash and signature.  There's no need to maintain a white list of
> either the file hashes or signed hashes.  All that is needed is the
> set of permitted public keys (eg. keys on the trusted IMA keyring).

You are right - Thanks for the info.

> Even though on the local system, files signed by the system owner
> would be permitted, the attestation server would be able to control
> access to whatever service.  For example, Trusted Network Connect
> (TNC) could control network access.  By measuring the keys on the
> builtin/secondary keyrings, that control is not based on who signed
> the software package, but based on who signed the certificate of the
> key that signed the software package.  My concern is how this level of
> indirection could be abused.
Since the signer of certificate of the key that signed the software 
package changes much less frequently compared to the certificate of the 
key used to sign the software package, the operational overhead on the 
server side is significantly reduced.

I understand there is another level of indirection here. But I am also 
not clear how this can be abused.

> All of this would still be true, if you measured the keys on the
> trusted IMA keyring, but without the level of indirection described
> above.  Depending on your use case scenario, the problem with this
> approach is maintaining a list of all the certificates that have been
> signed by keys on the builtin, and if enabled, the secondary keyrings.

Yes - that is the issue we are trying to avoid. Especially since the 
list of signing certificates can grow faster than the signer of those 
certificates (that are present in the builtin/secondary keyrings)

> In the last LSS-NA BoF, Monty suggested, for a different use case, one
> that needs to seal keys, measuring keys and extending a separate PCR.
Thanks for the info. I will gather more information on this one.

  -lakshmi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

On Tue, 2019-09-03 at 08:54 -0700, Lakshmi Ramasubramanian wrote:
> On 8/30/19 11:41 AM, Mimi Zohar wrote:
> 
> > No, the measurement list ima-sig template record contains both the
> > file hash and signature.  There's no need to maintain a white list of
> > either the file hashes or signed hashes.  All that is needed is the
> > set of permitted public keys (eg. keys on the trusted IMA keyring).
> 
> You are right - Thanks for the info.
> 
> > Even though on the local system, files signed by the system owner
> > would be permitted, the attestation server would be able to control
> > access to whatever service.  For example, Trusted Network Connect
> > (TNC) could control network access.  By measuring the keys on the
> > builtin/secondary keyrings, that control is not based on who signed
> > the software package, but based on who signed the certificate of the
> > key that signed the software package.  My concern is how this level of
> > indirection could be abused.
> Since the signer of certificate of the key that signed the software 
> package changes much less frequently compared to the certificate of the 
> key used to sign the software package, the operational overhead on the 
> server side is significantly reduced.
> 
> I understand there is another level of indirection here. But I am also 
> not clear how this can be abused.

The remote attestation server could gate any service based on the
certificate signer.  The first gated service, based on this feature,
will probably be network access (eg. TNC).  If/when this feature is
upstreamed, every company, including financial institutes,
organizations, and governments will become THE certificate signer for
their organization, in order to limit access to their network and
systems.  Once that happens, how long will it be until the same
feature will be abused and used to limit the individual's ability to
pick and choose which applications may run on their systems.[1]

Mimi

[1] Refer to Richard Stallman's last paragraph https://www.gnu.org/phi
losophy/can-you-trust.en.html

> > All of this would still be true, if you measured the keys on the
> > trusted IMA keyring, but without the level of indirection described
> > above.  Depending on your use case scenario, the problem with this
> > approach is maintaining a list of all the certificates that have been
> > signed by keys on the builtin, and if enabled, the secondary keyrings.
> 
> Yes - that is the issue we are trying to avoid. Especially since the 
> list of signing certificates can grow faster than the signer of those 
> certificates (that are present in the builtin/secondary keyrings)
> 
> > In the last LSS-NA BoF, Monty suggested, for a different use case, one
> > that needs to seal keys, measuring keys and extending a separate PCR.
> Thanks for the info. I will gather more information on this one.
> 
>   -lakshmi
> 

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[James Morris]

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

On Mon, 9 Sep 2019, Mimi Zohar wrote:

> The remote attestation server could gate any service based on the
> certificate signer.  The first gated service, based on this feature,
> will probably be network access (eg. TNC).  If/when this feature is
> upstreamed, every company, including financial institutes,
> organizations, and governments will become THE certificate signer for
> their organization, in order to limit access to their network and
> systems.

This is already happening at scale, and a primary use-case for the 
patchset.

> Once that happens, how long will it be until the same
> feature will be abused and used to limit the individual's ability to
> pick and choose which applications may run on their systems.[1]

Isn't this already happening (in a non-abusive way) with mobile devices?

> Mimi
> 
> [1] Refer to Richard Stallman's last paragraph https://www.gnu.org/phi
> losophy/can-you-trust.en.html

Please consider if you really want to be endorsing this individual.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Sasha Levin]

On Mon, Sep 09, 2019 at 09:31:21AM -0400, Mimi Zohar wrote:
>On Tue, 2019-09-03 at 08:54 -0700, Lakshmi Ramasubramanian wrote:
>> On 8/30/19 11:41 AM, Mimi Zohar wrote:
>>
>> > No, the measurement list ima-sig template record contains both the
>> > file hash and signature.  There's no need to maintain a white list of
>> > either the file hashes or signed hashes.  All that is needed is the
>> > set of permitted public keys (eg. keys on the trusted IMA keyring).
>>
>> You are right - Thanks for the info.
>>
>> > Even though on the local system, files signed by the system owner
>> > would be permitted, the attestation server would be able to control
>> > access to whatever service.  For example, Trusted Network Connect
>> > (TNC) could control network access.  By measuring the keys on the
>> > builtin/secondary keyrings, that control is not based on who signed
>> > the software package, but based on who signed the certificate of the
>> > key that signed the software package.  My concern is how this level of
>> > indirection could be abused.
>> Since the signer of certificate of the key that signed the software
>> package changes much less frequently compared to the certificate of the
>> key used to sign the software package, the operational overhead on the
>> server side is significantly reduced.
>>
>> I understand there is another level of indirection here. But I am also
>> not clear how this can be abused.
>
>The remote attestation server could gate any service based on the
>certificate signer.  The first gated service, based on this feature,
>will probably be network access (eg. TNC).  If/when this feature is
>upstreamed, every company, including financial institutes,

I'm not sure why upstreaming this code will matter for those entities
you're concerned about. Any entity that provides a signed kernel image
is very well capable of including out of tree patches in that image.

>organizations, and governments will become THE certificate signer for
>their organization, in order to limit access to their network and
>systems.  Once that happens, how long will it be until the same
>feature will be abused and used to limit the individual's ability to
>pick and choose which applications may run on their systems.[1]

We do not restrict end use of the kernel; this is one of the main
reasons that the kernel is licensed under GPLv2 rather than GPLv3.
Please see https://lwn.net/Articles/200422/ .

We'd love to work with you on the technical aspects of this code to make
it acceptable to the IMA maintainers, but this work can't just be NACKed
based on a perceived end use of it.

--
Thanks,
Sasha

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

On Thu, 2019-09-19 at 09:18 -0400, Sasha Levin wrote:
> We do not restrict end use of the kernel; this is one of the main
> reasons that the kernel is licensed under GPLv2 rather than GPLv3.
> Please see https://lwn.net/Articles/200422/ .

That's from a licensing perspective.  Linus has full control of what
is upstreamed.

> 
> We'd love to work with you on the technical aspects of this code to make
> it acceptable to the IMA maintainers, but this work can't just be NACKed
> based on a perceived end use of it.

Perhaps if more people/companies thought about how technology could be
abused, before creating it, we, as a society, wouldn't be where we are
today.

On 9/1 I commented on this patch set from a technical perspective,
saying:

IMA measures, appraises, and audits files based on policy[1].  If
you're going to measure keys, all of the code should be within the IMA
subdirectory.  The only code outside of the IMA subdirectory is either
an LSM or IMA hook.  If an LSM hook already exists, use it.  If an LSM
hook doesn't exist and the location is generic that other LSMs would
be interested, define a new LSM hook, otherwise define a new IMA hook.

For example, to measure /boot/cmdline, the rule is "measure
func=KEXEC_CMDLINE template=ima-buf".  A similar rule for measuring
keys would look something like "measure func=KEYS template=ima-buf
pcr=<number>".

Remember "ifdef's" don't belong in C code[2].  Normally a stub
function is defined in an include file to avoid ifdefs.

Mimi

[1] Documentation/ABI/testing/ima_policy
[2] Refer to Documentation/process/coding-style.rst section "21)
Conditional Compilation".

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

On 9/19/19 10:12 AM, Mimi Zohar wrote:

Hi Mimi,

> On 9/1 I commented on this patch set from a technical perspective,
> saying: >
> IMA measures, appraises, and audits files based on policy[1].  If
> you're going to measure keys, all of the code should be within the IMA
> subdirectory.  The only code outside of the IMA subdirectory is either
> an LSM or IMA hook.  If an LSM hook already exists, use it.  If an LSM
> hook doesn't exist and the location is generic that other LSMs would
> be interested, define a new LSM hook, otherwise define a new IMA hook.

I am having trouble addressing the above feedback. Appreciate if you 
could provide guidance:

The key(s) in the trusted keys keyring (builtin, secondary, etc.) are 
added early in the kernel boot process. But IMA is initialized later.
If I have a LSM\IMA hook, that gets called when key(s) are added to the 
trusted keys keyring, I won't be able to invoke IMA on "key add" since 
IMA is not yet initialized.

Right now, I have the key measurement function in ima_init. I can gate 
that based on policy (similar to how Prakhar has done kexec_cmdline 
measurement) and follow the coding guidelines you have pointed to.
But it would still have to call keyring function to get the list of keys 
in the trusted keys keyring.

Are you fine if I take the above approach?

If not, could you please suggest a better way to handle it that meets 
the kernel layering guidelines?

Thanks,
  -lakshmi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

On Fri, 2019-10-04 at 12:29 -0700, Lakshmi Ramasubramanian wrote:
> On 9/19/19 10:12 AM, Mimi Zohar wrote:
> 
> Hi Mimi,
> 
> > On 9/1 I commented on this patch set from a technical perspective,
> > saying: >
> > IMA measures, appraises, and audits files based on policy[1].  If
> > you're going to measure keys, all of the code should be within the IMA
> > subdirectory.  The only code outside of the IMA subdirectory is either
> > an LSM or IMA hook.  If an LSM hook already exists, use it.  If an LSM
> > hook doesn't exist and the location is generic that other LSMs would
> > be interested, define a new LSM hook, otherwise define a new IMA hook.
> 
> I am having trouble addressing the above feedback. Appreciate if you 
> could provide guidance:
> 
> The key(s) in the trusted keys keyring (builtin, secondary, etc.) are 
> added early in the kernel boot process. But IMA is initialized later.
> If I have a LSM\IMA hook, that gets called when key(s) are added to the 
> trusted keys keyring, I won't be able to invoke IMA on "key add" since 
> IMA is not yet initialized.
> 
> Right now, I have the key measurement function in ima_init. I can gate 
> that based on policy (similar to how Prakhar has done kexec_cmdline 
> measurement) and follow the coding guidelines you have pointed to.
> But it would still have to call keyring function to get the list of keys 
> in the trusted keys keyring.
> 
> Are you fine if I take the above approach?
> 
> If not, could you please suggest a better way to handle it that meets 
> the kernel layering guidelines?

IMA is late because it is waiting for the TPM to be available.
 Another option would be to queue the measurements and then replay
them once the TPM and IMA are available.

I'm not sure I like this approach any better.

Mimi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

On 10/4/19 12:57 PM, Mimi Zohar wrote:

> 
> IMA is late because it is waiting for the TPM to be available.
 >
> Another option would be to queue the measurements and then replay
> them once the TPM and IMA are available.
> 
> I'm not sure I like this approach any better.

I agree - I too don't like this approach (queue the measurements and 
then replay). Even in that approach IMA will have to invoke functions 
outside of IMA to retrieve the stored measurements.

I prefer gathering data on trusted keys in ima_init, but gate it by IMA 
policy and follow the other coding guidelines you have suggested earlier 
(similar to the approach taken for kexec_cmdline measurement).

Please let me know if you agree - I can send the new patch set by next week.

Thanks,
  -lakshmi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

On Fri, 2019-10-04 at 13:10 -0700, Lakshmi Ramasubramanian wrote:
> On 10/4/19 12:57 PM, Mimi Zohar wrote:
> 
> > 
> > IMA is late because it is waiting for the TPM to be available.
>  >
> > Another option would be to queue the measurements and then replay
> > them once the TPM and IMA are available.
> > 
> > I'm not sure I like this approach any better.
> 
> I agree - I too don't like this approach (queue the measurements and 
> then replay). Even in that approach IMA will have to invoke functions 
> outside of IMA to retrieve the stored measurements.

The measurements could be added to an IMA pending measurement
workqueue, until the TPM is enabled, assuming there is a TPM, and then
processed.  All of this code would be within IMA.

> 
> I prefer gathering data on trusted keys in ima_init, but gate it by IMA 
> policy and follow the other coding guidelines you have suggested earlier 
> (similar to the approach taken for kexec_cmdline measurement).

So your intention is only to measure the initial keys added to these
keyrings, not anything subsequently added to the secondary keyring?

> Please let me know if you agree - I can send the new patch set by next week.

Defining an LSM/IMA hook to measure keys, based on policy, seems
cleaner and more useful.

Mimi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

On 10/4/19 2:58 PM, Mimi Zohar wrote:

> The measurements could be added to an IMA pending measurement
> workqueue, until the TPM is enabled, assuming there is a TPM, and then
> processed.  All of this code would be within IMA.

Good point. I will look into this.

>> I prefer gathering data on trusted keys in ima_init, but gate it by IMA
>> policy and follow the other coding guidelines you have suggested earlier
>> (similar to the approach taken for kexec_cmdline measurement).
> 
> So your intention is only to measure the initial keys added to these
> keyrings, not anything subsequently added to the secondary keyring?

I am currently measuring only the initial keys. But I think including 
the ones added subsequently is a good idea.

> Defining an LSM/IMA hook to measure keys, based on policy, seems
> cleaner and more useful.

I agree.

thanks,
  -lakshmi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Mimi Zohar]

On Fri, 2019-10-04 at 17:10 -0700, Lakshmi Ramasubramanian wrote:
> On 10/4/19 2:58 PM, Mimi Zohar wrote:
> 
> > The measurements could be added to an IMA pending measurement
> > workqueue, until the TPM is enabled, assuming there is a TPM, and then
> > processed.  All of this code would be within IMA.
> 
> Good point. I will look into this.
> 
> >> I prefer gathering data on trusted keys in ima_init, but gate it by IMA
> >> policy and follow the other coding guidelines you have suggested earlier
> >> (similar to the approach taken for kexec_cmdline measurement).
> > 
> > So your intention is only to measure the initial keys added to these
> > keyrings, not anything subsequently added to the secondary keyring?
> 
> I am currently measuring only the initial keys. But I think including 
> the ones added subsequently is a good idea.
> 
> > Defining an LSM/IMA hook to measure keys, based on policy, seems
> > cleaner and more useful.
> 
> I agree.

As defining an early IMA workqueue and measuring keys are independent
of each other, they should be posted, reviewed, and upstreamed as
separate patch sets.

Mimi

Re: [PATCH 0/1] KEYS: Measure keys in trusted keyring

[Lakshmi Ramasubramanian]

On 10/6/19 6:17 AM, Mimi Zohar wrote:

> As defining an early IMA workqueue and measuring keys are independent
> of each other, they should be posted, reviewed, and upstreamed as
> separate patch sets.
> 
> Mimi
> 

Yes - I'll create separate patch sets for those changes.

thanks,
  -lakshmi