[PATCH 00/18] LSM: Prepare for explict LSM ordering

[PATCH 00/18] LSM: Prepare for explict LSM ordering

[Kees Cook]

This refactors the LSM registration and initialization infrastructure
to more centrally support different LSM types. What was considered a
"major" LSM is split into "exclusive" and future "blob sharing" (to be
added later). The "minor" LSMs become more well defined as a result.

Instead of continuing to (somewhat improperly) overload the kernel's
initcall system, this changes the LSM infrastructure to store a
registration structure (struct lsm_info) table instead, where metadata
about each LSM can be recorded (name, type, order, enable flag, init
function). This can be extended in the future to include things like
required blob size for the coming "blob sharing" LSMs.

The "major" LSMs had to individually negotiate which of them should be
enabled. This didn't provide a way to negotiate combinations of other
LSMs (as will be needed for "blob sharing" LSMs). This is solved by
providing the LSM infrastructure with all the details needed to make
the choice (exposing the per-LSM "enabled" flag, if used, the LSM type,
and ordering expectations).

In better defining the "minor" LSMs, it was possible to remove the
open-coded security_add_hooks() calls for "capability", "yama", and
"loadpin", and to redefine "integrity" properly as a "minor" LSM (it
actually defines _no_ hooks, but needs the early initialization).

With all LSMs being proessed centrally, it was possible to implement
sensible parsing of the "security=" boot commandline argument to provide
explicit ordering, which is helpful for the future "blob sharing" LSMs.

To better show LSMs activation some debug reporting was added (enabled
with the "lsm.debug" boot commandline option).

Finally, I added a WARN() around LSM initialization failures, which
appear to have always been silently ignored. (Realistically any LSM init
failures would have only been due to catastrophic kernel issues that
would render a system unworkable anyway, but it'd be better to expose
the problem as early as possible.)

-Kees

Kees Cook (18):
  vmlinux.lds.h: Avoid copy/paste of security_init section
  LSM: Rename .security_initcall section to .lsm_info
  LSM: Remove initcall tracing
  LSM: Convert from initcall to struct lsm_info
  vmlinux.lds.h: Move LSM_TABLE into INIT_DATA
  LSM: Convert security_initcall() into DEFINE_LSM()
  LSM: Add minor LSM initialization loop
  integrity: Initialize as LSM_TYPE_MINOR
  LSM: Record LSM name in struct lsm_info
  LSM: Plumb visibility into optional "enabled" state
  LSM: Lift LSM selection out of individual LSMs
  LSM: Introduce ordering details in struct lsm_info
  LoadPin: Initialize as LSM_TYPE_MINOR
  Yama: Initialize as LSM_TYPE_MINOR
  capability: Initialize as LSM_TYPE_MINOR
  LSM: Allow arbitrary LSM ordering
  LSM: Provide init debugging
  LSM: Don't ignore initialization failures

 .../admin-guide/kernel-parameters.txt         |  15 +-
 arch/arc/kernel/vmlinux.lds.S                 |   1 -
 arch/arm/kernel/vmlinux-xip.lds.S             |   1 -
 arch/arm64/kernel/vmlinux.lds.S               |   1 -
 arch/h8300/kernel/vmlinux.lds.S               |   1 -
 arch/microblaze/kernel/vmlinux.lds.S          |   2 -
 arch/powerpc/kernel/vmlinux.lds.S             |   2 -
 arch/um/include/asm/common.lds.S              |   2 -
 arch/xtensa/kernel/vmlinux.lds.S              |   1 -
 include/asm-generic/vmlinux.lds.h             |  25 +-
 include/linux/init.h                          |   2 -
 include/linux/lsm_hooks.h                     |  45 +++-
 include/linux/module.h                        |   1 -
 security/apparmor/lsm.c                       |  15 +-
 security/commoncap.c                          |   9 +-
 security/integrity/iint.c                     |   6 +-
 security/loadpin/loadpin.c                    |  11 +-
 security/security.c                           | 252 ++++++++++++++----
 security/selinux/hooks.c                      |  15 +-
 security/smack/smack_lsm.c                    |   7 +-
 security/tomoyo/tomoyo.c                      |   6 +-
 security/yama/yama_lsm.c                      |   8 +-
 22 files changed, 295 insertions(+), 133 deletions(-)

-- 
2.17.1

[PATCH 01/18] vmlinux.lds.h: Avoid copy/paste of security_init section

[Kees Cook]

Avoid copy/paste by defining SECURITY_INIT in terms of SECURITY_INITCALL.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/vmlinux.lds.h | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 7b75ff6e2fce..934a45395547 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -473,13 +473,6 @@
 #define RODATA          RO_DATA_SECTION(4096)
 #define RO_DATA(align)  RO_DATA_SECTION(align)
 
-#define SECURITY_INIT							\
-	.security_initcall.init : AT(ADDR(.security_initcall.init) - LOAD_OFFSET) { \
-		__security_initcall_start = .;				\
-		KEEP(*(.security_initcall.init))			\
-		__security_initcall_end = .;				\
-	}
-
 /*
  * .text section. Map to function alignment to avoid address changes
  * during second ld run in second ld pass when generating System.map
@@ -798,6 +791,12 @@
 		KEEP(*(.security_initcall.init))			\
 		__security_initcall_end = .;
 
+/* Older linker script style for security init. */
+#define SECURITY_INIT							\
+	.security_initcall.init : AT(ADDR(.security_initcall.init) - LOAD_OFFSET) { \
+		SECURITY_INITCALL					\
+	}
+
 #ifdef CONFIG_BLK_DEV_INITRD
 #define INIT_RAM_FS							\
 	. = ALIGN(4);							\
-- 
2.17.1

[PATCH 02/18] LSM: Rename .security_initcall section to .lsm_info

[Kees Cook]

In preparation for switching from initcall to just a regular set of
pointers in a section, rename the internal section name.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/vmlinux.lds.h | 10 +++++-----
 include/linux/init.h              |  4 ++--
 security/security.c               |  4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 934a45395547..5079a969e612 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -787,14 +787,14 @@
 		__con_initcall_end = .;
 
 #define SECURITY_INITCALL						\
-		__security_initcall_start = .;				\
-		KEEP(*(.security_initcall.init))			\
-		__security_initcall_end = .;
+		__start_lsm_info = .;					\
+		KEEP(*(.lsm_info.init))					\
+		__end_lsm_info = .;
 
 /* Older linker script style for security init. */
 #define SECURITY_INIT							\
-	.security_initcall.init : AT(ADDR(.security_initcall.init) - LOAD_OFFSET) { \
-		SECURITY_INITCALL					\
+	.lsm_info.init : AT(ADDR(.lsm_info.init) - LOAD_OFFSET) {	\
+		LSM_INFO						\
 	}
 
 #ifdef CONFIG_BLK_DEV_INITRD
diff --git a/include/linux/init.h b/include/linux/init.h
index 2538d176dd1f..77636539e77c 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -133,7 +133,7 @@ static inline initcall_t initcall_from_entry(initcall_entry_t *entry)
 #endif
 
 extern initcall_entry_t __con_initcall_start[], __con_initcall_end[];
-extern initcall_entry_t __security_initcall_start[], __security_initcall_end[];
+extern initcall_entry_t __start_lsm_info[], __end_lsm_info[];
 
 /* Used for contructor calls. */
 typedef void (*ctor_fn_t)(void);
@@ -236,7 +236,7 @@ extern bool initcall_debug;
 	static exitcall_t __exitcall_##fn __exit_call = fn
 
 #define console_initcall(fn)	___define_initcall(fn,, .con_initcall)
-#define security_initcall(fn)	___define_initcall(fn,, .security_initcall)
+#define security_initcall(fn)	___define_initcall(fn,, .lsm_info)
 
 struct obs_kernel_param {
 	const char *str;
diff --git a/security/security.c b/security/security.c
index 736e78da1ab9..d49d5ff8be4b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -51,9 +51,9 @@ static void __init do_security_initcalls(void)
 	initcall_t call;
 	initcall_entry_t *ce;
 
-	ce = __security_initcall_start;
+	ce = __start_lsm_info;
 	trace_initcall_level("security");
-	while (ce < __security_initcall_end) {
+	while (ce < __end_lsm_info) {
 		call = initcall_from_entry(ce);
 		trace_initcall_start(call);
 		ret = call();
-- 
2.17.1

[PATCH 03/18] LSM: Remove initcall tracing

[Kees Cook]

This partially reverts commit 58eacfffc417 ("init, tracing: instrument
security and console initcall trace events") since security init calls
are about to no longer resemble regular init calls.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 security/security.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/security/security.c b/security/security.c
index d49d5ff8be4b..913eb73ff3f9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -30,8 +30,6 @@
 #include <linux/string.h>
 #include <net/flow.h>
 
-#include <trace/events/initcall.h>
-
 #define MAX_LSM_EVM_XATTR	2
 
 /* Maximum number of letters for an LSM name string */
@@ -47,17 +45,13 @@ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
 
 static void __init do_security_initcalls(void)
 {
-	int ret;
 	initcall_t call;
 	initcall_entry_t *ce;
 
 	ce = __start_lsm_info;
-	trace_initcall_level("security");
 	while (ce < __end_lsm_info) {
 		call = initcall_from_entry(ce);
-		trace_initcall_start(call);
-		ret = call();
-		trace_initcall_finish(call, ret);
+		call();
 		ce++;
 	}
 }
-- 
2.17.1

[PATCH 04/18] LSM: Convert from initcall to struct lsm_info

[Kees Cook]

In preparation for doing more interesting LSM init probing, this converts
the existing initcall system into an explicit call into a function pointer
from a section-collected struct lsm_info array.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/init.h      |  2 --
 include/linux/lsm_hooks.h | 12 ++++++++++++
 include/linux/module.h    |  1 -
 security/integrity/iint.c |  1 +
 security/security.c       | 14 +++++---------
 5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/linux/init.h b/include/linux/init.h
index 77636539e77c..9c2aba1dbabf 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -133,7 +133,6 @@ static inline initcall_t initcall_from_entry(initcall_entry_t *entry)
 #endif
 
 extern initcall_entry_t __con_initcall_start[], __con_initcall_end[];
-extern initcall_entry_t __start_lsm_info[], __end_lsm_info[];
 
 /* Used for contructor calls. */
 typedef void (*ctor_fn_t)(void);
@@ -236,7 +235,6 @@ extern bool initcall_debug;
 	static exitcall_t __exitcall_##fn __exit_call = fn
 
 #define console_initcall(fn)	___define_initcall(fn,, .con_initcall)
-#define security_initcall(fn)	___define_initcall(fn,, .lsm_info)
 
 struct obs_kernel_param {
 	const char *str;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 97a020c616ad..f3ddf9fdbdce 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2039,6 +2039,18 @@ extern char *lsm_names;
 extern void security_add_hooks(struct security_hook_list *hooks, int count,
 				char *lsm);
 
+struct lsm_info {
+	int (*init)(void);
+};
+
+extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
+
+#define security_initcall(lsm)						\
+	static const struct lsm_info __lsm_##lsm			\
+		__used __section(.lsm_info.init)			\
+		__aligned(sizeof(unsigned long))			\
+		= { .init = lsm, }
+
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 /*
  * Assuring the safety of deleting a security module is up to
diff --git a/include/linux/module.h b/include/linux/module.h
index f807f15bebbe..264979283756 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -123,7 +123,6 @@ extern void cleanup_module(void);
 #define late_initcall_sync(fn)		module_init(fn)
 
 #define console_initcall(fn)		module_init(fn)
-#define security_initcall(fn)		module_init(fn)
 
 /* Each module must use one module_init(). */
 #define module_init(initfn)					\
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 5a6810041e5c..70d21b566955 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -22,6 +22,7 @@
 #include <linux/file.h>
 #include <linux/uaccess.h>
 #include <linux/security.h>
+#include <linux/lsm_hooks.h>
 #include "integrity.h"
 
 static struct rb_root integrity_iint_tree = RB_ROOT;
diff --git a/security/security.c b/security/security.c
index 913eb73ff3f9..74ab98f82d34 100644
--- a/security/security.c
+++ b/security/security.c
@@ -43,16 +43,12 @@ char *lsm_names;
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
 	CONFIG_DEFAULT_SECURITY;
 
-static void __init do_security_initcalls(void)
+static void __init major_lsm_init(void)
 {
-	initcall_t call;
-	initcall_entry_t *ce;
+	struct lsm_info *lsm;
 
-	ce = __start_lsm_info;
-	while (ce < __end_lsm_info) {
-		call = initcall_from_entry(ce);
-		call();
-		ce++;
+	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+		lsm->init();
 	}
 }
 
@@ -81,7 +77,7 @@ int __init security_init(void)
 	/*
 	 * Load all the remaining security modules.
 	 */
-	do_security_initcalls();
+	major_lsm_init();
 
 	return 0;
 }
-- 
2.17.1

[PATCH 05/18] vmlinux.lds.h: Move LSM_TABLE into INIT_DATA

[Kees Cook]

Since the struct lsm_info table is not an initcall, we can just move it
into INIT_DATA like all the other tables.

Cc: linux-arch@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arc/kernel/vmlinux.lds.S        |  1 -
 arch/arm/kernel/vmlinux-xip.lds.S    |  1 -
 arch/arm64/kernel/vmlinux.lds.S      |  1 -
 arch/h8300/kernel/vmlinux.lds.S      |  1 -
 arch/microblaze/kernel/vmlinux.lds.S |  2 --
 arch/powerpc/kernel/vmlinux.lds.S    |  2 --
 arch/um/include/asm/common.lds.S     |  2 --
 arch/xtensa/kernel/vmlinux.lds.S     |  1 -
 include/asm-generic/vmlinux.lds.h    | 24 +++++++++++-------------
 9 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/arch/arc/kernel/vmlinux.lds.S b/arch/arc/kernel/vmlinux.lds.S
index f35ed578e007..8fb16bdabdcf 100644
--- a/arch/arc/kernel/vmlinux.lds.S
+++ b/arch/arc/kernel/vmlinux.lds.S
@@ -71,7 +71,6 @@ SECTIONS
 		INIT_SETUP(L1_CACHE_BYTES)
 		INIT_CALLS
 		CON_INITCALL
-		SECURITY_INITCALL
 	}
 
 	.init.arch.info : {
diff --git a/arch/arm/kernel/vmlinux-xip.lds.S b/arch/arm/kernel/vmlinux-xip.lds.S
index 3593d5c1acd2..8c74037ade22 100644
--- a/arch/arm/kernel/vmlinux-xip.lds.S
+++ b/arch/arm/kernel/vmlinux-xip.lds.S
@@ -96,7 +96,6 @@ SECTIONS
 		INIT_SETUP(16)
 		INIT_CALLS
 		CON_INITCALL
-		SECURITY_INITCALL
 		INIT_RAM_FS
 	}
 
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 605d1b60469c..7d23d591b03c 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -166,7 +166,6 @@ SECTIONS
 		INIT_SETUP(16)
 		INIT_CALLS
 		CON_INITCALL
-		SECURITY_INITCALL
 		INIT_RAM_FS
 		*(.init.rodata.* .init.bss)	/* from the EFI stub */
 	}
diff --git a/arch/h8300/kernel/vmlinux.lds.S b/arch/h8300/kernel/vmlinux.lds.S
index 35716a3048de..49f716c0a1df 100644
--- a/arch/h8300/kernel/vmlinux.lds.S
+++ b/arch/h8300/kernel/vmlinux.lds.S
@@ -56,7 +56,6 @@ SECTIONS
 	__init_begin = .;
 	INIT_TEXT_SECTION(4)
 	INIT_DATA_SECTION(4)
-	SECURITY_INIT
 	__init_end = .;
 	_edata = . ;
 	_begin_data = LOADADDR(.data);
diff --git a/arch/microblaze/kernel/vmlinux.lds.S b/arch/microblaze/kernel/vmlinux.lds.S
index 289d0e7f3e3a..e1f3e8741292 100644
--- a/arch/microblaze/kernel/vmlinux.lds.S
+++ b/arch/microblaze/kernel/vmlinux.lds.S
@@ -117,8 +117,6 @@ SECTIONS {
 		CON_INITCALL
 	}
 
-	SECURITY_INIT
-
 	__init_end_before_initramfs = .;
 
 	.init.ramfs : AT(ADDR(.init.ramfs) - LOAD_OFFSET) {
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 07ae018e550e..105a976323aa 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -212,8 +212,6 @@ SECTIONS
 		CON_INITCALL
 	}
 
-	SECURITY_INIT
-
 	. = ALIGN(8);
 	__ftr_fixup : AT(ADDR(__ftr_fixup) - LOAD_OFFSET) {
 		__start___ftr_fixup = .;
diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index 7adb4e6b658a..4049f2c46387 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -53,8 +53,6 @@
 	CON_INITCALL
   }
 
-  SECURITY_INIT
-
   .exitcall : {
 	__exitcall_begin = .;
 	*(.exitcall.exit)
diff --git a/arch/xtensa/kernel/vmlinux.lds.S b/arch/xtensa/kernel/vmlinux.lds.S
index a1c3edb8ad56..b727b18a68ac 100644
--- a/arch/xtensa/kernel/vmlinux.lds.S
+++ b/arch/xtensa/kernel/vmlinux.lds.S
@@ -197,7 +197,6 @@ SECTIONS
     INIT_SETUP(XCHAL_ICACHE_LINESIZE)
     INIT_CALLS
     CON_INITCALL
-    SECURITY_INITCALL
     INIT_RAM_FS
   }
 
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 5079a969e612..b31ea8bdfef9 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -203,6 +203,15 @@
 #define EARLYCON_TABLE()
 #endif
 
+#ifdef CONFIG_SECURITY
+#define LSM_TABLE()	. = ALIGN(8);					\
+			__start_lsm_info = .;				\
+			KEEP(*(.lsm_info.init))				\
+			__end_lsm_info = .;
+#else
+#define LSM_TABLE()
+#endif
+
 #define ___OF_TABLE(cfg, name)	_OF_TABLE_##cfg(name)
 #define __OF_TABLE(cfg, name)	___OF_TABLE(cfg, name)
 #define OF_TABLE(cfg, name)	__OF_TABLE(IS_ENABLED(cfg), name)
@@ -597,7 +606,8 @@
 	IRQCHIP_OF_MATCH_TABLE()					\
 	ACPI_PROBE_TABLE(irqchip)					\
 	ACPI_PROBE_TABLE(timer)						\
-	EARLYCON_TABLE()
+	EARLYCON_TABLE()						\
+	LSM_TABLE()
 
 #define INIT_TEXT							\
 	*(.init.text .init.text.*)					\
@@ -786,17 +796,6 @@
 		KEEP(*(.con_initcall.init))				\
 		__con_initcall_end = .;
 
-#define SECURITY_INITCALL						\
-		__start_lsm_info = .;					\
-		KEEP(*(.lsm_info.init))					\
-		__end_lsm_info = .;
-
-/* Older linker script style for security init. */
-#define SECURITY_INIT							\
-	.lsm_info.init : AT(ADDR(.lsm_info.init) - LOAD_OFFSET) {	\
-		LSM_INFO						\
-	}
-
 #ifdef CONFIG_BLK_DEV_INITRD
 #define INIT_RAM_FS							\
 	. = ALIGN(4);							\
@@ -963,7 +962,6 @@
 		INIT_SETUP(initsetup_align)				\
 		INIT_CALLS						\
 		CON_INITCALL						\
-		SECURITY_INITCALL					\
 		INIT_RAM_FS						\
 	}
 
-- 
2.17.1

[PATCH 06/18] LSM: Convert security_initcall() into DEFINE_LSM()

[Kees Cook]

Instead of using argument-based initializers, switch to defining the
contents of struct lsm_info on a per-LSM basis. This also drops
the final use of the now inaccurate "initcall" naming.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h  | 6 ++++--
 security/apparmor/lsm.c    | 4 +++-
 security/integrity/iint.c  | 4 +++-
 security/selinux/hooks.c   | 4 +++-
 security/smack/smack_lsm.c | 4 +++-
 security/tomoyo/tomoyo.c   | 4 +++-
 6 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index f3ddf9fdbdce..f8e618e2bdd2 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2045,11 +2045,13 @@ struct lsm_info {
 
 extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
 
-#define security_initcall(lsm)						\
+#define DEFINE_LSM(lsm)							\
 	static const struct lsm_info __lsm_##lsm			\
 		__used __section(.lsm_info.init)			\
 		__aligned(sizeof(unsigned long))			\
-		= { .init = lsm, }
+		= {							\
+
+#define END_LSM	  }
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 /*
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 8b8b70620bbe..7fa7b4464cf4 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1606,4 +1606,6 @@ static int __init apparmor_init(void)
 	return error;
 }
 
-security_initcall(apparmor_init);
+DEFINE_LSM(apparmor)
+	.init = apparmor_init,
+END_LSM;
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 70d21b566955..20e60df929a3 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -175,7 +175,9 @@ static int __init integrity_iintcache_init(void)
 			      0, SLAB_PANIC, init_once);
 	return 0;
 }
-security_initcall(integrity_iintcache_init);
+DEFINE_LSM(integrity)
+	.init = integrity_iintcache_init,
+END_LSM;
 
 
 /*
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index ad9a9b8e9979..469a90806bc6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7202,7 +7202,9 @@ void selinux_complete_init(void)
 
 /* SELinux requires early initialization in order to label
    all processes and objects when they are created. */
-security_initcall(selinux_init);
+DEFINE_LSM(selinux)
+	.init = selinux_init,
+END_LSM;
 
 #if defined(CONFIG_NETFILTER)
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 340fc30ad85d..1e1ace718e75 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4882,4 +4882,6 @@ static __init int smack_init(void)
  * Smack requires early initialization in order to label
  * all processes and objects when they are created.
  */
-security_initcall(smack_init);
+DEFINE_LSM(smack)
+	.init = smack_init,
+END_LSM;
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 9f932e2d6852..a280d4eab456 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -550,4 +550,6 @@ static int __init tomoyo_init(void)
 	return 0;
 }
 
-security_initcall(tomoyo_init);
+DEFINE_LSM(tomoyo)
+	.init = tomoyo_init,
+END_LSM;
-- 
2.17.1

[PATCH 07/18] LSM: Add minor LSM initialization loop

[Kees Cook]

Split initialization loop into two phases: "exclusive" LSMs and "minor"
LSMs.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h | 6 ++++++
 security/security.c       | 8 +++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index f8e618e2bdd2..ec3419b9b16f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2039,7 +2039,13 @@ extern char *lsm_names;
 extern void security_add_hooks(struct security_hook_list *hooks, int count,
 				char *lsm);
 
+enum lsm_type {
+	LSM_TYPE_EXCLUSIVE = 0,
+	LSM_TYPE_MINOR,
+};
+
 struct lsm_info {
+	enum lsm_type type;	/* Optional: default is LSM_TYPE_EXCLUSIVE */
 	int (*init)(void);
 };
 
diff --git a/security/security.c b/security/security.c
index 74ab98f82d34..da2a923f2609 100644
--- a/security/security.c
+++ b/security/security.c
@@ -43,12 +43,13 @@ char *lsm_names;
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
 	CONFIG_DEFAULT_SECURITY;
 
-static void __init major_lsm_init(void)
+static void __init lsm_init(enum lsm_type type)
 {
 	struct lsm_info *lsm;
 
 	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
-		lsm->init();
+		if (lsm->type == type)
+			lsm->init();
 	}
 }
 
@@ -73,11 +74,12 @@ int __init security_init(void)
 	capability_add_hooks();
 	yama_add_hooks();
 	loadpin_add_hooks();
+	lsm_init(LSM_TYPE_MINOR);
 
 	/*
 	 * Load all the remaining security modules.
 	 */
-	major_lsm_init();
+	lsm_init(LSM_TYPE_EXCLUSIVE);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 07/18] LSM: Add minor LSM initialization loop

[Jann Horn]

On Sun, Sep 16, 2018 at 3:11 AM Kees Cook <keescook@chromium.org> wrote:
> Split initialization loop into two phases: "exclusive" LSMs and "minor"
> LSMs.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/lsm_hooks.h | 6 ++++++
>  security/security.c       | 8 +++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index f8e618e2bdd2..ec3419b9b16f 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2039,7 +2039,13 @@ extern char *lsm_names;
>  extern void security_add_hooks(struct security_hook_list *hooks, int count,
>                                 char *lsm);
>
> +enum lsm_type {
> +       LSM_TYPE_EXCLUSIVE = 0,
> +       LSM_TYPE_MINOR,
> +};

Is the intent of this explicit zero assignment that LSM_TYPE_EXCLUSIVE
should be the default? If so, perhaps a comment "/* default */", or
something like that, might be helpful.

Re: [PATCH 07/18] LSM: Add minor LSM initialization loop

[Kees Cook]

On Sat, Sep 15, 2018 at 6:27 PM, Jann Horn <jannh@google.com> wrote:
> On Sun, Sep 16, 2018 at 3:11 AM Kees Cook <keescook@chromium.org> wrote:
>> Split initialization loop into two phases: "exclusive" LSMs and "minor"
>> LSMs.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  include/linux/lsm_hooks.h | 6 ++++++
>>  security/security.c       | 8 +++++---
>>  2 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index f8e618e2bdd2..ec3419b9b16f 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -2039,7 +2039,13 @@ extern char *lsm_names;
>>  extern void security_add_hooks(struct security_hook_list *hooks, int count,
>>                                 char *lsm);
>>
>> +enum lsm_type {
>> +       LSM_TYPE_EXCLUSIVE = 0,
>> +       LSM_TYPE_MINOR,
>> +};
>
> Is the intent of this explicit zero assignment that LSM_TYPE_EXCLUSIVE
> should be the default? If so, perhaps a comment "/* default */", or
> something like that, might be helpful.

You cut the patch quote off where I do exactly that:

>> +       enum lsm_type type;     /* Optional: default is LSM_TYPE_EXCLUSIVE */

:)

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 08/18] integrity: Initialize as LSM_TYPE_MINOR

[Kees Cook]

The integrity LSM isn't really an LSM in that it never calls
security_add_hooks(), but it uses the early security init because its
hooks need to run before the VFS layer initializes. This is the very
definition of a non-exclusive LSM, so mark it as such.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 security/integrity/iint.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 20e60df929a3..d886183848c4 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -176,6 +176,7 @@ static int __init integrity_iintcache_init(void)
 	return 0;
 }
 DEFINE_LSM(integrity)
+	.type = LSM_TYPE_MINOR,
 	.init = integrity_iintcache_init,
 END_LSM;
 
-- 
2.17.1

[PATCH 09/18] LSM: Record LSM name in struct lsm_info

[Kees Cook]

In preparation for making LSM selections outside of the LSMs, include
the name of LSMs in struct lsm_info.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ec3419b9b16f..a7833193e9e9 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2045,6 +2045,7 @@ enum lsm_type {
 };
 
 struct lsm_info {
+	const char *name;	/* Populated automatically. */
 	enum lsm_type type;	/* Optional: default is LSM_TYPE_EXCLUSIVE */
 	int (*init)(void);
 };
@@ -2052,10 +2053,13 @@ struct lsm_info {
 extern struct lsm_info __start_lsm_info[], __end_lsm_info[];
 
 #define DEFINE_LSM(lsm)							\
+	static const char __lsm_name_##lsm[] __initconst		\
+		__aligned(1) = #lsm;					\
 	static const struct lsm_info __lsm_##lsm			\
 		__used __section(.lsm_info.init)			\
 		__aligned(sizeof(unsigned long))			\
 		= {							\
+			.name = __lsm_name_##lsm,			\
 
 #define END_LSM	  }
 
-- 
2.17.1

[PATCH 10/18] LSM: Plumb visibility into optional "enabled" state

[Kees Cook]

In preparation for lifting the "is this LSM enabled?" logic out of the
individual LSMs, pass in any special enabled state tracking (as needed
for SELinux, AppArmor, and LoadPin). This must be an "int" to include
handling cases where "enabled" is exposed via sysctl which has no "bool"
type (i.e. LoadPin's use).

LoadPin's "enabled" tracking will be added later when it is marked as
LSM_TYPE_MINOR.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h | 1 +
 security/apparmor/lsm.c   | 5 +++--
 security/selinux/hooks.c  | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index a7833193e9e9..8a3a6cd26f03 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2046,6 +2046,7 @@ enum lsm_type {
 
 struct lsm_info {
 	const char *name;	/* Populated automatically. */
+	int *enabled;		/* Optional: NULL means enabled. */
 	enum lsm_type type;	/* Optional: default is LSM_TYPE_EXCLUSIVE */
 	int (*init)(void);
 };
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 7fa7b4464cf4..6cd630b34c3b 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1303,8 +1303,8 @@ bool aa_g_paranoid_load = true;
 module_param_named(paranoid_load, aa_g_paranoid_load, aabool, S_IRUGO);
 
 /* Boot time disable flag */
-static bool apparmor_enabled = CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE;
-module_param_named(enabled, apparmor_enabled, bool, S_IRUGO);
+static int apparmor_enabled = CONFIG_SECURITY_APPARMOR_BOOTPARAM_VALUE;
+module_param_named(enabled, apparmor_enabled, int, 0444);
 
 static int __init apparmor_enabled_setup(char *str)
 {
@@ -1607,5 +1607,6 @@ static int __init apparmor_init(void)
 }
 
 DEFINE_LSM(apparmor)
+	.enabled = &apparmor_enabled,
 	.init = apparmor_init,
 END_LSM;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 469a90806bc6..78b5afc188f3 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7203,6 +7203,7 @@ void selinux_complete_init(void)
 /* SELinux requires early initialization in order to label
    all processes and objects when they are created. */
 DEFINE_LSM(selinux)
+	.enabled = &selinux_enabled,
 	.init = selinux_init,
 END_LSM;
 
-- 
2.17.1

[PATCH 11/18] LSM: Lift LSM selection out of individual LSMs

[Kees Cook]

In order to adjust LSM selection logic in the future, this moves the
selection logic up out of the individual LSMs, making their init functions
only run when actually enabled.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h  |  1 -
 security/apparmor/lsm.c    |  6 ---
 security/security.c        | 75 ++++++++++++++++++++++++++------------
 security/selinux/hooks.c   | 10 -----
 security/smack/smack_lsm.c |  3 --
 security/tomoyo/tomoyo.c   |  2 -
 6 files changed, 51 insertions(+), 46 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8a3a6cd26f03..6e71e1c47fa1 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2094,7 +2094,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
 #define __lsm_ro_after_init	__ro_after_init
 #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
 
-extern int __init security_module_enable(const char *module);
 extern void __init capability_add_hooks(void);
 #ifdef CONFIG_SECURITY_YAMA
 extern void __init yama_add_hooks(void);
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 6cd630b34c3b..56c0982b48cd 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1542,12 +1542,6 @@ static int __init apparmor_init(void)
 {
 	int error;
 
-	if (!apparmor_enabled || !security_module_enable("apparmor")) {
-		aa_info_message("AppArmor disabled by boot time parameter");
-		apparmor_enabled = false;
-		return 0;
-	}
-
 	aa_secids_init();
 
 	error = aa_setup_dfa_engine();
diff --git a/security/security.c b/security/security.c
index da2a923f2609..3fedbee5f3ec 100644
--- a/security/security.c
+++ b/security/security.c
@@ -43,13 +43,63 @@ char *lsm_names;
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
 	CONFIG_DEFAULT_SECURITY;
 
+static struct lsm_info *exclusive __initdata;
+
+/* Mark an LSM's enabled flag, if it exists. */
+static void __init set_enabled(struct lsm_info *lsm, bool enabled)
+{
+	if (lsm->enabled)
+		*lsm->enabled = enabled;
+}
+
+/* Is an LSM allowed to be enabled? */
+static bool __init lsm_enabled(struct lsm_info *lsm)
+{
+	/* Report explicit disabling. */
+	if (lsm->enabled && !*lsm->enabled) {
+		pr_info("%s disabled with boot parameter\n", lsm->name);
+		return false;
+	}
+
+	/* If LSM isn't exclusive, ignore exclusive LSM selection rules. */
+	if (lsm->type != LSM_TYPE_EXCLUSIVE)
+		return true;
+
+	/* Disabled if another exclusive LSM already selected. */
+	if (exclusive)
+		return false;
+
+	/* Disabled if this LSM isn't the chosen one. */
+	if (strcmp(lsm->name, chosen_lsm) != 0)
+		return false;
+
+	return true;
+}
+
+/* Check if LSM should be enabled. Mark any that are disabled. */
+static void __init maybe_enable_lsm(struct lsm_info *lsm)
+{
+	int enabled = lsm_enabled(lsm);
+
+	/* Record enablement. */
+	set_enabled(lsm, enabled);
+
+	/* If selected, initialize the LSM. */
+	if (enabled) {
+		if (lsm->type == LSM_TYPE_EXCLUSIVE) {
+			exclusive = lsm;
+		}
+		lsm->init();
+	}
+}
+
 static void __init lsm_init(enum lsm_type type)
 {
 	struct lsm_info *lsm;
 
 	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
 		if (lsm->type == type)
-			lsm->init();
+			maybe_enable_lsm(lsm);
 	}
 }
 
@@ -128,29 +178,6 @@ static int lsm_append(char *new, char **result)
 	return 0;
 }
 
-/**
- * security_module_enable - Load given security module on boot ?
- * @module: the name of the module
- *
- * Each LSM must pass this method before registering its own operations
- * to avoid security registration races. This method may also be used
- * to check if your LSM is currently loaded during kernel initialization.
- *
- * Returns:
- *
- * true if:
- *
- * - The passed LSM is the one chosen by user at boot time,
- * - or the passed LSM is configured as the default and the user did not
- *   choose an alternate LSM at boot time.
- *
- * Otherwise, return false.
- */
-int __init security_module_enable(const char *module)
-{
-	return !strcmp(module, chosen_lsm);
-}
-
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @hooks: the hooks to add
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 78b5afc188f3..5478abf51f3a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7133,16 +7133,6 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 
 static __init int selinux_init(void)
 {
-	if (!security_module_enable("selinux")) {
-		selinux_enabled = 0;
-		return 0;
-	}
-
-	if (!selinux_enabled) {
-		pr_info("SELinux:  Disabled at boot.\n");
-		return 0;
-	}
-
 	pr_info("SELinux:  Initializing.\n");
 
 	memset(&selinux_state, 0, sizeof(selinux_state));
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 1e1ace718e75..6e127c357ca2 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4834,9 +4834,6 @@ static __init int smack_init(void)
 	struct cred *cred;
 	struct task_smack *tsp;
 
-	if (!security_module_enable("smack"))
-		return 0;
-
 	smack_inode_cache = KMEM_CACHE(inode_smack, 0);
 	if (!smack_inode_cache)
 		return -ENOMEM;
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index a280d4eab456..0471390409c5 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -540,8 +540,6 @@ static int __init tomoyo_init(void)
 {
 	struct cred *cred = (struct cred *) current_cred();
 
-	if (!security_module_enable("tomoyo"))
-		return 0;
 	/* register ourselves with the security framework */
 	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
 	printk(KERN_INFO "TOMOYO Linux initialized\n");
-- 
2.17.1

Re: [PATCH 11/18] LSM: Lift LSM selection out of individual LSMs

[Jann Horn]

On Sun, Sep 16, 2018 at 3:14 AM Kees Cook <keescook@chromium.org> wrote:
> In order to adjust LSM selection logic in the future, this moves the
> selection logic up out of the individual LSMs, making their init functions
> only run when actually enabled.
[...]
> +/* Is an LSM allowed to be enabled? */
> +static bool __init lsm_enabled(struct lsm_info *lsm)
> +{
> +       /* Report explicit disabling. */
> +       if (lsm->enabled && !*lsm->enabled) {
> +               pr_info("%s disabled with boot parameter\n", lsm->name);
> +               return false;
> +       }
> +
> +       /* If LSM isn't exclusive, ignore exclusive LSM selection rules. */
> +       if (lsm->type != LSM_TYPE_EXCLUSIVE)
> +               return true;
> +
> +       /* Disabled if another exclusive LSM already selected. */
> +       if (exclusive)
> +               return false;

What is this check for, given that you have the strcmp() just below
here? From a quick look, it (together with everything else that
touches the "exclusive" variable) seems superfluous to me, unless
there are two LSMs with the same name (which really shouldn't happen,
right?).

> +       /* Disabled if this LSM isn't the chosen one. */
> +       if (strcmp(lsm->name, chosen_lsm) != 0)
> +               return false;
> +
> +       return true;
> +}

Re: [PATCH 11/18] LSM: Lift LSM selection out of individual LSMs

[Kees Cook]

On Sat, Sep 15, 2018 at 6:32 PM, Jann Horn <jannh@google.com> wrote:
> On Sun, Sep 16, 2018 at 3:14 AM Kees Cook <keescook@chromium.org> wrote:
>> In order to adjust LSM selection logic in the future, this moves the
>> selection logic up out of the individual LSMs, making their init functions
>> only run when actually enabled.
> [...]
>> +/* Is an LSM allowed to be enabled? */
>> +static bool __init lsm_enabled(struct lsm_info *lsm)
>> +{
>> +       /* Report explicit disabling. */
>> +       if (lsm->enabled && !*lsm->enabled) {
>> +               pr_info("%s disabled with boot parameter\n", lsm->name);
>> +               return false;
>> +       }
>> +
>> +       /* If LSM isn't exclusive, ignore exclusive LSM selection rules. */
>> +       if (lsm->type != LSM_TYPE_EXCLUSIVE)
>> +               return true;
>> +
>> +       /* Disabled if another exclusive LSM already selected. */
>> +       if (exclusive)
>> +               return false;
>
> What is this check for, given that you have the strcmp() just below
> here? From a quick look, it (together with everything else that
> touches the "exclusive" variable) seems superfluous to me, unless
> there are two LSMs with the same name (which really shouldn't happen,
> right?).
>
>> +       /* Disabled if this LSM isn't the chosen one. */
>> +       if (strcmp(lsm->name, chosen_lsm) != 0)
>> +               return false;
>> +
>> +       return true;
>> +}

Mainly it's for composition with later patches where the name check is
moved. It seemed easier to explain the logical progression with the
hunk here.

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 12/18] LSM: Introduce ordering details in struct lsm_info

[Kees Cook]

Only minor LSMs have any ordering currently, but only capabilities
actually need to go first, so provide either "absolutely first" or
"mutable" ordering currently. Default order is "mutable".

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h | 7 +++++++
 security/security.c       | 9 ++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 6e71e1c47fa1..89e6ec8eac07 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2044,10 +2044,17 @@ enum lsm_type {
 	LSM_TYPE_MINOR,
 };
 
+enum lsm_order {
+	LSM_ORDER_FIRST = -1,	/* This is only for capabilities. */
+	LSM_ORDER_MUTABLE = 0,
+	LSM_ORDER_MAX,
+};
+
 struct lsm_info {
 	const char *name;	/* Populated automatically. */
 	int *enabled;		/* Optional: NULL means enabled. */
 	enum lsm_type type;	/* Optional: default is LSM_TYPE_EXCLUSIVE */
+	enum lsm_order order;	/* Optional: default is LSM_ORDER_MUTABLE */
 	int (*init)(void);
 };
 
diff --git a/security/security.c b/security/security.c
index 3fedbee5f3ec..19afd7949426 100644
--- a/security/security.c
+++ b/security/security.c
@@ -96,10 +96,13 @@ static void __init maybe_enable_lsm(struct lsm_info *lsm)
 static void __init lsm_init(enum lsm_type type)
 {
 	struct lsm_info *lsm;
+	enum lsm_order order;
 
-	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
-		if (lsm->type == type)
-			maybe_enable_lsm(lsm);
+	for (order = LSM_ORDER_FIRST; order < LSM_ORDER_MAX; order++) {
+		for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+			if (lsm->type == type && lsm->order == order)
+				maybe_enable_lsm(lsm);
+		}
 	}
 }
 
-- 
2.17.1

[PATCH 13/18] LoadPin: Initialize as LSM_TYPE_MINOR

[Kees Cook]

This converts LoadPin to use the new LSM_TYPE_MINOR marking.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h  |  5 -----
 security/loadpin/loadpin.c | 11 +++++++++--
 security/security.c        |  1 -
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 89e6ec8eac07..5e0ca4a05091 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2107,10 +2107,5 @@ extern void __init yama_add_hooks(void);
 #else
 static inline void __init yama_add_hooks(void) { }
 #endif
-#ifdef CONFIG_SECURITY_LOADPIN
-void __init loadpin_add_hooks(void);
-#else
-static inline void loadpin_add_hooks(void) { };
-#endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 0716af28808a..8798d0b9b8e9 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -184,12 +184,19 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
 };
 
-void __init loadpin_add_hooks(void)
+static int __init loadpin_init(void)
 {
-	pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
+	pr_info("ready to pin\n");
 	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
+	return 0;
 }
 
+DEFINE_LSM(loadpin)
+	.enabled = &enabled,
+	.type = LSM_TYPE_MINOR,
+	.init = loadpin_init,
+END_LSM;
+
 /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
 module_param(enabled, int, 0);
 MODULE_PARM_DESC(enabled, "Pin module/firmware loading (default: true)");
diff --git a/security/security.c b/security/security.c
index 19afd7949426..65d7ba1bc1ef 100644
--- a/security/security.c
+++ b/security/security.c
@@ -126,7 +126,6 @@ int __init security_init(void)
 	 */
 	capability_add_hooks();
 	yama_add_hooks();
-	loadpin_add_hooks();
 	lsm_init(LSM_TYPE_MINOR);
 
 	/*
-- 
2.17.1

[PATCH 14/18] Yama: Initialize as LSM_TYPE_MINOR

[Kees Cook]

This converts Yama to use the new LSM_TYPE_MINOR marking.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h | 5 -----
 security/security.c       | 1 -
 security/yama/yama_lsm.c  | 8 +++++++-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5e0ca4a05091..0564153130c8 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2102,10 +2102,5 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
 #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
 
 extern void __init capability_add_hooks(void);
-#ifdef CONFIG_SECURITY_YAMA
-extern void __init yama_add_hooks(void);
-#else
-static inline void __init yama_add_hooks(void) { }
-#endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/security.c b/security/security.c
index 65d7ba1bc1ef..c6ca07fc0771 100644
--- a/security/security.c
+++ b/security/security.c
@@ -125,7 +125,6 @@ int __init security_init(void)
 	 * Load minor LSMs, with the capability module always first.
 	 */
 	capability_add_hooks();
-	yama_add_hooks();
 	lsm_init(LSM_TYPE_MINOR);
 
 	/*
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index ffda91a4a1aa..e970917926d9 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -477,9 +477,15 @@ static void __init yama_init_sysctl(void)
 static inline void yama_init_sysctl(void) { }
 #endif /* CONFIG_SYSCTL */
 
-void __init yama_add_hooks(void)
+static int __init yama_init(void)
 {
 	pr_info("Yama: becoming mindful.\n");
 	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
 	yama_init_sysctl();
+	return 0;
 }
+
+DEFINE_LSM(yama)
+	.type = LSM_TYPE_MINOR,
+	.init = yama_init,
+END_LSM;
-- 
2.17.1

[PATCH 15/18] capability: Initialize as LSM_TYPE_MINOR

[Kees Cook]

This converts capabilities to use the new LSM_TYPE_MINOR marking, as well
as the LSM_ORDER_FIRST position.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/lsm_hooks.h | 2 --
 security/commoncap.c      | 9 ++++++++-
 security/security.c       | 1 -
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 0564153130c8..f2949744a5d3 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2101,6 +2101,4 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
 #define __lsm_ro_after_init	__ro_after_init
 #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
 
-extern void __init capability_add_hooks(void);
-
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/commoncap.c b/security/commoncap.c
index 2e489d6a3ac8..44e7a9260f89 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1366,10 +1366,17 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(vm_enough_memory, cap_vm_enough_memory),
 };
 
-void __init capability_add_hooks(void)
+static int __init capability_init(void)
 {
 	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
 				"capability");
+	return 0;
 }
 
+DEFINE_LSM(capability)
+	.order = LSM_ORDER_FIRST,
+	.type = LSM_TYPE_MINOR,
+	.init = capability_init,
+END_LSM;
+
 #endif /* CONFIG_SECURITY */
diff --git a/security/security.c b/security/security.c
index c6ca07fc0771..67532326a0ce 100644
--- a/security/security.c
+++ b/security/security.c
@@ -124,7 +124,6 @@ int __init security_init(void)
 	/*
 	 * Load minor LSMs, with the capability module always first.
 	 */
-	capability_add_hooks();
 	lsm_init(LSM_TYPE_MINOR);
 
 	/*
-- 
2.17.1

[PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

To prepare for having a third type of LSM ("shared blob"), this implements
dynamic handling of LSM ordering. The visible change here is that the
"security=" boot commandline is now a comma-separated ordered list of
all LSMs, not just the single "exclusive" LSM. This means that the
"minor" LSMs can now be disabled at boot time by omitting them from the
commandline. Additionally LSM ordering becomes entirely mutable for LSMs
with LSM_ORDER_MUTABLE ("capability" is not mutable and is always enabled
first).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../admin-guide/kernel-parameters.txt         |  13 +-
 security/security.c                           | 145 ++++++++++++++----
 2 files changed, 126 insertions(+), 32 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9871e649ffef..6d6bb9481193 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4027,11 +4027,14 @@
 			Note: increases power consumption, thus should only be
 			enabled if running jitter sensitive (HPC/RT) workloads.
 
-	security=	[SECURITY] Choose a security module to enable at boot.
-			If this boot parameter is not specified, only the first
-			security module asking for security registration will be
-			loaded. An invalid security module name will be treated
-			as if no module has been chosen.
+	security=	[SECURITY] An ordered comma-separated list of
+			security modules to attempt to enable at boot. If
+			this boot parameter is not specified, only the
+			security modules asking for initialization will be
+			enabled (see CONFIG_DEFAULT_SECURITY). Duplicate
+			or invalid security modules will be ignored. The
+			capability module is always loaded first, without
+			regard to this parameter.
 
 	selinux=	[SELINUX] Disable or enable SELinux at boot time.
 			Format: { "0" | "1" }
diff --git a/security/security.c b/security/security.c
index 67532326a0ce..f09a4bb3cb86 100644
--- a/security/security.c
+++ b/security/security.c
@@ -32,17 +32,18 @@
 
 #define MAX_LSM_EVM_XATTR	2
 
-/* Maximum number of letters for an LSM name string */
-#define SECURITY_NAME_MAX	10
+/* How many LSMs were built into the kernel? */
+#define LSM_COUNT (__end_lsm_info - __start_lsm_info)
 
 struct security_hook_heads security_hook_heads __lsm_ro_after_init;
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 
 char *lsm_names;
 /* Boot-time LSM user choice */
-static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
-	CONFIG_DEFAULT_SECURITY;
+static const char *bootparam_lsms;
 
+/* Ordered list of possible LSMs to initialize. */
+static struct lsm_info **possible_lsms __initdata;
 static struct lsm_info *exclusive __initdata;
 
 /* Mark an LSM's enabled flag, if it exists. */
@@ -52,6 +53,108 @@ static void __init set_enabled(struct lsm_info *lsm, bool enabled)
 		*lsm->enabled = enabled;
 }
 
+/* Is an LSM already listed in the possible LSMs list? */
+static bool __init possible_lsm(struct lsm_info *lsm)
+{
+	struct lsm_info **check;
+
+	for (check = possible_lsms; *check; check++)
+		if (*check == lsm)
+			return true;
+
+	return false;
+}
+
+/* Append an LSM to the list of possible LSMs to initialize. */
+static int last_lsm __initdata;
+static void __init append_possible_lsm(struct lsm_info *lsm, const char *from)
+{
+	/* Ignore duplicate selections. */
+	if (possible_lsm(lsm)) {
+		return;
+	}
+
+	if (WARN(last_lsm == LSM_COUNT, "%s: out of LSM slots!?\n", from))
+		return;
+
+	possible_lsms[last_lsm++] = lsm;
+}
+
+/* Default boot: populate possible LSMs list with builtin ordering. */
+static void __init prepare_lsm_order_builtin(void)
+{
+	struct lsm_info *lsm;
+
+	/* All minor LSMs should go next. */
+	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+		if (lsm->type == LSM_TYPE_MINOR &&
+		    lsm->order == LSM_ORDER_MUTABLE)
+			append_possible_lsm(lsm, "builtin minor");
+	}
+
+	/* Then the CONFIG_DEFAULT_SECURITY exclusive LSM. */
+	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+		if (lsm->type == LSM_TYPE_EXCLUSIVE &&
+		    !strcmp(CONFIG_DEFAULT_SECURITY, lsm->name))
+			append_possible_lsm(lsm, "builtin default");
+	}
+
+	/* Then other exclusive LSMs, in case above is disabled. */
+	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+		if (lsm->type == LSM_TYPE_EXCLUSIVE &&
+		    strcmp(CONFIG_DEFAULT_SECURITY, lsm->name))
+			append_possible_lsm(lsm, "builtin extra");
+	}
+}
+
+/* "security=" boot: populate possible LSMs list from boot commandline. */
+static void __init prepare_lsm_order_commandline(void)
+{
+	struct lsm_info *lsm;
+	char *sep, *name, *next;
+
+	sep = kstrdup(bootparam_lsms, GFP_KERNEL);
+	next = sep;
+	/* Walk commandline list, looking for matching LSMs. */
+	while ((name = strsep(&next, ",")) != NULL) {
+		for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+			if (lsm->order == LSM_ORDER_MUTABLE &&
+			    !strcmp(lsm->name, name)) {
+				append_possible_lsm(lsm, "commandline");
+			}
+		}
+	}
+	kfree(sep);
+
+	/* Mark any LSMs missing from commandline as explicitly disabled. */
+	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+		if (lsm->order == LSM_ORDER_MUTABLE) {
+			if (possible_lsm(lsm))
+				continue;
+
+			set_enabled(lsm, false);
+		}
+	}
+}
+
+/* Populate possible LSMs list from build order or commandline order. */
+static void __init prepare_lsm_order(void)
+{
+	struct lsm_info *lsm;
+
+	/* LSM_ORDER_FIRST is always first. */
+	for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
+		if (lsm->order == LSM_ORDER_FIRST)
+			append_possible_lsm(lsm, "first");
+	}
+
+	/* If no commandline order defined, use builtin order. */
+	if (!bootparam_lsms)
+		prepare_lsm_order_builtin();
+	else
+		prepare_lsm_order_commandline();
+}
+
 /* Is an LSM allowed to be enabled? */
 static bool __init lsm_enabled(struct lsm_info *lsm)
 {
@@ -69,10 +172,6 @@ static bool __init lsm_enabled(struct lsm_info *lsm)
 	if (exclusive)
 		return false;
 
-	/* Disabled if this LSM isn't the chosen one. */
-	if (strcmp(lsm->name, chosen_lsm) != 0)
-		return false;
-
 	return true;
 }
 
@@ -93,17 +192,13 @@ static void __init maybe_enable_lsm(struct lsm_info *lsm)
 	}
 }
 
-static void __init lsm_init(enum lsm_type type)
+/* Initialize all possible LSMs in order, if they are enabled. */
+static void __init lsm_init(void)
 {
-	struct lsm_info *lsm;
-	enum lsm_order order;
+	struct lsm_info **lsm;
 
-	for (order = LSM_ORDER_FIRST; order < LSM_ORDER_MAX; order++) {
-		for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
-			if (lsm->type == type && lsm->order == order)
-				maybe_enable_lsm(lsm);
-		}
-	}
+	for (lsm = possible_lsms; *lsm; lsm++)
+		maybe_enable_lsm(*lsm);
 }
 
 /**
@@ -119,25 +214,21 @@ int __init security_init(void)
 	for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct hlist_head);
 	     i++)
 		INIT_HLIST_HEAD(&list[i]);
+	possible_lsms = kcalloc(LSM_COUNT + 1, sizeof(*possible_lsms),
+				GFP_KERNEL);
 	pr_info("Security Framework initialized\n");
 
-	/*
-	 * Load minor LSMs, with the capability module always first.
-	 */
-	lsm_init(LSM_TYPE_MINOR);
-
-	/*
-	 * Load all the remaining security modules.
-	 */
-	lsm_init(LSM_TYPE_EXCLUSIVE);
+	prepare_lsm_order();
+	lsm_init();
 
+	kfree(possible_lsms);
 	return 0;
 }
 
 /* Save user chosen LSM */
 static int __init choose_lsm(char *str)
 {
-	strncpy(chosen_lsm, str, SECURITY_NAME_MAX);
+	bootparam_lsms = str;
 	return 1;
 }
 __setup("security=", choose_lsm);
-- 
2.17.1

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/15/2018 5:30 PM, Kees Cook wrote:
> To prepare for having a third type of LSM ("shared blob"), this implements
> dynamic handling of LSM ordering. The visible change here is that the
> "security=" boot commandline is now a comma-separated ordered list of
> all LSMs, not just the single "exclusive" LSM. This means that the
> "minor" LSMs can now be disabled at boot time by omitting them from the
> commandline. Additionally LSM ordering becomes entirely mutable for LSMs
> with LSM_ORDER_MUTABLE ("capability" is not mutable and is always enabled
> first).

Today if I have Yama enabled and use security=apparmor I get a
module list of capability,yama,apparmor. With this change I would
get a different result, capability,apparmor. I am personally OK with
this, but I think others may see it as a violation of compatibility.

One solution is to leave security= as is, not affecting "minor"
modules and only allowing specification of one major module, and adding
another boot option security.stack= that overrides a security= option
and that takes the list as you've implemented here.

An icky alternative would be to say that any security= specification
with no commas in it retains the old behavior. So
	security=apparmor
	security=apparmor,
would get you
	capability,yama,apparmor
	capability,apparmor
respectively.

Another option would be to require negation on the minor modules,
such as
	security=apparmor,-loadpin

I can't honestly say which I like least or best.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Sun, Sep 16, 2018 at 11:49 AM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
> On 9/15/2018 5:30 PM, Kees Cook wrote:
>> To prepare for having a third type of LSM ("shared blob"), this implements
>> dynamic handling of LSM ordering. The visible change here is that the
>> "security=" boot commandline is now a comma-separated ordered list of
>> all LSMs, not just the single "exclusive" LSM. This means that the
>> "minor" LSMs can now be disabled at boot time by omitting them from the
>> commandline. Additionally LSM ordering becomes entirely mutable for LSMs
>> with LSM_ORDER_MUTABLE ("capability" is not mutable and is always enabled
>> first).
>
> Today if I have Yama enabled and use security=apparmor I get a
> module list of capability,yama,apparmor. With this change I would
> get a different result, capability,apparmor. I am personally OK with
> this, but I think others may see it as a violation of compatibility.

Correct. That is the problem I had asked about earlier: it means
people with existing security= for specifying the active major LSM
will _disable_ all the minor LSMs after this change. It makes me
uncomfortable.

> One solution is to leave security= as is, not affecting "minor"
> modules and only allowing specification of one major module, and adding

I would much prefer this, yes.

A question remains: how do we map the existing "security=" selection
of a "major" LSM against what will be next "exclusive" plus tomoyo,
and in the extreme case, nothing?

Perhaps as part of deprecating "security=", we could just declare that
it is selecting between SELinux, AppArmor, Smack, and Tomoyo only?

> another boot option security.stack= that overrides a security= option
> and that takes the list as you've implemented here.

or "lsm.stack=" that overrides "security=" entirely?

> An icky alternative would be to say that any security= specification
> with no commas in it retains the old behavior. So
>         security=apparmor
>         security=apparmor,
> would get you
>         capability,yama,apparmor
>         capability,apparmor
> respectively.
>
> Another option would be to require negation on the minor modules,
> such as
>         security=apparmor,-loadpin
>
> I can't honestly say which I like least or best.

The trailing comma thing gets us some compatibility, but we still have
to decide which things should be exclusive-via-"security=" since with
blob-sharing it already becomes possible to do selinux + tomoyo.

The -$lsm style may make it hard to sensibly order any unspecified
LSMs. I guess it could just fall back to "follow builtin ordering of
unspecified LSMs" (unless someone had, maybe, "-all").

so, if builtin ordering after blob-sharing is
capability,integrity,yama,loadpin,{selinux,apparmor,smack},tomoyo

security=apparmor  is  capability,apparmor,integrity,yama,loadpin,tomoyo

security=yama,smack,-all  is  capability,yama,smack

security=loadpin,selinux,yama,-integrity  is
capability,loadpin,selinux,yama,tomoyo


Whatever we design, it needs to handle both the blob-sharing
near-future, and have an eye towards "extreme stacking" in the
some-day future. In both cases, the idea of a "major" LSM starts to
get very very hazy.

As for how we classify things, based on hooks...

now:
    always: capability
    major: selinux,apparmor,smack,tomoyo
    minor: yama,loadpin
    init-only: integrity

blob-sharing:
    always: capability
    exclusive: selinux,apparmor,smack
    sharing: tomoyo,integrity,yama,loadpin

extreme:
    always: capability
    sharing: selinux,apparmor,smack,tomoyo,integrity,yama,loadpin

The most special are capability (unconditional, run first) and
integrity (init-only, no security_add_hooks() call).

Can we classify things as MAC and non-MAC for "major" vs "minor"? SARA
and Landlock aren't MAC (and neither is integrity), or should we do
the "-$lsm" thing instead?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Tetsuo Handa]

On 2018/09/17 8:00, Kees Cook wrote:
> On Sun, Sep 16, 2018 at 11:49 AM, Casey Schaufler
> <casey@schaufler-ca.com> wrote:
>> One solution is to leave security= as is, not affecting "minor"
>> modules and only allowing specification of one major module, and adding
> 
> I would much prefer this, yes.
> 
> A question remains: how do we map the existing "security=" selection
> of a "major" LSM against what will be next "exclusive" plus tomoyo,
> and in the extreme case, nothing?
> 
> Perhaps as part of deprecating "security=", we could just declare that
> it is selecting between SELinux, AppArmor, Smack, and Tomoyo only?
> 
>> another boot option security.stack= that overrides a security= option
>> and that takes the list as you've implemented here.
> 
> or "lsm.stack=" that overrides "security=" entirely?


Yes, I think we can add new option.

For example, introducing lsm= and obsoleting security= (because total length for
kernel command line is limited while enumeration makes the parameter value longer).

  security= works like current behavior.

  lsm= requires explicit enumeration of all modules (except capability which has to
  be always enabled) which should be enabled at boot.

  security= is ignored if lsm= is specified.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/16/2018 4:00 PM, Kees Cook wrote:
> On Sun, Sep 16, 2018 at 11:49 AM, Casey Schaufler
> <casey@schaufler-ca.com> wrote:
>> On 9/15/2018 5:30 PM, Kees Cook wrote:
>>> To prepare for having a third type of LSM ("shared blob"), this implements
>>> dynamic handling of LSM ordering. The visible change here is that the
>>> "security=" boot commandline is now a comma-separated ordered list of
>>> all LSMs, not just the single "exclusive" LSM. This means that the
>>> "minor" LSMs can now be disabled at boot time by omitting them from the
>>> commandline. Additionally LSM ordering becomes entirely mutable for LSMs
>>> with LSM_ORDER_MUTABLE ("capability" is not mutable and is always enabled
>>> first).
>> Today if I have Yama enabled and use security=apparmor I get a
>> module list of capability,yama,apparmor. With this change I would
>> get a different result, capability,apparmor. I am personally OK with
>> this, but I think others may see it as a violation of compatibility.
> Correct. That is the problem I had asked about earlier: it means
> people with existing security= for specifying the active major LSM
> will _disable_ all the minor LSMs after this change. It makes me
> uncomfortable.
>
>> One solution is to leave security= as is, not affecting "minor"
>> modules and only allowing specification of one major module, and adding
> I would much prefer this, yes.
>
> A question remains: how do we map the existing "security=" selection
> of a "major" LSM against what will be next "exclusive" plus tomoyo,
> and in the extreme case, nothing?
>
> Perhaps as part of deprecating "security=", we could just declare that
> it is selecting between SELinux, AppArmor, Smack, and Tomoyo only?

I'd be happier keeping yama and loadpin as the special cases. Someone
might want to say security=landlock and expect the existing "minor"
module behavior.

>> another boot option security.stack= that overrides a security= option
>> and that takes the list as you've implemented here.
> or "lsm.stack=" that overrides "security=" entirely?

I thought about that. In some ways that would be most sane.

>> An icky alternative would be to say that any security= specification
>> with no commas in it retains the old behavior. So
>>         security=apparmor
>>         security=apparmor,
>> would get you
>>         capability,yama,apparmor
>>         capability,apparmor
>> respectively.
>>
>> Another option would be to require negation on the minor modules,
>> such as
>>         security=apparmor,-loadpin
>>
>> I can't honestly say which I like least or best.
> The trailing comma thing gets us some compatibility, but we still have
> to decide which things should be exclusive-via-"security=" since with
> blob-sharing it already becomes possible to do selinux + tomoyo.
>
> The -$lsm style may make it hard to sensibly order any unspecified
> LSMs. I guess it could just fall back to "follow builtin ordering of
> unspecified LSMs" (unless someone had, maybe, "-all").

That's why I'm not especially happy with either one.

> so, if builtin ordering after blob-sharing is
> capability,integrity,yama,loadpin,{selinux,apparmor,smack},tomoyo
>
> security=apparmor  is  capability,apparmor,integrity,yama,loadpin,tomoyo

I would expect capability,integrity,yama,loadpin,apparmor to reflect
today's behavior.

> security=yama,smack,-all  is  capability,yama,smack

Yes

> security=loadpin,selinux,yama,-integrity  is
> capability,loadpin,selinux,yama,tomoyo

I think that the negation should only apply to
integrity, yama and loadpin. All blob-using modules
must be explicitly stated if you want to use them.

> Whatever we design, it needs to handle both the blob-sharing
> near-future, and have an eye towards "extreme stacking" in the
> some-day future. In both cases, the idea of a "major" LSM starts to
> get very very hazy.

Long term the only distinction is "minor" and blob using. So long as
there's a way to enforce incompatibility (i.e. not Smack and SELinux)
in the sorter term we can adopt that mindset already.

> As for how we classify things, based on hooks...
>
> now:
>     always: capability
>     major: selinux,apparmor,smack,tomoyo
>     minor: yama,loadpin
>     init-only: integrity
>
> blob-sharing:
>     always: capability
>     exclusive: selinux,apparmor,smack
>     sharing: tomoyo,integrity,yama,loadpin
>
> extreme:
>     always: capability
>     sharing: selinux,apparmor,smack,tomoyo,integrity,yama,loadpin
>
> The most special are capability (unconditional, run first) and
> integrity (init-only, no security_add_hooks() call).
>
> Can we classify things as MAC and non-MAC for "major" vs "minor"? SARA
> and Landlock aren't MAC (and neither is integrity), or should we do
> the "-$lsm" thing instead?

I don't like using MAC because the use of the module isn't the issue,
it's the interfaces used. As ugly as it is, I like the -$lsm better.

>
> -Kees
>

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Mon, Sep 17, 2018 at 8:06 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> The trailing comma thing gets us some compatibility, but we still have
>> to decide which things should be exclusive-via-"security=" since with
>> blob-sharing it already becomes possible to do selinux + tomoyo.
>>
>> The -$lsm style may make it hard to sensibly order any unspecified
>> LSMs. I guess it could just fall back to "follow builtin ordering of
>> unspecified LSMs" (unless someone had, maybe, "-all").
>
> That's why I'm not especially happy with either one.
>
>> so, if builtin ordering after blob-sharing is
>> capability,integrity,yama,loadpin,{selinux,apparmor,smack},tomoyo
>>
>> security=apparmor  is  capability,apparmor,integrity,yama,loadpin,tomoyo
>
> I would expect capability,integrity,yama,loadpin,apparmor to reflect
> today's behavior.

If that's desired then we have to declare tomoyo as "exclusive" even
though it doesn't use blobs. But then what happens in the extreme
stacking case? Do we add "lsm.extreme=1" to change how security= is
parsed?

>> security=yama,smack,-all  is  capability,yama,smack
>
> Yes
>
>> security=loadpin,selinux,yama,-integrity  is
>> capability,loadpin,selinux,yama,tomoyo
>
> I think that the negation should only apply to
> integrity, yama and loadpin. All blob-using modules
> must be explicitly stated if you want to use them.

What about tomoyo, though? It's presently considered a major LSM (i.e.
security=tomoyo disables the other majors), but it doesn't use blobs.

>> Whatever we design, it needs to handle both the blob-sharing
>> near-future, and have an eye towards "extreme stacking" in the
>> some-day future. In both cases, the idea of a "major" LSM starts to
>> get very very hazy.
>
> Long term the only distinction is "minor" and blob using. So long as
> there's a way to enforce incompatibility (i.e. not Smack and SELinux)
> in the sorter term we can adopt that mindset already.

Given that tomoyo doesn't share blobs and integrity doesn't register
hooks, how would they be considered in that world? Or rather, what
distinguishes a "minor" LSM? It seems there are three cases: uses
blobs with sharing, uses blobs without sharing, uses no blobs. What
happens if an LSM grows a feature that needs blob sharing? If "uses no
blobs" should be considered "shares blobs", then there is no
distinction between "minor" and "blob sharing".

>> As for how we classify things, based on hooks...
>>
>> now:
>>     always: capability
>>     major: selinux,apparmor,smack,tomoyo
>>     minor: yama,loadpin
>>     init-only: integrity
>>
>> blob-sharing:
>>     always: capability
>>     exclusive: selinux,apparmor,smack
>>     sharing: tomoyo,integrity,yama,loadpin
>>
>> extreme:
>>     always: capability
>>     sharing: selinux,apparmor,smack,tomoyo,integrity,yama,loadpin
>>
>> The most special are capability (unconditional, run first) and
>> integrity (init-only, no security_add_hooks() call).
>>
>> Can we classify things as MAC and non-MAC for "major" vs "minor"? SARA
>> and Landlock aren't MAC (and neither is integrity), or should we do
>> the "-$lsm" thing instead?
>
> I don't like using MAC because the use of the module isn't the issue,
> it's the interfaces used. As ugly as it is, I like the -$lsm better.

Agreed on MAC. And yes, I think -$lsm is best here. Should we overload
"security=" or add "lsm.stacking="?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 9:24 AM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 8:06 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> The trailing comma thing gets us some compatibility, but we still have
>>> to decide which things should be exclusive-via-"security=" since with
>>> blob-sharing it already becomes possible to do selinux + tomoyo.
>>>
>>> The -$lsm style may make it hard to sensibly order any unspecified
>>> LSMs. I guess it could just fall back to "follow builtin ordering of
>>> unspecified LSMs" (unless someone had, maybe, "-all").
>> That's why I'm not especially happy with either one.
>>
>>> so, if builtin ordering after blob-sharing is
>>> capability,integrity,yama,loadpin,{selinux,apparmor,smack},tomoyo
>>>
>>> security=apparmor  is  capability,apparmor,integrity,yama,loadpin,tomoyo
>> I would expect capability,integrity,yama,loadpin,apparmor to reflect
>> today's behavior.
> If that's desired then we have to declare tomoyo as "exclusive" even
> though it doesn't use blobs. But then what happens in the extreme
> stacking case? Do we add "lsm.extreme=1" to change how security= is
> parsed?

TOMOYO uses the cred blob pointer. When the blob is shared TOMOYO
has to be allocated a pointer size chunk to store the pointer in.
Smack has the same behavior on file blobs.


>>> security=yama,smack,-all  is  capability,yama,smack
>> Yes
>>
>>> security=loadpin,selinux,yama,-integrity  is
>>> capability,loadpin,selinux,yama,tomoyo
>> I think that the negation should only apply to
>> integrity, yama and loadpin. All blob-using modules
>> must be explicitly stated if you want to use them.
> What about tomoyo, though? It's presently considered a major LSM (i.e.
> security=tomoyo disables the other majors), but it doesn't use blobs.
>
>>> Whatever we design, it needs to handle both the blob-sharing
>>> near-future, and have an eye towards "extreme stacking" in the
>>> some-day future. In both cases, the idea of a "major" LSM starts to
>>> get very very hazy.
>> Long term the only distinction is "minor" and blob using. So long as
>> there's a way to enforce incompatibility (i.e. not Smack and SELinux)
>> in the sorter term we can adopt that mindset already.
> Given that tomoyo doesn't share blobs and integrity doesn't register
> hooks, how would they be considered in that world? Or rather, what
> distinguishes a "minor" LSM? It seems there are three cases: uses
> blobs with sharing, uses blobs without sharing, uses no blobs. What
> happens if an LSM grows a feature that needs blob sharing? If "uses no
> blobs" should be considered "shares blobs", then there is no
> distinction between "minor" and "blob sharing".

Today the distinction is based on how the module registers hooks.
Modules that use blobs (including TOMOYO) use security_module_enable()
and those that don't just use security_add_hooks(). The "pick one"
policy is enforced in security_module_enable(), which is why you can
have as many non-blob users as you like. You could easily have a
non-blob using module that was exclusive simply by using
security_module_enable().

In the stacking case you could have integrity_init() call
security_module_enable() but not security_add_hooks(). You wouldn't
want to do that without stacking configured, because that would
make integrity exclusive.
 

>>> As for how we classify things, based on hooks...
>>>
>>> now:
>>>     always: capability
>>>     major: selinux,apparmor,smack,tomoyo
>>>     minor: yama,loadpin
>>>     init-only: integrity
>>>
>>> blob-sharing:
>>>     always: capability
>>>     exclusive: selinux,apparmor,smack
>>>     sharing: tomoyo,integrity,yama,loadpin
>>>
>>> extreme:
>>>     always: capability
>>>     sharing: selinux,apparmor,smack,tomoyo,integrity,yama,loadpin
>>>
>>> The most special are capability (unconditional, run first) and
>>> integrity (init-only, no security_add_hooks() call).
>>>
>>> Can we classify things as MAC and non-MAC for "major" vs "minor"? SARA
>>> and Landlock aren't MAC (and neither is integrity), or should we do
>>> the "-$lsm" thing instead?
>> I don't like using MAC because the use of the module isn't the issue,
>> it's the interfaces used. As ugly as it is, I like the -$lsm better.
> Agreed on MAC. And yes, I think -$lsm is best here. Should we overload
> "security=" or add "lsm.stacking="?

Keep security=$lsm with the existing exclusive behavior.

Add lsm=$lsm1,...,$lsmN which requires a full list of modules

If you want to be fancy (I don't!) you could add

lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Mon, Sep 17, 2018 at 10:13 AM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
> TOMOYO uses the cred blob pointer. When the blob is shared TOMOYO
> has to be allocated a pointer size chunk to store the pointer in.
> Smack has the same behavior on file blobs.

Oh dang, yes, I got confused over secid and other "extreme" shared things.

So one change of my series would be to declare tomoyo as "exclusive" too.

> Today the distinction is based on how the module registers hooks.
> Modules that use blobs (including TOMOYO) use security_module_enable()
> and those that don't just use security_add_hooks(). The "pick one"
> policy is enforced in security_module_enable(), which is why you can
> have as many non-blob users as you like. You could easily have a
> non-blob using module that was exclusive simply by using
> security_module_enable().

True. With my removal of security_module_enable(), yes, it makes sense
to mark all LSMs that were calling it before as exclusive, rather than
focusing on whether they would be exclusive under the blob-sharing
situation.

> Keep security=$lsm with the existing exclusive behavior.
> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>
> If you want to be fancy (I don't!) you could add
>
> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack

We've got two issues: ordering and enablement. It's been strongly
suggested that we should move away from per-LSM enable/disable flags
(to which I agree). If ordering should be separate from enablement (to
avoid the "booted kernel with new LSM built in, but my lsm="..." line
didn't include it so it's disabled case), then I think we need to
split the logic (otherwise we just reinvented "security=" with similar
problems).

Should "lsm=" allow arbitrary ordering? (I think yes.)

Should "lsm=" imply implicit enable/disable? (I think no: unlisted
LSMs are implicitly auto-appended to the explicit list)

So then we could have "lsm.enable=..." and "lsm.disable=...".

If builtin list was:
capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
then:

    lsm.disable=loadpin lsm=smack

becomes

    capability,smack,yama,integrity

and

    CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
    selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity

becomes

    capability,integrity,yama,loadpin,apparmor


If "lsm=" _does_ imply enablement, then how does it interact with
per-LSM disabling? i.e. what does "apparmor.enabled=0
lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
on a CONFIG-default-off LSM without specifying all the other LSMs too?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 11:14 AM, Kees Cook wrote:
>
>> Keep security=$lsm with the existing exclusive behavior.
>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>
>> If you want to be fancy (I don't!) you could add
>>
>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
> We've got two issues: ordering and enablement. It's been strongly
> suggested that we should move away from per-LSM enable/disable flags
> (to which I agree).

I also agree. There are way too many ways to turn off some LSMs.

> If ordering should be separate from enablement (to
> avoid the "booted kernel with new LSM built in, but my lsm="..." line
> didn't include it so it's disabled case), then I think we need to
> split the logic (otherwise we just reinvented "security=" with similar
> problems).

We could reduce the problem by declaring that LSM ordering is
not something you can specify on the boot line. I can see value
in specifying it when you build the kernel, but your circumstances
would have to be pretty strange to change it at boot time.

> Should "lsm=" allow arbitrary ordering? (I think yes.)

I say no. Assume you can specify it at build time. When would
you want to change the order? Why would you?

> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
> LSMs are implicitly auto-appended to the explicit list)

If you want to add something that isn't there instead of making
it explicit you want "lsm.enable=" not "lsm=".

> So then we could have "lsm.enable=..." and "lsm.disable=...".
>
> If builtin list was:
> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
> then:
>
>     lsm.disable=loadpin lsm=smack

Methinks this should be lsm.disable=loadpin lsm.enable=smack

> becomes
>
>     capability,smack,yama,integrity
>
> and
>
>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity

Do you mean
	selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo lsm.enable=integrity
	selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
	selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity lsm.disable=smack lsm.disable=tomoyo

> becomes
>
>     capability,integrity,yama,loadpin,apparmor
>
>
> If "lsm=" _does_ imply enablement, then how does it interact with
> per-LSM disabling? i.e. what does "apparmor.enabled=0
> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
> on a CONFIG-default-off LSM without specifying all the other LSMs too?

There should either be one option "lsm=", which is an explicit list or
two, "lsm.enable=" and "lsm.disable", which modify the built in default.

In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
apparmor off the list, but it's up to the AppArmor code to do that.

If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
of the security module is used, but it's up to the AppArmor code to do that.

If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
should have shut down AppArmor before it looked to see the "apparmor.enabled=1",
so it will remain disabled.

If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
specified is used giving "lsm.disable=apparmor".

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[John Johansen]

On 09/17/2018 12:23 PM, Casey Schaufler wrote:
> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>
>>> Keep security=$lsm with the existing exclusive behavior.
>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>
>>> If you want to be fancy (I don't!) you could add
>>>
>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>> We've got two issues: ordering and enablement. It's been strongly
>> suggested that we should move away from per-LSM enable/disable flags
>> (to which I agree).
> 
> I also agree. There are way too many ways to turn off some LSMs.
> 
I wont disagree, but its largely because we didn't have this discussion
when we should have. 

>> If ordering should be separate from enablement (to
>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>> didn't include it so it's disabled case), then I think we need to
>> split the logic (otherwise we just reinvented "security=" with similar
>> problems).
> 
> We could reduce the problem by declaring that LSM ordering is
> not something you can specify on the boot line. I can see value
> in specifying it when you build the kernel, but your circumstances
> would have to be pretty strange to change it at boot time.
> 

if there is LSM ordering the getting

  lsm=B,A,C

is not the behavior I would expect from specifying

  lsm=A,B,C


>> Should "lsm=" allow arbitrary ordering? (I think yes.)
> 
> I say no. Assume you can specify it at build time. When would
> you want to change the order? Why would you?
> 

because maybe you care about the denial message from one LSM more than
you do from another. Since stacking is bail on first fail the order
could be important from an auditing POV

Auditing is why apparmor's internal stacking is not bail on first
fail.

>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>> LSMs are implicitly auto-appended to the explicit list)
> 
> If you want to add something that isn't there instead of making
> it explicit you want "lsm.enable=" not "lsm=".
> 
>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>
>> If builtin list was:
>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>> then:
>>
>>     lsm.disable=loadpin lsm=smack
> 
> Methinks this should be lsm.disable=loadpin lsm.enable=smack
> 

that would only work if order is not important

>> becomes
>>
>>     capability,smack,yama,integrity
>>
>> and
>>
>>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
> 
> Do you mean
> 	selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo lsm.enable=integrity
> 	selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
> 	selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity lsm.disable=smack lsm.disable=tomoyo
> 
>> becomes
>>
>>     capability,integrity,yama,loadpin,apparmor
>>
>>
>> If "lsm=" _does_ imply enablement, then how does it interact with
>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
> 
> There should either be one option "lsm=", which is an explicit list or
> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
> 
maybe but this breaks with current behavior as their is a mismatch between
how the major lsms do selection/enablement and the minor ones.

I personally would prefer

  lsm=

but that breaks how the minor lsms are currently enable

> In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
> apparmor off the list, but it's up to the AppArmor code to do that.
>> If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
> of the security module is used, but it's up to the AppArmor code to do that.
> 
current behavior

> If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
> should have shut down AppArmor before it looked to see the "apparmor.enabled=1",
> so it will remain disabled.
> 
yep, current behavior

> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
> specified is used giving "lsm.disable=apparmor".
> 
makes sense

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 12:55 PM, John Johansen wrote:
> On 09/17/2018 12:23 PM, Casey Schaufler wrote:
>> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>>> Keep security=$lsm with the existing exclusive behavior.
>>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>>
>>>> If you want to be fancy (I don't!) you could add
>>>>
>>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>>> We've got two issues: ordering and enablement. It's been strongly
>>> suggested that we should move away from per-LSM enable/disable flags
>>> (to which I agree).
>> I also agree. There are way too many ways to turn off some LSMs.
>>
> I wont disagree, but its largely because we didn't have this discussion
> when we should have.

True that.


>>> If ordering should be separate from enablement (to
>>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>>> didn't include it so it's disabled case), then I think we need to
>>> split the logic (otherwise we just reinvented "security=" with similar
>>> problems).
>> We could reduce the problem by declaring that LSM ordering is
>> not something you can specify on the boot line. I can see value
>> in specifying it when you build the kernel, but your circumstances
>> would have to be pretty strange to change it at boot time.
>>
> if there is LSM ordering the getting
>
>   lsm=B,A,C
>
> is not the behavior I would expect from specifying
>
>   lsm=A,B,C

Right. You'd expect that they'd be used in the order specified.

>>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>> I say no. Assume you can specify it at build time. When would
>> you want to change the order? Why would you?
>>
> because maybe you care about the denial message from one LSM more than
> you do from another. Since stacking is bail on first fail the order
> could be important from an auditing POV

I understand that a distribution would want to specify the order
for support purposes and that a developer would want to specify
the order to ensure reproducible behavior. But they are going to
be controlling their kernel builds. I'm not suggesting that the
order shouldn't be capable of build time specification. What I
don't see is a reason to rearrange it at boot time.

> Auditing is why apparmor's internal stacking is not bail on first
> fail.

Within a security module I get that. But we've already got the
priority wrong for audit in general, because you only get to the
LSM if the traditional code approves. Every guidance I ever got
said you should do the MAC checks first, because you're much more
concerned about getting audit records about MAC failures than DAC.

>>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>>> LSMs are implicitly auto-appended to the explicit list)
>> If you want to add something that isn't there instead of making
>> it explicit you want "lsm.enable=" not "lsm=".
>>
>>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>>
>>> If builtin list was:
>>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>>> then:
>>>
>>>     lsm.disable=loadpin lsm=smack
>> Methinks this should be lsm.disable=loadpin lsm.enable=smack
>>
> that would only work if order is not important

It works unless you want to change the order at boot, and
I still don't see a use case for that.

>>> becomes
>>>
>>>     capability,smack,yama,integrity
>>>
>>> and
>>>
>>>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>> Do you mean
>> 	selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo lsm.enable=integrity
>> 	selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
>> 	selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity lsm.disable=smack lsm.disable=tomoyo
>>
>>> becomes
>>>
>>>     capability,integrity,yama,loadpin,apparmor
>>>
>>>
>>> If "lsm=" _does_ imply enablement, then how does it interact with
>>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>> There should either be one option "lsm=", which is an explicit list or
>> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
>>
> maybe but this breaks with current behavior as their is a mismatch between
> how the major lsms do selection/enablement and the minor ones.

Which is why you have to continue supporting "security=".

> I personally would prefer
>
>   lsm=
>
> but that breaks how the minor lsms are currently enable

I don't know if I'd say "breaks", but it would require change.

>> In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
>> apparmor off the list, but it's up to the AppArmor code to do that.
>>> If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
>> of the security module is used, but it's up to the AppArmor code to do that.
>>
> current behavior

That's right.

>> If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
>> should have shut down AppArmor before it looked to see the "apparmor.enabled=1",
>> so it will remain disabled.
>>
> yep, current behavior

2 for 2!


>> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
>> specified is used giving "lsm.disable=apparmor".
>>
> makes sense

The rules for modification are pretty obvious. The downside is, as
you point out, that they don't address ordering. Maybe we address that
directly:

	lsm.order=*,tomoyo

		TOMOYO should be last.

	lsm.order=apparmor,*

		AppArmor should be first.


	lsm.order=*,sara,selinux,*

		SELinux should come directly after SARA but we otherwise don't care.

	lsm.order=smack,*,landlock,*

		Smack should be first and LandLock should come sometime later.

	lsm.order=*,yama,*

		Is meaningless.

Modules not listed may go anywhere there is a "*" in the order.
An lsm.order= without a "*" is an error, and ignored.
If a module is specified in lsm.order but not built in it is ignored.
If a module is specified but disabled it is ignored.
The capability module goes first regardless.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[John Johansen]

On 09/17/2018 02:57 PM, Casey Schaufler wrote:
> On 9/17/2018 12:55 PM, John Johansen wrote:
>> On 09/17/2018 12:23 PM, Casey Schaufler wrote:
>>> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>>>> Keep security=$lsm with the existing exclusive behavior.
>>>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>>>
>>>>> If you want to be fancy (I don't!) you could add
>>>>>
>>>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>>>> We've got two issues: ordering and enablement. It's been strongly
>>>> suggested that we should move away from per-LSM enable/disable flags
>>>> (to which I agree).
>>> I also agree. There are way too many ways to turn off some LSMs.
>>>
>> I wont disagree, but its largely because we didn't have this discussion
>> when we should have.
> 
> True that.
> 
> 
>>>> If ordering should be separate from enablement (to
>>>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>>>> didn't include it so it's disabled case), then I think we need to
>>>> split the logic (otherwise we just reinvented "security=" with similar
>>>> problems).
>>> We could reduce the problem by declaring that LSM ordering is
>>> not something you can specify on the boot line. I can see value
>>> in specifying it when you build the kernel, but your circumstances
>>> would have to be pretty strange to change it at boot time.
>>>
>> if there is LSM ordering the getting
>>
>>   lsm=B,A,C
>>
>> is not the behavior I would expect from specifying
>>
>>   lsm=A,B,C
> 
> Right. You'd expect that they'd be used in the order specified.
> 

and yet you argue for something different ;)

>>>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>>> I say no. Assume you can specify it at build time. When would
>>> you want to change the order? Why would you?
>>>
>> because maybe you care about the denial message from one LSM more than
>> you do from another. Since stacking is bail on first fail the order
>> could be important from an auditing POV
> 
> I understand that a distribution would want to specify the order
> for support purposes and that a developer would want to specify
> the order to ensure reproducible behavior. But they are going to
> be controlling their kernel builds. I'm not suggesting that the
> order shouldn't be capable of build time specification. What I
> don't see is a reason to rearrange it at boot time.
> 

Because not all users have the same priority as the distro. It can
also aid in debugging and testing of LSMs in a stacked situation.

>> Auditing is why apparmor's internal stacking is not bail on first
>> fail.
> 
> Within a security module I get that. But we've already got the
> priority wrong for audit in general, because you only get to the
> LSM if the traditional code approves. Every guidance I ever got

true

> said you should do the MAC checks first, because you're much more
> concerned about getting audit records about MAC failures than DAC.
> 

yep, wouldn't that be nice to have

>>>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>>>> LSMs are implicitly auto-appended to the explicit list)
>>> If you want to add something that isn't there instead of making
>>> it explicit you want "lsm.enable=" not "lsm=".
>>>
>>>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>>>
>>>> If builtin list was:
>>>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>>>> then:
>>>>
>>>>     lsm.disable=loadpin lsm=smack
>>> Methinks this should be lsm.disable=loadpin lsm.enable=smack
>>>
>> that would only work if order is not important
> 
> It works unless you want to change the order at boot, and
> I still don't see a use case for that.

see above

> 
>>>> becomes
>>>>
>>>>     capability,smack,yama,integrity
>>>>
>>>> and
>>>>
>>>>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>>>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>>> Do you mean
>>> 	selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo lsm.enable=integrity
>>> 	selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
>>> 	selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity lsm.disable=smack lsm.disable=tomoyo
>>>
>>>> becomes
>>>>
>>>>     capability,integrity,yama,loadpin,apparmor
>>>>
>>>>
>>>> If "lsm=" _does_ imply enablement, then how does it interact with
>>>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>>>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>>>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>>> There should either be one option "lsm=", which is an explicit list or
>>> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
>>>
>> maybe but this breaks with current behavior as their is a mismatch between
>> how the major lsms do selection/enablement and the minor ones.
> 
> Which is why you have to continue supporting "security=".
> 
I would argue that switching to lsm= isn't exactly a fix either as we have
the whole minor lsm problem that we are currently debating.

>> I personally would prefer
>>
>>   lsm=
>>
>> but that breaks how the minor lsms are currently enable
> 
> I don't know if I'd say "breaks", but it would require change.
> 
depends how you look at it. Its a change to how its interacted with but so
is switching to lsm=

or making the minor module kconfig automatically add the current minor
lsms to a default lsm selection list, and making $lsm.disable behave
like apparmor or selinux=0.

we got it wrong early on, so now we have to live with something not
as clean as it could have been


>>> In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
>>> apparmor off the list, but it's up to the AppArmor code to do that.
>>>> If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
>>> of the security module is used, but it's up to the AppArmor code to do that.
>>>
>> current behavior
> 
> That's right.
> 
>>> If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
>>> should have shut down AppArmor before it looked to see the "apparmor.enabled=1",
>>> so it will remain disabled.
>>>
>> yep, current behavior
> 
> 2 for 2!
> 
> 
>>> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
>>> specified is used giving "lsm.disable=apparmor".
>>>
>> makes sense
> 
> The rules for modification are pretty obvious. The downside is, as
> you point out, that they don't address ordering. Maybe we address that
> directly:
> 
> 	lsm.order=*,tomoyo
> 
> 		TOMOYO should be last.
> 
> 	lsm.order=apparmor,*
> 
> 		AppArmor should be first.
> 
> 
> 	lsm.order=*,sara,selinux,*
> 
> 		SELinux should come directly after SARA but we otherwise don't care.
> 
> 	lsm.order=smack,*,landlock,*
> 
> 		Smack should be first and LandLock should come sometime later.
> 
> 	lsm.order=*,yama,*
> 
> 		Is meaningless.
> 
> Modules not listed may go anywhere there is a "*" in the order.
> An lsm.order= without a "*" is an error, and ignored.
> If a module is specified in lsm.order but not built in it is ignored.
> If a module is specified but disabled it is ignored.
> The capability module goes first regardless.
> 

I don't mind using lsm.order if we must but really do not like the '*'
idea. It makes this way more complicated than it needs to be

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 8128 bytes --]


On 9/18/18 00:36, John Johansen wrote:
> On 09/17/2018 02:57 PM, Casey Schaufler wrote:
>> On 9/17/2018 12:55 PM, John Johansen wrote:
>>> On 09/17/2018 12:23 PM, Casey Schaufler wrote:
>>>> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>>>>> Keep security=$lsm with the existing exclusive behavior.
>>>>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>>>>
>>>>>> If you want to be fancy (I don't!) you could add
>>>>>>
>>>>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>>>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>>>>> We've got two issues: ordering and enablement. It's been strongly
>>>>> suggested that we should move away from per-LSM enable/disable flags
>>>>> (to which I agree).
>>>> I also agree. There are way too many ways to turn off some LSMs.
>>>>
>>> I wont disagree, but its largely because we didn't have this discussion
>>> when we should have.
>>
>> True that.
>>
>>
>>>>> If ordering should be separate from enablement (to
>>>>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>>>>> didn't include it so it's disabled case), then I think we need to
>>>>> split the logic (otherwise we just reinvented "security=" with similar
>>>>> problems).
>>>> We could reduce the problem by declaring that LSM ordering is
>>>> not something you can specify on the boot line. I can see value
>>>> in specifying it when you build the kernel, but your circumstances
>>>> would have to be pretty strange to change it at boot time.
>>>>
>>> if there is LSM ordering the getting
>>>
>>>   lsm=B,A,C
>>>
>>> is not the behavior I would expect from specifying
>>>
>>>   lsm=A,B,C
>>
>> Right. You'd expect that they'd be used in the order specified.
>>
> 
> and yet you argue for something different ;)
> 
>>>>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>>>> I say no. Assume you can specify it at build time. When would
>>>> you want to change the order? Why would you?
>>>>
>>> because maybe you care about the denial message from one LSM more than
>>> you do from another. Since stacking is bail on first fail the order
>>> could be important from an auditing POV
>>
>> I understand that a distribution would want to specify the order
>> for support purposes and that a developer would want to specify
>> the order to ensure reproducible behavior. But they are going to
>> be controlling their kernel builds. I'm not suggesting that the
>> order shouldn't be capable of build time specification. What I
>> don't see is a reason to rearrange it at boot time.
>>
> 
> Because not all users have the same priority as the distro. It can
> also aid in debugging and testing of LSMs in a stacked situation.
> 
>>> Auditing is why apparmor's internal stacking is not bail on first
>>> fail.
>>
>> Within a security module I get that. But we've already got the
>> priority wrong for audit in general, because you only get to the
>> LSM if the traditional code approves. Every guidance I ever got
> 
> true
> 
>> said you should do the MAC checks first, because you're much more
>> concerned about getting audit records about MAC failures than DAC.
>>
> 
> yep, wouldn't that be nice to have
> 
>>>>> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
>>>>> LSMs are implicitly auto-appended to the explicit list)
>>>> If you want to add something that isn't there instead of making
>>>> it explicit you want "lsm.enable=" not "lsm=".
>>>>
>>>>> So then we could have "lsm.enable=..." and "lsm.disable=...".
>>>>>
>>>>> If builtin list was:
>>>>> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
>>>>> then:
>>>>>
>>>>>     lsm.disable=loadpin lsm=smack
>>>> Methinks this should be lsm.disable=loadpin lsm.enable=smack
>>>>
>>> that would only work if order is not important
>>
>> It works unless you want to change the order at boot, and
>> I still don't see a use case for that.
> 
> see above
> 
>>
>>>>> becomes
>>>>>
>>>>>     capability,smack,yama,integrity
>>>>>
>>>>> and
>>>>>
>>>>>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>>>>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>>>> Do you mean
>>>> 	selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo lsm.enable=integrity
>>>> 	selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
>>>> 	selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity lsm.disable=smack lsm.disable=tomoyo
>>>>
>>>>> becomes
>>>>>
>>>>>     capability,integrity,yama,loadpin,apparmor
>>>>>
>>>>>
>>>>> If "lsm=" _does_ imply enablement, then how does it interact with
>>>>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>>>>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>>>>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>>>> There should either be one option "lsm=", which is an explicit list or
>>>> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
>>>>
>>> maybe but this breaks with current behavior as their is a mismatch between
>>> how the major lsms do selection/enablement and the minor ones.
>>
>> Which is why you have to continue supporting "security=".
>>
> I would argue that switching to lsm= isn't exactly a fix either as we have
> the whole minor lsm problem that we are currently debating.
> 
>>> I personally would prefer
>>>
>>>   lsm=
>>>
>>> but that breaks how the minor lsms are currently enable
>>
>> I don't know if I'd say "breaks", but it would require change.
>>
> depends how you look at it. Its a change to how its interacted with but so
> is switching to lsm=
> 
> or making the minor module kconfig automatically add the current minor
> lsms to a default lsm selection list, and making $lsm.disable behave
> like apparmor or selinux=0.
> 
> we got it wrong early on, so now we have to live with something not
> as clean as it could have been
> 
> 
>>>> In the "lsm=" case "apparmor.enabled=0" should be equivalent to leaving
>>>> apparmor off the list, but it's up to the AppArmor code to do that.
>>>>> If "lsm.enable=apparmor apparmor.enabled=0" is specified the explict wish
>>>> of the security module is used, but it's up to the AppArmor code to do that.
>>>>
>>> current behavior
>>
>> That's right.
>>
>>>> If "lsm.disable=apparmor apparmor.enabled=1" is specified the infrastructure
>>>> should have shut down AppArmor before it looked to see the "apparmor.enabled=1",
>>>> so it will remain disabled.
>>>>
>>> yep, current behavior
>>
>> 2 for 2!
>>
>>
>>>> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
>>>> specified is used giving "lsm.disable=apparmor".
>>>>
>>> makes sense
>>
>> The rules for modification are pretty obvious. The downside is, as
>> you point out, that they don't address ordering. Maybe we address that
>> directly:
>>
>> 	lsm.order=*,tomoyo
>>
>> 		TOMOYO should be last.
>>
>> 	lsm.order=apparmor,*
>>
>> 		AppArmor should be first.
>>
>>
>> 	lsm.order=*,sara,selinux,*
>>
>> 		SELinux should come directly after SARA but we otherwise don't care.
>>
>> 	lsm.order=smack,*,landlock,*
>>
>> 		Smack should be first and LandLock should come sometime later.
>>
>> 	lsm.order=*,yama,*
>>
>> 		Is meaningless.
>>
>> Modules not listed may go anywhere there is a "*" in the order.
>> An lsm.order= without a "*" is an error, and ignored.
>> If a module is specified in lsm.order but not built in it is ignored.
>> If a module is specified but disabled it is ignored.
>> The capability module goes first regardless.
>>
> 
> I don't mind using lsm.order if we must but really do not like the '*'
> idea. It makes this way more complicated than it needs to be
> 
> 

Landlock, because it target unprivileged users, should only be called
after all other major (access-control) LSMs. The admin or distro must
not be able to change that order in any way. This constraint doesn't
apply to current LSMs, though.

 Mickaël


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün <mic@digikod.net> wrote:
> Landlock, because it target unprivileged users, should only be called
> after all other major (access-control) LSMs. The admin or distro must
> not be able to change that order in any way. This constraint doesn't
> apply to current LSMs, though.

Good point! It will be easy to add LSM_ORDER_LAST, though, given the
machinery introduced in this series.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[John Johansen]

On 09/17/2018 04:20 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> Landlock, because it target unprivileged users, should only be called
>> after all other major (access-control) LSMs. The admin or distro must
>> not be able to change that order in any way. This constraint doesn't
>> apply to current LSMs, though.
> 
> Good point! It will be easy to add LSM_ORDER_LAST, though, given the
> machinery introduced in this series.
> 

And when we have two LSMs that want to use that?

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Mon, Sep 17, 2018 at 4:26 PM, John Johansen
<john.johansen@canonical.com> wrote:
> On 09/17/2018 04:20 PM, Kees Cook wrote:
>> On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> Landlock, because it target unprivileged users, should only be called
>>> after all other major (access-control) LSMs. The admin or distro must
>>> not be able to change that order in any way. This constraint doesn't
>>> apply to current LSMs, though.
>>
>> Good point! It will be easy to add LSM_ORDER_LAST, though, given the
>> machinery introduced in this series.
>>
>
> And when we have two LSMs that want to use that?

We'll cross that bridge when we come to it, but perhaps "last
exclusive"? (lsm.enable/disable to choose)

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 4:28 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 4:26 PM, John Johansen
> <john.johansen@canonical.com> wrote:
>> On 09/17/2018 04:20 PM, Kees Cook wrote:
>>> On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>> Landlock, because it target unprivileged users, should only be called
>>>> after all other major (access-control) LSMs. The admin or distro must
>>>> not be able to change that order in any way. This constraint doesn't
>>>> apply to current LSMs, though.
>>> Good point! It will be easy to add LSM_ORDER_LAST, though, given the
>>> machinery introduced in this series.
>>>
>> And when we have two LSMs that want to use that?
> We'll cross that bridge when we come to it, but perhaps "last
> exclusive"? (lsm.enable/disable to choose)

If we define restrictions on use of LSM_ORDER_LAST like we have
for LSM_ORDER_FIRST (only for capabilities) before anyone starts
abusing it we may be OK. Since an LSM_ORDER_LAST has to know that
it can't count on getting called (a non-last module may return -EACCES)
I don't see any way that having multiple LSM_ORDER_LAST modules in
any given order would be a real problem. Of course, a module could be
doing state management that *really* requires it be last, but that
would be a badly designed module and someone sensible would NAK it.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 4:20 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün <mic@digikod.net> wrote:
>> Landlock, because it target unprivileged users, should only be called
>> after all other major (access-control) LSMs. The admin or distro must
>> not be able to change that order in any way. This constraint doesn't
>> apply to current LSMs, though.

What harm would it cause for Landlock to get called before SELinux?
I certainly see why it seems like it ought to get called after, but
would it really make a difference?

> Good point! It will be easy to add LSM_ORDER_LAST, though, given the
> machinery introduced in this series.
>
> -Kees
>

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 1208 bytes --]


On 9/18/18 01:30, Casey Schaufler wrote:
> On 9/17/2018 4:20 PM, Kees Cook wrote:
>> On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>> Landlock, because it target unprivileged users, should only be called
>>> after all other major (access-control) LSMs. The admin or distro must
>>> not be able to change that order in any way. This constraint doesn't
>>> apply to current LSMs, though.
> 
> What harm would it cause for Landlock to get called before SELinux?
> I certainly see why it seems like it ought to get called after, but
> would it really make a difference?

If an unprivileged process is able to infer some properties of a file
being requested (thanks to one of its eBPF program doing checks on this
process accesses), whereas this file access would be denied by a
privileged LSM, then there is a side channel attack allowing this
process to indirectly get information otherwise inaccessible.

In other words, an unprivileged process should not be allowed to sneak
itself (via an eBPF program) before SELinux for instance. SELinux should
be able to block such information gathering the same way it can block a
fstat(2) requested by a process.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 4:47 PM, Mickaël Salaün wrote:
> On 9/18/18 01:30, Casey Schaufler wrote:
>> On 9/17/2018 4:20 PM, Kees Cook wrote:
>>> On Mon, Sep 17, 2018 at 4:10 PM, Mickaël Salaün <mic@digikod.net> wrote:
>>>> Landlock, because it target unprivileged users, should only be called
>>>> after all other major (access-control) LSMs. The admin or distro must
>>>> not be able to change that order in any way. This constraint doesn't
>>>> apply to current LSMs, though.
>> What harm would it cause for Landlock to get called before SELinux?
>> I certainly see why it seems like it ought to get called after, but
>> would it really make a difference?
> If an unprivileged process is able to infer some properties of a file
> being requested (thanks to one of its eBPF program doing checks on this
> process accesses), whereas this file access would be denied by a
> privileged LSM, then there is a side channel attack allowing this
> process to indirectly get information otherwise inaccessible.
>
> In other words, an unprivileged process should not be allowed to sneak
> itself (via an eBPF program) before SELinux for instance. SELinux should
> be able to block such information gathering the same way it can block a
> fstat(2) requested by a process.

The argument would feel a bit stronger if LSM checks happened before
the DAC checks. The opportunity to sneak a check in already exists, but
not with the tools you get with eBPF. For now at least I'll grant that
there's good reason for Landlock to go last.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[John Johansen]

On 09/17/2018 04:10 PM, Mickaël Salaün wrote:
> 

<< snip >>

>>>>> If "lsm.enable=apparmor lsm.disable=apparmor" is specified the last value
>>>>> specified is used giving "lsm.disable=apparmor".
>>>>>
>>>> makes sense
>>>
>>> The rules for modification are pretty obvious. The downside is, as
>>> you point out, that they don't address ordering. Maybe we address that
>>> directly:
>>>
>>> 	lsm.order=*,tomoyo
>>>
>>> 		TOMOYO should be last.
>>>
>>> 	lsm.order=apparmor,*
>>>
>>> 		AppArmor should be first.
>>>
>>>
>>> 	lsm.order=*,sara,selinux,*
>>>
>>> 		SELinux should come directly after SARA but we otherwise don't care.
>>>
>>> 	lsm.order=smack,*,landlock,*
>>>
>>> 		Smack should be first and LandLock should come sometime later.
>>>
>>> 	lsm.order=*,yama,*
>>>
>>> 		Is meaningless.
>>>
>>> Modules not listed may go anywhere there is a "*" in the order.
>>> An lsm.order= without a "*" is an error, and ignored.
>>> If a module is specified in lsm.order but not built in it is ignored.
>>> If a module is specified but disabled it is ignored.
>>> The capability module goes first regardless.
>>>
>>
>> I don't mind using lsm.order if we must but really do not like the '*'
>> idea. It makes this way more complicated than it needs to be
>>
>>
> 
> Landlock, because it target unprivileged users, should only be called
> after all other major (access-control) LSMs. The admin or distro must
> not be able to change that order in any way. This constraint doesn't
> apply to current LSMs, though.
> 

And yet another complication :)

I don't know that we can enforce a strict only after all other LSMs. Imagine
the hypothetical case of 2 LSMs targeting unprivileged users. Which one
should be called first?

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 3:36 PM, John Johansen wrote:
> On 09/17/2018 02:57 PM, Casey Schaufler wrote:
>> On 9/17/2018 12:55 PM, John Johansen wrote:
>>> On 09/17/2018 12:23 PM, Casey Schaufler wrote:
>>>> On 9/17/2018 11:14 AM, Kees Cook wrote:
>>>>>> Keep security=$lsm with the existing exclusive behavior.
>>>>>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>>>>>
>>>>>> If you want to be fancy (I don't!) you could add
>>>>>>
>>>>>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>>>>>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
>>>>> We've got two issues: ordering and enablement. It's been strongly
>>>>> suggested that we should move away from per-LSM enable/disable flags
>>>>> (to which I agree).
>>>> I also agree. There are way too many ways to turn off some LSMs.
>>>>
>>> I wont disagree, but its largely because we didn't have this discussion
>>> when we should have.
>> True that.
>>
>>
>>>>> If ordering should be separate from enablement (to
>>>>> avoid the "booted kernel with new LSM built in, but my lsm="..." line
>>>>> didn't include it so it's disabled case), then I think we need to
>>>>> split the logic (otherwise we just reinvented "security=" with similar
>>>>> problems).
>>>> We could reduce the problem by declaring that LSM ordering is
>>>> not something you can specify on the boot line. I can see value
>>>> in specifying it when you build the kernel, but your circumstances
>>>> would have to be pretty strange to change it at boot time.
>>>>
>>> if there is LSM ordering the getting
>>>
>>>   lsm=B,A,C
>>>
>>> is not the behavior I would expect from specifying
>>>
>>>   lsm=A,B,C
>> Right. You'd expect that they'd be used in the order specified.
>>
> and yet you argue for something different ;)

A foolish consistency is the hobgoblin of little minds.
Or, more to the point in this case, I don't see a way to
accomplish the ends well, so I'm casting about for something
that no one hates too badly.

>>>>> Should "lsm=" allow arbitrary ordering? (I think yes.)
>>>> I say no. Assume you can specify it at build time. When would
>>>> you want to change the order? Why would you?
>>>>
>>> because maybe you care about the denial message from one LSM more than
>>> you do from another. Since stacking is bail on first fail the order
>>> could be important from an auditing POV
>> I understand that a distribution would want to specify the order
>> for support purposes and that a developer would want to specify
>> the order to ensure reproducible behavior. But they are going to
>> be controlling their kernel builds. I'm not suggesting that the
>> order shouldn't be capable of build time specification. What I
>> don't see is a reason to rearrange it at boot time.
>>
> Because not all users have the same priority as the distro. It can
> also aid in debugging and testing of LSMs in a stacked situation.

My assumption is that specifying the LSM order on the boot line
by hand is going to be pretty rare. So it doesn't have to be easy,
it just needs to be sane.

> ... <snip>
>>>>> becomes
>>>>>
>>>>>     capability,smack,yama,integrity
>>>>>
>>>>> and
>>>>>
>>>>>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>>>>>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
>>>> Do you mean
>>>> 	selinux.enable=0 lsm.enable=loadpin lsm.disable=smack,tomoyo lsm.enable=integrity
>>>> 	selinux.enable=0 lsm.enable=loadpin,integrity lsm.disable=smack,tomoyo
>>>> 	selinux.enable=0 lsm.enable=loadpin lsm.enable=integrity lsm.disable=smack lsm.disable=tomoyo
>>>>
>>>>> becomes
>>>>>
>>>>>     capability,integrity,yama,loadpin,apparmor
>>>>>
>>>>>
>>>>> If "lsm=" _does_ imply enablement, then how does it interact with
>>>>> per-LSM disabling? i.e. what does "apparmor.enabled=0
>>>>> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
>>>>> on a CONFIG-default-off LSM without specifying all the other LSMs too?
>>>> There should either be one option "lsm=", which is an explicit list or
>>>> two, "lsm.enable=" and "lsm.disable", which modify the built in default.
>>>>
>>> maybe but this breaks with current behavior as their is a mismatch between
>>> how the major lsms do selection/enablement and the minor ones.
>> Which is why you have to continue supporting "security=".
>>
> I would argue that switching to lsm= isn't exactly a fix either as we have
> the whole minor lsm problem that we are currently debating.

I'm finding it hard to argue for "lsm=" because it's too clumsy.


>>> I personally would prefer
>>>
>>>   lsm=
>>>
>>> but that breaks how the minor lsms are currently enable
>> I don't know if I'd say "breaks", but it would require change.
>>
> depends how you look at it. Its a change to how its interacted with but so
> is switching to lsm=
>
> or making the minor module kconfig automatically add the current minor
> lsms to a default lsm selection list, and making $lsm.disable behave
> like apparmor or selinux=0.
>
> we got it wrong early on, so now we have to live with something not
> as clean as it could have been

It's not the first time and won't be the last.

 

>>> ... <snip>
>> The rules for modification are pretty obvious. The downside is, as
>> you point out, that they don't address ordering. Maybe we address that
>> directly:
>>
>> 	lsm.order=*,tomoyo
>>
>> 		TOMOYO should be last.
>>
>> 	lsm.order=apparmor,*
>>
>> 		AppArmor should be first.
>>
>>
>> 	lsm.order=*,sara,selinux,*
>>
>> 		SELinux should come directly after SARA but we otherwise don't care.
>>
>> 	lsm.order=smack,*,landlock,*
>>
>> 		Smack should be first and LandLock should come sometime later.
>>
>> 	lsm.order=*,yama,*
>>
>> 		Is meaningless.
>>
>> Modules not listed may go anywhere there is a "*" in the order.
>> An lsm.order= without a "*" is an error, and ignored.
>> If a module is specified in lsm.order but not built in it is ignored.
>> If a module is specified but disabled it is ignored.
>> The capability module goes first regardless.
>>
> I don't mind using lsm.order if we must but really do not like the '*'
> idea. It makes this way more complicated than it needs to be

We could arbitrarily say that anything unspecified goes after what
shows up in lsm.order (like lsm.order=yama,smack,* )

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Mon, Sep 17, 2018 at 3:36 PM, John Johansen
<john.johansen@canonical.com> wrote:
> On 09/17/2018 02:57 PM, Casey Schaufler wrote:
>> Modules not listed may go anywhere there is a "*" in the order.
>> An lsm.order= without a "*" is an error, and ignored.
>> If a module is specified in lsm.order but not built in it is ignored.
>> If a module is specified but disabled it is ignored.
>> The capability module goes first regardless.
>
> I don't mind using lsm.order if we must but really do not like the '*'
> idea. It makes this way more complicated than it needs to be

Having the "*" means that _not_ having it in "lsm.order=" is an
implicit form of LSM disabling. And I think we've gotten to the point
where we agree on the enable/disable logic, so I don't want to mess
that up again.

For enable/disable, I think we're agreed on:

    lsm.enable=$lsm
    lsm.disable=$lsm

lsm.disable takes precedent for disabling. (e.g. "lsm.disable=apparmor
apparmor.enable=1" will leave apparmor disabled)
lsm.enable will allow per-LSM enable/disable to operate. (e.g.
"lsm.enable=apparmor apparmor.enable=0" will leave apparmor disabled)

lsm.enable/disable ordering will be "last match": "lsm.disable=smack
lsm.enable=smack" will leave smack enabled. The legacy per-LSM
enable/disable ordering is the same, but ordering between
lsm.enable/disable and the per-LSM options is NOT ordered. i.e. the
precedent mentioned in the prior paragraph.

To support "security=", we'll still have some kind of legacy
LSM_FLAG_MAJOR to perform implicit disabling of the non-operational
other "major" LSMs. This means "security=$foo" will be a short-hand
for "lsm.disable=all-LSM_FLAG_MAJOR-who-are-not-$foo". This will
exactly match current behavior (i.e. "security=smack" and if smack
fails initialization, we do not then fall back to another major).


I think we have to support runtime ordering for the reasons John
specifies. Additionally, I have the sense that anything we can
configure in Kconfig ultimately ends up being expressed at runtime
too, so better to just make sure the design includes it now.

What we have now:

"first" then "order-doesn't-matter-minors" then "exclusive-major"

- we can't change first.
- exclusivity-ordering only matters in the face of enable/disable
which we have solved now (?)

so, ordering can be totally arbitrary after "first" (but before some
future "last"). We must not allow a token for "everything else" since
that overlaps with enable/disable, so "everything else" stay implicit
(I would argue a trailing implicit ordering).

The one complication I see with ordering, then, is that if we change
the exclusivity over time, we change what may be present on the
system. For example, right now tomoyo is exclusive. Once we have
blob-sharing, it doesn't need to be.

so: lsm.order=tomoyo  after this series means
"capability,tomoyo,yama,loadpin,integrity", but when tomoyo becomes
non-exclusive, suddenly we get
"capability,tomoyo,yama,loadpin,{selinux,smack,apparmor},integrity".
(i.e. if selinux is disabled then move on to trying smack, then
apparmor, etc.)

I would argue that this is a design feature (LSMs aren't left behind),
and order of enabled exclusive LSMs "wins" the choice for the
exclusivity (instead of operating "by name" the way "security="
works).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 5:00 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 3:36 PM, John Johansen
> <john.johansen@canonical.com> wrote:
>> On 09/17/2018 02:57 PM, Casey Schaufler wrote:
>>> Modules not listed may go anywhere there is a "*" in the order.
>>> An lsm.order= without a "*" is an error, and ignored.
>>> If a module is specified in lsm.order but not built in it is ignored.
>>> If a module is specified but disabled it is ignored.
>>> The capability module goes first regardless.
>> I don't mind using lsm.order if we must but really do not like the '*'
>> idea. It makes this way more complicated than it needs to be
> Having the "*" means that _not_ having it in "lsm.order=" is an
> implicit form of LSM disabling.

That's not what I said. What I said was that without a "*" the ordering
goes back to what was specified at build time. lsm.order does nothing
with enablement or disablement. If you say "lsm.order=smack,sara,*" and
sara is not compiled in you get smack followed by everything else.

> And I think we've gotten to the point
> where we agree on the enable/disable logic, so I don't want to mess
> that up again.
>
> For enable/disable, I think we're agreed on:
>
>     lsm.enable=$lsm
>     lsm.disable=$lsm

Works for me.

> lsm.disable takes precedent for disabling. (e.g. "lsm.disable=apparmor
> apparmor.enable=1" will leave apparmor disabled)
> lsm.enable will allow per-LSM enable/disable to operate. (e.g.
> "lsm.enable=apparmor apparmor.enable=0" will leave apparmor disabled)
>
> lsm.enable/disable ordering will be "last match": "lsm.disable=smack
> lsm.enable=smack" will leave smack enabled.

So far do good.

> The legacy per-LSM
> enable/disable ordering is the same, but ordering between
> lsm.enable/disable and the per-LSM options is NOT ordered. i.e. the
> precedent mentioned in the prior paragraph.

That is, capability,yama,loadpin,<major>

> To support "security=", we'll still have some kind of legacy
> LSM_FLAG_MAJOR to perform implicit disabling of the non-operational
> other "major" LSMs. This means "security=$foo" will be a short-hand
> for "lsm.disable=all-LSM_FLAG_MAJOR-who-are-not-$foo". This will
> exactly match current behavior (i.e. "security=smack" and if smack
> fails initialization, we do not then fall back to another major).

Right.

> I think we have to support runtime ordering for the reasons John
> specifies. Additionally, I have the sense that anything we can
> configure in Kconfig ultimately ends up being expressed at runtime
> too, so better to just make sure the design includes it now.

Right.

> What we have now:
>
> "first" then "order-doesn't-matter-minors" then "exclusive-major"
>
> - we can't change first.
> - exclusivity-ordering only matters in the face of enable/disable
> which we have solved now (?)

I'm not sure where you get the conclusion we've solved this.
Today I can't say "lsm.enable=smack lsm.enable=apparmor", and
there's no mechanism to prevent that.

> so, ordering can be totally arbitrary after "first" (but before some
> future "last"). We must not allow a token for "everything else" since
> that overlaps with enable/disable, so "everything else" stay implicit
> (I would argue a trailing implicit ordering).

There's an assumption you're making that I'm not getting. Where does
this overlap between ordering and enable/disable come from?

> The one complication I see with ordering, then, is that if we change
> the exclusivity over time, we change what may be present on the
> system. For example, right now tomoyo is exclusive. Once we have
> blob-sharing, it doesn't need to be.
>
> so: lsm.order=tomoyo  after this series means
> "capability,tomoyo,yama,loadpin,integrity", but when tomoyo becomes
> non-exclusive, suddenly we get
> "capability,tomoyo,yama,loadpin,{selinux,smack,apparmor},integrity".
> (i.e. if selinux is disabled then move on to trying smack, then
> apparmor, etc.)

We're missing a description of what happens at build time.
It's hard to see what you expect to happen if I want to build in
all the major modules and don't plan to use the boot command line
options.

> I would argue that this is a design feature (LSMs aren't left behind),
> and order of enabled exclusive LSMs "wins" the choice for the
> exclusivity (instead of operating "by name" the way "security="
> works).

I think I see more, but I'm guessing. At build time it looks like
you're dropping the specification on the "major" module. We can't
do that because I want to build kernels that run Smack by default
but include SELinux for when I'm feeling less evil than normal.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Mon, Sep 17, 2018 at 5:24 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 9/17/2018 5:00 PM, Kees Cook wrote:
>> The legacy per-LSM
>> enable/disable ordering is the same, but ordering between
>> lsm.enable/disable and the per-LSM options is NOT ordered. i.e. the
>> precedent mentioned in the prior paragraph.
>
> That is, capability,yama,loadpin,<major>

Yeah, sorry, I didn't mean LSM order there, I meant the commandline
order of appearance of the options. If you mix them, the last
lsm.enable/disable for an LSM is the "real" setting, and the last
$LSM.enabled= setting is the last of _that_ one.

>> To support "security=", we'll still have some kind of legacy
>> LSM_FLAG_MAJOR to perform implicit disabling of the non-operational
>> other "major" LSMs. This means "security=$foo" will be a short-hand
>> for "lsm.disable=all-LSM_FLAG_MAJOR-who-are-not-$foo". This will
>> exactly match current behavior (i.e. "security=smack" and if smack
>> fails initialization, we do not then fall back to another major).
>
> Right.

Cool.

>> I think we have to support runtime ordering for the reasons John
>> specifies. Additionally, I have the sense that anything we can
>> configure in Kconfig ultimately ends up being expressed at runtime
>> too, so better to just make sure the design includes it now.
>
> Right.
>
>> What we have now:
>>
>> "first" then "order-doesn't-matter-minors" then "exclusive-major"
>>
>> - we can't change first.
>> - exclusivity-ordering only matters in the face of enable/disable
>> which we have solved now (?)
>
> I'm not sure where you get the conclusion we've solved this.
> Today I can't say "lsm.enable=smack lsm.enable=apparmor", and
> there's no mechanism to prevent that.
>
>> so, ordering can be totally arbitrary after "first" (but before some
>> future "last"). We must not allow a token for "everything else" since
>> that overlaps with enable/disable, so "everything else" stay implicit
>> (I would argue a trailing implicit ordering).
>
> There's an assumption you're making that I'm not getting. Where does
> this overlap between ordering and enable/disable come from?

Handling exclusivity means the non-active LSMs are disabled. We had
been saying "the other majors are disabled", but the concept of major
will become arbitrary. If instead we move to "first exclusive wins
among the exclusives", we still have the "the others are disabled"
case. So exclusivity begets disabling.

>> The one complication I see with ordering, then, is that if we change
>> the exclusivity over time, we change what may be present on the
>> system. For example, right now tomoyo is exclusive. Once we have
>> blob-sharing, it doesn't need to be.
>>
>> so: lsm.order=tomoyo  after this series means
>> "capability,tomoyo,yama,loadpin,integrity", but when tomoyo becomes
>> non-exclusive, suddenly we get
>> "capability,tomoyo,yama,loadpin,{selinux,smack,apparmor},integrity".
>> (i.e. if selinux is disabled then move on to trying smack, then
>> apparmor, etc.)
>
> We're missing a description of what happens at build time.
> It's hard to see what you expect to happen if I want to build in
> all the major modules and don't plan to use the boot command line
> options.
>
>> I would argue that this is a design feature (LSMs aren't left behind),
>> and order of enabled exclusive LSMs "wins" the choice for the
>> exclusivity (instead of operating "by name" the way "security="
>> works).
>
> I think I see more, but I'm guessing. At build time it looks like
> you're dropping the specification on the "major" module. We can't
> do that because I want to build kernels that run Smack by default
> but include SELinux for when I'm feeling less evil than normal.

Do we need build time _ordering_, or can we just go with build time
"first exclusive"? For the v1, I went with "first exclusive" from
CONFIG_SECURITY_DEFAULT, and left the rest of the ordering up to the
Makefile.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Casey Schaufler]

On 9/17/2018 5:45 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 5:24 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 9/17/2018 5:00 PM, Kees Cook wrote:
>>> The legacy per-LSM
>>> enable/disable ordering is the same, but ordering between
>>> lsm.enable/disable and the per-LSM options is NOT ordered. i.e. the
>>> precedent mentioned in the prior paragraph.
>> That is, capability,yama,loadpin,<major>
> Yeah, sorry, I didn't mean LSM order there, I meant the commandline
> order of appearance of the options. If you mix them, the last
> lsm.enable/disable for an LSM is the "real" setting, and the last
> $LSM.enabled= setting is the last of _that_ one.
>
>>> To support "security=", we'll still have some kind of legacy
>>> LSM_FLAG_MAJOR to perform implicit disabling of the non-operational
>>> other "major" LSMs. This means "security=$foo" will be a short-hand
>>> for "lsm.disable=all-LSM_FLAG_MAJOR-who-are-not-$foo". This will
>>> exactly match current behavior (i.e. "security=smack" and if smack
>>> fails initialization, we do not then fall back to another major).
>> Right.
> Cool.
>
>>> I think we have to support runtime ordering for the reasons John
>>> specifies. Additionally, I have the sense that anything we can
>>> configure in Kconfig ultimately ends up being expressed at runtime
>>> too, so better to just make sure the design includes it now.
>> Right.
>>
>>> What we have now:
>>>
>>> "first" then "order-doesn't-matter-minors" then "exclusive-major"
>>>
>>> - we can't change first.
>>> - exclusivity-ordering only matters in the face of enable/disable
>>> which we have solved now (?)
>> I'm not sure where you get the conclusion we've solved this.
>> Today I can't say "lsm.enable=smack lsm.enable=apparmor", and
>> there's no mechanism to prevent that.
>>
>>> so, ordering can be totally arbitrary after "first" (but before some
>>> future "last"). We must not allow a token for "everything else" since
>>> that overlaps with enable/disable, so "everything else" stay implicit
>>> (I would argue a trailing implicit ordering).
>> There's an assumption you're making that I'm not getting. Where does
>> this overlap between ordering and enable/disable come from?
> Handling exclusivity means the non-active LSMs are disabled. We had
> been saying "the other majors are disabled", but the concept of major
> will become arbitrary. If instead we move to "first exclusive wins
> among the exclusives", we still have the "the others are disabled"
> case. So exclusivity begets disabling.
>
>>> The one complication I see with ordering, then, is that if we change
>>> the exclusivity over time, we change what may be present on the
>>> system. For example, right now tomoyo is exclusive. Once we have
>>> blob-sharing, it doesn't need to be.
>>>
>>> so: lsm.order=tomoyo  after this series means
>>> "capability,tomoyo,yama,loadpin,integrity", but when tomoyo becomes
>>> non-exclusive, suddenly we get
>>> "capability,tomoyo,yama,loadpin,{selinux,smack,apparmor},integrity".
>>> (i.e. if selinux is disabled then move on to trying smack, then
>>> apparmor, etc.)
>> We're missing a description of what happens at build time.
>> It's hard to see what you expect to happen if I want to build in
>> all the major modules and don't plan to use the boot command line
>> options.
>>
>>> I would argue that this is a design feature (LSMs aren't left behind),
>>> and order of enabled exclusive LSMs "wins" the choice for the
>>> exclusivity (instead of operating "by name" the way "security="
>>> works).
>> I think I see more, but I'm guessing. At build time it looks like
>> you're dropping the specification on the "major" module. We can't
>> do that because I want to build kernels that run Smack by default
>> but include SELinux for when I'm feeling less evil than normal.
> Do we need build time _ordering_, or can we just go with build time
> "first exclusive"? For the v1, I went with "first exclusive" from
> CONFIG_SECURITY_DEFAULT, and left the rest of the ordering up to the
> Makefile.

If I read you correctly, "first exclusive" would suit my needs just fine.
I like the notion of build time ordering because I hate using the boot
command line.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[Kees Cook]

On Mon, Sep 17, 2018 at 5:57 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> If I read you correctly, "first exclusive" would suit my needs just fine.
> I like the notion of build time ordering because I hate using the boot
> command line.

Okay, excellent. I think I have enough for a v2 on this. I'll crank it out...

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[John Johansen]

On 09/17/2018 05:45 PM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 5:24 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 9/17/2018 5:00 PM, Kees Cook wrote:
>>> The legacy per-LSM
>>> enable/disable ordering is the same, but ordering between
>>> lsm.enable/disable and the per-LSM options is NOT ordered. i.e. the
>>> precedent mentioned in the prior paragraph.
>>
>> That is, capability,yama,loadpin,<major>
> 
> Yeah, sorry, I didn't mean LSM order there, I meant the commandline
> order of appearance of the options. If you mix them, the last
> lsm.enable/disable for an LSM is the "real" setting, and the last
> $LSM.enabled= setting is the last of _that_ one.
> 
>>> To support "security=", we'll still have some kind of legacy
>>> LSM_FLAG_MAJOR to perform implicit disabling of the non-operational
>>> other "major" LSMs. This means "security=$foo" will be a short-hand
>>> for "lsm.disable=all-LSM_FLAG_MAJOR-who-are-not-$foo". This will
>>> exactly match current behavior (i.e. "security=smack" and if smack
>>> fails initialization, we do not then fall back to another major).
>>
>> Right.
> 
> Cool.
> 
>>> I think we have to support runtime ordering for the reasons John
>>> specifies. Additionally, I have the sense that anything we can
>>> configure in Kconfig ultimately ends up being expressed at runtime
>>> too, so better to just make sure the design includes it now.
>>
>> Right.
>>
>>> What we have now:
>>>
>>> "first" then "order-doesn't-matter-minors" then "exclusive-major"
>>>
>>> - we can't change first.
>>> - exclusivity-ordering only matters in the face of enable/disable
>>> which we have solved now (?)
>>
>> I'm not sure where you get the conclusion we've solved this.
>> Today I can't say "lsm.enable=smack lsm.enable=apparmor", and
>> there's no mechanism to prevent that.
>>
>>> so, ordering can be totally arbitrary after "first" (but before some
>>> future "last"). We must not allow a token for "everything else" since
>>> that overlaps with enable/disable, so "everything else" stay implicit
>>> (I would argue a trailing implicit ordering).
>>
>> There's an assumption you're making that I'm not getting. Where does
>> this overlap between ordering and enable/disable come from?
> 
> Handling exclusivity means the non-active LSMs are disabled. We had
> been saying "the other majors are disabled", but the concept of major
> will become arbitrary. If instead we move to "first exclusive wins
> among the exclusives", we still have the "the others are disabled"
> case. So exclusivity begets disabling.
> 
>>> The one complication I see with ordering, then, is that if we change
>>> the exclusivity over time, we change what may be present on the
>>> system. For example, right now tomoyo is exclusive. Once we have
>>> blob-sharing, it doesn't need to be.
>>>
>>> so: lsm.order=tomoyo  after this series means
>>> "capability,tomoyo,yama,loadpin,integrity", but when tomoyo becomes
>>> non-exclusive, suddenly we get
>>> "capability,tomoyo,yama,loadpin,{selinux,smack,apparmor},integrity".
>>> (i.e. if selinux is disabled then move on to trying smack, then
>>> apparmor, etc.)
>>
>> We're missing a description of what happens at build time.
>> It's hard to see what you expect to happen if I want to build in
>> all the major modules and don't plan to use the boot command line
>> options.
>>
>>> I would argue that this is a design feature (LSMs aren't left behind),
>>> and order of enabled exclusive LSMs "wins" the choice for the
>>> exclusivity (instead of operating "by name" the way "security="
>>> works).
>>
>> I think I see more, but I'm guessing. At build time it looks like
>> you're dropping the specification on the "major" module. We can't
>> do that because I want to build kernels that run Smack by default
>> but include SELinux for when I'm feeling less evil than normal.
> 
> Do we need build time _ordering_, or can we just go with build time

depends on what you mean by build time ordering. Ordering like we
currently have based on link order, no. The ability to specify
the lsm order just like on the command line yes.

Distros are very much going to have a preferred order they want
LSMs to make supporting this more tractable when trying to deal
with issues.

> "first exclusive"? For the v1, I went with "first exclusive" from
> CONFIG_SECURITY_DEFAULT, and left the rest of the ordering up to the
> Makefile.
> 

First exclusive from CONFIG_SECURITY_DEFAULT is fine for now, I am
more interested in making sure we get it right for when exclusivity
goes away.

Re: [PATCH 16/18] LSM: Allow arbitrary LSM ordering

[John Johansen]

On 09/17/2018 11:14 AM, Kees Cook wrote:
> On Mon, Sep 17, 2018 at 10:13 AM, Casey Schaufler
> <casey@schaufler-ca.com> wrote:
>> TOMOYO uses the cred blob pointer. When the blob is shared TOMOYO
>> has to be allocated a pointer size chunk to store the pointer in.
>> Smack has the same behavior on file blobs.
> 
> Oh dang, yes, I got confused over secid and other "extreme" shared things.
> 
> So one change of my series would be to declare tomoyo as "exclusive" too.
> 
>> Today the distinction is based on how the module registers hooks.
>> Modules that use blobs (including TOMOYO) use security_module_enable()
>> and those that don't just use security_add_hooks(). The "pick one"
>> policy is enforced in security_module_enable(), which is why you can
>> have as many non-blob users as you like. You could easily have a
>> non-blob using module that was exclusive simply by using
>> security_module_enable().
> 
> True. With my removal of security_module_enable(), yes, it makes sense
> to mark all LSMs that were calling it before as exclusive, rather than
> focusing on whether they would be exclusive under the blob-sharing
> situation.
> 
>> Keep security=$lsm with the existing exclusive behavior.
>> Add lsm=$lsm1,...,$lsmN which requires a full list of modules
>>
>> If you want to be fancy (I don't!) you could add
>>
>> lsm.add=$lsm1,...,$lsmN which adds the modules to the stack
>> lsm.delete=$lsm1,...,$lsmN which deletes modules from the stack
> 
> We've got two issues: ordering and enablement. It's been strongly
> suggested that we should move away from per-LSM enable/disable flags
> (to which I agree). If ordering should be separate from enablement (to
> avoid the "booted kernel with new LSM built in, but my lsm="..." line
> didn't include it so it's disabled case), then I think we need to
> split the logic (otherwise we just reinvented "security=" with similar
> problems).
> 
> Should "lsm=" allow arbitrary ordering? (I think yes.)
> yes

> Should "lsm=" imply implicit enable/disable? (I think no: unlisted
> LSMs are implicitly auto-appended to the explicit list)
> 

maybe, adding $lsm to the list could possibly considered as enabling it,
but not having it there doesn't necessarily imply it isn't

> So then we could have "lsm.enable=..." and "lsm.disable=...".
> 
> If builtin list was:
> capability,yama,loadpin,integrity,{selinux,smack,tomoyo,apparmor}
> then:
> 
>     lsm.disable=loadpin lsm=smack
> 
> becomes
> 
>     capability,smack,yama,integrity
> 
> and
> 
>     CONFIG_SECURITY_LOADPIN_DEFAULT_ENABLED=n
>     selinux.enable=0 lsm.add=loadpin lsm.disable=smack,tomoyo lsm=integrity
> 
> becomes
> 
>     capability,integrity,yama,loadpin,apparmor
> 
> 
> If "lsm=" _does_ imply enablement, then how does it interact with
> per-LSM disabling? i.e. what does "apparmor.enabled=0
> lsm=yama,apparmor" mean? If it means "turn on apparmor" how do I turn
> on a CONFIG-default-off LSM without specifying all the other LSMs too?
> 

currently using

   security=apparmor apparmor=0

means apparmor is the one given the chance to register but it declines
which means you just get capabilities. And with

   # caveat not part of the current stacking patchset
   security=selinux,apparmor  apparmor=0

you end up with

   capability,selinux

However apparmor=1 does not imply apparmor is the available LSM

that is

   security=selinux  apparmor=1

gives you
   
   capability,selinux

if iirc selinux=X behaves the same way

However it is not clear to me whether this is the behavior that we
would want for $lsm.enabled, $lsm.disabled. It appears to be in
conflict with how yama, loadpin and IMA currently work.

[PATCH 17/18] LSM: Provide init debugging

[Kees Cook]

Booting with "lsm.debug" will report details on how LSM ordering
decisions are being made. Additionally changes tense of "Framework
initialized" to "... initializing" since it hadn't finished its
work yet.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 .../admin-guide/kernel-parameters.txt         |  2 ++
 security/security.c                           | 30 ++++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 6d6bb9481193..c3e44a27c86a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2274,6 +2274,8 @@
 	ltpc=		[NET]
 			Format: <io>,<irq>,<dma>
 
+	lsm.debug	[SECURITY] Enable LSM initialization debugging output.
+
 	machvec=	[IA-64] Force the use of a particular machine-vector
 			(machvec) in a generic kernel.
 			Example: machvec=hpzx1_swiotlb
diff --git a/security/security.c b/security/security.c
index f09a4bb3cb86..3b84b7eeb08c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -12,6 +12,8 @@
  *	(at your option) any later version.
  */
 
+#define pr_fmt(fmt) "LSM: " fmt
+
 #include <linux/bpf.h>
 #include <linux/capability.h>
 #include <linux/dcache.h>
@@ -46,6 +48,13 @@ static const char *bootparam_lsms;
 static struct lsm_info **possible_lsms __initdata;
 static struct lsm_info *exclusive __initdata;
 
+static bool debug __initdata;
+#define init_debug(...)						\
+	do {							\
+		if (debug)					\
+			pr_info(__VA_ARGS__);			\
+	} while (0)
+
 /* Mark an LSM's enabled flag, if it exists. */
 static void __init set_enabled(struct lsm_info *lsm, bool enabled)
 {
@@ -71,6 +80,7 @@ static void __init append_possible_lsm(struct lsm_info *lsm, const char *from)
 {
 	/* Ignore duplicate selections. */
 	if (possible_lsm(lsm)) {
+		init_debug("duplicate: %s\n", lsm->name);
 		return;
 	}
 
@@ -78,6 +88,7 @@ static void __init append_possible_lsm(struct lsm_info *lsm, const char *from)
 		return;
 
 	possible_lsms[last_lsm++] = lsm;
+	init_debug("%s possible: %s\n", from, lsm->name);
 }
 
 /* Default boot: populate possible LSMs list with builtin ordering. */
@@ -117,12 +128,18 @@ static void __init prepare_lsm_order_commandline(void)
 	next = sep;
 	/* Walk commandline list, looking for matching LSMs. */
 	while ((name = strsep(&next, ",")) != NULL) {
+		bool found = false;
+
 		for (lsm = __start_lsm_info; lsm < __end_lsm_info; lsm++) {
 			if (lsm->order == LSM_ORDER_MUTABLE &&
 			    !strcmp(lsm->name, name)) {
 				append_possible_lsm(lsm, "commandline");
+				found = true;
 			}
 		}
+
+		if (!found)
+			init_debug("ignoring: %s\n", name);
 	}
 	kfree(sep);
 
@@ -133,6 +150,7 @@ static void __init prepare_lsm_order_commandline(void)
 				continue;
 
 			set_enabled(lsm, false);
+			init_debug("disabled: %s\n", lsm->name);
 		}
 	}
 }
@@ -187,6 +205,7 @@ static void __init maybe_enable_lsm(struct lsm_info *lsm)
 	if (enabled) {
 		if (lsm->type == LSM_TYPE_EXCLUSIVE) {
 			exclusive = lsm;
+			init_debug("exclusive: %s\n", exclusive->name);
 		}
 		lsm->init();
 	}
@@ -211,12 +230,13 @@ int __init security_init(void)
 	int i;
 	struct hlist_head *list = (struct hlist_head *) &security_hook_heads;
 
+	pr_info("Security Framework initializing\n");
+
 	for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct hlist_head);
 	     i++)
 		INIT_HLIST_HEAD(&list[i]);
 	possible_lsms = kcalloc(LSM_COUNT + 1, sizeof(*possible_lsms),
 				GFP_KERNEL);
-	pr_info("Security Framework initialized\n");
 
 	prepare_lsm_order();
 	lsm_init();
@@ -233,6 +253,14 @@ static int __init choose_lsm(char *str)
 }
 __setup("security=", choose_lsm);
 
+/* Enable LSM order debugging. */
+static int __init enable_debug(char *str)
+{
+	debug = true;
+	return 1;
+}
+__setup("lsm.debug", enable_debug);
+
 static bool match_last_lsm(const char *list, const char *lsm)
 {
 	const char *last;
-- 
2.17.1

[PATCH 18/18] LSM: Don't ignore initialization failures

[Kees Cook]

LSM initialization failures have traditionally been ignored. We should
at least WARN when something goes wrong.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 security/security.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/security/security.c b/security/security.c
index 3b84b7eeb08c..a7796e522f72 100644
--- a/security/security.c
+++ b/security/security.c
@@ -203,11 +203,15 @@ static void __init maybe_enable_lsm(struct lsm_info *lsm)
 
 	/* If selected, initialize the LSM. */
 	if (enabled) {
+		int ret;
+
 		if (lsm->type == LSM_TYPE_EXCLUSIVE) {
 			exclusive = lsm;
 			init_debug("exclusive: %s\n", exclusive->name);
 		}
-		lsm->init();
+
+		ret = lsm->init();
+		WARN(ret, "%s failed to initialize: %d\n", lsm->name, ret);
 	}
 }
 
-- 
2.17.1