LSM: Multiple concurrent LSMs

Quoted 5 lines · Casey Schaufler

Here's my post vacation update. I am considering moving away from the array of LSM vectors to lists within each hook. The primary advantage would be that multiple sparse LSMs would spend less time checking on NULL hooks. Thoughts?

I had actually been working up to suggesting this, so yes, I'm in favor of it. Much nicer for the likely sparse condition.

Quoted 4 lines

It would also make security_reset_ops considerably less pleasant, but that is code that gets ifdefed out except for one configuration of one LSM.

That seems fine to me.

Quoted 6 lines

Version 5 of the patch addresses:
1. Tetsuo Handa pointed out that handling of failures alloc hooks was still not correct in v4. The code now only calls the free hook for LSMs that have had their alloc hook successfully called. The alloc hooks have been de-macro-ized, too.

Excellent.

Quoted 2 lines

2. Removed the Yama special case. It is no longer necessary.

:)

Quoted 1 line

Signed-off-by Casey Schaufler <[email protected]>

Reviewed-by Kees Cook <[email protected]>

Quoted 19 lines

security/security.c +17 -1

+/*
+ * Only SELinux calls reset_security_ops.
+ */
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+int reset_security_ops(struct security_operations *ops)
 {
-       security_ops = &default_security_ops;
+       struct security_operations *empty;
+
+       /*
+        * This LSM is configured to own /proc/.../attr.
+        * Don't leave that important interface unattended.
+        */
+       if (lsm_present == ops->order)
+               return -EBUSY;
+
+       empty = kzalloc(sizeof(struct security_operations), GFP_KERNEL);
+       if (empty == NULL)
+               return -ENOMEM;

security/security.c static void __init do_security_initcalls(void) · line 99

Quoted 7 lines

security/security.c +6 -0

+
+       strcpy(empty->name, ops->name);
+       empty->order = ops->order;
+       composer_ops[ops->order] = empty;
+       return 0;
 }
+#endif /* CONFIG_SECURITY_SELINUX_DISABLE */

Won't this leak if called more than once?

Quoted 26 lines

security/security.c +7 -18

-/**
- * register_security - registers a security framework with the kernel
- * @ops: a pointer to the struct security_options that is to be registered
- *
- * This function allows a security module to register itself with the
- * kernel security subsystem.  Some rudimentary checking is done on the @ops
- * value passed to this function. You'll need to check first if your LSM
- * is allowed to register its @ops by calling security_module_enable(@ops).
- *
- * If there is already a security module registered with the kernel,
- * an error will be returned.  Otherwise %0 is returned on success.
- */
-int __init register_security(struct security_operations *ops)
-{
-       if (verify(ops)) {
-               printk(KERN_DEBUG "%s could not verify "
-                      "security_operations structure.\n", __func__);
-               return -EINVAL;
+       if (lsm_count >= COMPOSER_MAX) {
+               pr_warn("Too many security modules. %s not loaded.\n",
+                               ops->name);
+               return 0;
+       }
+       /*
+        * Set up the operation vector early, but only once.

security/security.c __setup("security=", choose_lsm); · line 142

Quoted 10 lines

security/security.c +9 -0

+        * This allows LSM specific file systems to check to see if they
+        * should come on line.
+        */
+       if (ops == NULL) {
+               pr_debug("%s could not verify security_operations.\n",
+                               __func__);
+               return 0;
        }
+       if (allowed_lsms[0] != '\0' && strstr(allowed_lsms, ops->name) == NULL)
+               return 0;

This means we must always avoid LSMs that could be substrings of another LSM. Should this check maybe be improved to test for start/end and commas?

Off the top of my head, and seems horrible. Maybe there's a cleaner way:

if (allowed_lsms[0] != '\0') {
    char * found;
    int len;

    found = strstr(allowed_lsms, ops->name);
    if (!found)
        return 0;
    if (found != allowed_lsms && found[-1] != ',')
        return 0;
    len = strlen(ops->name);
    if (strlen(found) < len || (found[len] != '\0' && found[len] != ','))
        return 0;
}

Quoted 22 lines

security/security.c +18 -0

+#define call_int_hook(RC, FUNC, ...)                                   \
+       do {                                                            \
+               int called = 0;                                         \
+               int thisrc;                                             \
+               int i;                                                  \
+                                                                       \
+               RC = 0;                                                 \
+               for (i = 1; i < lsm_count; i++) {                       \
+                       if (!composer_ops[i]->FUNC)                     \
+                               continue;                               \
+                       thisrc = composer_ops[i]->FUNC(__VA_ARGS__);    \
+                       if (thisrc)                                     \
+                               RC = thisrc;                            \
+                       called = 1;                                     \
+               }                                                       \
+               if (!called && composer_ops[0]->FUNC)                   \
+                       RC = composer_ops[0]->FUNC(__VA_ARGS__);        \
+       } while (0)
[...]
 int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
                         unsigned long arg4, unsigned long arg5)
 {

security/security.c __setup("security=", choose_lsm); · line 267

Quoted 10 lines

security/security.c +2 -6

-#ifdef CONFIG_SECURITY_YAMA_STACKED
        int rc;
-       rc = yama_task_prctl(option, arg2, arg3, arg4, arg5);
-       if (rc != -ENOSYS)
-               return rc;
-#endif
-       return security_ops->task_prctl(option, arg2, arg3, arg4, arg5);
+       call_int_hook(rc, task_prctl, option, arg2, arg3, arg4, arg5);
+       return rc;
 }

prctl control needs special handling. When an option is unhandled, it'll return -ENOSYS. Also, some prctls return non-zero results, so they could mask each other. I think it would make sense to walk the composer list as long as the return value is -ENOSYS. As soon as it isn't, then stop the call chain and return the value. Ultimately, this means that the LSM order defines who get access to a given prctl option (though with caps last).

int i, rc, called = 0;

for (i = 1; i < lsm_count; i++) {
    if (!composer_ops[i]->task_prctl)
        continue;
    rc = composer_ops[i]->task_prctl(option, arg2, arg3, arg4, arg5);
    called = 1;
    if (rc != -ENOSYS)
        return rc;
}
if (!called and composer_ops[0]->task_prctl)
    return composer_ops[0]->task_prctl(option, arg2, arg3, arg4, arg5);

return rc;

-Kees

Quoted 7 lines · Kees Cook

prctl control needs special handling. When an option is unhandled, it'll return -ENOSYS. Also, some prctls return non-zero results, so they could mask each other. I think it would make sense to walk the composer list as long as the return value is -ENOSYS. As soon as it isn't, then stop the call chain and return the value. Ultimately, this means that the LSM order defines who get access to a given prctl option (though with caps last).

Quoted 12 lines

int i, rc, called = 0;

for (i = 1; i < lsm_count; i++) {
    if (!composer_ops[i]->task_prctl)
        continue;
    rc = composer_ops[i]->task_prctl(option, arg2, arg3, arg4, arg5);
    called = 1;
    if (rc != -ENOSYS)
        return rc;
}
if (!called and composer_ops[0]->task_prctl)
    return composer_ops[0]->task_prctl(option, arg2, arg3, arg4, arg5);

Quoted 1 line

return rc;

With optimization, security_task_prctl() would look like below? I removed "int called" usage so that LSM modules which implement ->task_prctl need not to call cap_task_prctl(). Also, I assume "for (i = 1; i < lsm_count; i++)" will be changed to "for (i = 0; i < lsm_count; i++)" if this optimization is accepted.

int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
unsigned long arg4, unsigned long arg5)

{
	int i;
	for (i = 1; i < lsm_count; i++) {
		int rc;
		if (!composer_ops[i]->task_prctl)
			continue;
		rc = composer_ops[i]->task_prctl(option, arg2, arg3, arg4, arg5);
		if (rc != -ENOSYS)
			return rc;
	}
	return cap_task_prctl(option, arg2, arg3, arg4, arg5);
}

Same thing applies to security_bprm_set_creds() etc. so that LSM modules which implement ->bprm_set_creds need not to call cap_bprm_set_creds()?

int security_bprm_set_creds(struct linux_binprm *bprm)

{
	int i;
	for (i = 1; i < lsm_count; i++) {
		int rc;
		if (!composer_ops[i]->bprm_set_creds)
			continue;
		rc = composer_ops[i]->bprm_set_creds(bprm);
		if (rc)
			return rc;
	}
	return cap_bprm_set_creds(bprm);
}

Quoted 1 line · Casey Schaufler

I still think that we want to "call all" rather than "bail on fail".

If prctl() is "bail out upon handled" and security_bprm_set_creds() is "bail out upon fail", wouldn't it look natural to do like below?

int security_inode_permission(struct inode *inode, int mask) {

(quoted diff) +0 -0

 	if (unlikely(IS_PRIVATE(inode)))
 		return 0;

	for (i = 1; i < lsm_count; i++) {
		int rc;
		if (!composer_ops[i]->inode_permission)
			continue;
		rc = composer_ops[i]->inode_permission(inode, mask);
		if (rc)
			return rc;
	}
	return 0;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

This is what I think we can optimize.

Only compile tested. This may not boot.

Calls to common cap functions (e.g. cap_bprm_set_creds()) are not yet eliminated from each LSM modules. Common cap functions can be now eliminated from each LSM modules because these common cap functions are called from security/security.c (though I think I've made several mistakes while optimizing).

Revived register_security() so that individual LSM modules can determine whether that module is listed on the activation list or not; and can take appropriate action (probably call panic()) if registration failed when that module is listed on the activation list.

Updated register_security() to allow control of LSM hook call ordering. Revived CONFIG_DEFAULT_SECURITY so that Linux distributors can specify list of LSM modules which should be enabled by default (e.g. "selinux", "apparmor,yama") while compiling other LSM modules which are not enabled unless explicitly specified by security= kernel boot parameter.

What do you think?

Quoted 11 lines · Casey Schaufler

security/security.c +11 -0

+#define call_int_hook(RC, FUNC, ...)					\
+	do {								\
+		int called = 0;						\
+		int thisrc;						\
+		int i;							\
+									\
+		RC = 0;							\
+		for (i = 1; i < lsm_count; i++) {			\
+			if (!composer_ops[i]->FUNC)			\
+				continue;				\
+			thisrc = composer_ops[i]->FUNC(__VA_ARGS__);	\

security/security.c __setup("security=", choose_lsm); · line 228

Quoted 7 lines

security/security.c +7 -0

+			if (thisrc)					\
+				RC = thisrc;				\
+			called = 1;					\
+		}							\
+		if (!called && composer_ops[0]->FUNC)			\
+			RC = composer_ops[0]->FUNC(__VA_ARGS__);	\
+	} while (0)

Why can't we simplify like below? What is special with composer_ops[0] ?

#define call_int_hook(RC, FUNC, ...)					\
	do {								\
		int i;							\
									\
		RC = 0;							\
		for (i = 0; i < lsm_count; i++) {			\
			if (!composer_ops[i]->FUNC)			\
				continue;				\
			RC = composer_ops[i]->FUNC(__VA_ARGS__);	\
			if (RC)						\
				break;					\
		}							\
	} while (0)

Quoted 17 lines

int security_inode_alloc(struct inode *inode)
{

security/security.c +13 -2

-	inode->i_security = NULL;
-	return security_ops->inode_alloc_security(inode);
+	int i;
+	int rc;
+	struct lsm_blob tblob;
+	struct lsm_blob *bp = NULL;
+
+	memset(&tblob, 0, sizeof(tblob));
+	inode->i_security = &tblob;
+
+	for (rc = 0, i = 1; i < lsm_count && rc == 0; i++)
+		if (composer_ops[i]->inode_alloc_security)
+			rc = composer_ops[i]->inode_alloc_security(inode);
+
+	if (tblob.lsm_setcount != 0) {

security/security.c int security_bprm_check(struct linux_binprm *bprm) · line 520

Quoted 7 lines

security/security.c +7 -0

+		if (rc == 0)
+			bp = kmemdup(&tblob, sizeof(tblob), GFP_KERNEL);
+		if (bp == NULL) {
+			if (rc == 0)
+				rc = -ENOMEM;
+			for (i--; i >= 1; i--) {
+				if (composer_ops[i]->inode_free_security)

	if (!composer_ops[i]->inode_free_security)

security/security.c int security_bprm_check(struct linux_binprm *bprm) · line 529

Quoted 10 lines

security/security.c +8 -0

+					continue;
+				composer_ops[i]->inode_free_security(inode);
+			}
+			lsm_blob_cleanup(rc, &tblob, __func__);
+		}
+	}
+	inode->i_security = bp;
+	return rc;
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

What about adding "struct security_operations *next;" to "struct security_operations" like below?

struct security_operations {

(quoted diff) +2 -0

 	char name[SECURITY_NAME_MAX + 1];
+ 	int index; /* Index number used by lsm_get_blob()/lsm_set_blob(). */
+ 	struct security_operations *next; /* Next LSM to call if not NULL. */
 	int foo();
 	int bar_alloc();
 	void bar_free();
 };

I think the basic change looks like

int security_foo()
{

}

int security_bar_alloc()
{

(quoted diff) +5 -1

-	return security_ops->bar_alloc();
+	int rc;
+	struct security_operations *ops;
+	struct security_operations *tmp;
+	for (ops = security_ops; ops; ops = ops->next)
+		if (ops->bar_alloc) {

and unregistration will look like

int remove_security_ops(struct security_operations *lsm) {

(quoted diff) +0 -0

 	struct security_operations *ops = security_ops;
 	struct security_operations *prev = ops;

and registration will look like

int add_security_ops(struct security_operations *lsm) {

(quoted diff) +0 -0

 	static int index;
 	struct security_operations *ops = security_ops;
 	while (1) {
 		if (ops == lsm)
 			return 0;

.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html