linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] LSM: Measure security module state
@ 2020-06-13  2:41 Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 1/5] IMA: Add LSM_STATE func to measure LSM data Lakshmi Ramasubramanian
                   ` (4 more replies)
  0 siblings, 5 replies; 19+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-13  2:41 UTC (permalink / raw)
  To: zohar, stephen.smalley, casey
  Cc: jmorris, linux-integrity, linux-security-module, linux-kernel

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


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

* [PATCH 1/5] IMA: Add LSM_STATE func to measure LSM data
  2020-06-13  2:41 [PATCH 0/5] LSM: Measure security module state Lakshmi Ramasubramanian
@ 2020-06-13  2:41 ` Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 2/5] IMA: Define an IMA hook " Lakshmi Ramasubramanian
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 19+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-13  2:41 UTC (permalink / raw)
  To: zohar, stephen.smalley, casey
  Cc: jmorris, linux-integrity, linux-security-module, linux-kernel

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


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

* [PATCH 2/5] IMA: Define an IMA hook to measure LSM data
  2020-06-13  2:41 [PATCH 0/5] LSM: Measure security module state Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 1/5] IMA: Add LSM_STATE func to measure LSM data Lakshmi Ramasubramanian
@ 2020-06-13  2:41 ` Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 3/5] LSM: Add security_state function pointer in lsm_info struct Lakshmi Ramasubramanian
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 19+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-13  2:41 UTC (permalink / raw)
  To: zohar, stephen.smalley, casey
  Cc: jmorris, linux-integrity, linux-security-module, linux-kernel

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


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

* [PATCH 3/5] LSM: Add security_state function pointer in lsm_info struct
  2020-06-13  2:41 [PATCH 0/5] LSM: Measure security module state Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 1/5] IMA: Add LSM_STATE func to measure LSM data Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 2/5] IMA: Define an IMA hook " Lakshmi Ramasubramanian
@ 2020-06-13  2:41 ` Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 4/5] LSM: Define SELinux function to measure security state Lakshmi Ramasubramanian
  2020-06-13  2:41 ` [PATCH 5/5] LSM: Define workqueue for measuring security module state Lakshmi Ramasubramanian
  4 siblings, 0 replies; 19+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-13  2:41 UTC (permalink / raw)
  To: zohar, stephen.smalley, casey
  Cc: jmorris, linux-integrity, linux-security-module, linux-kernel

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


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

* [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-13  2:41 [PATCH 0/5] LSM: Measure security module state Lakshmi Ramasubramanian
                   ` (2 preceding siblings ...)
  2020-06-13  2:41 ` [PATCH 3/5] LSM: Add security_state function pointer in lsm_info struct Lakshmi Ramasubramanian
@ 2020-06-13  2:41 ` Lakshmi Ramasubramanian
  2020-06-15 11:57   ` Stephen Smalley
  2020-06-13  2:41 ` [PATCH 5/5] LSM: Define workqueue for measuring security module state Lakshmi Ramasubramanian
  4 siblings, 1 reply; 19+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-13  2:41 UTC (permalink / raw)
  To: zohar, stephen.smalley, casey
  Cc: jmorris, linux-integrity, linux-security-module, linux-kernel

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


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

* [PATCH 5/5] LSM: Define workqueue for measuring security module state
  2020-06-13  2:41 [PATCH 0/5] LSM: Measure security module state Lakshmi Ramasubramanian
                   ` (3 preceding siblings ...)
  2020-06-13  2:41 ` [PATCH 4/5] LSM: Define SELinux function to measure security state Lakshmi Ramasubramanian
@ 2020-06-13  2:41 ` Lakshmi Ramasubramanian
  2020-06-15 13:33   ` Stephen Smalley
  4 siblings, 1 reply; 19+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-13  2:41 UTC (permalink / raw)
  To: zohar, stephen.smalley, casey
  Cc: jmorris, linux-integrity, linux-security-module, linux-kernel

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


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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-13  2:41 ` [PATCH 4/5] LSM: Define SELinux function to measure security state Lakshmi Ramasubramanian
@ 2020-06-15 11:57   ` Stephen Smalley
  2020-06-15 12:15     ` Stephen Smalley
  2020-06-15 16:45     ` Lakshmi Ramasubramanian
  0 siblings, 2 replies; 19+ messages in thread
From: Stephen Smalley @ 2020-06-15 11:57 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: Mimi Zohar, Stephen Smalley, Casey Schaufler, James Morris,
	linux-integrity, LSM List, linux-kernel

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.

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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 11:57   ` Stephen Smalley
@ 2020-06-15 12:15     ` Stephen Smalley
  2020-06-15 16:45     ` Lakshmi Ramasubramanian
  1 sibling, 0 replies; 19+ messages in thread
From: Stephen Smalley @ 2020-06-15 12:15 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: Mimi Zohar, Stephen Smalley, Casey Schaufler, James Morris,
	linux-integrity, LSM List, linux-kernel

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.

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

* Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state
  2020-06-13  2:41 ` [PATCH 5/5] LSM: Define workqueue for measuring security module state Lakshmi Ramasubramanian
@ 2020-06-15 13:33   ` Stephen Smalley
  2020-06-15 14:59     ` Mimi Zohar
  0 siblings, 1 reply; 19+ messages in thread
From: Stephen Smalley @ 2020-06-15 13:33 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: Mimi Zohar, Stephen Smalley, Casey Schaufler, James Morris,
	linux-integrity, LSM List, linux-kernel

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.

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

* Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state
  2020-06-15 13:33   ` Stephen Smalley
@ 2020-06-15 14:59     ` Mimi Zohar
  2020-06-15 15:47       ` Stephen Smalley
  0 siblings, 1 reply; 19+ messages in thread
From: Mimi Zohar @ 2020-06-15 14:59 UTC (permalink / raw)
  To: Stephen Smalley, Lakshmi Ramasubramanian
  Cc: Stephen Smalley, Casey Schaufler, James Morris, linux-integrity,
	LSM List, linux-kernel

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

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

* Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state
  2020-06-15 14:59     ` Mimi Zohar
@ 2020-06-15 15:47       ` Stephen Smalley
  2020-06-15 16:10         ` Mimi Zohar
  0 siblings, 1 reply; 19+ messages in thread
From: Stephen Smalley @ 2020-06-15 15:47 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Lakshmi Ramasubramanian, Stephen Smalley, Casey Schaufler,
	James Morris, linux-integrity, LSM List, linux-kernel

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?

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

* Re: [PATCH 5/5] LSM: Define workqueue for measuring security module state
  2020-06-15 15:47       ` Stephen Smalley
@ 2020-06-15 16:10         ` Mimi Zohar
  0 siblings, 0 replies; 19+ messages in thread
From: Mimi Zohar @ 2020-06-15 16:10 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Lakshmi Ramasubramanian, Stephen Smalley, Casey Schaufler,
	James Morris, linux-integrity, LSM List, linux-kernel

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

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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 11:57   ` Stephen Smalley
  2020-06-15 12:15     ` Stephen Smalley
@ 2020-06-15 16:45     ` Lakshmi Ramasubramanian
  2020-06-15 17:33       ` Casey Schaufler
  2020-06-15 20:31       ` Stephen Smalley
  1 sibling, 2 replies; 19+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-15 16:45 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Mimi Zohar, Stephen Smalley, Casey Schaufler, James Morris,
	linux-integrity, LSM List, linux-kernel

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

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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 16:45     ` Lakshmi Ramasubramanian
@ 2020-06-15 17:33       ` Casey Schaufler
  2020-06-15 17:44         ` Mimi Zohar
  2020-06-15 20:31       ` Stephen Smalley
  1 sibling, 1 reply; 19+ messages in thread
From: Casey Schaufler @ 2020-06-15 17:33 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, Stephen Smalley
  Cc: Mimi Zohar, Stephen Smalley, James Morris, linux-integrity,
	LSM List, linux-kernel, 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.
>



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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 17:33       ` Casey Schaufler
@ 2020-06-15 17:44         ` Mimi Zohar
  2020-06-15 23:18           ` Casey Schaufler
  2020-06-16  8:38           ` John Johansen
  0 siblings, 2 replies; 19+ messages in thread
From: Mimi Zohar @ 2020-06-15 17:44 UTC (permalink / raw)
  To: Casey Schaufler, Lakshmi Ramasubramanian, Stephen Smalley, John Johansen
  Cc: Stephen Smalley, James Morris, linux-integrity, LSM List, linux-kernel

(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

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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 16:45     ` Lakshmi Ramasubramanian
  2020-06-15 17:33       ` Casey Schaufler
@ 2020-06-15 20:31       ` Stephen Smalley
  1 sibling, 0 replies; 19+ messages in thread
From: Stephen Smalley @ 2020-06-15 20:31 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: Mimi Zohar, Stephen Smalley, Casey Schaufler, James Morris,
	linux-integrity, LSM List, linux-kernel

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.

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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 17:44         ` Mimi Zohar
@ 2020-06-15 23:18           ` Casey Schaufler
  2020-06-16  0:44             ` Mimi Zohar
  2020-06-16  8:38           ` John Johansen
  1 sibling, 1 reply; 19+ messages in thread
From: Casey Schaufler @ 2020-06-15 23:18 UTC (permalink / raw)
  To: Mimi Zohar, Lakshmi Ramasubramanian, Stephen Smalley
  Cc: Casey Schaufler, Stephen Smalley, James Morris, linux-integrity,
	LSM List, linux-kernel

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


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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 23:18           ` Casey Schaufler
@ 2020-06-16  0:44             ` Mimi Zohar
  0 siblings, 0 replies; 19+ messages in thread
From: Mimi Zohar @ 2020-06-16  0:44 UTC (permalink / raw)
  To: Casey Schaufler, Lakshmi Ramasubramanian, Stephen Smalley
  Cc: Stephen Smalley, James Morris, linux-integrity, LSM List, linux-kernel

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.


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

* Re: [PATCH 4/5] LSM: Define SELinux function to measure security state
  2020-06-15 17:44         ` Mimi Zohar
  2020-06-15 23:18           ` Casey Schaufler
@ 2020-06-16  8:38           ` John Johansen
  1 sibling, 0 replies; 19+ messages in thread
From: John Johansen @ 2020-06-16  8:38 UTC (permalink / raw)
  To: Mimi Zohar, Casey Schaufler, Lakshmi Ramasubramanian, Stephen Smalley
  Cc: Stephen Smalley, James Morris, linux-integrity, LSM List, linux-kernel

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.


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

end of thread, other threads:[~2020-06-16  8:39 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-13  2:41 [PATCH 0/5] LSM: Measure security module state Lakshmi Ramasubramanian
2020-06-13  2:41 ` [PATCH 1/5] IMA: Add LSM_STATE func to measure LSM data Lakshmi Ramasubramanian
2020-06-13  2:41 ` [PATCH 2/5] IMA: Define an IMA hook " Lakshmi Ramasubramanian
2020-06-13  2:41 ` [PATCH 3/5] LSM: Add security_state function pointer in lsm_info struct Lakshmi Ramasubramanian
2020-06-13  2:41 ` [PATCH 4/5] LSM: Define SELinux function to measure security state Lakshmi Ramasubramanian
2020-06-15 11:57   ` Stephen Smalley
2020-06-15 12:15     ` Stephen Smalley
2020-06-15 16:45     ` Lakshmi Ramasubramanian
2020-06-15 17:33       ` Casey Schaufler
2020-06-15 17:44         ` Mimi Zohar
2020-06-15 23:18           ` Casey Schaufler
2020-06-16  0:44             ` Mimi Zohar
2020-06-16  8:38           ` John Johansen
2020-06-15 20:31       ` Stephen Smalley
2020-06-13  2:41 ` [PATCH 5/5] LSM: Define workqueue for measuring security module state Lakshmi Ramasubramanian
2020-06-15 13:33   ` Stephen Smalley
2020-06-15 14:59     ` Mimi Zohar
2020-06-15 15:47       ` Stephen Smalley
2020-06-15 16:10         ` Mimi Zohar

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