[PATCH v4 0/6] IMA: Infrastructure for measurement of critical kernel data

[PATCH v4 0/6] IMA: Infrastructure for measurement of critical kernel data

[Tushar Sugandhi]

There are several kernel components that contain critical data which if
accidentally or maliciously altered, can compromise the security of the
kernel. Example of such components would include LSMs like SELinux, or
AppArmor; or device-mapper targets like dm-crypt, dm-verity etc.

Many of these components do not use the capabilities provided by kernel
integrity subsystem (IMA), and thus they don't use the benefits of
extended TPM PCR quotes and ultimately the benefits of remote attestation.

This series bridges this gap, so that potential kernel components that
contain data critical to the security of the kernel could take advantage
of IMA's measuring and quoting abilities - thus ultimately enabling
remote attestation for their specific data.

System administrators may want to pick and choose which kernel
components they would want to enable for measurements, quoting, and
remote attestation. To enable that, a new IMA policy is introduced.

And lastly, the functionality is exposed through a function
ima_measure_critical_data(). The functionality is generic enough to
measure the data of any kernel component at run-time. To ensure that
only data from supported sources are measured, the kernel component
needs to be added to a compile-time list of supported sources (an
"allowed list of components"). IMA validates the source passed to
ima_measure_critical_data() against this allowed list at run-time.

This series is based on the following repo/branch:

 repo: https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git
 branch: next-integrity
 commit aa662fc04f5b ("ima: Fix NULL pointer dereference in ima_file_hash")
        
Change Log v4:
Incorporated feedback from Mimi on v3.
 - Split patch #1 into two patches to move introduction of bool
   allow_empty_opt_list into the 2nd patch.
 - Reverted return type of process_buffer_measurement() from int to void
   which got rid of patch #2 from the v3 of the series.
 - Renamed the policy "critical_kernel_data_sources" to "data_sources".
 - Updated process_buffer_measurement() to avoid code and variable
   duplication in the if(measure_buf_hash) block.
 - Changed return type of ima_measure_critical_data() from int to void.
 - Updated patch description for patch #3 and #4 as per Mimi's feedback.

Change Log v3:
Incorporated feedback from Mimi on v2.
 - Renamed the policy "data_sources" to
   "critical_kernel_data_sources".
 - Added "critical_kernel_data_sources" description in
   Documentation/ima-policy.
 - Split CRITICAL_DATA + critical_kernel_data_sources into two separate
   patches.
 - Merged hook ima_measure_critical_data() + CRITICAL_DATA into a single
   patch.
 - Added functionality to validate data sources before measurement.

Change Log v2:
 - Reverted the unnecessary indentations in existing #define.
 - Updated the description to replace the word 'enlightened' with
   'supported'.
 - Reverted the unnecessary rename of attribute size to buf_len.
 - Introduced a boolean parameter measure_buf_hash as per community
   feedback to support measuring hash of the buffer, instead of the
   buffer itself.


Tushar Sugandhi (6):
  IMA: generalize keyring specific measurement constructs
  IMA: conditionally allow empty rule data
  IMA: update process_buffer_measurement to measure buffer hash
  IMA: add policy to measure critical data from kernel components
  IMA: add hook to measure critical data from kernel components
  IMA: validate supported kernel data sources before measurement

 Documentation/ABI/testing/ima_policy         |  11 +-
 include/linux/ima.h                          |   8 ++
 security/integrity/ima/ima.h                 |  37 ++++++-
 security/integrity/ima/ima_api.c             |   8 +-
 security/integrity/ima/ima_appraise.c        |   2 +-
 security/integrity/ima/ima_asymmetric_keys.c |   2 +-
 security/integrity/ima/ima_main.c            |  61 ++++++++++-
 security/integrity/ima/ima_policy.c          | 101 +++++++++++++++----
 security/integrity/ima/ima_queue_keys.c      |   3 +-
 9 files changed, 196 insertions(+), 37 deletions(-)

-- 
2.17.1

[PATCH v4 1/6] IMA: generalize keyring specific measurement constructs

[Tushar Sugandhi]

IMA functions such as ima_match_keyring(), process_buffer_measurement(),
ima_match_policy() etc. handle data specific to keyrings. Currently,
these constructs are not generic to handle any func specific data.
This makes it harder to extend without code duplication.

Refactor the keyring specific measurement constructs to be generic and
reusable in other measurement scenarios.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h        |  6 ++---
 security/integrity/ima/ima_api.c    |  6 ++---
 security/integrity/ima/ima_main.c   |  6 ++---
 security/integrity/ima/ima_policy.c | 38 ++++++++++++++++-------------
 4 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 38043074ce5e..8875085db689 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -255,7 +255,7 @@ static inline void ima_process_queued_keys(void) {}
 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,
-		   const char *keyring);
+		   const char *func_data);
 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,
@@ -267,7 +267,7 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct ima_template_desc *template_desc);
 void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr, const char *keyring);
+				int pcr, const char *func_data);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
@@ -283,7 +283,7 @@ const char *ima_d_path(const struct path *path, char **pathbuf, char *filename);
 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,
-		     const char *keyring);
+		     const char *func_data);
 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 4f39fb93f278..af218babd198 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -170,7 +170,7 @@ 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=
- * @keyring: keyring name used to determine the action
+ * @func_data: private data specific to @func, can be NULL.
  *
  * The policy is defined in terms of keypairs:
  *		subj=, obj=, type=, func=, mask=, fsmagic=
@@ -186,14 +186,14 @@ 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,
-		   const char *keyring)
+		   const char *func_data)
 {
 	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, keyring);
+				template_desc, func_data);
 }
 
 /*
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..c870fd6d2f83 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -732,13 +732,13 @@ 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 name to determine the action to be performed
+ * @func_data: private data specific to @func, can be NULL.
  *
  * Based on policy, the buffer is measured into the ima log.
  */
 void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr, const char *keyring)
+				int pcr, const char *func_data)
 {
 	int ret = 0;
 	const char *audit_cause = "ENOMEM";
@@ -770,7 +770,7 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 	if (func) {
 		security_task_getsecid(current, &secid);
 		action = ima_get_action(inode, current_cred(), secid, 0, func,
-					&pcr, &template, keyring);
+					&pcr, &template, func_data);
 		if (!(action & IMA_MEASURE))
 			return;
 	}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index fe1df373c113..31a772d8a86b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -451,15 +451,19 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 }
 
 /**
- * ima_match_keyring - determine whether the keyring matches the measure rule
- * @rule: a pointer to a rule
- * @keyring: name of the keyring to match against the measure rule
+ * ima_match_rule_data - determine whether the given func_data matches
+ *			 the measure rule data
+ * @rule: IMA policy rule
+ * @opt_list: rule data to match func_data against
+ * @func_data: data to match against the measure rule data
  * @cred: a pointer to a credentials structure for user validation
  *
- * Returns true if keyring matches one in the rule, false otherwise.
+ * Returns true if func_data matches one in the rule, false otherwise.
  */
-static bool ima_match_keyring(struct ima_rule_entry *rule,
-			      const char *keyring, const struct cred *cred)
+static bool ima_match_rule_data(struct ima_rule_entry *rule,
+				const struct ima_rule_opt_list *opt_list,
+				const char *func_data,
+				const struct cred *cred)
 {
 	bool matched = false;
 	size_t i;
@@ -467,14 +471,14 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
 	if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
 		return false;
 
-	if (!rule->keyrings)
+	if (!opt_list)
 		return true;
 
-	if (!keyring)
+	if (!func_data)
 		return false;
 
-	for (i = 0; i < rule->keyrings->count; i++) {
-		if (!strcmp(rule->keyrings->items[i], keyring)) {
+	for (i = 0; i < opt_list->count; i++) {
+		if (!strcmp(opt_list->items[i], func_data)) {
 			matched = true;
 			break;
 		}
@@ -491,20 +495,21 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
  * @secid: the secid of the task to be validated
  * @func: LIM hook identifier
  * @mask: requested action (MAY_READ | MAY_WRITE | MAY_APPEND | MAY_EXEC)
- * @keyring: keyring name to check in policy for KEY_CHECK func
+ * @func_data: private data specific to @func, can be NULL.
  *
  * Returns true on rule match, false on failure.
  */
 static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			    const struct cred *cred, u32 secid,
 			    enum ima_hooks func, int mask,
-			    const char *keyring)
+			    const char *func_data)
 {
 	int i;
 
 	if (func == KEY_CHECK) {
 		return (rule->flags & IMA_FUNC) && (rule->func == func) &&
-		       ima_match_keyring(rule, keyring, cred);
+		       ima_match_rule_data(rule, rule->keyrings, func_data,
+					   cred);
 	}
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
@@ -608,8 +613,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
- * @keyring: the keyring name, if given, to be used to check in the policy.
- *           keyring can be NULL if func is anything other than KEY_CHECK.
+ * @func_data: private data specific to @func, can be NULL.
  *
  * Measure decision based on func/mask/fsmagic and LSM(subj/obj/type)
  * conditions.
@@ -621,7 +625,7 @@ 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,
-		     const char *keyring)
+		     const char *func_data)
 {
 	struct ima_rule_entry *entry;
 	int action = 0, actmask = flags | (flags << 1);
@@ -636,7 +640,7 @@ int ima_match_policy(struct inode *inode, const struct cred *cred, u32 secid,
 			continue;
 
 		if (!ima_match_rules(entry, inode, cred, secid, func, mask,
-				     keyring))
+				     func_data))
 			continue;
 
 		action |= entry->flags & IMA_ACTION_FLAGS;
-- 
2.17.1

[PATCH v4 2/6] IMA: conditionally allow empty rule data

[Tushar Sugandhi]

ima_match_rule_data() permits the func to pass empty func_data.
For instance, for the following func, the func_data keyrings= is
optional.
    measure func=KEY_CHECK keyrings=.ima

But a new func in future may want to constrain the func_data to
be non-empty.  ima_match_rule_data() should support this constraint
and it shouldn't be hard-coded in ima_match_rule_data().

Update ima_match_rule_data() to conditionally allow empty func_data
for the func that needs it.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima_policy.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 31a772d8a86b..8866e84d0062 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -456,6 +456,7 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
  * @rule: IMA policy rule
  * @opt_list: rule data to match func_data against
  * @func_data: data to match against the measure rule data
+ * @allow_empty_opt_list: If true matches all func_data
  * @cred: a pointer to a credentials structure for user validation
  *
  * Returns true if func_data matches one in the rule, false otherwise.
@@ -463,6 +464,7 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 static bool ima_match_rule_data(struct ima_rule_entry *rule,
 				const struct ima_rule_opt_list *opt_list,
 				const char *func_data,
+				bool allow_empty_opt_list,
 				const struct cred *cred)
 {
 	bool matched = false;
@@ -472,7 +474,7 @@ static bool ima_match_rule_data(struct ima_rule_entry *rule,
 		return false;
 
 	if (!opt_list)
-		return true;
+		return allow_empty_opt_list;
 
 	if (!func_data)
 		return false;
@@ -509,7 +511,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 	if (func == KEY_CHECK) {
 		return (rule->flags & IMA_FUNC) && (rule->func == func) &&
 		       ima_match_rule_data(rule, rule->keyrings, func_data,
-					   cred);
+					   true, cred);
 	}
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
-- 
2.17.1

[PATCH v4 3/6] IMA: update process_buffer_measurement to measure buffer hash

[Tushar Sugandhi]

process_buffer_measurement() currently only measures the input buffer.
In case of SeLinux policy measurement, the policy being measured could
be large (several MB). This may result in a large entry in IMA
measurement log.

Introduce a boolean parameter measure_buf_hash to support measuring
hash of a buffer, which would be much smaller, instead of the buffer
itself.

To use the functionality introduced in this patch, the attestation
client and the server changes need to go hand in hand. The
client/kernel would know what data is being measured as-is
(e.g. KEXEC_CMDLINE), and what data has it’s hash measured (e.g. SeLinux
Policy). And the attestation server should verify data/hash accordingly.

Just like the data being measured in other cases, the attestation server
will know what are possible values of the large buffers being measured.
e.g. the possible valid SeLinux policy values that are being pushed to
the client. The attestation server will have to maintain the hash of
those buffer values.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h                 |  3 ++-
 security/integrity/ima/ima_appraise.c        |  2 +-
 security/integrity/ima/ima_asymmetric_keys.c |  2 +-
 security/integrity/ima/ima_main.c            | 25 ++++++++++++++++++--
 security/integrity/ima/ima_queue_keys.c      |  3 ++-
 5 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 8875085db689..0f77e0b697a3 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -267,7 +267,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct ima_template_desc *template_desc);
 void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr, const char *func_data);
+				int pcr, const char *func_data,
+				bool measure_buf_hash);
 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 372d16382960..20adffe5bf58 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -336,7 +336,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
 		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
 			process_buffer_measurement(NULL, digest, digestsize,
 						   "blacklisted-hash", NONE,
-						   pcr, NULL);
+						   pcr, NULL, false);
 	}
 
 	return rc;
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index 1c68c500c26f..a74095793936 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -60,5 +60,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
 	 */
 	process_buffer_measurement(NULL, payload, payload_len,
 				   keyring->description, KEY_CHECK, 0,
-				   keyring->description);
+				   keyring->description, false);
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c870fd6d2f83..6888fc372abf 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -733,12 +733,15 @@ int ima_load_data(enum kernel_load_data_id id)
  * @func: IMA hook
  * @pcr: pcr to extend the measurement
  * @func_data: private data specific to @func, can be NULL.
+ * @measure_buf_hash: if set to true - will measure hash of the buf,
+ *                    instead of buf
  *
  * Based on policy, the buffer is measured into the ima log.
  */
 void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 				const char *eventname, enum ima_hooks func,
-				int pcr, const char *func_data)
+				int pcr, const char *func_data,
+				bool measure_buf_hash)
 {
 	int ret = 0;
 	const char *audit_cause = "ENOMEM";
@@ -753,6 +756,8 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 		struct ima_digest_data hdr;
 		char digest[IMA_MAX_DIGEST_SIZE];
 	} hash = {};
+	char digest_hash[IMA_MAX_DIGEST_SIZE];
+	int hash_len = hash_digest_size[ima_hash_algo];
 	int violation = 0;
 	int action = 0;
 	u32 secid;
@@ -801,6 +806,21 @@ void process_buffer_measurement(struct inode *inode, const void *buf, int size,
 		goto out;
 	}
 
+	if (measure_buf_hash) {
+		memcpy(digest_hash, hash.hdr.digest, hash_len);
+
+		ret = ima_calc_buffer_hash(digest_hash,
+					   hash_len,
+					   iint.ima_hash);
+		if (ret < 0) {
+			audit_cause = "measure_buf_hash_error";
+			goto out;
+		}
+
+		event_data.buf = digest_hash;
+		event_data.buf_len = hash_len;
+	}
+
 	ret = ima_alloc_init_template(&event_data, &entry, template);
 	if (ret < 0) {
 		audit_cause = "alloc_entry";
@@ -842,7 +862,8 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 		return;
 
 	process_buffer_measurement(file_inode(f.file), buf, size,
-				   "kexec-cmdline", KEXEC_CMDLINE, 0, NULL);
+				   "kexec-cmdline", KEXEC_CMDLINE, 0, NULL,
+				   false);
 	fdput(f);
 }
 
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index 69a8626a35c0..c2f2ad34f9b7 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -162,7 +162,8 @@ void ima_process_queued_keys(void)
 						   entry->payload_len,
 						   entry->keyring_name,
 						   KEY_CHECK, 0,
-						   entry->keyring_name);
+						   entry->keyring_name,
+						   false);
 		list_del(&entry->list);
 		ima_free_key_entry(entry);
 	}
-- 
2.17.1

[PATCH v4 4/6] IMA: add policy to measure critical data from kernel components

[Tushar Sugandhi]

There would be several candidate kernel components suitable for IMA
measurement. Not all of them would have support for IMA measurement.
Also, system administrators may not want to measure data for all of
them, even when they support IMA measurement. An IMA policy option 
specific to various kernel components is needed to measure their
respective critical data.

This policy option needs to be constrained to measure data for
specific kernel components that are specified as input values to the
policy option.

Add a new IMA policy option - "data_sources:=" to allow measuring
various critical kernel components. This policy option would enable the
system administrators to limit the measurement to the components
listed in "data_sources:=", if the components support IMA measurement.

The new policy option "data_sources:=" is different from the existing
policy option "keyrings:=". 

In case of "keyrings:=", a policy may measure all keyrings (when
"keyrings:=" option is not provided for func KEY_CHECK), or may
constrain which keyrings need to be measured (when "keyrings:=" option
is provided for func KEY_CHECK).

But unlike "keyrings:=", the entries in "data_sources:=" would have
different data format. Further, the components listed in
"data_sources:=" need to be modified to call IMA to measure their
data. Therefore, unlike "keyrings:=", IMA shouldn't measure all of the
components by default, when "data_sources:=" is not specified. Because
measuring non-vetted components just by specifying them as a policy
option value may impact the overall reliability of the system.

To address this, "data_sources:=" should be a mandatory policy option
for func=CRITICAL_DATA. This func is introduced in the 5th patch in this
series). And the compile-time vetting functionality described above is
introduced in the 6th patch in this series.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy |  3 +++
 security/integrity/ima/ima_policy.c  | 29 +++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index cd572912c593..a81cf79fb255 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -48,6 +48,9 @@ Description:
 			template:= name of a defined IMA template type
 			(eg, ima-ng). Only valid when action is "measure".
 			pcr:= decimal value
+			data_sources:= list of kernel components
+			(eg, selinux|apparmor|dm-crypt) that contain data critical
+			to the security of the kernel.
 
 		default policy:
 			# PROC_SUPER_MAGIC
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8866e84d0062..89452245f54a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -33,6 +33,7 @@
 #define IMA_PCR		0x0100
 #define IMA_FSNAME	0x0200
 #define IMA_KEYRINGS	0x0400
+#define IMA_DATA_SOURCES	0x0800
 
 #define UNKNOWN		0
 #define MEASURE		0x0001	/* same as IMA_MEASURE */
@@ -84,6 +85,7 @@ struct ima_rule_entry {
 	} lsm[MAX_LSM_RULES];
 	char *fsname;
 	struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
+	struct ima_rule_opt_list *data_sources; /* Measure data from these sources */
 	struct ima_template_desc *template;
 };
 
@@ -911,7 +913,7 @@ enum {
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
 	Opt_appraise_type, Opt_appraise_flag,
 	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
-	Opt_err
+	Opt_data_sources, Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -948,6 +950,7 @@ static const match_table_t policy_tokens = {
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
 	{Opt_keyrings, "keyrings=%s"},
+	{Opt_data_sources, "data_sources=%s"},
 	{Opt_err, NULL}
 };
 
@@ -1312,6 +1315,24 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 
 			entry->flags |= IMA_KEYRINGS;
 			break;
+		case Opt_data_sources:
+			ima_log_string(ab, "data_sources",
+				       args[0].from);
+
+			if (entry->data_sources) {
+				result = -EINVAL;
+				break;
+			}
+
+			entry->data_sources = ima_alloc_rule_opt_list(args);
+			if (IS_ERR(entry->data_sources)) {
+				result = PTR_ERR(entry->data_sources);
+				entry->data_sources = NULL;
+				break;
+			}
+
+			entry->flags |= IMA_DATA_SOURCES;
+			break;
 		case Opt_fsuuid:
 			ima_log_string(ab, "fsuuid", args[0].from);
 
@@ -1692,6 +1713,12 @@ int ima_policy_show(struct seq_file *m, void *v)
 		seq_puts(m, " ");
 	}
 
+	if (entry->flags & IMA_DATA_SOURCES) {
+		seq_puts(m, "data_sources=");
+		ima_show_rule_opt_list(m, entry->data_sources);
+		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

[PATCH v4 5/6] IMA: add hook to measure critical data from kernel components

[Tushar Sugandhi]

Currently, IMA does not provide a generic function for kernel components
to measure their data. A generic function provided by IMA would
enable various parts of the kernel with easier and faster on-boarding to
use IMA infrastructure, would avoid code duplication, and consistent
usage of IMA policy option "data_sources:=" across the kernel.

Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
ima_measure_critical_data() to support measuring various critical kernel
components. Limit the measurement to the components that are specified
in the IMA policy - CRITICAL_DATA+data_sources.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 Documentation/ABI/testing/ima_policy |  8 ++++++-
 include/linux/ima.h                  |  8 +++++++
 security/integrity/ima/ima.h         |  1 +
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/ima/ima_main.c    | 26 +++++++++++++++++++++
 security/integrity/ima/ima_policy.c  | 34 ++++++++++++++++++++++++----
 6 files changed, 72 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index a81cf79fb255..d33bb51309fc 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,7 +29,7 @@ Description:
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
-				[KEXEC_CMDLINE] [KEY_CHECK]
+				[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
@@ -51,6 +51,8 @@ Description:
 			data_sources:= list of kernel components
 			(eg, selinux|apparmor|dm-crypt) that contain data critical
 			to the security of the kernel.
+			Only valid when action is "measure" and func is
+			CRITICAL_DATA.
 
 		default policy:
 			# PROC_SUPER_MAGIC
@@ -128,3 +130,7 @@ Description:
 		keys added to .builtin_trusted_keys or .ima keyring:
 
 			measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
+
+		Example of measure rule using CRITICAL_DATA to measure critical data
+
+			measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
diff --git a/include/linux/ima.h b/include/linux/ima.h
index d15100de6cdd..4040f484ac63 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -26,6 +26,10 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 extern void ima_post_path_mknod(struct dentry *dentry);
 extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
 extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
+extern void ima_measure_critical_data(const char *event_name,
+				      const char *event_data_source,
+				      const void *buf, int buf_len,
+				      bool measure_buf_hash);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);
@@ -104,6 +108,10 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 }
 
 static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
+static inline void ima_measure_critical_data(const char *event_name,
+					     const char *event_data_source,
+					     const void *buf, int buf_len,
+					     bool measure_buf_hash) {}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 0f77e0b697a3..c1acf88e1b5d 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -200,6 +200,7 @@ static inline unsigned int ima_hash_key(u8 *digest)
 	hook(POLICY_CHECK, policy)			\
 	hook(KEXEC_CMDLINE, kexec_cmdline)		\
 	hook(KEY_CHECK, key)				\
+	hook(CRITICAL_DATA, critical_data)		\
 	hook(MAX_CHECK, none)
 
 #define __ima_hook_enumify(ENUM, str)	ENUM,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index af218babd198..9917e1730cb6 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -176,7 +176,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
  *		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 | KEY_CHECK
+ *	| KEXEC_CMDLINE | KEY_CHECK | CRITICAL_DATA
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6888fc372abf..d55896f28790 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -867,6 +867,32 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
 	fdput(f);
 }
 
+/**
+ * ima_measure_critical_data - measure critical data
+ * @event_name: name for the given data
+ * @event_data_source: name of the event data source
+ * @buf: pointer to buffer containing data to measure
+ * @buf_len: length of buffer(in bytes)
+ * @measure_buf_hash: if set to true - will measure hash of the buf,
+ *                    instead of buf
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_measure_critical_data(const char *event_name,
+			       const char *event_data_source,
+			       const void *buf, int buf_len,
+			       bool measure_buf_hash)
+{
+	if (!event_name || !event_data_source || !buf || !buf_len) {
+		pr_err("Invalid arguments passed to %s().\n", __func__);
+		return;
+	}
+
+	process_buffer_measurement(NULL, buf, buf_len, event_name,
+				   CRITICAL_DATA, 0, event_data_source,
+				   measure_buf_hash);
+}
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 89452245f54a..491017df7589 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -510,14 +510,23 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 {
 	int i;
 
-	if (func == KEY_CHECK) {
-		return (rule->flags & IMA_FUNC) && (rule->func == func) &&
-		       ima_match_rule_data(rule, rule->keyrings, func_data,
-					   true, cred);
-	}
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
 		return false;
+
+	switch (func) {
+	case KEY_CHECK:
+		return ((rule->func == func) &&
+			ima_match_rule_data(rule, rule->keyrings,
+					    func_data, true, cred));
+	case CRITICAL_DATA:
+		return ((rule->func == func) &&
+			ima_match_rule_data(rule, rule->data_sources,
+					    func_data, false, cred));
+	default:
+		break;
+	}
+
 	if ((rule->flags & IMA_MASK) &&
 	    (rule->mask != mask && func != POST_SETATTR))
 		return false;
@@ -1113,6 +1122,19 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 		if (ima_rule_contains_lsm_cond(entry))
 			return false;
 
+		break;
+	case CRITICAL_DATA:
+		if (entry->action & ~(MEASURE | DONT_MEASURE))
+			return false;
+
+		if (!(entry->flags & IMA_DATA_SOURCES) ||
+		    (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+		    IMA_DATA_SOURCES)))
+			return false;
+
+		if (ima_rule_contains_lsm_cond(entry))
+			return false;
+
 		break;
 	default:
 		return false;
@@ -1245,6 +1267,8 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			else if (IS_ENABLED(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) &&
 				 strcmp(args[0].from, "KEY_CHECK") == 0)
 				entry->func = KEY_CHECK;
+			else if (strcmp(args[0].from, "CRITICAL_DATA") == 0)
+				entry->func = CRITICAL_DATA;
 			else
 				result = -EINVAL;
 			if (!result)
-- 
2.17.1

[PATCH v4 6/6] IMA: validate supported kernel data sources before measurement

[Tushar Sugandhi]

Currently, IMA does not restrict random data sources from measuring
their data using ima_measure_critical_data(). Any kernel data source can
call the function, and it's data will get measured as long as the input
event_data_source is part of the IMA policy - RITICAL_DATA+data_sources.
Supporting random data sources at run-time may impact the reliability of
the system.

To ensure that only data from supported sources are measured, the kernel
component needs to be added to a compile-time list of supported sources
(an "allowed list of components"). IMA then validates the input
parameter - "event_data_source" passed to ima_measure_critical_data()
against this allowed list at run-time.

This compile time list must be updated when kernel components are
updated to measure their data using IMA.

Provide an infrastructure for kernel data sources to be added to
IMA's supported data sources list at compile-time. Update
ima_measure_critical_data() to validate, at run-time, that the data
source is supported before measuring the data coming from that source.

Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
---
 security/integrity/ima/ima.h      | 29 +++++++++++++++++++++++++++++
 security/integrity/ima/ima_main.c |  6 ++++++
 2 files changed, 35 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c1acf88e1b5d..4a35db010d91 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -228,6 +228,35 @@ extern const char *const func_tokens[];
 
 struct modsig;
 
+#define __ima_supported_kernel_data_sources(source)	\
+	source(MIN_SOURCE, min_source)			\
+	source(MAX_SOURCE, max_source)
+
+#define __ima_enum_stringify(ENUM, str) (#str),
+
+enum ima_supported_kernel_data_sources {
+	__ima_supported_kernel_data_sources(__ima_hook_enumify)
+};
+
+static const char * const ima_supported_kernel_data_sources_str[] = {
+	__ima_supported_kernel_data_sources(__ima_enum_stringify)
+};
+
+static inline bool ima_kernel_data_source_is_supported(const char *source)
+{
+	int i;
+
+	if (!source)
+		return false;
+
+	for (i = MIN_SOURCE + 1; i < MAX_SOURCE; i++) {
+		if (!strcmp(ima_supported_kernel_data_sources_str[i], source))
+			return true;
+	}
+
+	return false;
+}
+
 #ifdef CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS
 /*
  * To track keys that need to be measured.
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index d55896f28790..61f9642747a8 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -888,6 +888,12 @@ void ima_measure_critical_data(const char *event_name,
 		return;
 	}
 
+	if (!ima_kernel_data_source_is_supported(event_data_source)) {
+		pr_err("measuring data source %s is not permitted",
+		       event_data_source);
+		return;
+	}
+
 	process_buffer_measurement(NULL, buf, buf_len, event_name,
 				   CRITICAL_DATA, 0, event_data_source,
 				   measure_buf_hash);
-- 
2.17.1

Re: [dm-devel] [PATCH v4 1/6] IMA: generalize keyring specific measurement constructs

[Mimi Zohar]

Hi Tushar,

On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:

> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index fe1df373c113..31a772d8a86b 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -451,15 +451,19 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>  }
>  
>  /**
> - * ima_match_keyring - determine whether the keyring matches the measure rule
> - * @rule: a pointer to a rule
> - * @keyring: name of the keyring to match against the measure rule
> + * ima_match_rule_data - determine whether the given func_data matches
> + *			 the measure rule data
> + * @rule: IMA policy rule
> + * @opt_list: rule data to match func_data against
> + * @func_data: data to match against the measure rule data
>   * @cred: a pointer to a credentials structure for user validation
>   *
> - * Returns true if keyring matches one in the rule, false otherwise.
> + * Returns true if func_data matches one in the rule, false otherwise.
>   */
> -static bool ima_match_keyring(struct ima_rule_entry *rule,
> -			      const char *keyring, const struct cred *cred)
> +static bool ima_match_rule_data(struct ima_rule_entry *rule,
> +				const struct ima_rule_opt_list *opt_list,
> +				const char *func_data,
> +				const struct cred *cred)
>  {
>  	bool matched = false;
>  	size_t i;
> @@ -467,14 +471,14 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
>  	if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
>  		return false;
>  
> -	if (!rule->keyrings)
> +	if (!opt_list)
>  		return true;

The opt_list should be based on rule->func.  There shouldn't be a need
to pass it as a variable.

Mimi

>  
> -	if (!keyring)
> +	if (!func_data)
>  		return false;
>  
> -	for (i = 0; i < rule->keyrings->count; i++) {
> -		if (!strcmp(rule->keyrings->items[i], keyring)) {
> +	for (i = 0; i < opt_list->count; i++) {
> +		if (!strcmp(opt_list->items[i], func_data)) {
>  			matched = true;
>  			break;
>  		}

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 2/6] IMA: conditionally allow empty rule data

[Mimi Zohar]

Hi Tushar,

On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
> ima_match_rule_data() permits the func to pass empty func_data.
> For instance, for the following func, the func_data keyrings= is
> optional.
>     measure func=KEY_CHECK keyrings=.ima
> 
> But a new func in future may want to constrain the func_data to
> be non-empty.  ima_match_rule_data() should support this constraint
> and it shouldn't be hard-coded in ima_match_rule_data().
> 
> Update ima_match_rule_data() to conditionally allow empty func_data
> for the func that needs it.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Policy rules may constrain what is measured, but that decision should
be left to the system owner or admin.

Mimi

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 4/6] IMA: add policy to measure critical data from kernel components

[Mimi Zohar]

Hi Tushar,

The above Subject line should be truncated to "IMA: add policy to
measure critical data".    

On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
> There would be several candidate kernel components suitable for IMA
> measurement. Not all of them would have support for IMA measurement.

This intro is besides the point.  The patch description should describe
what is meant by "critical data".

> Also, system administrators may not want to measure data for all of
> them, even when they support IMA measurement.
> An IMA policy option 
> specific to various kernel components is needed to measure their
> respective critical data.
> 
> This policy option needs to be constrained to measure data for
> specific kernel components that are specified as input values to the
> policy option.
> 
> Add a new IMA policy option - "data_sources:=" to allow measuring
> various critical kernel components. This policy option would enable the
> system administrators to limit the measurement to the components
> listed in "data_sources:=", if the components support IMA measurement.
> 
> The new policy option "data_sources:=" is different from the existing
> policy option "keyrings:=". 
> 
> In case of "keyrings:=", a policy may measure all keyrings (when
> "keyrings:=" option is not provided for func KEY_CHECK), or may
> constrain which keyrings need to be measured (when "keyrings:=" option
> is provided for func KEY_CHECK).
> 
> But unlike "keyrings:=", the entries in "data_sources:=" would have
> different data format. Further, the components listed in
> "data_sources:=" need to be modified to call IMA to measure their
> data. Therefore, unlike "keyrings:=", IMA shouldn't measure all of the
> components by default, when "data_sources:=" is not specified. Because
> measuring non-vetted components just by specifying them as a policy
> option value may impact the overall reliability of the system.
> 
> To address this, "data_sources:=" should be a mandatory policy option
> for func=CRITICAL_DATA. This func is introduced in the 5th patch in this
> series). And the compile-time vetting functionality described above is
> introduced in the 6th patch in this series.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

I don't understand what you mean by "non-vetted" components, nor how
measuring critical data may impact the overall reliability of the
system.

The system owner or adminstrator defines what they want to measure,
including "critical data", based on the policy rules.  They might
decide that they want to constrain which "critical data" is measured by
specifying "data_sources:=", but that decision is their perogative.  
The default should allow measuring all critical data.

 A simple example of "critical data" could be some in memory structure,
along the lines of __ro_after_init, or hash of the memory structure. 
Once the data structure is initialized, the "critical data" measurement
shouldn't change.    From the attestation server perspective, the IMA
measurement list would contain a single record unless the critical data
changes.  The attestation server doesn't need to know anything about
the initial value, just that it has changed in order to trigger some
sort alert.

thanks,

Mimi

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 5/6] IMA: add hook to measure critical data from kernel components

[Mimi Zohar]

Hi Tushar,

On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
> Currently, IMA does not provide a generic function for kernel components
> to measure their data. A generic function provided by IMA would
> enable various parts of the kernel with easier and faster on-boarding to
> use IMA infrastructure, would avoid code duplication, and consistent
> usage of IMA policy option "data_sources:=" across the kernel.
> 
> Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
> ima_measure_critical_data() to support measuring various critical kernel
> components. Limit the measurement to the components that are specified
> in the IMA policy - CRITICAL_DATA+data_sources.
> 
> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>

Normally the new LSM or IMA hook is defined before defining a method of
constraining that hook.  Please drop 2/6 (IMA: conditionally allow
empty rule data) and reverse the order of 4/6 and 5/6.   That will
allow each patch to update the Documentation appropriately, making the
change self contained.

> ---
>  Documentation/ABI/testing/ima_policy |  8 ++++++-
>  include/linux/ima.h                  |  8 +++++++
>  security/integrity/ima/ima.h         |  1 +
>  security/integrity/ima/ima_api.c     |  2 +-
>  security/integrity/ima/ima_main.c    | 26 +++++++++++++++++++++
>  security/integrity/ima/ima_policy.c  | 34 ++++++++++++++++++++++++----
>  6 files changed, 72 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index a81cf79fb255..d33bb51309fc 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -29,7 +29,7 @@ Description:
>  		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>  				[FIRMWARE_CHECK]
>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> -				[KEXEC_CMDLINE] [KEY_CHECK]
> +				[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
>  			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>  			       [[^]MAY_EXEC]
>  			fsmagic:= hex value
> @@ -51,6 +51,8 @@ Description:
>  			data_sources:= list of kernel components
>  			(eg, selinux|apparmor|dm-crypt) that contain data critical
>  			to the security of the kernel.
> +			Only valid when action is "measure" and func is
> +			CRITICAL_DATA.
>  
>  		default policy:
>  			# PROC_SUPER_MAGIC
> @@ -128,3 +130,7 @@ Description:
>  		keys added to .builtin_trusted_keys or .ima keyring:
>  
>  			measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
> +
> +		Example of measure rule using CRITICAL_DATA to measure critical data
> +
> +			measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt


As data sources are added, the documentation example should be updated
to reflect the new source.  Please do not include examples that don't
yet exist.


> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 6888fc372abf..d55896f28790 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -867,6 +867,32 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>  	fdput(f);
>  }
>  
> +/**
> + * ima_measure_critical_data - measure critical data
> + * @event_name: name for the given data
> + * @event_data_source: name of the event data source
> + * @buf: pointer to buffer containing data to measure
> + * @buf_len: length of buffer(in bytes)
> + * @measure_buf_hash: if set to true - will measure hash of the buf,
> + *                    instead of buf
> + *
> + * Buffers can only be measured, not appraised.
> + */

Perhaps the reason for defining both the event_name and
event_data_source will become clearer with an example.  At this point I
can only guess as to why both are needed (e.g. perhaps a data source
defines multiple events).

While "Buffers can only be measured, not appraised" is true, it was cut
& pasted from ima_kexec_cmdline.  Measuring the kexec boot cmdline is
self describing.  Here, a larger, more detailed IMA hook description
would be appropriate.

thanks,

Mimi

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 1/6] IMA: generalize keyring specific measurement constructs

[Tushar Sugandhi]

Thanks Mimi for your overall feedback on this series.
Really appreciate it.

On 2020-10-22 12:39 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
> 
>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>> index fe1df373c113..31a772d8a86b 100644
>> --- a/security/integrity/ima/ima_policy.c
>> +++ b/security/integrity/ima/ima_policy.c
>> @@ -451,15 +451,19 @@ int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
>>   }
>>   
>>   /**
>> - * ima_match_keyring - determine whether the keyring matches the measure rule
>> - * @rule: a pointer to a rule
>> - * @keyring: name of the keyring to match against the measure rule
>> + * ima_match_rule_data - determine whether the given func_data matches
>> + *			 the measure rule data
>> + * @rule: IMA policy rule
>> + * @opt_list: rule data to match func_data against
>> + * @func_data: data to match against the measure rule data
>>    * @cred: a pointer to a credentials structure for user validation
>>    *
>> - * Returns true if keyring matches one in the rule, false otherwise.
>> + * Returns true if func_data matches one in the rule, false otherwise.
>>    */
>> -static bool ima_match_keyring(struct ima_rule_entry *rule,
>> -			      const char *keyring, const struct cred *cred)
>> +static bool ima_match_rule_data(struct ima_rule_entry *rule,
>> +				const struct ima_rule_opt_list *opt_list,
>> +				const char *func_data,
>> +				const struct cred *cred)
>>   {
>>   	bool matched = false;
>>   	size_t i;
>> @@ -467,14 +471,14 @@ static bool ima_match_keyring(struct ima_rule_entry *rule,
>>   	if ((rule->flags & IMA_UID) && !rule->uid_op(cred->uid, rule->uid))
>>   		return false;
>>   
>> -	if (!rule->keyrings)
>> +	if (!opt_list)
>>   		return true;
> 
> The opt_list should be based on rule->func.  There shouldn't be a need
> to pass it as a variable.
> 
> Mimi
Makes sense. Will do. Thanks Mimi.
~Tushar
> 
>>   
>> -	if (!keyring)
>> +	if (!func_data)
>>   		return false;
>>   
>> -	for (i = 0; i < rule->keyrings->count; i++) {
>> -		if (!strcmp(rule->keyrings->items[i], keyring)) {
>> +	for (i = 0; i < opt_list->count; i++) {
>> +		if (!strcmp(opt_list->items[i], func_data)) {
>>   			matched = true;
>>   			break;
>>   		}

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 2/6] IMA: conditionally allow empty rule data

[Tushar Sugandhi]

On 2020-10-22 1:38 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
>> ima_match_rule_data() permits the func to pass empty func_data.
>> For instance, for the following func, the func_data keyrings= is
>> optional.
>>      measure func=KEY_CHECK keyrings=.ima
>>
>> But a new func in future may want to constrain the func_data to
>> be non-empty.  ima_match_rule_data() should support this constraint
>> and it shouldn't be hard-coded in ima_match_rule_data().
>>
>> Update ima_match_rule_data() to conditionally allow empty func_data
>> for the func that needs it.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Policy rules may constrain what is measured, but that decision should
> be left to the system owner or admin.
> 
> Mimi
> 
Agreed. As you mentioned in the patch 5/6 of this series,
I will get rid of this patch.

~Tushar

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 4/6] IMA: add policy to measure critical data from kernel components

[Tushar Sugandhi]

On 2020-10-22 2:15 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> The above Subject line should be truncated to "IMA: add policy to
> measure critical data".
> 
> On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
>> There would be several candidate kernel components suitable for IMA
>> measurement. Not all of them would have support for IMA measurement.
> 
> This intro is besides the point.  The patch description should describe
> what is meant by "critical data".
> 
Thanks. I will fix the description to address this.

>> Also, system administrators may not want to measure data for all of
>> them, even when they support IMA measurement.
>> An IMA policy option
>> specific to various kernel components is needed to measure their
>> respective critical data.
>>
>> This policy option needs to be constrained to measure data for
>> specific kernel components that are specified as input values to the
>> policy option.
>>
>> Add a new IMA policy option - "data_sources:=" to allow measuring
>> various critical kernel components. This policy option would enable the
>> system administrators to limit the measurement to the components
>> listed in "data_sources:=", if the components support IMA measurement.
>>
>> The new policy option "data_sources:=" is different from the existing
>> policy option "keyrings:=".
>>
>> In case of "keyrings:=", a policy may measure all keyrings (when
>> "keyrings:=" option is not provided for func KEY_CHECK), or may
>> constrain which keyrings need to be measured (when "keyrings:=" option
>> is provided for func KEY_CHECK).
>>
>> But unlike "keyrings:=", the entries in "data_sources:=" would have
>> different data format. Further, the components listed in
>> "data_sources:=" need to be modified to call IMA to measure their
>> data. Therefore, unlike "keyrings:=", IMA shouldn't measure all of the
>> components by default, when "data_sources:=" is not specified. Because
>> measuring non-vetted components just by specifying them as a policy
>> option value may impact the overall reliability of the system.
>>
>> To address this, "data_sources:=" should be a mandatory policy option
>> for func=CRITICAL_DATA. This func is introduced in the 5th patch in this
>> series). And the compile-time vetting functionality described above is
>> introduced in the 6th patch in this series.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> I don't understand what you mean by "non-vetted" components, nor how
> measuring critical data may impact the overall reliability of the
> system.
> 
Tushar: Before we introduced the mechanism to check supported
data-sources at compile time (patch 6/6 of this series), there was a
back-and-forth on whether “data_sources:=” should be a mandatory policy
option, or optional like “keyrings:=”. And we decided to make the
“data_sources:=” mandatory. But now that we have the compile time check
(patch 6/6 of this series), we can switch to make “data_sources:=”
optional (with the default to allow measuring all critical data – just 
like what you suggested below). I will make the code and description 
changes accordingly.
> The system owner or adminstrator defines what they want to measure,
> including "critical data", based on the policy rules.  They might
> decide that they want to constrain which "critical data" is measured by
> specifying "data_sources:=", but that decision is their perogative.
> The default should allow measuring all critical data.
> 
Makes sense.
To summarize, we will make the decision which sources to measure- based
on the sources defined in the allow list (in patch 6) and the sources
defined in “data_sources:=”. If “data_sources:=” is not present, we will
measure all sources defined in the allow list.
Hope my this understanding is correct based on your feedback.

>   A simple example of "critical data" could be some in memory structure,
> along the lines of __ro_after_init, or hash of the memory structure.
> Once the data structure is initialized, the "critical data" measurement
> shouldn't change.    From the attestation server perspective, the IMA
> measurement list would contain a single record unless the critical data
> changes.  The attestation server doesn't need to know anything about
> the initial value, just that it has changed in order to trigger some
> sort alert.
Yes agreed. After the updates (based on your feedback) I stated above,
the behavior should remain consistent with what you described here.
> 
> thanks,
> 
> Mimi
> 


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 5/6] IMA: add hook to measure critical data from kernel components

[Tushar Sugandhi]

On 2020-10-22 3:35 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
>> Currently, IMA does not provide a generic function for kernel components
>> to measure their data. A generic function provided by IMA would
>> enable various parts of the kernel with easier and faster on-boarding to
>> use IMA infrastructure, would avoid code duplication, and consistent
>> usage of IMA policy option "data_sources:=" across the kernel.
>>
>> Add a new IMA func CRITICAL_DATA and a corresponding IMA hook
>> ima_measure_critical_data() to support measuring various critical kernel
>> components. Limit the measurement to the components that are specified
>> in the IMA policy - CRITICAL_DATA+data_sources.
>>
>> Signed-off-by: Tushar Sugandhi <tusharsu@linux.microsoft.com>
> 
> Normally the new LSM or IMA hook is defined before defining a method of
> constraining that hook.  Please drop 2/6 (IMA: conditionally allow
> empty rule data) and reverse the order of 4/6 and 5/6.   That will
> allow each patch to update the Documentation appropriately, making the
> change self contained.
> 
Sure. I will drop 2/6, and reverse the order of 4/6 and 5/6.
>> ---
>>   Documentation/ABI/testing/ima_policy |  8 ++++++-
>>   include/linux/ima.h                  |  8 +++++++
>>   security/integrity/ima/ima.h         |  1 +
>>   security/integrity/ima/ima_api.c     |  2 +-
>>   security/integrity/ima/ima_main.c    | 26 +++++++++++++++++++++
>>   security/integrity/ima/ima_policy.c  | 34 ++++++++++++++++++++++++----
>>   6 files changed, 72 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
>> index a81cf79fb255..d33bb51309fc 100644
>> --- a/Documentation/ABI/testing/ima_policy
>> +++ b/Documentation/ABI/testing/ima_policy
>> @@ -29,7 +29,7 @@ Description:
>>   		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>>   				[FIRMWARE_CHECK]
>>   				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
>> -				[KEXEC_CMDLINE] [KEY_CHECK]
>> +				[KEXEC_CMDLINE] [KEY_CHECK] [CRITICAL_DATA]
>>   			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
>>   			       [[^]MAY_EXEC]
>>   			fsmagic:= hex value
>> @@ -51,6 +51,8 @@ Description:
>>   			data_sources:= list of kernel components
>>   			(eg, selinux|apparmor|dm-crypt) that contain data critical
>>   			to the security of the kernel.
>> +			Only valid when action is "measure" and func is
>> +			CRITICAL_DATA.
>>   
>>   		default policy:
>>   			# PROC_SUPER_MAGIC
>> @@ -128,3 +130,7 @@ Description:
>>   		keys added to .builtin_trusted_keys or .ima keyring:
>>   
>>   			measure func=KEY_CHECK keyrings=.builtin_trusted_keys|.ima
>> +
>> +		Example of measure rule using CRITICAL_DATA to measure critical data
>> +
>> +			measure func=CRITICAL_DATA data_sources=selinux|apparmor|dm-crypt
> 
> 
> As data sources are added, the documentation example should be updated
> to reflect the new source.  Please do not include examples that don't
> yet exist.
> 
Makes sense. Will do.
> 
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 6888fc372abf..d55896f28790 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -867,6 +867,32 @@ void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
>>   	fdput(f);
>>   }
>>   
>> +/**
>> + * ima_measure_critical_data - measure critical data
>> + * @event_name: name for the given data
>> + * @event_data_source: name of the event data source
>> + * @buf: pointer to buffer containing data to measure
>> + * @buf_len: length of buffer(in bytes)
>> + * @measure_buf_hash: if set to true - will measure hash of the buf,
>> + *                    instead of buf
>> + *
>> + * Buffers can only be measured, not appraised.
>> + */
> 
> Perhaps the reason for defining both the event_name and
> event_data_source will become clearer with an example.  At this point I
> can only guess as to why both are needed (e.g. perhaps a data source
> defines multiple events).
> 
Yes. Precisely. For example, in “dm-crypt” case: the data source is
“dm-crypt” and possible events are “add_target”, “post_suspend”,
"resume" etc. I will add a more detailed hook description as you
suggested below, and explain this point in it.
> While "Buffers can only be measured, not appraised" is true, it was cut
> & pasted from ima_kexec_cmdline.  Measuring the kexec boot cmdline is
> self describing.  Here, a larger, more detailed IMA hook description
> would be appropriate.
Will add. Thanks Mimi.
> 
> thanks,
> 
> Mimi
> 


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 0/6] IMA: Infrastructure for measurement of critical kernel data

[Mimi Zohar]

Hi Tushar,

On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
> There are several kernel components that contain critical data which if
> accidentally or maliciously altered, can compromise the security of the
> kernel. Example of such components would include LSMs like SELinux, or
> AppArmor; or device-mapper targets like dm-crypt, dm-verity etc.

^"the integrity of the system."

This cover letter needs to be re-written from a higher perspective,
explaining what is meant by "critical data" (e.g. kernel subsystem
specific information only stored in kernel memory).

> 
> Many of these components do not use the capabilities provided by kernel
> integrity subsystem (IMA), and thus they don't use the benefits of
> extended TPM PCR quotes and ultimately the benefits of remote attestation.

True, up until recently IMA only measured files, nothing else.  Why is
this paragraph needed?  What new information is provided?

> This series bridges this gap, so that potential kernel components that
> contain data critical to the security of the kernel could take advantage
> of IMA's measuring and quoting abilities - thus ultimately enabling
> remote attestation for their specific data.

Perhaps, something more along the lines, "This patch set defines a new
IMA hook named ... to measure critical data."

> 
> System administrators may want to pick and choose which kernel
> components they would want to enable for measurements, quoting, and
> remote attestation. To enable that, a new IMA policy is introduced.

Reverse the order of this paragraph and the following one, describing
the new feature and only afterwards explaining how it may be
constrained.

> 
> And lastly, the functionality is exposed through a function
> ima_measure_critical_data(). The functionality is generic enough to
> measure the data of any kernel component at run-time. To ensure that
> only data from supported sources are measured, the kernel component
> needs to be added to a compile-time list of supported sources (an
> "allowed list of components"). IMA validates the source passed to
> ima_measure_critical_data() against this allowed list at run-time.

This patch set must include at least one example of measuring critical
data, before it can be upstreamed.  Tushar, please coordinate with
Lakshmi and Raphael.

thanks,

Mimi

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v4 0/6] IMA: Infrastructure for measurement of critical kernel data

[Tushar Sugandhi]

Hi Mimi,
Thanks a lot for your continual engagement and
providing us valuable feedback on this work.

On 2020-10-24 8:35 p.m., Mimi Zohar wrote:
> Hi Tushar,
> 
> On Wed, 2020-09-23 at 12:20 -0700, Tushar Sugandhi wrote:
>> There are several kernel components that contain critical data which if
>> accidentally or maliciously altered, can compromise the security of the
>> kernel. Example of such components would include LSMs like SELinux, or
>> AppArmor; or device-mapper targets like dm-crypt, dm-verity etc.
> 
> ^"the integrity of the system."
> 
Ok. I will revisit this cover letter again, when we post the next
version of the series. We also need to update the cover letter to
include the description for new patches to be added in this series, as
per your suggestion below. {built-in policy patch (by Lakshmi) and an
example measurement patch (SeLinux by Lakshmi)}

> This cover letter needs to be re-written from a higher perspective,
> explaining what is meant by "critical data" (e.g. kernel subsystem
> specific information only stored in kernel memory).
> 
>>
>> Many of these components do not use the capabilities provided by kernel
>> integrity subsystem (IMA), and thus they don't use the benefits of
>> extended TPM PCR quotes and ultimately the benefits of remote attestation.
> 
> True, up until recently IMA only measured files, nothing else.  Why is
> this paragraph needed?  What new information is provided?
> 
Here, I was attempting to describe the problem (what needs to be
solved), before proposing a solution below. But maybe it is not adding
value. I will get rid of the above paragraph in the next iteration.

>> This series bridges this gap, so that potential kernel components that
>> contain data critical to the security of the kernel could take advantage
>> of IMA's measuring and quoting abilities - thus ultimately enabling
>> remote attestation for their specific data.
> 
> Perhaps, something more along the lines, "This patch set defines a new
> IMA hook named ... to measure critical data."
> 
Will do.
>>
>> System administrators may want to pick and choose which kernel
>> components they would want to enable for measurements, quoting, and
>> remote attestation. To enable that, a new IMA policy is introduced.
> 
> Reverse the order of this paragraph and the following one, describing
> the new feature and only afterwards explaining how it may be
> constrained.
> 
Makes total sense. Will do.
>>
>> And lastly, the functionality is exposed through a function
>> ima_measure_critical_data(). The functionality is generic enough to
>> measure the data of any kernel component at run-time. To ensure that
>> only data from supported sources are measured, the kernel component
>> needs to be added to a compile-time list of supported sources (an
>> "allowed list of components"). IMA validates the source passed to
>> ima_measure_critical_data() against this allowed list at run-time.
> 
> This patch set must include at least one example of measuring critical
> data, before it can be upstreamed.  Tushar, please coordinate with
> Lakshmi and Raphael.
> 
Yes. I am coordinating with Lakshmi/Raphael on including one of the
examples. (SeLinux as per your feedback)

BTW, we also have one more data source (dm-crypt) currently in review,
that uses critical data measurement infrastructure to measure its kernel
in-memory data.
https://patchwork.kernel.org/patch/11844817/

Thanks again for all the help and support with the patches.

~Tushar

> thanks,
> 
> Mimi
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel