linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/10] KEYS: Measure keys when they are created or updated
@ 2019-11-11 19:32 Lakshmi Ramasubramanian
  2019-11-11 19:32 ` [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys Lakshmi Ramasubramanian
                   ` (10 more replies)
  0 siblings, 11 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

Problem Statement:

Keys created or updated in the system are currently not being measured.
Therefore an attestation service, for instance, would not be able to
attest whether or not the trusted keys keyring(s), for instance, contain
only known good (trusted) keys.

IMA measures system files, command line arguments passed to kexec,
boot aggregate, etc. It can be used to measure keys as well.
But there is no mechanism available in the kernel for IMA to
know when a key is created or updated.

This change aims to address measuring keys created or updated
in the system:

  => Patches #1 through #5 update IMA policy functions to handle
     measurement of keys based on configured IMA policy.

  => Patches #6 and #7 add IMA hook for measuring keys and the call
     to the IMA hook from key_create_or_update function.
     Keys are processed immediately - no support for
     deferred processing.

  => Patches #8 through #10 add support for queuing keys if
     custom IMA policies have not been applied yet and process
     the queued keys when custom IMA policies are applied.

To achieve the above the following changes have been made:

 - Added a new IMA hook namely, ima_post_key_create_or_update, which
   measures the key. This IMA hook is called from key_create_or_update
   function. The key measurement can be controlled through IMA policy.

   In this change set a new IMA policy function KEYRING_CHECK has been
   added to measure keys. The policy can optionally specify a set of
   keyrings to measure. By default all keyrings are included in
   the measurement when KEYRING_CHECK policy is specified.

   # 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

Testing performed:

  * Booted the kernel with this change.
  * Executed keyctl tests from the Linux Test Project (LTP)
  * All keys are measured when only KEYRING_CHECK is set.
  * Only keys added to the given keyrings are measured
    when keyrings option is set.
  * Keys are not measured when KEYRING_CHECK is not set.
  * Key is queued for measurement if IMA is not yet initialized
    and processed when IMA is initialized.
  * Key is measured rightaway when IMA is initialized.
  * Added a new key to a keyring and verified "key create" code path.
    => In this case added a key to .ima keyring.
  * Added the same key again and verified "key update" code path.
    => Add the same key to .ima keyring.

Change Log:

  v5:

  => Reorganized the patches to add measurement of keys through
     the IMA hook without any queuing and then added queuing support.
  => Updated the queuing functions to minimize code executed inside mutex.
  => Process queued keys after custom IMA policies have been applied.

  v4:

  => Rebased the changes to v5.4-rc3
  => Applied the following dependent patch set first
     and then added new changes.
  https://lore.kernel.org/linux-integrity/1572492694-6520-1-git-send-email-zohar@linux.ibm.com
  => Refactored the patch set to separate out changes related to
     func KEYRING_CHECK and options keyrings into different patches.
  => Moved the functions to queue and dequeue keys for measurement
     from ima_queue.c to a new file ima_asymmetric_keys.c.
  => Added a new config namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
     to compile ima_asymmetric_keys.c

  v3:

  => Added KEYRING_CHECK for measuring keys. This can optionally specify
     keyrings to measure.
  => Updated ima_get_action() and related functions to return
     the keyrings if specified in the policy.
  => process_buffer_measurement() function is updated to take keyring
     as a parameter. The key will be measured if the policy includes
     the keyring in the list of measured keyrings. If the policy does not
     specify any keyrings then all keys are measured.

  v2:

  => Per suggestion from Mimi reordered the patch set to first
     enable measuring keys added or updated in the system.
     And, then scope the measurement to keys added to 
     builtin_trusted_keys keyring through ima policy.
  => Removed security_key_create_or_update function and instead
     call ima hook, to measure the key, directly from 
     key_create_or_update function.

  v1:

  => LSM function for key_create_or_update. It calls ima.
  => Added ima hook for measuring keys
  => ima measures keys based on ima policy.

  v0:

  => Added LSM hook for key_create_or_update.
  => Measure keys added to builtin or secondary trusted keys keyring.

Background:

Currently ima measures file hashes and .ima signatures. ima signatures
are validated against keys in the ".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.

Although ima supports the above configuration, not having an insight
into what keys are present in these trusted keys keyrings would prevent
an attestation service from validating a client machine.
 
On systems with CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
enabled, measuring keys in the  ".builtin_trusted_keys" 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 change, 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.

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 .builtin_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 ".builtin_trusted_keys"),
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 ".builtin_trusted_keys"


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
    => Built-In trusted key attests IMA key on the client
    => Attestation service attests the Built-In 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
".builtin_trusted_keys"

Lakshmi Ramasubramanian (10):
  IMA: Added KEYRING_CHECK func in IMA policy to measure keys
  IMA: Added keyrings= option in IMA policy to only measure keys added
    to the specified keyrings.
  IMA: Read keyrings= option from the IMA policy into ima_rule_entry
  IMA: Updated IMA policy functions to return keyrings option read from
    the policy
  IMA: Measure key if the IMA policy allows measurement for the keyring
    to which the key is linked to
  IMA: Defined an IMA hook to measure keys on key create or update
  KEYS: Call the IMA hook to measure key when a new key is created or an
    existing key is updated
  IMA: Added a flag to determine whether IMA hook can process the key
    now or has to queue for processing later
  IMA: Defined functions to queue and dequeue keys for measurement
  IMA: Call queue and dequeue functions to measure keys.

 Documentation/ABI/testing/ima_policy         |  16 +-
 include/linux/ima.h                          |  13 ++
 security/integrity/ima/Kconfig               |  14 ++
 security/integrity/ima/Makefile              |   1 +
 security/integrity/ima/ima.h                 |  32 ++-
 security/integrity/ima/ima_api.c             |   8 +-
 security/integrity/ima/ima_appraise.c        |   4 +-
 security/integrity/ima/ima_asymmetric_keys.c | 201 +++++++++++++++++++
 security/integrity/ima/ima_main.c            |  31 ++-
 security/integrity/ima/ima_policy.c          |  53 ++++-
 security/keys/key.c                          |   9 +
 11 files changed, 366 insertions(+), 16 deletions(-)
 create mode 100644 security/integrity/ima/ima_asymmetric_keys.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
@ 2019-11-11 19:32 ` Lakshmi Ramasubramanian
  2019-11-12 17:04   ` Mimi Zohar
  2019-11-11 19:32 ` [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings Lakshmi Ramasubramanian
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

IMA policy needs to support a func to enable measurement of
asymmetric keys.

This patch defines a new IMA policy func namely KEYRING_CHECK to
measure asymmetric keys.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy | 6 ++++++
 security/integrity/ima/ima.h         | 1 +
 security/integrity/ima/ima_policy.c  | 4 +++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 29aaedf33246..341df49b5ad1 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -30,6 +30,7 @@ Description:
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
 				[KEXEC_CMDLINE]
+				[KEYRING_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
@@ -113,3 +114,8 @@ Description:
 		Example of appraise rule allowing modsig appended signatures:
 
 			appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
+
+		Example of measure rule using KEYRING_CHECK to measure
+		all keys:
+
+			measure func=KEYRING_CHECK
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df4ca482fb53..7f23405b2718 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -193,6 +193,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
 	hook(KEXEC_CMDLINE)		\
+	hook(KEYRING_CHECK)		\
 	hook(MAX_CHECK)
 #define __ima_hook_enumify(ENUM)	ENUM,
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index f19a895ad7cd..9ca32ffaaa9d 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -373,7 +373,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 {
 	int i;
 
-	if (func == KEXEC_CMDLINE) {
+	if ((func == KEXEC_CMDLINE) || (func == KEYRING_CHECK)) {
 		if ((rule->flags & IMA_FUNC) && (rule->func == func))
 			return true;
 		return false;
@@ -997,6 +997,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = POLICY_CHECK;
 			else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
 				entry->func = KEXEC_CMDLINE;
+			else if (strcmp(args[0].from, "KEYRING_CHECK") == 0)
+				entry->func = KEYRING_CHECK;
 			else
 				result = -EINVAL;
 			if (!result)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings.
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
  2019-11-11 19:32 ` [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys Lakshmi Ramasubramanian
@ 2019-11-11 19:32 ` Lakshmi Ramasubramanian
  2019-11-12 17:05   ` Mimi Zohar
  2019-11-11 19:32 ` [PATCH v5 03/10] IMA: Read keyrings= option from the IMA policy into ima_rule_entry Lakshmi Ramasubramanian
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

IMA policy needs to support measuring only those keys linked to
a specific set of keyrings.

This patch defines a new IMA policy option namely "keyrings=" that
can be used to specify a set of keyrings. If this option is specified
in the policy for func=KEYRING_CHECK then only the keys linked to
the keyrings given in "keyrings=" option are measured.

If "keyrings=" option is not specified for func=KEYRING_CHECK then
all keys are measured.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy | 10 +++++++++-
 security/integrity/ima/ima_policy.c  |  2 ++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 341df49b5ad1..be2874fa3928 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,7 +25,7 @@ Description:
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [template=] [permit_directio]
-				[appraise_flag=]
+				[appraise_flag=] [keyrings=]
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
@@ -43,6 +43,9 @@ Description:
 			appraise_flag:= [check_blacklist]
 			Currently, blacklist check is only for files signed with appended
 			signature.
+			keyrings:= list of keyrings
+			(eg, .builtin_trusted_keys|.ima). Only valid
+			when action is "measure" and func is KEYRING_CHECK.
 			template:= name of a defined IMA template type
 			(eg, ima-ng). Only valid when action is "measure".
 			pcr:= decimal value
@@ -119,3 +122,8 @@ Description:
 		all keys:
 
 			measure func=KEYRING_CHECK
+
+		Example of measure rule using KEYRING_CHECK to only measure
+		keys added to .builtin_trusted_keys or .ima keyring:
+
+			measure func=KEYRING_CHECK keyrings=.builtin_trusted_keys|.ima
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 9ca32ffaaa9d..a0f7ffa80736 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -34,6 +34,7 @@
 #define IMA_EUID	0x0080
 #define IMA_PCR		0x0100
 #define IMA_FSNAME	0x0200
+#define IMA_KEYRINGS	0x0400
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -79,6 +80,7 @@ struct ima_rule_entry {
 		int type;	/* audit type */
 	} lsm[MAX_LSM_RULES];
 	char *fsname;
+	char *keyrings; /* Measure keys added to these keyrings */
 	struct ima_template_desc *template;
 };
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 03/10] IMA: Read keyrings= option from the IMA policy into ima_rule_entry
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
  2019-11-11 19:32 ` [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys Lakshmi Ramasubramanian
  2019-11-11 19:32 ` [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings Lakshmi Ramasubramanian
@ 2019-11-11 19:32 ` Lakshmi Ramasubramanian
  2019-11-11 19:32 ` [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy Lakshmi Ramasubramanian
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

"keyrings=" option, if specified in the IMA policy, needs to be
stored in the list of IMA rules when the configured IMA policy is read.

This patch defines a new policy token enum namely Opt_keyrings
for reading "keyrings=" option from the IMA policy.

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

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a0f7ffa80736..1aee3c8b9cf6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -768,7 +768,8 @@ enum {
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
 	Opt_appraise_type, Opt_appraise_flag,
-	Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
+	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
+	Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -804,6 +805,7 @@ static const match_table_t policy_tokens = {
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
+	{Opt_keyrings, "keyrings=%s"},
 	{Opt_err, NULL}
 };
 
@@ -1053,6 +1055,23 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			result = 0;
 			entry->flags |= IMA_FSNAME;
 			break;
+		case Opt_keyrings:
+			ima_log_string(ab, "keyrings", args[0].from);
+
+			if ((entry->keyrings) ||
+			    (entry->action != MEASURE) ||
+			    (entry->func != KEYRING_CHECK)) {
+				result = -EINVAL;
+				break;
+			}
+			entry->keyrings = kstrdup(args[0].from, GFP_KERNEL);
+			if (!entry->keyrings) {
+				result = -ENOMEM;
+				break;
+			}
+			result = 0;
+			entry->flags |= IMA_KEYRINGS;
+			break;
 		case Opt_fsuuid:
 			ima_log_string(ab, "fsuuid", args[0].from);
 
@@ -1428,6 +1447,13 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_KEYRINGS) {
+		if (entry->keyrings != NULL)
+			snprintf(tbuf, sizeof(tbuf), "%s", entry->keyrings);
+		seq_printf(m, pt(Opt_keyrings), tbuf);
+		seq_puts(m, " ");
+	}
+
 	if (entry->flags & IMA_PCR) {
 		snprintf(tbuf, sizeof(tbuf), "%d", entry->pcr);
 		seq_printf(m, pt(Opt_pcr), tbuf);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (2 preceding siblings ...)
  2019-11-11 19:32 ` [PATCH v5 03/10] IMA: Read keyrings= option from the IMA policy into ima_rule_entry Lakshmi Ramasubramanian
@ 2019-11-11 19:32 ` Lakshmi Ramasubramanian
  2019-11-12 17:05   ` Mimi Zohar
  2019-11-11 19:32 ` [PATCH v5 05/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the key is linked to Lakshmi Ramasubramanian
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

keyrings option read from the IMA policy needs to be provided to
the callers that determine the action to be performed.

This patch updates ima_get_action() and ima_match_policy() functions
to return the keyrings option specified in the IMA policy.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima.h          | 6 ++++--
 security/integrity/ima/ima_api.c      | 8 +++++---
 security/integrity/ima/ima_appraise.c | 2 +-
 security/integrity/ima/ima_main.c     | 5 +++--
 security/integrity/ima/ima_policy.c   | 7 ++++++-
 5 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7f23405b2718..387829afb9a2 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -208,7 +208,8 @@ struct modsig;
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr,
-		   struct ima_template_desc **template_desc);
+		   struct ima_template_desc **template_desc,
+		   char **keyrings);
 int ima_must_measure(struct inode *inode, int mask, enum ima_hooks func);
 int ima_collect_measurement(struct integrity_iint_cache *iint,
 			    struct file *file, void *buf, loff_t size,
@@ -235,7 +236,8 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
 /* IMA policy related functions */
 int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 		     enum ima_hooks func, int mask, int flags, int *pcr,
-		     struct ima_template_desc **template_desc);
+		     struct ima_template_desc **template_desc,
+		     char **keyrings);
 void ima_init_policy(void);
 void ima_update_policy(void);
 void ima_update_policy_flag(void);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 610759fe63b8..fa2cd71ddf1a 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -169,12 +169,13 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  * @func: caller identifier
  * @pcr: pointer filled in if matched measure policy sets pcr=
  * @template_desc: pointer filled in if matched measure policy sets template=
+ * @keyrings: pointer filled in if matched measure policy sets keyrings=
  *
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
  *	subj,obj, and type: are LSM specific.
  *	func: FILE_CHECK | BPRM_CHECK | CREDS_CHECK | MMAP_CHECK | MODULE_CHECK
- *	| KEXEC_CMDLINE
+ *	| KEXEC_CMDLINE | KEYRING_CHECK
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
@@ -183,14 +184,15 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr,
-		   struct ima_template_desc **template_desc)
+		   struct ima_template_desc **template_desc,
+		   char **keyrings)
 {
 	int flags = IMA_MEASURE | IMA_AUDIT | IMA_APPRAISE | IMA_HASH;
 
 	flags &= ima_policy_flag;
 
 	return ima_match_policy(inode, cred, secid, func, mask, flags, pcr,
-				template_desc);
+				template_desc, keyrings);
 }
 
 /*
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 300c8d2943c5..47ad4f56c0a8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -55,7 +55,7 @@ int ima_must_appraise(struct inode *inode, int mask, enum ima_hooks func)
 
 	security_task_getsecid(current, &secid);
 	return ima_match_policy(inode, current_cred(), secid, func, mask,
-				IMA_APPRAISE | IMA_HASH, NULL, NULL);
+				IMA_APPRAISE | IMA_HASH, NULL, NULL, NULL);
 }
 
 static int ima_fix_xattr(struct dentry *dentry,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d7e987baf127..68e15ff1fe8d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -215,7 +215,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 	 * Included is the appraise submask.
 	 */
 	action = ima_get_action(inode, cred, secid, mask, func, &pcr,
-				&template_desc);
+				&template_desc, NULL);
 	violation_check = ((func == FILE_CHECK || func == MMAP_CHECK) &&
 			   (ima_policy_flag & IMA_MEASURE));
 	if (!action && !violation_check)
@@ -647,6 +647,7 @@ void process_buffer_measurement(const void *buf, int size,
 					    .buf = buf,
 					    .buf_len = size};
 	struct ima_template_desc *template = NULL;
+	char *keyrings = NULL;
 	struct {
 		struct ima_digest_data hdr;
 		char digest[IMA_MAX_DIGEST_SIZE];
@@ -665,7 +666,7 @@ void process_buffer_measurement(const void *buf, int size,
 	if (func) {
 		security_task_getsecid(current, &secid);
 		action = ima_get_action(NULL, current_cred(), secid, 0, func,
-					&pcr, &template);
+					&pcr, &template, &keyrings);
 		if (!(action & IMA_MEASURE))
 			return;
 	}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1aee3c8b9cf6..d1889eee9287 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -481,6 +481,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
  * @pcr: set the pcr to extend
  * @template_desc: the template that should be used for this rule
+ * @keyrings: set the keyrings for this rule, if specified
  *
  * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
  * conditions.
@@ -491,7 +492,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
  */
 int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 		     enum ima_hooks func, int mask, int flags, int *pcr,
-		     struct ima_template_desc **template_desc)
+		     struct ima_template_desc **template_desc,
+		     char **keyrings)
 {
 	struct ima_rule_entry *entry;
 	int action = 0, actmask = flags | (flags << 1);
@@ -527,6 +529,9 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 		if ((pcr) && (entry->flags & IMA_PCR))
 			*pcr = entry->pcr;
 
+		if ((keyrings) && (entry->flags & IMA_KEYRINGS))
+			*keyrings = entry->keyrings;
+
 		if (template_desc && entry->template)
 			*template_desc = entry->template;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 05/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the key is linked to
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (3 preceding siblings ...)
  2019-11-11 19:32 ` [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy Lakshmi Ramasubramanian
@ 2019-11-11 19:32 ` Lakshmi Ramasubramanian
  2019-11-11 19:32 ` [PATCH v5 06/10] IMA: Defined an IMA hook to measure keys on key create or update Lakshmi Ramasubramanian
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

process_buffer_measurement() needs to check if the keyring to which
the given key is linked to is listed in the keyrings option in
the IMA policy.

This patch adds a new parameter "keyring" to process_buffer_measurement().

If process_buffer_measurement() is called with func KEYRING_CHECK and
the name of the keyring to which the key is linked to, then the given
key is measured if:
  1, IMA policy did not specify "keyrings=" option.
  2, Or, the given keyring name is listed in the "keyrings=" option.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima.h          |  2 +-
 security/integrity/ima/ima_appraise.c |  2 +-
 security/integrity/ima/ima_main.c     | 26 ++++++++++++++++++++++++--
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 387829afb9a2..f15199f7ff2a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -221,7 +221,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct ima_template_desc *template_desc);
 void process_buffer_measurement(const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr);
+				int pcr, const char *keyring);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 47ad4f56c0a8..a9649b04b9f1 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -330,7 +330,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
 		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
 			process_buffer_measurement(digest, digestsize,
 						   "blacklisted-hash", NONE,
-						   pcr);
+						   pcr, NULL);
 	}
 
 	return rc;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 68e15ff1fe8d..1bd5dbd8a077 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -632,12 +632,22 @@ int ima_load_data(enum kernel_load_data_id id)
  * @eventname: event name to be used for the buffer entry.
  * @func: IMA hook
  * @pcr: pcr to extend the measurement
+ * @keyring: keyring for the measurement
+ *
+ *	The following scenarios are possible with respect to
+ *	the parameter "keyring":
+ *	1, keyring is NULL. In this case buffer is measured.
+ *	2, keyring is not NULL, but ima_get_action returned
+ *	   a NULL keyrings. In this case also the buffer is measured.
+ *	3, keyring is not NULL and ima_get_action returned
+ *	   a non-NULL keyrings. In this case measure the buffer
+ *	   only if the given keyring is present in the keyrings.
  *
  * Based on policy, the buffer is measured into the ima log.
  */
 void process_buffer_measurement(const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr)
+				int pcr, const char *keyring)
 {
 	int ret = 0;
 	struct ima_template_entry *entry = NULL;
@@ -656,6 +666,13 @@ void process_buffer_measurement(const void *buf, int size,
 	int action = 0;
 	u32 secid;
 
+	/*
+	 * If IMA is not yet initialized or IMA policy is empty
+	 * then there is no need to measure.
+	 */
+	if (!ima_policy_flag)
+		return;
+
 	/*
 	 * Both LSM hooks and auxilary based buffer measurements are
 	 * based on policy.  To avoid code duplication, differentiate
@@ -671,6 +688,11 @@ void process_buffer_measurement(const void *buf, int size,
 			return;
 	}
 
+	if ((keyring != NULL) && (keyrings != NULL)
+	    && (strstr(keyrings, keyring) == NULL)) {
+		return;
+	}
+
 	if (!pcr)
 		pcr = CONFIG_IMA_MEASURE_PCR_IDX;
 
@@ -719,7 +741,7 @@ void ima_kexec_cmdline(const void *buf, int size)
 {
 	if (buf && size != 0)
 		process_buffer_measurement(buf, size, "kexec-cmdline",
-					   KEXEC_CMDLINE, 0);
+					   KEXEC_CMDLINE, 0, NULL);
 }
 
 static int __init init_ima(void)
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 06/10] IMA: Defined an IMA hook to measure keys on key create or update
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (4 preceding siblings ...)
  2019-11-11 19:32 ` [PATCH v5 05/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the key is linked to Lakshmi Ramasubramanian
@ 2019-11-11 19:32 ` Lakshmi Ramasubramanian
  2019-11-11 19:33 ` [PATCH v5 07/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key is updated Lakshmi Ramasubramanian
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:32 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

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.

The IMA hook is defined in a new file namely ima_asymmetric_keys.c

Note that currently IMA subsystem can be enabled without
enabling KEYS subsystem.

Adding support for measuring asymmetric keys in IMA requires KEYS
subsystem to be enabled. To handle this dependency a new config
namely CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS has been added. Enabling
this config requires the following configs to be enabled:
    CONFIG_IMA, CONFIG_KEYS, CONFIG_ASYMMETRIC_KEY_TYPE, and
    CONFIG_ASYMMETRIC_PUBLIC_KEY_SUBTYPE.

The new file ima_asymmetric_keys.c is built only if
CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS is enabled.

This config is turned off by default.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/Kconfig               | 14 +++++++
 security/integrity/ima/Makefile              |  1 +
 security/integrity/ima/ima_asymmetric_keys.c | 44 ++++++++++++++++++++
 3 files changed, 59 insertions(+)
 create mode 100644 security/integrity/ima/ima_asymmetric_keys.c

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 838476d780e5..c6d14884bc19 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -310,3 +310,17 @@ config IMA_APPRAISE_SIGNED_INIT
 	default n
 	help
 	   This option requires user-space init to be signed.
+
+config IMA_MEASURE_ASYMMETRIC_KEYS
+	bool "Enable measuring asymmetric keys on key create or update"
+	depends on IMA
+	depends on KEYS
+	depends on ASYMMETRIC_KEY_TYPE
+	depends on ASYMMETRIC_PUBLIC_KEY_SUBTYPE
+	default n
+	help
+	   This option enables measuring asymmetric keys when
+	   the key is created or updated. Additionally, IMA policy
+	   needs to be configured to either measure keys linked to
+	   any keyring or only measure keys linked to the keyrings
+	   specified in the IMA policy through the keyrings= option.
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 31d57cdf2421..3e9d0ad68c7b 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -12,3 +12,4 @@ ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
 ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
 ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
 obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
+obj-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
new file mode 100644
index 000000000000..7d6603bfcc06
--- /dev/null
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian (nramas@linux.microsoft.com)
+ *
+ * File: ima_asymmetric_keys.c
+ *       Defines an IMA hook to measure asymmetric keys on key
+ *       create or update.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <crypto/public_key.h>
+#include <keys/asymmetric-type.h>
+#include "ima.h"
+
+/**
+ * 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)
+{
+	const struct public_key *pk;
+
+	/* Only asymmetric keys are handled */
+	if (key->type != &key_type_asymmetric)
+		return;
+
+	/*
+	 * Get the public_key of the given asymmetric key to measure.
+	 */
+	pk = key->payload.data[asym_crypto];
+	process_buffer_measurement(pk->key, pk->keylen,
+				   keyring->description,
+				   KEYRING_CHECK, 0,
+				   keyring->description);
+}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 07/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key is updated
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (5 preceding siblings ...)
  2019-11-11 19:32 ` [PATCH v5 06/10] IMA: Defined an IMA hook to measure keys on key create or update Lakshmi Ramasubramanian
@ 2019-11-11 19:33 ` Lakshmi Ramasubramanian
  2019-11-11 19:33 ` [PATCH v5 08/10] IMA: Added a flag to determine whether IMA hook can process the key now or has to queue for processing later Lakshmi Ramasubramanian
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

key_create_or_update function needs to call the IMA hook to measure
the key when a new key is created or an existing key is updated.

This patch adds the call to the IMA hook from key_create_or_update
function.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 include/linux/ima.h | 13 +++++++++++++
 security/keys/key.c |  9 +++++++++
 2 files changed, 22 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 6d904754d858..ec5afe319ab7 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,6 +25,12 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 extern void ima_post_path_mknod(struct dentry *dentry);
 extern void ima_kexec_cmdline(const void *buf, int size);
 
+#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);
+#endif
+
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
@@ -101,6 +107,13 @@ static inline void ima_add_kexec_buffer(struct kimage *image)
 {}
 #endif
 
+#ifndef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
+static inline void ima_post_key_create_or_update(struct key *keyring,
+						 struct key *key,
+						 unsigned long flags,
+						 bool create) {}
+#endif
+
 #ifdef CONFIG_IMA_APPRAISE
 extern bool is_ima_appraise_enabled(void);
 extern void ima_inode_post_setattr(struct dentry *dentry);
diff --git a/security/keys/key.c b/security/keys/key.c
index 764f4c57913e..9782d4d046fd 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -13,6 +13,7 @@
 #include <linux/security.h>
 #include <linux/workqueue.h>
 #include <linux/random.h>
+#include <linux/ima.h>
 #include <linux/err.h>
 #include "internal.h"
 
@@ -936,6 +937,9 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 		goto error_link_end;
 	}
 
+	/* let the ima module know about the created key. */
+	ima_post_key_create_or_update(keyring, key, flags, true);
+
 	key_ref = make_key_ref(key, is_key_possessed(keyring_ref));
 
 error_link_end:
@@ -965,6 +969,11 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	}
 
 	key_ref = __key_update(key_ref, &prep);
+
+	/* let the ima module know about the updated key. */
+	if (!IS_ERR(key_ref))
+		ima_post_key_create_or_update(keyring, key, flags, false);
+
 	goto error_free_prep;
 }
 EXPORT_SYMBOL(key_create_or_update);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 08/10] IMA: Added a flag to determine whether IMA hook can process the key now or has to queue for processing later
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (6 preceding siblings ...)
  2019-11-11 19:33 ` [PATCH v5 07/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key is updated Lakshmi Ramasubramanian
@ 2019-11-11 19:33 ` Lakshmi Ramasubramanian
  2019-11-11 19:33 ` [PATCH v5 09/10] IMA: Defined functions to queue and dequeue keys for measurement Lakshmi Ramasubramanian
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

Keys should be processed only if custom IMA policies have been
applied. Prior to that the keys should be queued for processing later.

This patch defines a flag namely ima_process_keys_for_measurement
to check if the key should be processed immediately or should be queued.

ima_policy_flag cannot be relied upon because ima_policy_flag will
be set to 0 when either IMA is not initialized or the IMA policy
itself is empty.

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

diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 7d6603bfcc06..61c42d06a636 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -15,6 +15,8 @@
 #include <keys/asymmetric-type.h>
 #include "ima.h"
 
+bool ima_process_keys_for_measurement;
+
 /**
  * ima_post_key_create_or_update - measure asymmetric keys
  * @keyring: keyring to which the key is linked to
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 09/10] IMA: Defined functions to queue and dequeue keys for measurement
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (7 preceding siblings ...)
  2019-11-11 19:33 ` [PATCH v5 08/10] IMA: Added a flag to determine whether IMA hook can process the key now or has to queue for processing later Lakshmi Ramasubramanian
@ 2019-11-11 19:33 ` Lakshmi Ramasubramanian
  2019-11-11 19:33 ` [PATCH v5 10/10] IMA: Call queue and dequeue functions to measure keys Lakshmi Ramasubramanian
  2019-11-11 19:41 ` [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
  10 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

A key can be measured right away only if custom IMA policies
have been applied. Otherwise, the key should be queued up and
processed when custom IMA policies have been applied.

This patch defines functions to queue and dequeue keys for
measurement.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima.h                 |  23 ++++
 security/integrity/ima/ima_asymmetric_keys.c | 128 +++++++++++++++++++
 2 files changed, 151 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f15199f7ff2a..4e7fed8d224e 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -205,6 +205,29 @@ extern const char *const func_tokens[];
 
 struct modsig;
 
+#ifdef CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
+/*
+ * To track keys that need to be measured.
+ */
+struct ima_measure_key_entry {
+	struct list_head list;
+	void *public_key;
+	u32  public_key_len;
+	char *keyring_name;
+};
+
+bool ima_queue_key_for_measurement(struct key *keyring,
+				   struct key *key);
+void ima_process_queued_keys_for_measurement(void);
+#else
+static inline bool ima_queue_key_for_measurement(struct key *keyring,
+						 struct key *key)
+{
+	return false;
+}
+static inline void ima_process_queued_keys_for_measurement(void) {}
+#endif /* CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS */
+
 /* LIM API function definitions */
 int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
 		   int mask, enum ima_hooks func, int *pcr,
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 61c42d06a636..4a38b4957b8c 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -7,6 +7,7 @@
  * File: ima_asymmetric_keys.c
  *       Defines an IMA hook to measure asymmetric keys on key
  *       create or update.
+ *       Queue and de-queue functions for measuring asymmetric keys.
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -17,6 +18,133 @@
 
 bool ima_process_keys_for_measurement;
 
+/*
+ * To synchronize access to the list of keys that need to be measured
+ */
+static DEFINE_MUTEX(ima_measure_keys_mutex);
+static LIST_HEAD(ima_measure_keys);
+
+static void ima_free_measure_key_entry(struct ima_measure_key_entry *entry)
+{
+	if (entry != NULL) {
+		if (entry->public_key != NULL)
+			kzfree(entry->public_key);
+		if (entry->keyring_name != NULL)
+			kzfree(entry->keyring_name);
+		kzfree(entry);
+	}
+}
+
+static struct ima_measure_key_entry *ima_alloc_measure_key_entry(
+	struct key *keyring,
+	struct key *key)
+{
+	int rc = 0;
+	const struct public_key *pk;
+	size_t keyring_name_len;
+	struct ima_measure_key_entry *entry = NULL;
+
+	pk = key->payload.data[asym_crypto];
+	keyring_name_len = strlen(keyring->description) + 1;
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (entry != NULL) {
+		entry->public_key = kzalloc(pk->keylen, GFP_KERNEL);
+		entry->keyring_name =
+			kzalloc(keyring_name_len, GFP_KERNEL);
+	}
+
+	if ((entry == NULL) || (entry->public_key == NULL) ||
+	    (entry->keyring_name == NULL)) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	strcpy(entry->keyring_name, keyring->description);
+	memcpy(entry->public_key, pk->key, pk->keylen);
+	entry->public_key_len = pk->keylen;
+	rc = 0;
+
+out:
+	if (rc) {
+		ima_free_measure_key_entry(entry);
+		entry = NULL;
+	}
+
+	return entry;
+}
+
+bool ima_queue_key_for_measurement(struct key *keyring,
+				   struct key *key)
+{
+	bool queued = false;
+	struct ima_measure_key_entry *entry = NULL;
+
+	/*
+	 * ima_measure_keys_mutex should be taken before checking
+	 * ima_process_keys_for_measurement flag to avoid the race
+	 * condition between the IMA hook checking this flag and
+	 * calling ima_queue_key_for_measurement() to queue the key and
+	 * ima_process_queued_keys_for_measurement() setting this flag.
+	 */
+	mutex_lock(&ima_measure_keys_mutex);
+
+	if (!ima_process_keys_for_measurement) {
+		entry = ima_alloc_measure_key_entry(keyring, key);
+		if (entry != NULL) {
+			INIT_LIST_HEAD(&entry->list);
+			list_add_tail(&entry->list, &ima_measure_keys);
+			queued = true;
+		}
+	}
+
+	mutex_unlock(&ima_measure_keys_mutex);
+
+	return queued;
+}
+
+void ima_process_queued_keys_for_measurement(void)
+{
+	struct ima_measure_key_entry *entry, *tmp;
+	LIST_HEAD(temp_ima_measure_keys);
+
+	if (ima_process_keys_for_measurement)
+		return;
+
+	/*
+	 * Any queued keys will be processed now. From here on
+	 * keys should be processed right away.
+	 */
+	ima_process_keys_for_measurement = true;
+
+	/*
+	 * To avoid holding the mutex when processing queued keys,
+	 * transfer the queued keys with the mutex held to a temp list,
+	 * release the mutex, and then process the queued keys from
+	 * the temp list.
+	 *
+	 * Since ima_process_keys_for_measurement is set to true above,
+	 * any new key will be processed immediately and not be queued.
+	 */
+	INIT_LIST_HEAD(&temp_ima_measure_keys);
+	mutex_lock(&ima_measure_keys_mutex);
+	list_for_each_entry_safe(entry, tmp, &ima_measure_keys, list) {
+		list_del(&entry->list);
+		list_add_tail(&entry->list, &temp_ima_measure_keys);
+	}
+	mutex_unlock(&ima_measure_keys_mutex);
+
+	list_for_each_entry_safe(entry, tmp,
+				 &temp_ima_measure_keys, list) {
+		process_buffer_measurement(entry->public_key,
+					   entry->public_key_len,
+					   entry->keyring_name,
+					   KEYRING_CHECK, 0,
+					   entry->keyring_name);
+		list_del(&entry->list);
+		ima_free_measure_key_entry(entry);
+	}
+}
+
 /**
  * ima_post_key_create_or_update - measure asymmetric keys
  * @keyring: keyring to which the key is linked to
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v5 10/10] IMA: Call queue and dequeue functions to measure keys.
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (8 preceding siblings ...)
  2019-11-11 19:33 ` [PATCH v5 09/10] IMA: Defined functions to queue and dequeue keys for measurement Lakshmi Ramasubramanian
@ 2019-11-11 19:33 ` Lakshmi Ramasubramanian
  2019-11-11 19:41 ` [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
  10 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:33 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

Keys should be queued for measurement if custom IMA policies have
not been applied. Keys queued for measurement, if any, need to be
processed when custom IMA policies have been applied.

This patch adds the call to ima_queue_key_for_measurement in the IMA hook
function and the call to ima_process_queued_keys_for_measurement when
custom IMA policies have been applied.

NOTE:
If the kernel is built with CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS
then the IMA policy for measuring keys should be applied as part of
custom IMA policies. Keys will be queued up until custom policies
are applied and processed when applied.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_asymmetric_keys.c | 31 ++++++++++++++++++--
 security/integrity/ima/ima_policy.c          | 12 ++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 4a38b4957b8c..fab1d4672715 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -158,15 +158,42 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 				   unsigned long flags, bool create)
 {
 	const struct public_key *pk;
+	bool key_queued = false;
 
-	/* Only asymmetric keys are handled */
+	/* Only asymmetric keys are handled by this hook. */
 	if (key->type != &key_type_asymmetric)
 		return;
 
+	if (!ima_process_keys_for_measurement)
+		key_queued = ima_queue_key_for_measurement(keyring, key);
+
 	/*
-	 * Get the public_key of the given asymmetric key to measure.
+	 * Need to check again if the key was queued or not because
+	 * ima_process_keys_for_measurement could have flipped from
+	 * false to true after it was checked above, but before the key
+	 * could be queued by ima_queue_key_for_measurement().
 	 */
+	if (key_queued)
+		return;
+
+	/* Public key of the given asymmetric key is measured. */
 	pk = key->payload.data[asym_crypto];
+
+	/*
+	 * keyring->description points to the name of the keyring
+	 * (such as ".builtin_trusted_keys", ".ima", etc.) to
+	 * which the given key is linked to.
+	 *
+	 * The name of the keyring is passed in the "eventname"
+	 * parameter to process_buffer_measurement() and is set
+	 * in the "eventname" field in ima_event_data for
+	 * the key measurement IMA event.
+	 *
+	 * The name of the keyring is also passed in the "keyring"
+	 * parameter to process_buffer_measurement() to check
+	 * if the IMA policy is configured to measure a key linked
+	 * to the given keyring.
+	 */
 	process_buffer_measurement(pk->key, pk->keylen,
 				   keyring->description,
 				   KEYRING_CHECK, 0,
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index d1889eee9287..d130bdcbc174 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -759,6 +759,18 @@ void ima_update_policy(void)
 		kfree(arch_policy_entry);
 	}
 	ima_update_policy_flag();
+
+	/*
+	 * Custom IMA policies have been setup.
+	 * Process key(s) queued up for measurement now.
+	 *
+	 * NOTE:
+	 *   Custom IMA policies always overwrite builtin policies
+	 *   (policies compiled in code). If one wants measurement
+	 *   of asymmetric keys then it has to be configured in
+	 *   custom policies and updated here.
+	 */
+	ima_process_queued_keys_for_measurement();
 }
 
 /* Keep the enumeration in sync with the policy_tokens! */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 0/10] KEYS: Measure keys when they are created or updated
  2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
                   ` (9 preceding siblings ...)
  2019-11-11 19:33 ` [PATCH v5 10/10] IMA: Call queue and dequeue functions to measure keys Lakshmi Ramasubramanian
@ 2019-11-11 19:41 ` Lakshmi Ramasubramanian
  2019-11-12 17:08   ` Mimi Zohar
  10 siblings, 1 reply; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-11 19:41 UTC (permalink / raw)
  To: zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

On 11/11/2019 11:32 AM, Lakshmi Ramasubramanian wrote:

Hi Mimi,

> Problem Statement:
> 
> Keys created or updated in the system are currently not being measured.
> 
> This change aims to address measuring keys created or updated
> in the system:
> 
>    => Patches #1 through #5 update IMA policy functions to handle
>       measurement of keys based on configured IMA policy.
> 
>    => Patches #6 and #7 add IMA hook for measuring keys and the call
>       to the IMA hook from key_create_or_update function.
>       Keys are processed immediately - no support for
>       deferred processing.
> 
>    => Patches #8 through #10 add support for queuing keys if
>       custom IMA policies have not been applied yet and process
>       the queued keys when custom IMA policies are applied.

I was wondering if it'd be better to split this patch set into two sets:

1st set including the patches for measuring keys without queuing support 
(Patches #1 through #7)

2nd set including the patches that add queuing support (Patches #8 
through #10).

thanks,
  -lakshmi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys
  2019-11-11 19:32 ` [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys Lakshmi Ramasubramanian
@ 2019-11-12 17:04   ` Mimi Zohar
  2019-11-12 17:37     ` Lakshmi Ramasubramanian
  0 siblings, 1 reply; 22+ messages in thread
From: Mimi Zohar @ 2019-11-12 17:04 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
	jamorris, linux-integrity, linux-security-module, keyrings,
	linux-kernel

On Mon, 2019-11-11 at 11:32 -0800, Lakshmi Ramasubramanian wrote:
> IMA policy needs to support a func to enable measurement of
> asymmetric keys.
> 
> This patch defines a new IMA policy func namely KEYRING_CHECK to
> measure asymmetric keys.

This new feature measures "keys" based on policy, not "keyrings".
 Please change the name to KEY_CHECK.

> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  Documentation/ABI/testing/ima_policy | 6 ++++++
>  security/integrity/ima/ima.h         | 1 +
>  security/integrity/ima/ima_policy.c  | 4 +++-
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 29aaedf33246..341df49b5ad1 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -30,6 +30,7 @@ Description:
>  				[FIRMWARE_CHECK]
>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>  				[KEXEC_CMDLINE]
> +				[KEYRING_CHECK]
>  			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>  			       [[^]MAY_EXEC]
>  			fsmagic:= hex value
> @@ -113,3 +114,8 @@ Description:
>  		Example of appraise rule allowing modsig appended signatures:
>  
>  			appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
> +
> +		Example of measure rule using KEYRING_CHECK to measure
> +		all keys:
> +
> +			measure func=KEYRING_CHECK
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index df4ca482fb53..7f23405b2718 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -193,6 +193,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
>  	hook(KEXEC_INITRAMFS_CHECK)	\
>  	hook(POLICY_CHECK)		\
>  	hook(KEXEC_CMDLINE)		\
> +	hook(KEYRING_CHECK)		\
>  	hook(MAX_CHECK)
>  #define __ima_hook_enumify(ENUM)	ENUM,
>  
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index f19a895ad7cd..9ca32ffaaa9d 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -373,7 +373,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
>  {
>  	int i;
>  
> -	if (func == KEXEC_CMDLINE) {
> +	if ((func == KEXEC_CMDLINE) || (func == KEYRING_CHECK)) {
>  		if ((rule->flags & IMA_FUNC) && (rule->func == func))
>  			return true;
>  		return false;
> @@ -997,6 +997,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  				entry->func = POLICY_CHECK;
>  			else if (strcmp(args[0].from, "KEXEC_CMDLINE") == 0)
>  				entry->func = KEXEC_CMDLINE;
> +			else if (strcmp(args[0].from, "KEYRING_CHECK") == 0)
> +				entry->func = KEYRING_CHECK;
>  			else
>  				result = -EINVAL;
>  			if (!result)





^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings.
  2019-11-11 19:32 ` [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings Lakshmi Ramasubramanian
@ 2019-11-12 17:05   ` Mimi Zohar
  2019-11-12 17:43     ` Lakshmi Ramasubramanian
  0 siblings, 1 reply; 22+ messages in thread
From: Mimi Zohar @ 2019-11-12 17:05 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
	jamorris, linux-integrity, linux-security-module, keyrings,
	linux-kernel

The C maximum line length is 80 characters.  The subject line is even
less than that (~50).  The Subject line here could be "ima: add
support to limit measuring keys".

On Mon, 2019-11-11 at 11:32 -0800, Lakshmi Ramasubramanian wrote:
> IMA policy needs to support measuring only those keys linked to
> a specific set of keyrings.

Patch descriptions should be written in the imperative.  For example, 
"Limit measuring keys to those keys being loaded onto a specific
keyring."

> 
> This patch defines a new IMA policy option namely "keyrings=" that
> can be used to specify a set of keyrings. If this option is specified
> in the policy for func=KEYRING_CHECK then only the keys linked to
> the keyrings given in "keyrings=" option are measured.

This description does not seem to match the code, which for some
reason isn't included in this patch, nor in 3/10.  Please
combine/squash patches 2 & 3.  Missing from the combined patch is the
keyring matching code in ima_match_rules().

> 
> If "keyrings=" option is not specified for func=KEYRING_CHECK then
> all keys are measured.

The last sentence is unnecessary.  Please remove.

> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
>  Documentation/ABI/testing/ima_policy | 10 +++++++++-
>  security/integrity/ima/ima_policy.c  |  2 ++
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 341df49b5ad1..be2874fa3928 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,7 +25,7 @@ Description:
>  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
>  				 [obj_user=] [obj_role=] [obj_type=]]
>  			option:	[[appraise_type=]] [template=] [permit_directio]
> -				[appraise_flag=]
> +				[appraise_flag=] [keyrings=]
>  		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>  				[FIRMWARE_CHECK]
>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> @@ -43,6 +43,9 @@ Description:
>  			appraise_flag:= [check_blacklist]
>  			Currently, blacklist check is only for files signed with appended
>  			signature.
> +			keyrings:= list of keyrings
> +			(eg, .builtin_trusted_keys|.ima). Only valid
> +			when action is "measure" and func is KEYRING_CHECK.
>  			template:= name of a defined IMA template type
>  			(eg, ima-ng). Only valid when action is "measure".
>  			pcr:= decimal value
> @@ -119,3 +122,8 @@ Description:
>  		all keys:
>  
>  			measure func=KEYRING_CHECK
> +
> +		Example of measure rule using KEYRING_CHECK to only measure
> +		keys added to .builtin_trusted_keys or .ima keyring:
> +
> +			measure func=KEYRING_CHECK keyrings=.builtin_trusted_keys|.ima
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 9ca32ffaaa9d..a0f7ffa80736 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -34,6 +34,7 @@
>  #define IMA_EUID	0x0080
>  #define IMA_PCR		0x0100
>  #define IMA_FSNAME	0x0200
> +#define IMA_KEYRINGS	0x0400
>  
>  #define UNKNOWN		0
>  #define MEASURE		0x0001	/* same as IMA_MEASURE */
> @@ -79,6 +80,7 @@ struct ima_rule_entry {
>  		int type;	/* audit type */
>  	} lsm[MAX_LSM_RULES];
>  	char *fsname;
> +	char *keyrings; /* Measure keys added to these keyrings */
>  	struct ima_template_desc *template;
>  };
>  


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy
  2019-11-11 19:32 ` [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy Lakshmi Ramasubramanian
@ 2019-11-12 17:05   ` Mimi Zohar
  2019-11-12 17:47     ` Lakshmi Ramasubramanian
  0 siblings, 1 reply; 22+ messages in thread
From: Mimi Zohar @ 2019-11-12 17:05 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
	jamorris, linux-integrity, linux-security-module, keyrings,
	linux-kernel

On Mon, 2019-11-11 at 11:32 -0800, Lakshmi Ramasubramanian wrote:
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 1aee3c8b9cf6..d1889eee9287 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -481,6 +481,7 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
>   * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
>   * @pcr: set the pcr to extend
>   * @template_desc: the template that should be used for this rule
> + * @keyrings: set the keyrings for this rule, if specified
>   *
>   * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
>   * conditions.
> @@ -491,7 +492,8 @@ static int get_subaction(struct ima_rule_entry *rule, enum ima_hooks func)
>   */
>  int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
>  		     enum ima_hooks func, int mask, int flags, int *pcr,
> -		     struct ima_template_desc **template_desc)
> +		     struct ima_template_desc **template_desc,
> +		     char **keyrings)
>  {
>  	struct ima_rule_entry *entry;
>  	int action = 0, actmask = flags | (flags << 1);
> @@ -527,6 +529,9 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
>  		if ((pcr) && (entry->flags & IMA_PCR))
>  			*pcr = entry->pcr;
>  
> +		if ((keyrings) && (entry->flags & IMA_KEYRINGS))
> +			*keyrings = entry->keyrings;

ima_match_rules() determines whether the rule is in policy or not. It
returns true on rule match, false on failure.  There's no need to
return the list of keyrings.

Mimi 

> +
>  		if (template_desc && entry->template)
>  			*template_desc = entry->template;
>  


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 0/10] KEYS: Measure keys when they are created or updated
  2019-11-11 19:41 ` [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
@ 2019-11-12 17:08   ` Mimi Zohar
  2019-11-12 17:52     ` Lakshmi Ramasubramanian
  0 siblings, 1 reply; 22+ messages in thread
From: Mimi Zohar @ 2019-11-12 17:08 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
	jamorris, linux-integrity, linux-security-module, keyrings,
	linux-kernel

On Mon, 2019-11-11 at 11:41 -0800, Lakshmi Ramasubramanian wrote:
> On 11/11/2019 11:32 AM, Lakshmi Ramasubramanian wrote:
> 
> Hi Mimi,
> 
> > Problem Statement:

The above line isn't needed.

> > 
> > Keys created or updated in the system are currently not being measured.
> > 
> > This change aims to address measuring keys created or updated
> > in the system:
> > 
> >    => Patches #1 through #5 update IMA policy functions to handle
> >       measurement of keys based on configured IMA policy.
> > 
> >    => Patches #6 and #7 add IMA hook for measuring keys and the call
> >       to the IMA hook from key_create_or_update function.
> >       Keys are processed immediately - no support for
> >       deferred processing.
> > 
> >    => Patches #8 through #10 add support for queuing keys if
> >       custom IMA policies have not been applied yet and process
> >       the queued keys when custom IMA policies are applied.
> 
> I was wondering if it'd be better to split this patch set into two sets:
> 
> 1st set including the patches for measuring keys without queuing support 
> (Patches #1 through #7)

I've commented on patches 1 - 4.  There's still so much wrong with
this patch set.  Limiting the scope of the patch set sounds like
really a good idea. 

Mimi

> 
> 2nd set including the patches that add queuing support (Patches #8 
> through #10).



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys
  2019-11-12 17:04   ` Mimi Zohar
@ 2019-11-12 17:37     ` Lakshmi Ramasubramanian
  0 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-12 17:37 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

On 11/12/2019 9:04 AM, Mimi Zohar wrote:

> On Mon, 2019-11-11 at 11:32 -0800, Lakshmi Ramasubramanian wrote:
>> IMA policy needs to support a func to enable measurement of
>> asymmetric keys.
>>
>> This patch defines a new IMA policy func namely KEYRING_CHECK to
>> measure asymmetric keys.
> 
> This new feature measures "keys" based on policy, not "keyrings".
>   Please change the name to KEY_CHECK.

Good point - I will change the func name to KEY_CHECK.

  -lakshmi


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings.
  2019-11-12 17:05   ` Mimi Zohar
@ 2019-11-12 17:43     ` Lakshmi Ramasubramanian
  2019-11-12 17:58       ` Mimi Zohar
  0 siblings, 1 reply; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-12 17:43 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

On 11/12/2019 9:05 AM, Mimi Zohar wrote:

> The C maximum line length is 80 characters.  The subject line is even
> less than that (~50).  The Subject line here could be "ima: add
> support to limit measuring keys".
I'll update the subject line for the patches - limit to max 50 chars.

> 
> On Mon, 2019-11-11 at 11:32 -0800, Lakshmi Ramasubramanian wrote:
>> IMA policy needs to support measuring only those keys linked to
>> a specific set of keyrings.
> 
> Patch descriptions should be written in the imperative.  For example,
> "Limit measuring keys to those keys being loaded onto a specific
> keyring."
Will update.

> 
>>
>> This patch defines a new IMA policy option namely "keyrings=" that
>> can be used to specify a set of keyrings. If this option is specified
>> in the policy for func=KEYRING_CHECK then only the keys linked to
>> the keyrings given in "keyrings=" option are measured.
> 
> This description does not seem to match the code, which for some
> reason isn't included in this patch, nor in 3/10.  Please
> combine/squash patches 2 & 3.  Missing from the combined patch is the
> keyring matching code in ima_match_rules().

This patch defines "keyrings=" option in the IMA policy and adds the 
related field in ima_rule_entry struct.

The code for updating the new field in ima_rule_entry is in patch #4
[PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings 
option read from the policy

I'll update the description for this patch (#2).

  -lakshmi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy
  2019-11-12 17:05   ` Mimi Zohar
@ 2019-11-12 17:47     ` Lakshmi Ramasubramanian
  2019-11-12 18:06       ` Mimi Zohar
  0 siblings, 1 reply; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-12 17:47 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

On 11/12/2019 9:05 AM, Mimi Zohar wrote:

>>   int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
>>   		     enum ima_hooks func, int mask, int flags, int *pcr,
>> -		     struct ima_template_desc **template_desc)
>> +		     struct ima_template_desc **template_desc,
>> +		     char **keyrings)
>>   {
>>   	struct ima_rule_entry *entry;
>>   	int action = 0, actmask = flags | (flags << 1);
>> @@ -527,6 +529,9 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
>>   		if ((pcr) && (entry->flags & IMA_PCR))
>>   			*pcr = entry->pcr;
>>   
>> +		if ((keyrings) && (entry->flags & IMA_KEYRINGS))
>> +			*keyrings = entry->keyrings;
> 
> ima_match_rules() determines whether the rule is in policy or not. It
> returns true on rule match, false on failure.  There's no need to
> return the list of keyrings.

But the above code change is in ima_match_policy() - not in 
ima_match_rules() function.

ima_match_rules() function is updated in Patch #1 -
[PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys

I've updated that function to check if func is "KEYRING_CHECK" and 
return true\false as appropriate.

Am I missing something?

  -lakshmi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 0/10] KEYS: Measure keys when they are created or updated
  2019-11-12 17:08   ` Mimi Zohar
@ 2019-11-12 17:52     ` Lakshmi Ramasubramanian
  0 siblings, 0 replies; 22+ messages in thread
From: Lakshmi Ramasubramanian @ 2019-11-12 17:52 UTC (permalink / raw)
  To: Mimi Zohar, dhowells, matthewgarrett, sashal, jamorris,
	linux-integrity, linux-security-module, keyrings, linux-kernel

On 11/12/2019 9:08 AM, Mimi Zohar wrote:

> On Mon, 2019-11-11 at 11:41 -0800, Lakshmi Ramasubramanian wrote:
>> On 11/11/2019 11:32 AM, Lakshmi Ramasubramanian wrote:
>>
>> Hi Mimi,
>>
>>> Problem Statement:
> 
> The above line isn't needed.
Will update.

> I've commented on patches 1 - 4.  There's still so much wrong with
> this patch set.  Limiting the scope of the patch set sounds like
> really a good idea.
> 
> Mimi

I'll address your comments and send an update - I'll split this into 2 
patch sets.

thanks,
  -lakshmi


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings.
  2019-11-12 17:43     ` Lakshmi Ramasubramanian
@ 2019-11-12 17:58       ` Mimi Zohar
  0 siblings, 0 replies; 22+ messages in thread
From: Mimi Zohar @ 2019-11-12 17:58 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
	jamorris, linux-integrity, linux-security-module, keyrings,
	linux-kernel

On Tue, 2019-11-12 at 09:43 -0800, Lakshmi Ramasubramanian wrote:
> On 11/12/2019 9:05 AM, Mimi Zohar wrote:
> 
> > The C maximum line length is 80 characters.  The subject line is even
> > less than that (~50).  The Subject line here could be "ima: add
> > support to limit measuring keys".
> I'll update the subject line for the patches - limit to max 50 chars.
> 
> > 
> > On Mon, 2019-11-11 at 11:32 -0800, Lakshmi Ramasubramanian wrote:
> >> IMA policy needs to support measuring only those keys linked to
> >> a specific set of keyrings.
> > 
> > Patch descriptions should be written in the imperative.  For example,
> > "Limit measuring keys to those keys being loaded onto a specific
> > keyring."
> Will update.
> 
> > 
> >>
> >> This patch defines a new IMA policy option namely "keyrings=" that
> >> can be used to specify a set of keyrings. If this option is specified
> >> in the policy for func=KEYRING_CHECK then only the keys linked to
> >> the keyrings given in "keyrings=" option are measured.
> > 
> > This description does not seem to match the code, which for some
> > reason isn't included in this patch, nor in 3/10.  Please
> > combine/squash patches 2 & 3.  Missing from the combined patch is the
> > keyring matching code in ima_match_rules().
> 
> This patch defines "keyrings=" option in the IMA policy and adds the 
> related field in ima_rule_entry struct.
> 
> The code for updating the new field in ima_rule_entry is in patch #4
> [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings 
> option read from the policy

That's the problem.  The keyrings doesn't need to be returned, but
processed in ima_match_rules().

Mimi


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy
  2019-11-12 17:47     ` Lakshmi Ramasubramanian
@ 2019-11-12 18:06       ` Mimi Zohar
  0 siblings, 0 replies; 22+ messages in thread
From: Mimi Zohar @ 2019-11-12 18:06 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, dhowells, matthewgarrett, sashal,
	jamorris, linux-integrity, linux-security-module, keyrings,
	linux-kernel

On Tue, 2019-11-12 at 09:47 -0800, Lakshmi Ramasubramanian wrote:
> On 11/12/2019 9:05 AM, Mimi Zohar wrote:
> 
> >>   int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> >>   		     enum ima_hooks func, int mask, int flags, int *pcr,
> >> -		     struct ima_template_desc **template_desc)
> >> +		     struct ima_template_desc **template_desc,
> >> +		     char **keyrings)
> >>   {
> >>   	struct ima_rule_entry *entry;
> >>   	int action = 0, actmask = flags | (flags << 1);
> >> @@ -527,6 +529,9 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
> >>   		if ((pcr) && (entry->flags & IMA_PCR))
> >>   			*pcr = entry->pcr;
> >>   
> >> +		if ((keyrings) && (entry->flags & IMA_KEYRINGS))
> >> +			*keyrings = entry->keyrings;
> > 
> > ima_match_rules() determines whether the rule is in policy or not. It
> > returns true on rule match, false on failure.  There's no need to
> > return the list of keyrings.
> 
> But the above code change is in ima_match_policy() - not in 
> ima_match_rules() function.
> 
> ima_match_rules() function is updated in Patch #1 -
> [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys
> 
> I've updated that function to check if func is "KEYRING_CHECK" and 
> return true\false as appropriate.
> 
> Am I missing something?

The first patch adds basic support for the new "func".  This allows
measuring all keys.  ima_match_rules() then needs to be updated in the
patch that adds the "keyrings=" or "keyring=" support to limit it to a
specific keyring.

Mimi


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2019-11-12 18:06 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-11 19:32 [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
2019-11-11 19:32 ` [PATCH v5 01/10] IMA: Added KEYRING_CHECK func in IMA policy to measure keys Lakshmi Ramasubramanian
2019-11-12 17:04   ` Mimi Zohar
2019-11-12 17:37     ` Lakshmi Ramasubramanian
2019-11-11 19:32 ` [PATCH v5 02/10] IMA: Added keyrings= option in IMA policy to only measure keys added to the specified keyrings Lakshmi Ramasubramanian
2019-11-12 17:05   ` Mimi Zohar
2019-11-12 17:43     ` Lakshmi Ramasubramanian
2019-11-12 17:58       ` Mimi Zohar
2019-11-11 19:32 ` [PATCH v5 03/10] IMA: Read keyrings= option from the IMA policy into ima_rule_entry Lakshmi Ramasubramanian
2019-11-11 19:32 ` [PATCH v5 04/10] IMA: Updated IMA policy functions to return keyrings option read from the policy Lakshmi Ramasubramanian
2019-11-12 17:05   ` Mimi Zohar
2019-11-12 17:47     ` Lakshmi Ramasubramanian
2019-11-12 18:06       ` Mimi Zohar
2019-11-11 19:32 ` [PATCH v5 05/10] IMA: Measure key if the IMA policy allows measurement for the keyring to which the key is linked to Lakshmi Ramasubramanian
2019-11-11 19:32 ` [PATCH v5 06/10] IMA: Defined an IMA hook to measure keys on key create or update Lakshmi Ramasubramanian
2019-11-11 19:33 ` [PATCH v5 07/10] KEYS: Call the IMA hook to measure key when a new key is created or an existing key is updated Lakshmi Ramasubramanian
2019-11-11 19:33 ` [PATCH v5 08/10] IMA: Added a flag to determine whether IMA hook can process the key now or has to queue for processing later Lakshmi Ramasubramanian
2019-11-11 19:33 ` [PATCH v5 09/10] IMA: Defined functions to queue and dequeue keys for measurement Lakshmi Ramasubramanian
2019-11-11 19:33 ` [PATCH v5 10/10] IMA: Call queue and dequeue functions to measure keys Lakshmi Ramasubramanian
2019-11-11 19:41 ` [PATCH v5 0/10] KEYS: Measure keys when they are created or updated Lakshmi Ramasubramanian
2019-11-12 17:08   ` Mimi Zohar
2019-11-12 17:52     ` Lakshmi Ramasubramanian

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).