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

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

[Lakshmi Ramasubramanian]

Critical data structures of security modules are currently not measured.
Therefore an attestation service, for instance, would not be able to
attest whether 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 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-rc1

Lakshmi Ramasubramanian (5):
  IMA: Add LSM_STATE func to measure LSM data
  IMA: Define an IMA hook to measure LSM data
  LSM: Add security_state function pointer 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            |  5 ++
 security/integrity/ima/ima.h         |  1 +
 security/integrity/ima/ima_api.c     |  2 +-
 security/integrity/ima/ima_main.c    | 30 +++++++++
 security/integrity/ima/ima_policy.c  | 28 +++++++--
 security/security.c                  | 91 +++++++++++++++++++++++++++-
 security/selinux/hooks.c             | 43 +++++++++++++
 security/selinux/include/security.h  |  2 +
 security/selinux/selinuxfs.c         |  1 +
 11 files changed, 205 insertions(+), 8 deletions(-)

-- 
2.27.0

[PATCH 1/5] IMA: Add LSM_STATE func to measure LSM data

[Lakshmi Ramasubramanian]

Data provided by security modules need to be measured. 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 <nramas@linux.microsoft.com>
---
 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  | 28 +++++++++++++++++++++++-----
 4 files changed, 30 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 df93ac258e01..58c62269028a 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)		\
 	hook(KEXEC_CMDLINE)		\
 	hook(KEY_CHECK)			\
+	hook(LSM_STATE)			\
 	hook(MAX_CHECK)
 #define __ima_hook_enumify(ENUM)	ENUM,
 
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 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 e493063a3c34..1a6ee09e6993 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,8 @@ 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

[PATCH 2/5] IMA: Define an IMA hook to measure LSM data

[Lakshmi Ramasubramanian]

LSM requires an IMA hook to be defined by the IMA subsystem to measure
the data gathered from the security modules.

Define a new IMA hook, namely ima_lsm_state(), that the LSM will call
to measure the data gathered from the security modules.

Sample IMA log entry for LSM measurement:

10 47eed9... ima-buf sha256:402f6b... lsm-state:selinux 656e61626c65643d313b656e666f7263696e673d30

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

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..56681a648b3d 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_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_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 c1583d98c5e5..34be962054fb 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -827,6 +827,36 @@ void ima_kexec_cmdline(const void *buf, int size)
 					   KEXEC_CMDLINE, 0, NULL);
 }
 
+/**
+ * ima_lsm_state - measure LSM specific state
+ * @lsm_name: Name of the LSM
+ * @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_name, const void *buf, int size)
+{
+	const char *eventname = "lsm-state:";
+	char *lsmstatestring;
+	int lsmstatelen;
+
+	if (!lsm_name || !buf || !size)
+		return;
+
+	lsmstatelen = strlen(eventname) + strlen(lsm_name) + 1;
+	lsmstatestring = kzalloc(lsmstatelen, GFP_KERNEL);
+	if (!lsmstatestring)
+		return;
+
+	strcpy(lsmstatestring, eventname);
+	strcat(lsmstatestring, lsm_name);
+
+	process_buffer_measurement(buf, size, lsmstatestring,
+				   LSM_STATE, 0, NULL);
+	kfree(lsmstatestring);
+}
+
 static int __init init_ima(void)
 {
 	int error;
-- 
2.27.0

[PATCH 3/5] LSM: Add security_state function pointer in lsm_info struct

[Lakshmi Ramasubramanian]

The security modules that require their data to be measured need to
define a function that the LSM can call to gather the data.

Add a function pointer field namely security_state in lsm_info structure.
Update LSM to call this security module function, if defined, to gather
data and measure it by calling the IMA hook ima_lsm_state().

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 include/linux/lsm_hooks.h |  2 ++
 security/security.c       | 60 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 3e62dab77699..da248c3fd4ac 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1568,6 +1568,8 @@ struct lsm_info {
 	int *enabled;		/* Optional: controlled by CONFIG_LSM */
 	int (*init)(void);	/* Required. */
 	struct lsm_blob_sizes *blobs; /* Optional: for blob sharing. */
+	int (*security_state)(char **lsm_name, void **state,
+			      int *state_len); /*Optional */
 };
 
 extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
diff --git a/security/security.c b/security/security.c
index e0290b7e6a08..a6e2d1cd95af 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,57 @@ static void __init initialize_lsm(struct lsm_info *lsm)
 	}
 }
 
+static int measure_security_state(struct lsm_info *lsm)
+{
+	char *lsm_name = NULL;
+	void *state = NULL;
+	int state_len = 0;
+	int rc;
+
+	if (!lsm->security_state)
+		return 0;
+
+	rc = lsm->security_state(&lsm_name, &state, &state_len);
+	if ((rc == 0) && (state_len > 0)) {
+		ima_lsm_state(lsm_name, state, state_len);
+		kfree(state);
+		kfree(lsm_name);
+	}
+
+	return rc;
+}
+
+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_state)
+			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_state) {
+			security_state_lsms[inx].security_state =
+				(*lsm)->security_state;
+			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 +406,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

[PATCH 4/5] LSM: Define SELinux function to measure security state

[Lakshmi Ramasubramanian]

SELinux needs to implement the interface function, security_state(), for
the LSM to gather SELinux data for measuring. Define the security_state()
function in SELinux.

The security modules should be able to notify the LSM when there is
a change in the module's data. Define a function namely 
security_state_change() in the LSM that the security modules
can call to provide the updated data for measurement.

Call security_state_change() function from SELinux to report data
when SELinux's state is updated.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 include/linux/lsm_hooks.h           |  3 ++
 security/security.c                 |  5 ++++
 security/selinux/hooks.c            | 43 +++++++++++++++++++++++++++++
 security/selinux/include/security.h |  2 ++
 security/selinux/selinuxfs.c        |  1 +
 5 files changed, 54 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index da248c3fd4ac..a63de046139e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1572,6 +1572,9 @@ struct lsm_info {
 			      int *state_len); /*Optional */
 };
 
+/* Called by LSMs to notify security state change */
+extern void security_state_change(char *lsm_name, void *state, int state_len);
+
 extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
 extern struct lsm_info __start_early_lsm_info[], __end_early_lsm_info[];
 
diff --git a/security/security.c b/security/security.c
index a6e2d1cd95af..e7175db5a093 100644
--- a/security/security.c
+++ b/security/security.c
@@ -238,6 +238,11 @@ static void __init initialize_lsm(struct lsm_info *lsm)
 	}
 }
 
+void security_state_change(char *lsm_name, void *state, int state_len)
+{
+	ima_lsm_state(lsm_name, state, state_len);
+}
+
 static int measure_security_state(struct lsm_info *lsm)
 {
 	char *lsm_name = NULL;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7e954b555be6..bbc908a1fcd1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7225,6 +7225,47 @@ static __init int selinux_init(void)
 	return 0;
 }
 
+static int selinux_security_state(char **lsm_name, void **state,
+				  int *state_len)
+{
+	int rc = 0;
+	char *new_state;
+	static char *security_state_string = "enabled=%d;enforcing=%d";
+
+	*lsm_name = kstrdup("selinux", GFP_KERNEL);
+	if (!*lsm_name)
+		return -ENOMEM;
+
+	new_state = kzalloc(strlen(security_state_string) + 1, GFP_KERNEL);
+	if (!new_state) {
+		kfree(*lsm_name);
+		*lsm_name = NULL;
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	*state_len = sprintf(new_state, security_state_string,
+			     !selinux_disabled(&selinux_state),
+			     enforcing_enabled(&selinux_state));
+	*state = new_state;
+
+out:
+	return rc;
+}
+
+void notify_security_state_change(void)
+{
+	char *lsm_name = NULL;
+	void *state = NULL;
+	int state_len = 0;
+
+	if (!selinux_security_state(&lsm_name, &state, &state_len)) {
+		security_state_change(lsm_name, state, state_len);
+		kfree(state);
+		kfree(lsm_name);
+	}
+}
+
 static void delayed_superblock_init(struct super_block *sb, void *unused)
 {
 	selinux_set_mnt_opts(sb, NULL, 0, NULL);
@@ -7247,6 +7288,7 @@ DEFINE_LSM(selinux) = {
 	.enabled = &selinux_enabled_boot,
 	.blobs = &selinux_blob_sizes,
 	.init = selinux_init,
+	.security_state = selinux_security_state,
 };
 
 #if defined(CONFIG_NETFILTER)
@@ -7357,6 +7399,7 @@ int selinux_disable(struct selinux_state *state)
 	}
 
 	selinux_mark_disabled(state);
+	notify_security_state_change();
 
 	pr_info("SELinux:  Disabled at runtime.\n");
 
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index b0e02cfe3ce1..83c6ada45c7c 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -232,6 +232,8 @@ size_t security_policydb_len(struct selinux_state *state);
 int security_policycap_supported(struct selinux_state *state,
 				 unsigned int req_cap);
 
+void notify_security_state_change(void);
+
 #define SEL_VEC_MAX 32
 struct av_decision {
 	u32 allowed;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 4781314c2510..8a5ba32a7775 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);
+		notify_security_state_change();
 		if (new_value)
 			avc_ss_reset(state->avc, 0);
 		selnl_notify_setenforce(new_value);
-- 
2.27.0

[PATCH 5/5] LSM: Define workqueue for measuring security module state

[Lakshmi Ramasubramanian]

The data maintained by the security modules could be tampered with by
malware. The LSM needs to periodically query the state of
the security modules and measure the data when the state is changed.

Define a workqueue for handling this periodic query and measurement.

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

diff --git a/security/security.c b/security/security.c
index e7175db5a093..3dad6766cb9d 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 {							\
@@ -294,6 +299,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)
 {
@@ -417,6 +442,7 @@ static void __init ordered_lsm_init(void)
 	}
 
 	initialize_security_state_lsms();
+	initialize_security_state_monitor();
 
 	kfree(ordered_lsms);
 }
-- 
2.27.0

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Stephen Smalley]

On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> SELinux needs to implement the interface function, security_state(), for
> the LSM to gather SELinux data for measuring. Define the security_state()
> function in SELinux.
>
> The security modules should be able to notify the LSM when there is
> a change in the module's data. Define a function namely
> security_state_change() in the LSM that the security modules
> can call to provide the updated data for measurement.
>
> Call security_state_change() function from SELinux to report data
> when SELinux's state is updated.
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> diff --git a/security/security.c b/security/security.c
> index a6e2d1cd95af..e7175db5a093 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -238,6 +238,11 @@ static void __init initialize_lsm(struct lsm_info *lsm)
>         }
>  }
>
> +void security_state_change(char *lsm_name, void *state, int state_len)
> +{
> +       ima_lsm_state(lsm_name, state, state_len);
> +}
> +

What's the benefit of this trivial function instead of just calling
ima_lsm_state() directly?

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 7e954b555be6..bbc908a1fcd1 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -7225,6 +7225,47 @@ static __init int selinux_init(void)
>         return 0;
>  }
>
> +static int selinux_security_state(char **lsm_name, void **state,
> +                                 int *state_len)
> +{
> +       int rc = 0;
> +       char *new_state;
> +       static char *security_state_string = "enabled=%d;enforcing=%d";
> +
> +       *lsm_name = kstrdup("selinux", GFP_KERNEL);
> +       if (!*lsm_name)
> +               return -ENOMEM;
> +
> +       new_state = kzalloc(strlen(security_state_string) + 1, GFP_KERNEL);
> +       if (!new_state) {
> +               kfree(*lsm_name);
> +               *lsm_name = NULL;
> +               rc = -ENOMEM;
> +               goto out;
> +       }
> +
> +       *state_len = sprintf(new_state, security_state_string,
> +                            !selinux_disabled(&selinux_state),
> +                            enforcing_enabled(&selinux_state));

I think I mentioned this on a previous version of these patches, but I
would recommend including more than just the enabled and enforcing
states in your measurement.  Other low-hanging fruit would be the
other selinux_state booleans (checkreqprot, initialized,
policycap[0..__POLICYDB_CAPABILITY_MAX]).  Going a bit further one
could take a hash of the loaded policy by using security_read_policy()
and then computing a hash using whatever hash ima prefers over the
returned data,len pair.  You likely also need to think about how to
allow future extensibility of the state in a backward-compatible
manner, so that future additions do not immediately break systems
relying on older measurements.

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Stephen Smalley]

On Mon, Jun 15, 2020 at 7:57 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> I think I mentioned this on a previous version of these patches, but I
> would recommend including more than just the enabled and enforcing
> states in your measurement.  Other low-hanging fruit would be the
> other selinux_state booleans (checkreqprot, initialized,
> policycap[0..__POLICYDB_CAPABILITY_MAX]).  Going a bit further one
> could take a hash of the loaded policy by using security_read_policy()

On second thought, you probably a variant of security_read_policy()
since it would be a kernel-internal allocation and thus shouldn't use
vmalloc_user().

> and then computing a hash using whatever hash ima prefers over the
> returned data,len pair.  You likely also need to think about how to
> allow future extensibility of the state in a backward-compatible
> manner, so that future additions do not immediately break systems
> relying on older measurements.

Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

[Stephen Smalley]

On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> The data maintained by the security modules could be tampered with by
> malware. The LSM needs to periodically query the state of
> the security modules and measure the data when the state is changed.
>
> Define a workqueue for handling this periodic query and measurement.

Won't this make it difficult/impossible to predict the IMA PCR value?
Unless I missed it, you are going to end up measuring every N minutes
even if there was no change and therefore constantly be extending the
PCR.  That will break attestation or sealing against the IMA PCR.

Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

[Mimi Zohar]

On Mon, 2020-06-15 at 09:33 -0400, Stephen Smalley wrote:
> On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
> >
> > The data maintained by the security modules could be tampered with by
> > malware. The LSM needs to periodically query the state of
> > the security modules and measure the data when the state is changed.
> >
> > Define a workqueue for handling this periodic query and measurement.
> 
> Won't this make it difficult/impossible to predict the IMA PCR value?
> Unless I missed it, you are going to end up measuring every N minutes
> even if there was no change and therefore constantly be extending the
> PCR.  That will break attestation or sealing against the IMA PCR.

Even if it attempts to add the same measurement to the list multiple
times, unless something changed, there should only be one measurement
in the list.

Mimi

Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

[Stephen Smalley]

On Mon, Jun 15, 2020 at 10:59 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2020-06-15 at 09:33 -0400, Stephen Smalley wrote:
> > On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
> > <nramas@linux.microsoft.com> wrote:
> > >
> > > The data maintained by the security modules could be tampered with by
> > > malware. The LSM needs to periodically query the state of
> > > the security modules and measure the data when the state is changed.
> > >
> > > Define a workqueue for handling this periodic query and measurement.
> >
> > Won't this make it difficult/impossible to predict the IMA PCR value?
> > Unless I missed it, you are going to end up measuring every N minutes
> > even if there was no change and therefore constantly be extending the
> > PCR.  That will break attestation or sealing against the IMA PCR.
>
> Even if it attempts to add the same measurement to the list multiple
> times, unless something changed, there should only be one measurement
> in the list.

Is the PCR only extended once?

Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state

[Mimi Zohar]

On Mon, 2020-06-15 at 11:47 -0400, Stephen Smalley wrote:
> On Mon, Jun 15, 2020 at 10:59 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Mon, 2020-06-15 at 09:33 -0400, Stephen Smalley wrote:
> > > On Fri, Jun 12, 2020 at 10:42 PM Lakshmi Ramasubramanian
> > > <nramas@linux.microsoft.com> wrote:
> > > >
> > > > The data maintained by the security modules could be tampered with by
> > > > malware. The LSM needs to periodically query the state of
> > > > the security modules and measure the data when the state is changed.
> > > >
> > > > Define a workqueue for handling this periodic query and measurement.
> > >
> > > Won't this make it difficult/impossible to predict the IMA PCR value?
> > > Unless I missed it, you are going to end up measuring every N minutes
> > > even if there was no change and therefore constantly be extending the
> > > PCR.  That will break attestation or sealing against the IMA PCR.
> >
> > Even if it attempts to add the same measurement to the list multiple
> > times, unless something changed, there should only be one measurement
> > in the list.
> 
> Is the PCR only extended once?

Yes, otherwise you wouldn't be able to verify a quote.
 ima_lookup_digest_entry() first verifies the hash isn't in the cache,
before adding it to the measurement list and then extending the TPM.

Mimi

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Lakshmi Ramasubramanian]

On 6/15/20 4:57 AM, Stephen Smalley wrote:

Hi Stephen,

Thanks for reviewing the patches.

>> +void security_state_change(char *lsm_name, void *state, int state_len)
>> +{
>> +       ima_lsm_state(lsm_name, state, state_len);
>> +}
>> +
> 
> What's the benefit of this trivial function instead of just calling
> ima_lsm_state() directly?

One of the feedback Casey Schaufler had given earlier was that calling 
an IMA function directly from SELinux (or, any of the Security Modules) 
would be a layering violation.

LSM framework (security/security.c) already calls IMA functions now (for 
example, ima_bprm_check() is called from security_bprm_check()). I 
followed the same pattern for measuring LSM data as well.

Please let me know if I misunderstood Casey's comment.

>> +static int selinux_security_state(char **lsm_name, void **state,
>> +                                 int *state_len)
>> +{
>> +       int rc = 0;
>> +       char *new_state;
>> +       static char *security_state_string = "enabled=%d;enforcing=%d";
>> +
>> +       *lsm_name = kstrdup("selinux", GFP_KERNEL);
>> +       if (!*lsm_name)
>> +               return -ENOMEM;
>> +
>> +       new_state = kzalloc(strlen(security_state_string) + 1, GFP_KERNEL);
>> +       if (!new_state) {
>> +               kfree(*lsm_name);
>> +               *lsm_name = NULL;
>> +               rc = -ENOMEM;
>> +               goto out;
>> +       }
>> +
>> +       *state_len = sprintf(new_state, security_state_string,
>> +                            !selinux_disabled(&selinux_state),
>> +                            enforcing_enabled(&selinux_state));
> 
> I think I mentioned this on a previous version of these patches, but I
> would recommend including more than just the enabled and enforcing
> states in your measurement.  Other low-hanging fruit would be the
> other selinux_state booleans (checkreqprot, initialized,
> policycap[0..__POLICYDB_CAPABILITY_MAX]).  Going a bit further one
> could take a hash of the loaded policy by using security_read_policy()
> and then computing a hash using whatever hash ima prefers over the
> returned data,len pair.  You likely also need to think about how to
> allow future extensibility of the state in a backward-compatible
> manner, so that future additions do not immediately break systems
> relying on older measurements.
> 

Sure - I will address this one in the next update.

thanks,
  -lakshmi

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Casey Schaufler]

On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
> On 6/15/20 4:57 AM, Stephen Smalley wrote:
>
> Hi Stephen,
>
> Thanks for reviewing the patches.
>
>>> +void security_state_change(char *lsm_name, void *state, int state_len)
>>> +{
>>> +       ima_lsm_state(lsm_name, state, state_len);
>>> +}
>>> +
>>
>> What's the benefit of this trivial function instead of just calling
>> ima_lsm_state() directly?
>
> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.

Hiding the ima_lsm_state() call doesn't address the layering.
The point is that SELinux code being called from IMA (or the
other way around) breaks the subsystem isolation. Unfortunately,
it isn't obvious to me how you would go about what you're doing
without integrating the subsystems.

>
> LSM framework (security/security.c) already calls IMA functions now (for example, ima_bprm_check() is called from security_bprm_check()). I followed the same pattern for measuring LSM data as well.
>
> Please let me know if I misunderstood Casey's comment.
>

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Mimi Zohar]

(Cc'ing John)

On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote:
> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
> > On 6/15/20 4:57 AM, Stephen Smalley wrote:
> >
> > Hi Stephen,
> >
> > Thanks for reviewing the patches.
> >
> >>> +void security_state_change(char *lsm_name, void *state, int state_len)
> >>> +{
> >>> +       ima_lsm_state(lsm_name, state, state_len);
> >>> +}
> >>> +
> >>
> >> What's the benefit of this trivial function instead of just calling
> >> ima_lsm_state() directly?
> >
> > One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.
> 
> Hiding the ima_lsm_state() call doesn't address the layering.
> The point is that SELinux code being called from IMA (or the
> other way around) breaks the subsystem isolation. Unfortunately,
> it isn't obvious to me how you would go about what you're doing
> without integrating the subsystems.

Casey, I'm not sure why you think there is a layering issue here.
 There were multiple iterations of IMA before it was upstreamed.  One
iteration had separate integrity hooks(LIM).  Only when the IMA calls
and the security hooks are co-located, are they combined, as requested
by Linus.

There was some AppArmour discussion about calling IMA directly, but I
haven't heard about it in a while or seen the patch.

Mimi

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Stephen Smalley]

On Mon, Jun 15, 2020 at 12:45 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 6/15/20 4:57 AM, Stephen Smalley wrote:
> > I think I mentioned this on a previous version of these patches, but I
> > would recommend including more than just the enabled and enforcing
> > states in your measurement.  Other low-hanging fruit would be the
> > other selinux_state booleans (checkreqprot, initialized,
> > policycap[0..__POLICYDB_CAPABILITY_MAX]).  Going a bit further one
> > could take a hash of the loaded policy by using security_read_policy()
> > and then computing a hash using whatever hash ima prefers over the
> > returned data,len pair.  You likely also need to think about how to
> > allow future extensibility of the state in a backward-compatible
> > manner, so that future additions do not immediately break systems
> > relying on older measurements.
> >
>
> Sure - I will address this one in the next update.

Please add selinux list to the cc for future versions too.

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Casey Schaufler]

On 6/15/2020 10:44 AM, Mimi Zohar wrote:
> (Cc'ing John)
>
> On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote:
>> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
>>> On 6/15/20 4:57 AM, Stephen Smalley wrote:
>>>
>>> Hi Stephen,
>>>
>>> Thanks for reviewing the patches.
>>>
>>>>> +void security_state_change(char *lsm_name, void *state, int state_len)
>>>>> +{
>>>>> +       ima_lsm_state(lsm_name, state, state_len);
>>>>> +}
>>>>> +
>>>> What's the benefit of this trivial function instead of just calling
>>>> ima_lsm_state() directly?
>>> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.
>> Hiding the ima_lsm_state() call doesn't address the layering.
>> The point is that SELinux code being called from IMA (or the
>> other way around) breaks the subsystem isolation. Unfortunately,
>> it isn't obvious to me how you would go about what you're doing
>> without integrating the subsystems.
> Casey, I'm not sure why you think there is a layering issue here.

I don't think there is, after further review. If the IMA code called
selinux_dosomething() directly I'd be very concerned, but calling
security_dosomething() which then calls selinux_dosomething() is fine.
If YAMA called security_dosomething() I'd be very concerned, but that's
not what's happening here.

>  There were multiple iterations of IMA before it was upstreamed.  One
> iteration had separate integrity hooks(LIM).  Only when the IMA calls
> and the security hooks are co-located, are they combined, as requested
> by Linus.
>
> There was some AppArmour discussion about calling IMA directly, but I
> haven't heard about it in a while or seen the patch.
>
> Mimi

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[Mimi Zohar]

On Mon, 2020-06-15 at 16:18 -0700, Casey Schaufler wrote:
> On 6/15/2020 10:44 AM, Mimi Zohar wrote:
> > (Cc'ing John)
> >
> > On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote:
> >> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
> >>> On 6/15/20 4:57 AM, Stephen Smalley wrote:
> >>>
> >>> Hi Stephen,
> >>>
> >>> Thanks for reviewing the patches.
> >>>
> >>>>> +void security_state_change(char *lsm_name, void *state, int state_len)
> >>>>> +{
> >>>>> +       ima_lsm_state(lsm_name, state, state_len);
> >>>>> +}
> >>>>> +
> >>>> What's the benefit of this trivial function instead of just calling
> >>>> ima_lsm_state() directly?
> >>> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.
> >> Hiding the ima_lsm_state() call doesn't address the layering.
> >> The point is that SELinux code being called from IMA (or the
> >> other way around) breaks the subsystem isolation. Unfortunately,
> >> it isn't obvious to me how you would go about what you're doing
> >> without integrating the subsystems.
> > Casey, I'm not sure why you think there is a layering issue here.
> 
> I don't think there is, after further review. If the IMA code called
> selinux_dosomething() directly I'd be very concerned, but calling
> security_dosomething() which then calls selinux_dosomething() is fine.
> If YAMA called security_dosomething() I'd be very concerned, but that's
> not what's happening here.

As long as the call to IMA is not an LSM hook, there shouldn't be a
problem with an LSM calling IMA directly.  A perfect example is
measuring, appraising and/or auditing LSM policies.

Mimi

> 
> >  There were multiple iterations of IMA before it was upstreamed.  One
> > iteration had separate integrity hooks(LIM).  Only when the IMA calls
> > and the security hooks are co-located, are they combined, as requested
> > by Linus.
> >
> > There was some AppArmour discussion about calling IMA directly, but I
> > haven't heard about it in a while or seen the patch.

Re: [PATCH 4/5] LSM: Define SELinux function to measure security state

[John Johansen]

On 6/15/20 10:44 AM, Mimi Zohar wrote:
> (Cc'ing John)
> 
> On Mon, 2020-06-15 at 10:33 -0700, Casey Schaufler wrote:
>> On 6/15/2020 9:45 AM, Lakshmi Ramasubramanian wrote:
>>> On 6/15/20 4:57 AM, Stephen Smalley wrote:
>>>
>>> Hi Stephen,
>>>
>>> Thanks for reviewing the patches.
>>>
>>>>> +void security_state_change(char *lsm_name, void *state, int state_len)
>>>>> +{
>>>>> +       ima_lsm_state(lsm_name, state, state_len);
>>>>> +}
>>>>> +
>>>>
>>>> What's the benefit of this trivial function instead of just calling
>>>> ima_lsm_state() directly?
>>>
>>> One of the feedback Casey Schaufler had given earlier was that calling an IMA function directly from SELinux (or, any of the Security Modules) would be a layering violation.
>>
>> Hiding the ima_lsm_state() call doesn't address the layering.
>> The point is that SELinux code being called from IMA (or the
>> other way around) breaks the subsystem isolation. Unfortunately,
>> it isn't obvious to me how you would go about what you're doing
>> without integrating the subsystems.
> 
> Casey, I'm not sure why you think there is a layering issue here.
>  There were multiple iterations of IMA before it was upstreamed.  One
> iteration had separate integrity hooks(LIM).  Only when the IMA calls
> and the security hooks are co-located, are they combined, as requested
> by Linus.
> 
I don't see the layering violation here either, Casey has already
responded and I don't have anything to add

> There was some AppArmour discussion about calling IMA directly, but I
> haven't heard about it in a while or seen the patch.
> 
its lower priority than other work. I think calling IMA directly has use
cases but must be done very carefully, and well reviewed. I have would
have more concerns with IMA calling directly into the various LSMs.