[PATCH v3 0/5] LSM: Measure security module state

Critical data structures of security modules are currently not measured.
Therefore an attestation service, for instance, would not be able to
attest whether the security modules are always operating with the policies
and configuration that the system administrator had setup. The policies
and configuration for the security modules could be tampered with by
malware by exploiting Kernel vulnerabilities or modified through some
inadvertent actions on the system. Measuring such critical data would
enable an attestation service to better assess the state of the system.

IMA subsystem measures system files, command line arguments passed to
kexec, boot aggregate, keys, etc. It can be used to measure critical
data structures of security modules as well.

This change aims to address measuring critical data structures
of security modules when they are initialized, when they are updated
at runtime, and also periodically to detect any tampering.

This change set is based off of Linux Kernel version 5.8-rc5.

The following patch needs to be applied first before applying
the patches in this patch set:

    https://patchwork.kernel.org/patch/11612989/

Change log:

  v3:
      => Loop through policy_capabilities to build the state data
         to measure instead of hardcoding to current set of
         policy capabilities.
      => Added error log messages for failure conditions.

  v2:
      => Pass selinux_state struct as parameter to the function
         that measures SELinux data.
      => Use strings from selinux_policycap_names array for SELinux
         state measurement.
      => Refactored security_read_policy() to alloc kernel or user
         virtual memory and then read the SELinux policy.

  v1:
      => Per Stephen Smalley's suggestion added selinux_state booleans
         and hash of SELinux policy in the measured data for SELinux.
      => Call IMA hook from the security module directly instead of
         redirecting through the LSM.

Lakshmi Ramasubramanian (5):
  IMA: Add LSM_STATE func to measure LSM data
  IMA: Define an IMA hook to measure LSM data
  LSM: Add security_measure_data in lsm_info struct
  LSM: Define SELinux function to measure security state
  LSM: Define workqueue for measuring security module state

 Documentation/ABI/testing/ima_policy |   6 +-
 include/linux/ima.h                  |   4 +
 include/linux/lsm_hooks.h            |   3 +
 security/integrity/ima/ima.h         |   1 +
 security/integrity/ima/ima_api.c     |   2 +-
 security/integrity/ima/ima_main.c    |  17 +++
 security/integrity/ima/ima_policy.c  |  29 ++++-
 security/security.c                  |  74 ++++++++++++-
 security/selinux/Makefile            |   2 +
 security/selinux/hooks.c             |   4 +
 security/selinux/include/security.h  |  18 ++++
 security/selinux/measure.c           | 155 +++++++++++++++++++++++++++
 security/selinux/selinuxfs.c         |   1 +
 security/selinux/ss/services.c       |  66 ++++++++++--
 14 files changed, 365 insertions(+), 17 deletions(-)
 create mode 100644 security/selinux/measure.c

-- 
2.27.0

IMA subsystem needs to define an IMA hook that the security modules can
call to measure critical data of the security modules.

Define a new IMA hook, namely ima_lsm_state(), that the security modules
can call to measure data.

Signed-off-by: Lakshmi Ramasubramanian <redacted>
---
 include/linux/ima.h               |  4 ++++
 security/integrity/ima/ima_main.c | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..7e2686f4953a 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h

@@ -26,6 +26,7 @@ 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(const void *buf, int size);
+extern void ima_lsm_state(const char *lsm_event_name, const void *buf, int size);
 
 #ifdef CONFIG_IMA_KEXEC
 extern void ima_add_kexec_buffer(struct kimage *image);

@@ -104,6 +105,9 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
 }
 
 static inline void ima_kexec_cmdline(const void *buf, int size) {}
+
+static inline void ima_lsm_state(const char *lsm_event_name,
+				 const void *buf, int size) {}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8351b2fd48e0..04d9a1d35300 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c

@@ -835,6 +835,23 @@ void ima_kexec_cmdline(const void *buf, int size)
 					   KEXEC_CMDLINE, 0, NULL);
 }
 
+/**
+ * ima_lsm_state - measure LSM specific state
+ * @lsm_event_name: LSM event
+ * @buf: pointer to buffer containing LSM specific state
+ * @size: Number of bytes in buf
+ *
+ * Buffers can only be measured, not appraised.
+ */
+void ima_lsm_state(const char *lsm_event_name, const void *buf, int size)
+{
+	if (!lsm_event_name || !buf || !size)
+		return;
+
+	process_buffer_measurement(buf, size, lsm_event_name,
+				   LSM_STATE, 0, NULL);
+}
+
 static int __init init_ima(void)
 {
 	int error;

-- 
2.27.0

Data structures critical to the functioning of a security module could
be tampered with by malware or changed inadvertently at runtime
thereby disabling or reducing the security guarantees provided by
the security module. Such critical data need to be periodically checked
and measured, if there is any change. This would enable an attestation
service, for instance, to verify that the security modules are operating
with the configuration and policy setup by the system administrator.

Define a workqueue in the LSM and invoke the security modules in
the workqueue handler to check their data and measure.

Note that the data given by the security module would be measured by
the IMA subsystem only if it has changed since the last time it was
measured.

Signed-off-by: Lakshmi Ramasubramanian <redacted>
---
 security/security.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/security/security.c b/security/security.c
index 88ce1b780ffd..6e746a024ed5 100644
--- a/security/security.c
+++ b/security/security.c

@@ -89,6 +89,11 @@ static __initdata struct lsm_info *exclusive;
 static struct lsm_info *security_state_lsms;
 static int security_state_lsms_count;
 
+static long security_state_timeout = 300000; /* 5 Minutes */
+static void security_state_handler(struct work_struct *work);
+static DECLARE_DELAYED_WORK(security_state_delayed_work,
+			    security_state_handler);
+
 static __initdata bool debug;
 #define init_debug(...)						\
 	do {							\

@@ -277,6 +282,26 @@ static void __init initialize_security_state_lsms(void)
 	security_state_lsms_count = count;
 }
 
+static void initialize_security_state_monitor(void)
+{
+	if (security_state_lsms_count == 0)
+		return;
+
+	schedule_delayed_work(&security_state_delayed_work,
+			      msecs_to_jiffies(security_state_timeout));
+}
+
+static void security_state_handler(struct work_struct *work)
+{
+	int inx;
+
+	for (inx = 0; inx < security_state_lsms_count; inx++)
+		measure_security_state(&(security_state_lsms[inx]));
+
+	schedule_delayed_work(&security_state_delayed_work,
+			      msecs_to_jiffies(security_state_timeout));
+}
+
 /* Populate ordered LSMs list from comma-separated LSM name list. */
 static void __init ordered_lsm_parse(const char *order, const char *origin)
 {

@@ -400,6 +425,7 @@ static void __init ordered_lsm_init(void)
 	}
 
 	initialize_security_state_lsms();
+	initialize_security_state_monitor();
 
 	kfree(ordered_lsms);
 }

-- 
2.27.0

The security modules that require their data to be measured using
the IMA subsystem need to define a function that the LSM can call
to trigger the measurement.

Add a function pointer field namely security_measure_data in lsm_info
structure. Update LSM to call this security module function, if defined,
to measure the security module's data using the IMA subsystem.

Signed-off-by: Lakshmi Ramasubramanian <redacted>
---
 include/linux/lsm_hooks.h |  3 +++
 security/security.c       | 48 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 95b7c1d32062..17afdf319c55 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h

@@ -1591,6 +1591,9 @@ struct lsm_info {
 	int *enabled;		/* Optional: controlled by CONFIG_LSM */
 	int (*init)(void);	/* Required. */
 	struct lsm_blob_sizes *blobs; /* Optional: for blob sharing. */
+	void (*security_measure_data)(void); /* Optional: for measuring
+					      * security module data.
+					      */
 };
 
 extern struct lsm_info __start_lsm_info[], __end_lsm_info[];

diff --git a/security/security.c b/security/security.c
index 70a7ad357bc6..88ce1b780ffd 100644
--- a/security/security.c
+++ b/security/security.c

@@ -86,6 +86,9 @@ static __initconst const char * const builtin_lsm_order = CONFIG_LSM;
 static __initdata struct lsm_info **ordered_lsms;
 static __initdata struct lsm_info *exclusive;
 
+static struct lsm_info *security_state_lsms;
+static int security_state_lsms_count;
+
 static __initdata bool debug;
 #define init_debug(...)						\
 	do {							\

@@ -235,6 +238,45 @@ static void __init initialize_lsm(struct lsm_info *lsm)
 	}
 }
 
+static void measure_security_state(struct lsm_info *lsm)
+{
+	if (!lsm->security_measure_data)
+		return;
+
+	lsm->security_measure_data();
+}
+
+static void __init initialize_security_state_lsms(void)
+{
+	struct lsm_info **lsm;
+	int count = 0;
+	int inx;
+
+	for (lsm = ordered_lsms; *lsm; lsm++) {
+		if ((*lsm)->security_measure_data)
+			count++;
+	}
+
+	if (count == 0)
+		return;
+
+	security_state_lsms = kcalloc(count, sizeof(struct lsm_info),
+				      GFP_KERNEL);
+	if (!security_state_lsms)
+		return;
+
+	inx = 0;
+	for (lsm = ordered_lsms; *lsm; lsm++) {
+		if ((*lsm)->security_measure_data) {
+			security_state_lsms[inx].security_measure_data =
+				(*lsm)->security_measure_data;
+			inx++;
+		}
+	}
+
+	security_state_lsms_count = count;
+}
+
 /* Populate ordered LSMs list from comma-separated LSM name list. */
 static void __init ordered_lsm_parse(const char *order, const char *origin)
 {

@@ -352,8 +394,12 @@ static void __init ordered_lsm_init(void)
 
 	lsm_early_cred((struct cred *) current->cred);
 	lsm_early_task(current);
-	for (lsm = ordered_lsms; *lsm; lsm++)
+	for (lsm = ordered_lsms; *lsm; lsm++) {
 		initialize_lsm(*lsm);
+		measure_security_state(*lsm);
+	}
+
+	initialize_security_state_lsms();
 
 	kfree(ordered_lsms);
 }

-- 
2.27.0

SELinux configuration and policy are some of the critical data for this
security module that needs to be measured. To enable this measurement
SELinux needs to implement the interface function,
security_measure_data(), that the LSM can call.

Define the security_measure_data() function in SELinux to measure SELinux
configuration and policy. Call this function to measure SELinux data
when there is a change in the security module's state.

Sample measurement of SELinux state and hash of the policy:

10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b
10 f4a7...9408 ima-buf sha256:4941...68fc selinux-policy-hash 8d1d...1834

To verify the measurement check the following:

Execute the following command to extract the measured data
from the IMA log for SELinux configuration (selinux-state).

  cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-state" | cut -d' ' -f 6 | xxd -r -p

The output should be the list of key-value pairs. For example,
 enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;

To verify the measured data with the current SELinux state:

 => enabled should be set to 1 if /sys/fs/selinux folder exists,
    0 otherwise

For other entries, compare the integer value in the files
 => /sys/fs/selinux/enforce
 => /sys/fs/selinux/checkreqprot
And, each of the policy capabilities files under
 => /sys/fs/selinux/policy_capabilities

The data for selinux-policy-hash is the SHA256 hash of SELinux policy.

To verify the measured data with the current SELinux policy run
the following commands and verify the output hash values match.

  sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1

  cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-policy-hash" | cut -d' ' -f 6

Signed-off-by: Lakshmi Ramasubramanian <redacted>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---
 security/selinux/Makefile           |   2 +
 security/selinux/hooks.c            |   4 +
 security/selinux/include/security.h |  18 ++++
 security/selinux/measure.c          | 155 ++++++++++++++++++++++++++++
 security/selinux/selinuxfs.c        |   1 +
 security/selinux/ss/services.c      |  66 ++++++++++--
 6 files changed, 237 insertions(+), 9 deletions(-)
 create mode 100644 security/selinux/measure.c

diff --git a/security/selinux/Makefile b/security/selinux/Makefile
index 4d8e0e8adf0b..83d512116341 100644
--- a/security/selinux/Makefile
+++ b/security/selinux/Makefile

@@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o
 
 selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o
 
+selinux-$(CONFIG_IMA) += measure.o
+
 ccflags-y := -I$(srctree)/security/selinux -I$(srctree)/security/selinux/include
 
 $(addprefix $(obj)/,$(selinux-y)): $(obj)/flask.h

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index efa6108b1ce9..cda1d328339f 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c

@@ -7259,6 +7259,8 @@ static __init int selinux_init(void)
 
 	fs_validate_description("selinux", selinux_fs_parameters);
 
+	selinux_init_measurement();
+
 	return 0;
 }
 

@@ -7284,6 +7286,7 @@ DEFINE_LSM(selinux) = {
 	.enabled = &selinux_enabled_boot,
 	.blobs = &selinux_blob_sizes,
 	.init = selinux_init,
+	.security_measure_data = selinux_measure_data,
 };
 
 #if defined(CONFIG_NETFILTER)

@@ -7394,6 +7397,7 @@ int selinux_disable(struct selinux_state *state)
 	}
 
 	selinux_mark_disabled(state);
+	selinux_measure_state(state);
 
 	pr_info("SELinux:  Disabled at runtime.\n");
 

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index b0e02cfe3ce1..1f41c16a4845 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h

@@ -222,16 +222,34 @@ static inline bool selinux_policycap_genfs_seclabel_symlinks(void)
 	return state->policycap[POLICYDB_CAPABILITY_GENFS_SECLABEL_SYMLINKS];
 }
 
+static inline bool selinux_checkreqprot(const struct selinux_state *state)
+{
+	return READ_ONCE(state->checkreqprot);
+}
+
 int security_mls_enabled(struct selinux_state *state);
 int security_load_policy(struct selinux_state *state,
 			 void *data, size_t len);
 int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len);
+int security_read_policy_kernel(struct selinux_state *state,
+				void **data, size_t *len);
 size_t security_policydb_len(struct selinux_state *state);
 
 int security_policycap_supported(struct selinux_state *state,
 				 unsigned int req_cap);
 
+#ifdef CONFIG_IMA
+extern void __init selinux_init_measurement(void);
+extern void selinux_measure_data(void);
+extern void selinux_measure_state(struct selinux_state *selinux_state);
+#else
+static inline void __init selinux_init_measurement(void) {}
+static inline void selinux_measure_data(void) {}
+static inline void selinux_measure_state(
+	struct selinux_state *selinux_state) {}
+#endif
+
 #define SEL_VEC_MAX 32
 struct av_decision {
 	u32 allowed;

diff --git a/security/selinux/measure.c b/security/selinux/measure.c
new file mode 100644
index 000000000000..659011637ae7
--- /dev/null
+++ b/security/selinux/measure.c

@@ -0,0 +1,155 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Measure SELinux state using IMA subsystem.
+ */
+#include <linux/ima.h>
+#include "security.h"
+
+/* Pre-allocated buffer used for measuring state */
+static char *selinux_state_string;
+static size_t selinux_state_string_len;
+static char *str_format = "%s=%d;";
+static int selinux_state_count;
+
+void __init selinux_init_measurement(void)
+{
+	int i;
+
+	/*
+	 * enabled
+	 * enforcing
+	 * checkreqport
+	 * All policy capability flags
+	 */
+	selinux_state_count = 3 + __POLICYDB_CAPABILITY_MAX;
+
+	selinux_state_string_len = snprintf(NULL, 0, str_format,
+					    "enabled", 0);
+	selinux_state_string_len += snprintf(NULL, 0, str_format,
+					     "enforcing", 0);
+	selinux_state_string_len += snprintf(NULL, 0, str_format,
+					     "checkreqprot", 0);
+	for (i = 3; i < selinux_state_count; i++) {
+		selinux_state_string_len +=
+			snprintf(NULL, 0, str_format,
+				 selinux_policycap_names[i-3], 0);
+	}
+	++selinux_state_string_len;
+
+	selinux_state_string = kzalloc(selinux_state_string_len, GFP_KERNEL);
+	if (!selinux_state_string) {
+		pr_warn("Failed to alloc memory for SELinux measurement\n");
+		selinux_state_string_len = 0;
+	}
+}
+
+static int selinux_hash_policy(const char *hash_alg_name,
+			       void *policy, size_t policy_len,
+			       void **policy_hash, int *policy_hash_len)
+{
+	struct crypto_shash *tfm;
+	struct shash_desc *desc = NULL;
+	void *digest = NULL;
+	int desc_size;
+	int digest_size;
+	int ret = 0;
+
+	tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
+	if (IS_ERR(tfm))
+		return PTR_ERR(tfm);
+
+	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
+	digest_size = crypto_shash_digestsize(tfm);
+
+	digest = kmalloc(digest_size, GFP_KERNEL);
+	if (!digest) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	desc = kzalloc(desc_size, GFP_KERNEL);
+	if (!desc) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	desc->tfm = tfm;
+
+	ret = crypto_shash_digest(desc, policy, policy_len, digest);
+	if (ret < 0)
+		goto error;
+
+	*policy_hash_len = digest_size;
+	*policy_hash = digest;
+	digest = NULL;
+
+error:
+	kfree(desc);
+	kfree(digest);
+
+	crypto_free_shash(tfm);
+
+	return ret;
+}
+
+void selinux_measure_state(struct selinux_state *selinux_state)
+{
+	void *policy = NULL;
+	void *policy_hash = NULL;
+	size_t curr, buflen;
+	int i, policy_hash_len, rc = 0;
+
+	if (!selinux_initialized(selinux_state)) {
+		pr_warn("%s: SELinux not yet initialized.\n", __func__);
+		return;
+	}
+
+	if (!selinux_state_string) {
+		pr_warn("%s: Buffer for state not allocated.\n", __func__);
+		return;
+	}
+
+	curr = snprintf(selinux_state_string, selinux_state_string_len,
+			str_format, "enabled",
+			!selinux_disabled(selinux_state));
+	curr += snprintf((selinux_state_string + curr),
+			 (selinux_state_string_len - curr),
+			 str_format, "enforcing",
+			 enforcing_enabled(selinux_state));
+	curr += snprintf((selinux_state_string + curr),
+			 (selinux_state_string_len - curr),
+			 str_format, "checkreqprot",
+			 selinux_checkreqprot(selinux_state));
+
+	for (i = 3; i < selinux_state_count; i++) {
+		curr += snprintf((selinux_state_string + curr),
+				 (selinux_state_string_len - curr),
+				 str_format,
+				 selinux_policycap_names[i - 3],
+				 selinux_state->policycap[i - 3]);
+	}
+
+	if (curr >= 0 && curr < selinux_state_string_len)
+		ima_lsm_state("selinux-state", selinux_state_string, curr);
+	else {
+		rc = -EINVAL;
+		goto out;
+	}
+
+	rc = security_read_policy_kernel(selinux_state, &policy, &buflen);
+	if (!rc)
+		rc = selinux_hash_policy("sha256", policy, buflen,
+					 &policy_hash, &policy_hash_len);
+	if (!rc)
+		ima_lsm_state("selinux-policy-hash", policy_hash,
+			      policy_hash_len);
+
+out:
+	vfree(policy);
+	kfree(policy_hash);
+}
+
+void selinux_measure_data(void)
+{
+	selinux_measure_state(&selinux_state);
+}

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 4781314c2510..b1f70739d709 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c

@@ -173,6 +173,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf,
 			from_kuid(&init_user_ns, audit_get_loginuid(current)),
 			audit_get_sessionid(current));
 		enforcing_set(state, new_value);
+		selinux_measure_state(&selinux_state);
 		if (new_value)
 			avc_ss_reset(state->avc, 0);
 		selnl_notify_setenforce(new_value);

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index ef0afd878bfc..79a6b462f1fe 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c

@@ -3720,14 +3720,22 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
 }
 #endif /* CONFIG_NETLABEL */
 
+static int security_read_policy_len(struct selinux_state *state, size_t *len)
+{
+	if (!selinux_initialized(state))
+		return -EINVAL;
+
+	*len = security_policydb_len(state);
+	return 0;
+}
+
 /**
  * security_read_policy - read the policy.
  * @data: binary policy data
  * @len: length of data in bytes
- *
  */
-int security_read_policy(struct selinux_state *state,
-			 void **data, size_t *len)
+int security_read_selinux_policy(struct selinux_state *state,
+				 void **data, size_t *len)
 {
 	struct policydb *policydb = &state->ss->policydb;
 	int rc;

@@ -3736,12 +3744,6 @@ int security_read_policy(struct selinux_state *state,
 	if (!selinux_initialized(state))
 		return -EINVAL;
 
-	*len = security_policydb_len(state);
-
-	*data = vmalloc_user(*len);
-	if (!*data)
-		return -ENOMEM;
-
 	fp.data = *data;
 	fp.len = *len;
 

@@ -3754,5 +3756,51 @@ int security_read_policy(struct selinux_state *state,
 
 	*len = (unsigned long)fp.data - (unsigned long)*data;
 	return 0;
+}
+
+/**
+ * security_read_policy - read the policy.
+ * @data: binary policy data
+ * @len: length of data in bytes
+ *
+ */
+int security_read_policy(struct selinux_state *state,
+			 void **data, size_t *len)
+{
+	int rc;
+
+	rc = security_read_policy_len(state, len);
+	if (rc)
+		return rc;
+
+	*data = vmalloc_user(*len);
+	if (!*data)
+		return -ENOMEM;
+
+	return security_read_selinux_policy(state, data, len);
+}
+
+/**
+ * security_read_policy_kernel - read the policy.
+ * @data: binary policy data
+ * @len: length of data in bytes
+ *
+ * Allocates kernel memory for reading SELinux policy.
+ * This function is for internal use only and should not
+ * be used for returning data to user space
+ */
+int security_read_policy_kernel(struct selinux_state *state,
+				void **data, size_t *len)
+{
+	int rc;
+
+	rc = security_read_policy_len(state, len);
+	if (rc)
+		return rc;
+
+	*data = vmalloc(*len);
+	if (!*data)
+		return -ENOMEM;
 
+	return security_read_selinux_policy(state, data, len);
 }

-- 
2.27.0

Critical data structures of security modules need to be measured to
enable an attestation service to verify if the policies and
configuration have been setup correctly and that they haven't been
tampered with at runtime. A new IMA policy is required for handling
this measurement.

Define a new IMA policy func namely LSM_STATE to measure data provided
by security modules. Update ima_match_rules() to check for LSM_STATE
and ima_parse_rule() to handle LSM_STATE.

Signed-off-by: Lakshmi Ramasubramanian <redacted>
---
 Documentation/ABI/testing/ima_policy |  6 +++++-
 security/integrity/ima/ima.h         |  1 +
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/ima/ima_policy.c  | 29 +++++++++++++++++++++++-----
 4 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index cd572912c593..355bc3eade33 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] [LSM_STATE]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value

@@ -125,3 +125,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 LSM_STATE to measure LSM data:
+
+			measure func=LSM_STATE

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 4515975cc540..880fda11a61b 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(LSM_STATE, lsm_state)			\
 	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 bf22de8b7ce0..0cebd2404dcf 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 | LSM_STATE
  *	mask: contains the permission mask
  *	fsmagic: hex value
  *

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 66aa3e17a888..fc8457d9242b 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c

@@ -417,15 +417,31 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
 			    const char *keyring)
 {
 	int i;
+	int funcmatch = 0;
 
-	if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
+	switch (func) {
+	case KEXEC_CMDLINE:
+	case KEY_CHECK:
+	case LSM_STATE:
 		if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
 			if (func == KEY_CHECK)
-				return ima_match_keyring(rule, keyring, cred);
-			return true;
-		}
-		return false;
+				funcmatch = ima_match_keyring(rule, keyring,
+							      cred) ? 1 : -1;
+			else
+				funcmatch = 1;
+		} else
+			funcmatch = -1;
+
+		break;
+
+	default:
+		funcmatch = 0;
+		break;
 	}
+
+	if (funcmatch)
+		return (funcmatch == 1) ? true : false;
+
 	if ((rule->flags & IMA_FUNC) &&
 	    (rule->func != func && func != POST_SETATTR))
 		return false;

@@ -1068,6 +1084,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 				entry->func = KEXEC_CMDLINE;
 			else if (strcmp(args[0].from, "KEY_CHECK") == 0)
 				entry->func = KEY_CHECK;
+			else if (strcmp(args[0].from, "LSM_STATE") == 0)
+				entry->func = LSM_STATE;
+
 			else
 				result = -EINVAL;
 			if (!result)

-- 
2.27.0

Hi Lakshmi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All errors (new ones prefixed by >>):

   security/selinux/measure.c: In function 'selinux_measure_state':
   security/selinux/measure.c:132:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     132 |  if (curr >= 0 && curr < selinux_state_string_len)
         |           ^~

quoted

security/selinux/measure.c:148:2: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]

     148 |  vfree(policy);
         |  ^~~~~
         |  kvfree
   cc1: some warnings being treated as errors

vim +148 security/selinux/measure.c

    94	
    95	void selinux_measure_state(struct selinux_state *selinux_state)
    96	{
    97		void *policy = NULL;
    98		void *policy_hash = NULL;
    99		size_t curr, buflen;
   100		int i, policy_hash_len, rc = 0;
   101	
   102		if (!selinux_initialized(selinux_state)) {
   103			pr_warn("%s: SELinux not yet initialized.\n", __func__);
   104			return;
   105		}
   106	
   107		if (!selinux_state_string) {
   108			pr_warn("%s: Buffer for state not allocated.\n", __func__);
   109			return;
   110		}
   111	
   112		curr = snprintf(selinux_state_string, selinux_state_string_len,
   113				str_format, "enabled",
   114				!selinux_disabled(selinux_state));
   115		curr += snprintf((selinux_state_string + curr),
   116				 (selinux_state_string_len - curr),
   117				 str_format, "enforcing",
   118				 enforcing_enabled(selinux_state));
   119		curr += snprintf((selinux_state_string + curr),
   120				 (selinux_state_string_len - curr),
   121				 str_format, "checkreqprot",
   122				 selinux_checkreqprot(selinux_state));
   123	
   124		for (i = 3; i < selinux_state_count; i++) {
   125			curr += snprintf((selinux_state_string + curr),
   126					 (selinux_state_string_len - curr),
   127					 str_format,
   128					 selinux_policycap_names[i - 3],
   129					 selinux_state->policycap[i - 3]);
   130		}
   131	
 > 132		if (curr >= 0 && curr < selinux_state_string_len)
   133			ima_lsm_state("selinux-state", selinux_state_string, curr);
   134		else {
   135			rc = -EINVAL;
   136			goto out;
   137		}
   138	
   139		rc = security_read_policy_kernel(selinux_state, &policy, &buflen);
   140		if (!rc)
   141			rc = selinux_hash_policy("sha256", policy, buflen,
   142						 &policy_hash, &policy_hash_len);
   143		if (!rc)
   144			ima_lsm_state("selinux-policy-hash", policy_hash,
   145				      policy_hash_len);
   146	
   147	out:
 > 148		vfree(policy);
   149		kfree(policy_hash);
   150	}
   151	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Hi Lakshmi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>

All error/warnings (new ones prefixed by >>):

   security/selinux/measure.c: In function 'selinux_hash_policy':

quoted

security/selinux/measure.c:57:8: error: implicit declaration of function 'crypto_alloc_shash' [-Werror=implicit-function-declaration]

      57 |  tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
         |        ^~~~~~~~~~~~~~~~~~

quoted

security/selinux/measure.c:57:6: warning: assignment to 'struct crypto_shash *' from 'int' makes pointer from integer without a cast [-Wint-conversion]

      57 |  tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
         |      ^

quoted

security/selinux/measure.c:61:14: error: implicit declaration of function 'crypto_shash_descsize' [-Werror=implicit-function-declaration]

      61 |  desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
         |              ^~~~~~~~~~~~~~~~~~~~~

quoted

security/selinux/measure.c:61:50: error: dereferencing pointer to incomplete type 'struct shash_desc'

      61 |  desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
         |                                                  ^~~~~

quoted

security/selinux/measure.c:62:16: error: implicit declaration of function 'crypto_shash_digestsize' [-Werror=implicit-function-declaration]

      62 |  digest_size = crypto_shash_digestsize(tfm);
         |                ^~~~~~~~~~~~~~~~~~~~~~~

quoted

security/selinux/measure.c:78:8: error: implicit declaration of function 'crypto_shash_digest' [-Werror=implicit-function-declaration]

      78 |  ret = crypto_shash_digest(desc, policy, policy_len, digest);
         |        ^~~~~~~~~~~~~~~~~~~

quoted

security/selinux/measure.c:90:2: error: implicit declaration of function 'crypto_free_shash' [-Werror=implicit-function-declaration]

      90 |  crypto_free_shash(tfm);
         |  ^~~~~~~~~~~~~~~~~
   security/selinux/measure.c: In function 'selinux_measure_state':
   security/selinux/measure.c:132:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
     132 |  if (curr >= 0 && curr < selinux_state_string_len)
         |           ^~
   security/selinux/measure.c:148:2: error: implicit declaration of function 'vfree'; did you mean 'kvfree'? [-Werror=implicit-function-declaration]
     148 |  vfree(policy);
         |  ^~~~~
         |  kvfree
   cc1: some warnings being treated as errors

vim +/crypto_alloc_shash +57 security/selinux/measure.c

    45	
    46	static int selinux_hash_policy(const char *hash_alg_name,
    47				       void *policy, size_t policy_len,
    48				       void **policy_hash, int *policy_hash_len)
    49	{
    50		struct crypto_shash *tfm;
    51		struct shash_desc *desc = NULL;
    52		void *digest = NULL;
    53		int desc_size;
    54		int digest_size;
    55		int ret = 0;
    56	
  > 57		tfm = crypto_alloc_shash(hash_alg_name, 0, 0);
    58		if (IS_ERR(tfm))
    59			return PTR_ERR(tfm);
    60	
  > 61		desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
  > 62		digest_size = crypto_shash_digestsize(tfm);
    63	
    64		digest = kmalloc(digest_size, GFP_KERNEL);
    65		if (!digest) {
    66			ret = -ENOMEM;
    67			goto error;
    68		}
    69	
    70		desc = kzalloc(desc_size, GFP_KERNEL);
    71		if (!desc) {
    72			ret = -ENOMEM;
    73			goto error;
    74		}
    75	
    76		desc->tfm = tfm;
    77	
  > 78		ret = crypto_shash_digest(desc, policy, policy_len, digest);
    79		if (ret < 0)
    80			goto error;
    81	
    82		*policy_hash_len = digest_size;
    83		*policy_hash = digest;
    84		digest = NULL;
    85	
    86	error:
    87		kfree(desc);
    88		kfree(digest);
    89	
  > 90		crypto_free_shash(tfm);
    91	
    92		return ret;
    93	}
    94	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Signed-off-by: kernel test robot <redacted>
---
 services.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 79a6b462f1fe9..4374c75f91a21 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c

@@ -3734,8 +3734,8 @@ static int security_read_policy_len(struct selinux_state *state, size_t *len)
  * @data: binary policy data
  * @len: length of data in bytes
  */
-int security_read_selinux_policy(struct selinux_state *state,
-				 void **data, size_t *len)
+static int security_read_selinux_policy(struct selinux_state *state,
+					void **data, size_t *len)
 {
 	struct policydb *policydb = &state->ss->policydb;
 	int rc;

Hi Lakshmi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Lakshmi-Ramasubramanian/LSM-Measure-security-module-state/20200718-063111
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
config: x86_64-randconfig-s022-20200717 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-49-g707c5017-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>


sparse warnings: (new ones prefixed by >>)

quoted

security/selinux/ss/services.c:3737:5: sparse: sparse: symbol 'security_read_selinux_policy' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

On 7/17/20 8:14 PM, kernel test robot wrote:

Hi Lakshmi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on integrity/next-integrity]
[cannot apply to pcmoore-selinux/next security/next-testing linus/master v5.8-rc5 next-20200717]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

Thank you for catching this.

I did not see these failures with the compiler and make options I'd used.

Am able to reproduce the errors with the instructions you'd provided.
Will post the updated patches shortly.

thanks,
  -lakshmi

On Fri, Jul 17, 2020 at 6:28 PM Lakshmi Ramasubramanian
[off-list ref] wrote:

SELinux configuration and policy are some of the critical data for this
security module that needs to be measured. To enable this measurement
SELinux needs to implement the interface function,
security_measure_data(), that the LSM can call.

Define the security_measure_data() function in SELinux to measure SELinux
configuration and policy. Call this function to measure SELinux data
when there is a change in the security module's state.

Sample measurement of SELinux state and hash of the policy:

10 e32e...5ac3 ima-buf sha256:86e8...4594 selinux-state 656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b
10 f4a7...9408 ima-buf sha256:4941...68fc selinux-policy-hash 8d1d...1834

To verify the measurement check the following:

Execute the following command to extract the measured data
from the IMA log for SELinux configuration (selinux-state).

  cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-state" | cut -d' ' -f 6 | xxd -r -p

The output should be the list of key-value pairs. For example,
 enabled=1;enforcing=0;checkreqprot=1;network_peer_controls=1;open_perms=1;extended_socket_class=1;always_check_network=0;cgroup_seclabel=1;nnp_nosuid_transition=1;genfs_seclabel_symlinks=0;

To verify the measured data with the current SELinux state:

 => enabled should be set to 1 if /sys/fs/selinux folder exists,
    0 otherwise

For other entries, compare the integer value in the files
 => /sys/fs/selinux/enforce
 => /sys/fs/selinux/checkreqprot
And, each of the policy capabilities files under
 => /sys/fs/selinux/policy_capabilities

The data for selinux-policy-hash is the SHA256 hash of SELinux policy.

To verify the measured data with the current SELinux policy run
the following commands and verify the output hash values match.

  sha256sum /sys/fs/selinux/policy | cut -d' ' -f 1

  cat /sys/kernel/security/integrity/ima/ascii_runtime_measurements | grep -m 1 "selinux-policy-hash" | cut -d' ' -f 6

Signed-off-by: Lakshmi Ramasubramanian <redacted>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
---

checkreqprot (spelling)

What about initialized?  Or do you consider that to be implicitly
true/1 else we wouldn't be taking a measurement?  Only caveat there is
that it provides one more means of disabling measurements (at the same
time as disabling enforcement) by setting it to false/0 via kernel
write flaw.

+        * All policy capability flags
+        */
+       selinux_state_count = 3 + __POLICYDB_CAPABILITY_MAX;
+
+       selinux_state_string_len = snprintf(NULL, 0, str_format,
+                                           "enabled", 0);
+       selinux_state_string_len += snprintf(NULL, 0, str_format,
+                                            "enforcing", 0);
+       selinux_state_string_len += snprintf(NULL, 0, str_format,
+                                            "checkreqprot", 0);
+       for (i = 3; i < selinux_state_count; i++) {
+               selinux_state_string_len +=
+                       snprintf(NULL, 0, str_format,
+                                selinux_policycap_names[i-3], 0);
+       }

What's the benefit of this pattern versus just making the loop go from
0 to __POLICYDB_CAPABILITY_MAX and using selinux_policycap_names[i]?

+void selinux_measure_state(struct selinux_state *selinux_state)
+{
+       void *policy = NULL;
+       void *policy_hash = NULL;
+       size_t curr, buflen;
+       int i, policy_hash_len, rc = 0;
+
+       if (!selinux_initialized(selinux_state)) {
+               pr_warn("%s: SELinux not yet initialized.\n", __func__);
+               return;
+       }

We could measure the global state variables before full SELinux
initialization (i.e. policy load).
Only the policy hash depends on having loaded the policy.

+
+       if (!selinux_state_string) {
+               pr_warn("%s: Buffer for state not allocated.\n", __func__);
+               return;
+       }
+
+       curr = snprintf(selinux_state_string, selinux_state_string_len,
+                       str_format, "enabled",
+                       !selinux_disabled(selinux_state));
+       curr += snprintf((selinux_state_string + curr),
+                        (selinux_state_string_len - curr),
+                        str_format, "enforcing",
+                        enforcing_enabled(selinux_state));
+       curr += snprintf((selinux_state_string + curr),
+                        (selinux_state_string_len - curr),
+                        str_format, "checkreqprot",
+                        selinux_checkreqprot(selinux_state));
+
+       for (i = 3; i < selinux_state_count; i++) {
+               curr += snprintf((selinux_state_string + curr),
+                                (selinux_state_string_len - curr),
+                                str_format,
+                                selinux_policycap_names[i - 3],
+                                selinux_state->policycap[i - 3]);
+       }

Same question here as for the previous loop; seems cleaner to go from
0 to __POLICYDB_CAPABILITY_MAX and use [i].

What public git tree / branch would you recommend trying to use your
patches against?  Didn't seem to apply to any of the obvious ones.

On 7/20/20 7:31 AM, Stephen Smalley wrote:

quoted

+void __init selinux_init_measurement(void)
+{
+       int i;
+
+       /*
+        * enabled
+        * enforcing
+        * checkreqport

checkreqprot (spelling)

:( - will fix that.

What about initialized?  Or do you consider that to be implicitly
true/1 else we wouldn't be taking a measurement?  Only caveat there is
that it provides one more means of disabling measurements (at the same
time as disabling enforcement) by setting it to false/0 via kernel
write flaw.

Yes - I was thinking measuring SELinux state would be meaningful only 
when initialized is set to true/1.

I can include "initialized" as well in the measurement.

quoted

+        * All policy capability flags
+        */
+       selinux_state_count = 3 + __POLICYDB_CAPABILITY_MAX;
+
+       selinux_state_string_len = snprintf(NULL, 0, str_format,
+                                           "enabled", 0);
+       selinux_state_string_len += snprintf(NULL, 0, str_format,
+                                            "enforcing", 0);
+       selinux_state_string_len += snprintf(NULL, 0, str_format,
+                                            "checkreqprot", 0);
+       for (i = 3; i < selinux_state_count; i++) {
+               selinux_state_string_len +=
+                       snprintf(NULL, 0, str_format,
+                                selinux_policycap_names[i-3], 0);
+       }

What's the benefit of this pattern versus just making the loop go from
0 to __POLICYDB_CAPABILITY_MAX and using selinux_policycap_names[i]?

No real benefit - I was just trying to use selinux_state_count.
I'll change the loop to go from 0 to POLICY_CAP_MAX

quoted

+void selinux_measure_state(struct selinux_state *selinux_state)
+{
+       void *policy = NULL;
+       void *policy_hash = NULL;
+       size_t curr, buflen;
+       int i, policy_hash_len, rc = 0;
+
+       if (!selinux_initialized(selinux_state)) {
+               pr_warn("%s: SELinux not yet initialized.\n", __func__);
+               return;
+       }

We could measure the global state variables before full SELinux
initialization (i.e. policy load).
Only the policy hash depends on having loaded the policy.

Thanks for the information. I'll measure the state variables always and 
measure policy only if "initialized" is true/1.

quoted

+
+       if (!selinux_state_string) {
+               pr_warn("%s: Buffer for state not allocated.\n", __func__);
+               return;
+       }
+
+       curr = snprintf(selinux_state_string, selinux_state_string_len,
+                       str_format, "enabled",
+                       !selinux_disabled(selinux_state));
+       curr += snprintf((selinux_state_string + curr),
+                        (selinux_state_string_len - curr),
+                        str_format, "enforcing",
+                        enforcing_enabled(selinux_state));
+       curr += snprintf((selinux_state_string + curr),
+                        (selinux_state_string_len - curr),
+                        str_format, "checkreqprot",
+                        selinux_checkreqprot(selinux_state));
+
+       for (i = 3; i < selinux_state_count; i++) {
+               curr += snprintf((selinux_state_string + curr),
+                                (selinux_state_string_len - curr),
+                                str_format,
+                                selinux_policycap_names[i - 3],
+                                selinux_state->policycap[i - 3]);
+       }

Same question here as for the previous loop; seems cleaner to go from
0 to __POLICYDB_CAPABILITY_MAX and use [i].

Will change it.

What public git tree / branch would you recommend trying to use your
patches against?  Didn't seem to apply to any of the obvious ones.

Please try it on Mimi's next-integrity branch

https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/log/?h=next-integrity

You can try it on Linus's mainline as well if you apply the following 
patch first (have mentioned that in the Cover letter as well)


     https://patchwork.kernel.org/patch/11612989/

Thanks for trying out the changes. Please let me know the defects you find.

Just to let you know - I am making the following change (will update in 
the next patch):

  => Save the last policy hash and state string in selinux_state struct.
  => Measure policy and hash only if it has changed since the last 
measurement.
  => Also, suffix the IMA event name used with time stamp. For example,

10 e32e...5ac3 ima-buf sha256:86e8...4594
selinux-state-1595257807:874963248 
656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b

10 f4a7...9408 ima-buf sha256:4941...68fc
selinux-policy-hash-1595257807:874963248
8d1d...1834

The above will ensure the following sequence will be measured:
  #1 State A - Measured
  #2 Change from State A to State B - Measured
  #3 Change from State B back to State A - Since the measured data is 
same as in #1, the change will be measured only if the event name is 
different between #1 and #3

thanks,
  -lakshmi

On Mon, Jul 20, 2020 at 11:17 AM Lakshmi Ramasubramanian
[off-list ref] wrote:

Thanks for trying out the changes. Please let me know the defects you find.

Just to let you know - I am making the following change (will update in
the next patch):

  => Save the last policy hash and state string in selinux_state struct.
  => Measure policy and hash only if it has changed since the last
measurement.
  => Also, suffix the IMA event name used with time stamp. For example,

10 e32e...5ac3 ima-buf sha256:86e8...4594
selinux-state-1595257807:874963248
656e61626c65643d313b656e666f7263696e673d303b636865636b72657170726f743d313b6e6574706565723d313b6f70656e7065726d3d313b657874736f636b636c6173733d313b616c776179736e6574776f726b3d303b6367726f75707365636c6162656c3d313b6e6e706e6f737569647472616e736974696f6e3d313b67656e66737365636c6162656c73796d6c696e6b3d303b

10 f4a7...9408 ima-buf sha256:4941...68fc
selinux-policy-hash-1595257807:874963248
8d1d...1834

The above will ensure the following sequence will be measured:
  #1 State A - Measured
  #2 Change from State A to State B - Measured
  #3 Change from State B back to State A - Since the measured data is
same as in #1, the change will be measured only if the event name is
different between #1 and #3

Perhaps the timestamp / sequence number should be part of the hashed
data instead of the event name?
I can see the appraiser wanting to know two things:
1) The current state of the system (e.g. is it enforcing, is the
currently loaded policy the expected one?).
2) Has the system ever been in an unexpected state (e.g. was it
temporarily switched to permissive or had an unexpected policy
loaded?)

I applied the patch series on top of the next-integrity branch, added
measure func=LSM_STATE to ima-policy, and booted that kernel.  I get
the following entries in ascii_runtime_measurements, but seemingly
missing the final field:

10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
selinux-state
10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
selinux-policy-hash

Thus, I cannot verify. What am I missing?

On Mon, 2020-07-20 at 13:06 -0400, Stephen Smalley wrote:

I applied the patch series on top of the next-integrity branch, added
measure func=LSM_STATE to ima-policy, and booted that kernel.  I get
the following entries in ascii_runtime_measurements, but seemingly
missing the final field:

10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
selinux-state
10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
selinux-policy-hash

Thus, I cannot verify. What am I missing?

Missing is "template=ima-buf" on the policy rule.

Tyler's patch set just added some support for verifying the policy.
 Refer to ima_validate_rule().  There are still some things missing.
 For example, nayna noticed that making sure that asymmetric key
support is enabled.  Another example is requiring "template=" for any
of the buffer measurements.  Template names can be defined
dynamically, so it will need to support either format:

measure func=KEXEC_CMDLINE template=ima-buf 
measure func=KEXEC_CMDLINE template=d-ng|n-ng|buf

Mimi

On 7/20/20 10:06 AM, Stephen Smalley wrote:

quoted

The above will ensure the following sequence will be measured:
   #1 State A - Measured
   #2 Change from State A to State B - Measured
   #3 Change from State B back to State A - Since the measured data is
same as in #1, the change will be measured only if the event name is
different between #1 and #3

Perhaps the timestamp / sequence number should be part of the hashed
data instead of the event name?

If the timestamp/seqno is part of the hashed data, on every call to 
measure IMA will add a new entry in the IMA log. This would fill up the 
IMA log - even when there is no change in the measured data.

To avoid that I keep the last measurement in SELinux and measure only 
when there is a change with the timestamp in the event name.

I can see the appraiser wanting to know two things:
1) The current state of the system (e.g. is it enforcing, is the
currently loaded policy the expected one?).
2) Has the system ever been in an unexpected state (e.g. was it
temporarily switched to permissive or had an unexpected policy
loaded?)

Yes - you are right.
The appraiser will have to look at the entire IMA log (and the 
corresponding TPM PCR data) to know the above.

Time t0 => State of the system measured
Time tn => State changed and the new state measured
Time tm => State changed again and the new state measured.

Say, the measurement at "Time tn" was an illegal change, the appraiser 
would know.

I applied the patch series on top of the next-integrity branch, added
measure func=LSM_STATE to ima-policy, and booted that kernel.  I get
the following entries in ascii_runtime_measurements, but seemingly
missing the final field:

10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
selinux-state
10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
selinux-policy-hash

Thus, I cannot verify. What am I missing?

Looks like the template used is ima-ng which doesn't include the 
measured buffer. Please set template to "ima-buf" in the policy.

For example,
measure func=LSM_STATE template=ima-buf

thanks,
  -lakshmi

On Mon, Jul 20, 2020 at 1:34 PM Lakshmi Ramasubramanian
[off-list ref] wrote:

On 7/20/20 10:06 AM, Stephen Smalley wrote:

quoted

quoted

The above will ensure the following sequence will be measured:
   #1 State A - Measured
   #2 Change from State A to State B - Measured
   #3 Change from State B back to State A - Since the measured data is
same as in #1, the change will be measured only if the event name is
different between #1 and #3

Perhaps the timestamp / sequence number should be part of the hashed
data instead of the event name?

If the timestamp/seqno is part of the hashed data, on every call to
measure IMA will add a new entry in the IMA log. This would fill up the
IMA log - even when there is no change in the measured data.

To avoid that I keep the last measurement in SELinux and measure only
when there is a change with the timestamp in the event name.

quoted

I can see the appraiser wanting to know two things:
1) The current state of the system (e.g. is it enforcing, is the
currently loaded policy the expected one?).
2) Has the system ever been in an unexpected state (e.g. was it
temporarily switched to permissive or had an unexpected policy
loaded?)

Yes - you are right.
The appraiser will have to look at the entire IMA log (and the
corresponding TPM PCR data) to know the above.

Time t0 => State of the system measured
Time tn => State changed and the new state measured
Time tm => State changed again and the new state measured.

Say, the measurement at "Time tn" was an illegal change, the appraiser
would know.

quoted

I applied the patch series on top of the next-integrity branch, added
measure func=LSM_STATE to ima-policy, and booted that kernel.  I get
the following entries in ascii_runtime_measurements, but seemingly
missing the final field:

10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
selinux-state
10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
selinux-policy-hash

Thus, I cannot verify. What am I missing?

Looks like the template used is ima-ng which doesn't include the
measured buffer. Please set template to "ima-buf" in the policy.

For example,
measure func=LSM_STATE template=ima-buf

It seems like one shouldn't need to manually specify it if it is the
only template that yields a useful result for the LSM_STATE function?

On Mon, Jul 20, 2020 at 1:40 PM Stephen Smalley
[off-list ref] wrote:

On Mon, Jul 20, 2020 at 1:34 PM Lakshmi Ramasubramanian
[off-list ref] wrote:

quoted

On 7/20/20 10:06 AM, Stephen Smalley wrote:

quoted

quoted

The above will ensure the following sequence will be measured:
   #1 State A - Measured
   #2 Change from State A to State B - Measured
   #3 Change from State B back to State A - Since the measured data is
same as in #1, the change will be measured only if the event name is
different between #1 and #3

Perhaps the timestamp / sequence number should be part of the hashed
data instead of the event name?

If the timestamp/seqno is part of the hashed data, on every call to
measure IMA will add a new entry in the IMA log. This would fill up the
IMA log - even when there is no change in the measured data.

To avoid that I keep the last measurement in SELinux and measure only
when there is a change with the timestamp in the event name.

quoted

I can see the appraiser wanting to know two things:
1) The current state of the system (e.g. is it enforcing, is the
currently loaded policy the expected one?).
2) Has the system ever been in an unexpected state (e.g. was it
temporarily switched to permissive or had an unexpected policy
loaded?)

Yes - you are right.
The appraiser will have to look at the entire IMA log (and the
corresponding TPM PCR data) to know the above.

Time t0 => State of the system measured
Time tn => State changed and the new state measured
Time tm => State changed again and the new state measured.

Say, the measurement at "Time tn" was an illegal change, the appraiser
would know.

quoted

I applied the patch series on top of the next-integrity branch, added
measure func=LSM_STATE to ima-policy, and booted that kernel.  I get
the following entries in ascii_runtime_measurements, but seemingly
missing the final field:

10 8a09c48af4f8a817f59b495bd82971e096e2e367 ima-ng
sha256:21c3d7b09b62b4d0b3ed15ba990f816b94808f90b76787bfae755c4b3a44cd24
selinux-state
10 e610908931d70990a2855ddb33c16af2d82ce56a ima-ng
sha256:c8898652afd5527ef4eaf8d85f5fee1d91fcccee34bc97f6e55b96746bedb318
selinux-policy-hash

Thus, I cannot verify. What am I missing?

Looks like the template used is ima-ng which doesn't include the
measured buffer. Please set template to "ima-buf" in the policy.

For example,
measure func=LSM_STATE template=ima-buf

It seems like one shouldn't need to manually specify it if it is the
only template that yields a useful result for the LSM_STATE function?

Actually, if we used ima-ng template for selinux-policy-hash, then
instead of needing to hash the policy
first and passing the hash to IMA, we could just pass the policy as
the buffer and IMA would take care of the hashing, right?
And we only need to use ima-buf for the selinux-state if we want the
measurement list to include the string value that
was hashed; if we just want to compare against a known-good, it would
suffice to use ima-ng for it as well, right?

On 7/20/20 10:49 AM, Stephen Smalley wrote:

quoted

quoted

Looks like the template used is ima-ng which doesn't include the
measured buffer. Please set template to "ima-buf" in the policy.

For example,
measure func=LSM_STATE template=ima-buf

It seems like one shouldn't need to manually specify it if it is the
only template that yields a useful result for the LSM_STATE function?

Actually, if we used ima-ng template for selinux-policy-hash, then
instead of needing to hash the policy
first and passing the hash to IMA, we could just pass the policy as
the buffer and IMA would take care of the hashing, right?

That is correct.

The IMA hook I've added to measure LSM structures is a generic one that 
can be used by any security module (SM). I feel it would be better to 
not have policy or state or any such SM specific logic in IMA, but leave 
that to the individual SM to handle.

What do you think?

And we only need to use ima-buf for the selinux-state if we want the
measurement list to include the string value that
was hashed; if we just want to compare against a known-good, it would
suffice to use ima-ng for it as well, right?

  -lakshmi

On Mon, Jul 20, 2020 at 2:27 PM Lakshmi Ramasubramanian
[off-list ref] wrote:

On 7/20/20 10:49 AM, Stephen Smalley wrote:

quoted

quoted

quoted

Looks like the template used is ima-ng which doesn't include the
measured buffer. Please set template to "ima-buf" in the policy.

For example,
measure func=LSM_STATE template=ima-buf

It seems like one shouldn't need to manually specify it if it is the
only template that yields a useful result for the LSM_STATE function?

Actually, if we used ima-ng template for selinux-policy-hash, then
instead of needing to hash the policy
first and passing the hash to IMA, we could just pass the policy as
the buffer and IMA would take care of the hashing, right?

That is correct.

The IMA hook I've added to measure LSM structures is a generic one that
can be used by any security module (SM). I feel it would be better to
not have policy or state or any such SM specific logic in IMA, but leave
that to the individual SM to handle.

What do you think?

It is correct to remain security module agnostic.  However, I think
you can remain LSM-neutral while still avoiding the double hashing of
the policy here.  Can't you just pass in the policy itself as the
buffer and let IMA hash it?  Then you can let the policy author decide
on the template to be used (ima-buf versus ima-ng).  If you want to
support the use of different templates for different "kinds" of LSM
state (e.g. state versus policy) you could either provide two funcs
(LSM_STATE, LSM_POLICY) or otherwise support selection based on some
other attribute.

On 7/20/20 11:44 AM, Stephen Smalley wrote:

quoted

quoted

Actually, if we used ima-ng template for selinux-policy-hash, then
instead of needing to hash the policy
first and passing the hash to IMA, we could just pass the policy as
the buffer and IMA would take care of the hashing, right?

That is correct.

The IMA hook I've added to measure LSM structures is a generic one that
can be used by any security module (SM). I feel it would be better to
not have policy or state or any such SM specific logic in IMA, but leave
that to the individual SM to handle.

What do you think?

It is correct to remain security module agnostic.  However, I think
you can remain LSM-neutral while still avoiding the double hashing of
the policy here.  Can't you just pass in the policy itself as the
buffer and let IMA hash it?

Yes - that is an option. If I do that then, as you have stated below, 
we'll need to two funcs -
one that will only add the hash but not the entire data payload in the 
IMA log (i.e., "ima-ng")
and, the other that handles hashing and including date payload (i.e., 
"ima-buf").

   Then you can let the policy author decide

on the template to be used (ima-buf versus ima-ng).  If you want to
support the use of different templates for different "kinds" of LSM
state (e.g. state versus policy) you could either provide two funcs
(LSM_STATE, LSM_POLICY) or otherwise support selection based on some
other attribute.

I can do the above.

  -lakshmi