[PATCH RFC v2 0/3] security: Add ModAutoRestrict LSM

[PATCH RFC v2 0/3] security: Add ModAutoRestrict LSM

[Djalal Harouni]

Hi List,

This is RFC v2 of the Module auto-loading restriction feature. The
module has been renamed ModAutoRestrict LSM.

This RFC is a work in progress update.

There are still minor things to fix which are listed in the TODO
section. Also I used Tetsuo approach of stacking task->security, since
now that the task_security_alloc() hook is in linux-security/next I took
advantage of it and switched to use that hook and applied Tetsuo
task->security per LSM blob on top of it. I can also switch to Casey
approach. However, since these are internal implementation details which
are not exposed, I do not think that this is a big issue. Thanks for the
work now it is clear that we are able to easily stack new LSMs.

Patches are against linux-security/next HEAD 622f6e3265707eb

==============

ModAutoRestrict is a Linux Security Module that applies restrictions on
automatic module loading operations. This is selectable at build-time
with CONFIG_SECURITY_MODAUTORESTRICT, and can be controlled at run-time
through sysctls in /proc/sys/kernel/modautorestrict/autoload or as a
per-process setting via a prctl() interface.

A userspace request to use a kernel feature that is implemented by modules
that are not loaded may trigger the module auto-load feature to load
these modules in order to satisfy userspace. However as today's Linux use
cases cover embedded systems to containers where applications are running
in their own separate environments, reducing or preventing operations
that may affect external environments is an important constraint.
Therefore, we need a way to control if automatic module loading is
allowed or which applications are allowed to trigger the module
auto-load feature.

The ModAutoRestrict LSM allows system administrators or sandbox
mechanisms to control the module auto-load feature and prevent loading
unneeded modules or abuse the interface.

The settings can be applied globally using a sysctl interface which
completes the core kernel interface "modules_disable".

The feature is also available as a prctl() interface. This allows to
apply restrictions when sandboxing processes. On embedded Linux systems,
or containers where only some containers/processes should have the
right privileges to load modules, this allows to restrict those
processes from inserting modules. Only privileged processes can be
allowed to perform so. A more restrictive access can be applied where
the module autoload feature is completely disabled.
In this schema the access rules are per-process and inherited by
children created by fork(2) and clone(2), and preserved across execve(2).

Interface:

*) The per-process prctl() settings are:

 prctl(PR_MOD_AUTO_RESTRICT_OPTS, PR_SET_MOD_AUTO_RESTRICT, value, 0, 0)

 Where value means:

 0 - Classic module auto-load permissions, nothing changes.

 1 - The current process must have CAP_SYS_MODULE to be able to
     auto-load modules. CAP_NET_ADMIN should allow to auto-load
     modules with a 'netdev-%s' alias.

 2 - Current process can not auto-load modules. Once set, this prctl
     value can not be changed.

 The per-process value may only be increased, never decreased, thus ensuring
 that once applied, processes can never relaxe their setting.

*) The global sysctl setting can be set by writting an integer value to
   '/proc/sys/kernel/modautorestrict/autoload'

 The valid values are:

 0 - Classic module auto-load permissions, nothing changes.

 1 - Processes must have CAP_SYS_MODULE to be able to auto-load modules.
     CAP_NET_ADMIN should allow to auto-load modules with a 'netdev-%s'
     alias.

 2 - Processes can not auto-load modules. Once set, this sysctl value
     can not be changed.

*) Access rules:
   First the prctl() settings are checked, if the access is not denied
   then the global sysctl settings are checked.

The original idea and inspiration is from grsecurity 'GRKERNSEC_MODHARDEN'.


The sample code here can be used to test the feature:
https://gist.githubusercontent.com/tixxdz/39a583358f04d40b4d3e5571f95c075b/raw/7fb416285412891e2637fba149da930fe0898356/modautorestrict_test.c

# TODO list:
*) Confirm the struct task_struct->security stacking mechanism. 
*) Add a logging mechanism.
*) Remove the use of security_kernel_read_file hook. Use only
   security_kernel_module_request and make sure that we cover all cases.
*) Convert documentation to .rst


# Changes since v1:
*) Renamed module to ModAutoRestrict
*) Improved documentation to explicity refer to module autoloading.
*) Switched to use the new task_security_alloc() hook.
*) Switched from rhash tables to use task->security since it is in
   linux-security/next branch now.
*) Check all parameters passed to prctl() syscall.
*) Many other bug fixes and documentation improvements.

All Kees Cook comments handled, except for the removing of
security_kernel_read_file which I will do in next iteration when
I make sure that I did not miss something. Thank you!


Tetsuo Handa (1):
        LSM: Allow per LSM module per "struct task_struct" blob

Djalal Harouni (2):
        security: add the ModAutoRestrict Linux Security Module
        Documentation: add ModAutoRestrict LSM documentation


 Documentation/security/00-INDEX            |   2 +
 Documentation/security/ModAutoRestrict.txt |  77 ++++++
 MAINTAINERS                                |   7 +
 include/linux/lsm_hooks.h                  |  41 +++-
 include/uapi/linux/prctl.h                 |   5 +
 security/Kconfig                           |   1 +
 security/Makefile                          |  16 +-
 security/modautorestrict/Kconfig           |  15 ++
 security/modautorestrict/Makefile          |   3 +
 security/modautorestrict/modauto_lsm.c     | 372 +++++++++++++++++++++++++++++
 security/security.c                        |  38 ++-
 11 files changed, 568 insertions(+), 9 deletions(-)

[PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Djalal Harouni]

From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Since several modules are planning to use per "struct task_struct" blob,
we need a layer for isolating it. Therefore, this patch introduces per LSM
module per "struct task_struct" blob.

It would be possible to remember location in security_hook_heads.task_alloc
list and undo up to the corresponding security_hook_heads.task_free list
when task_alloc hook failed. But this patch calls all task_free hooks
without checking whether the corresponding task_alloc hook was called
since most modules should be able to handle this correctly.

How to calculate amount of needed bytes and allocate memory for initial
task is a chicken-or-egg syndrome. We need to know how many bytes needs
to be allocated for initial task's security blobs before security_init()
is called. But security_reserve_task_blob_index() which calculates amount
of needed bytes is called from security_init(). We will need to split
module registration into three steps. The first step is call
security_reserve_task_blob_index() on all modules which should be
activated. The second step is allocate memory for the initial task's
security blob. The third step is actually activate all modules which
should be activated.

Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 include/linux/lsm_hooks.h | 36 +++++++++++++++++++++++++++++++++++-
 security/security.c       | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 2 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 080f34e..ca19cdb 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -27,6 +27,7 @@
 #include <linux/security.h>
 #include <linux/init.h>
 #include <linux/rculist.h>
+#include <linux/sched.h> /* "struct task_struct" */
 
 /**
  * Security hooks for program execution operations.
@@ -536,7 +537,10 @@
  * @task_alloc:
  *	@task task being allocated.
  *	@clone_flags contains the flags indicating what should be shared.
- *	Handle allocation of task-related resources.
+ *	Handle allocation of task-related resources. Note that task_free is
+ *	called even if task_alloc failed. This means that all task_free users
+ *	have to be prepared for task_free being called without corresponding
+ *	task_alloc call.
  *	Returns a zero on success, negative values on failure.
  * @task_free:
  *	@task task about to be freed.
@@ -1947,4 +1951,34 @@ void __init loadpin_add_hooks(void);
 static inline void loadpin_add_hooks(void) { };
 #endif
 
+/*
+ * Per "struct task_struct" security blob is managed using index numbers.
+ *
+ * Any user who wants to use per "struct task_struct" security blob reserves an
+ * index number using security_reserve_task_blob_index() before calling
+ * security_add_hooks().
+ *
+ * security_reserve_task_blob_index() returns an index number which has to be
+ * passed to task_security() by that user when fetching security blob of given
+ * "struct task_struct" for that user.
+ *
+ * Security blob for newly allocated "struct task_struct" is allocated and
+ * initialized with 0 inside security_task_alloc(), before calling each user's
+ * task_alloc hook. Be careful with task_free hook, for security_task_free()
+ * can be called when calling each user's task_alloc hook failed.
+ *
+ * Note that security_reserve_task_blob_index() uses "u16". It is not a good
+ * idea to directly reserve large amount. Sum of reserved amount by all users
+ * should be less than PAGE_SIZE bytes, for per "struct task_struct" security
+ * blob is allocated for each "struct task_struct" using kcalloc().
+ */
+extern u16 __init security_reserve_task_blob_index(const u16 size);
+static inline void *task_security(const struct task_struct *task,
+				  const u16 index)
+{
+	unsigned long *p = task->security;
+
+	return p + index;
+}
+
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/security.c b/security/security.c
index 549bddc..4dc6bca 100644
--- a/security/security.c
+++ b/security/security.c
@@ -32,6 +32,7 @@
 /* Maximum number of letters for an LSM name string */
 #define SECURITY_NAME_MAX	10
 
+static u16 lsm_max_per_task_blob_index __ro_after_init;
 struct security_hook_heads security_hook_heads __lsm_ro_after_init;
 char *lsm_names;
 /* Boot-time LSM user choice */
@@ -75,6 +76,15 @@ int __init security_init(void)
 	 */
 	do_security_initcalls();
 
+	/*
+	 * Allocate per "struct task_struct" blob for initial task.
+	 * Initialization is done later by each module which needs it.
+	 */
+	if (lsm_max_per_task_blob_index)
+		current->security = kcalloc(lsm_max_per_task_blob_index,
+					    sizeof(unsigned long),
+					    GFP_KERNEL | __GFP_NOFAIL);
+
 	return 0;
 }
 
@@ -86,6 +96,15 @@ static int __init choose_lsm(char *str)
 }
 __setup("security=", choose_lsm);
 
+u16 __init security_reserve_task_blob_index(const u16 size)
+{
+	const u16 index = lsm_max_per_task_blob_index;
+	u16 requested = DIV_ROUND_UP(size, sizeof(unsigned long));
+
+	lsm_max_per_task_blob_index += requested;
+	return index;
+}
+
 static int lsm_append(char *new, char **result)
 {
 	char *cp;
@@ -939,12 +958,28 @@ int security_task_create(unsigned long clone_flags)
 
 int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
 {
-	return call_int_hook(task_alloc, 0, task, clone_flags);
+	int rc;
+	const unsigned long index = lsm_max_per_task_blob_index;
+
+	if (index) {
+		task->security = kcalloc(index, sizeof(unsigned long),
+					 GFP_KERNEL);
+		if (unlikely(!task->security))
+			return -ENOMEM;
+	}
+	rc = call_int_hook(task_alloc, 0, task, clone_flags);
+	if (unlikely(rc))
+		security_task_free(task);
+	return rc;
 }
 
 void security_task_free(struct task_struct *task)
 {
 	call_void_hook(task_free, task);
+	if (lsm_max_per_task_blob_index) {
+		kfree(task->security);
+		task->security = NULL;
+	}
 }
 
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
-- 
2.10.2

[PATCH RFC v2 2/3] security: add the ModAutoRestrict Linux Security Module

[Djalal Harouni]

This adds the ModAutoRestrict Linux Security Module. The new module
is a stackable LSM that has been tested with Yama and SELinux, all
the three modules running.

The module applies restrictions on automatic module loading operations.
If it is selected, every request to use a kernel feature that is implemented
by modules that are not loaded will first have to satisfy the access rules
of ModAutoRestrict LSM. If the access is authorized then the module will
be automatically loaded. Furthermore, this allows system administrators
or sandbox mechanisms to prevent loading unneeded modules or abuse the
interface.

The settings can be applied globally using a sysctl interface which
completes the core kernel interface "modules_disable" that works only
on two modes: allow or deny, ignoring that module loading can be either
an explicit operation or an implicit one via automatic loading. The
CAP_SYS_MODULE capability can be used to restrict explicit operations,
however implicit module loading is in general not subject to permission
checks which allows unprivileged to insert modules.

Using the new ModAutoRestrict settings allow to control if implicit
operations are allowed or denied.
This behaviour was inspired from grsecurity's GRKERNSEC_MODHARDEN option.

The feature is also available as a prctl() interface. This allows to
apply restrictions when sandboxing processes. On embedded Linux systems,
or containers where only some containers/processes should have the
right privileges to load modules, this allows to restrict those
processes from inserting modules through the autoload feature, only
privileged processes can be allowed to perform so. A more restrictive
access can be applied where the module autoload feature is completely
disabled.
In this schema the access rules are per-process and inherited by
children created by fork(2) and clone(2), and preserved across execve(2).

Current interface:

*) The per-process prctl() settings are:

 prctl(PR_MOD_AUTO_RESTRICT_OPTS, PR_SET_MOD_AUTO_RESTRICT, value, 0, 0)

 Where value means:

 0 - Classic module auto-load permissions, nothing changes.

 1 - The current process must have CAP_SYS_MODULE to be able to
     auto-load modules. CAP_NET_ADMIN should allow to auto-load
     modules with a 'netdev-%s' alias.

 2 - Current process can not auto-load modules. Once set, this prctl
     value can not be changed.

 The per-process value may only be increased, never decreased, thus ensuring
 that once applied, processes can never relaxe their setting.

*) The global sysctl setting can be set by writting an integer value to
   '/proc/sys/kernel/modautorestrict/autoload'

 The valid values are:

 0 - Classic module auto-load permissions, nothing changes.

 1 - Processes must have CAP_SYS_MODULE to be able to auto-load modules.
     CAP_NET_ADMIN should allow to auto-load modules with a 'netdev-%s'
     alias.

 2 - Processes can not auto-load modules. Once set, this sysctl value
     can not be changed.

*) Access rules:
   First the prctl() settings are checked, if the access is not denied
   then the global sysctl settings are checked.

Cc: Andy Lutomirski <luto@kernel.org>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
---
 MAINTAINERS                            |   7 +
 include/linux/lsm_hooks.h              |   5 +
 include/uapi/linux/prctl.h             |   5 +
 security/Kconfig                       |   1 +
 security/Makefile                      |  16 +-
 security/modautorestrict/Kconfig       |  15 ++
 security/modautorestrict/Makefile      |   3 +
 security/modautorestrict/modauto_lsm.c | 372 +++++++++++++++++++++++++++++++++
 security/security.c                    |   1 +
 9 files changed, 418 insertions(+), 7 deletions(-)
 create mode 100644 security/modautorestrict/Kconfig
 create mode 100644 security/modautorestrict/Makefile
 create mode 100644 security/modautorestrict/modauto_lsm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c45c02b..38d17cd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11326,6 +11326,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git yama/tip
 S:	Supported
 F:	security/yama/
 
+MODAUTORESTRICT SECURITY MODULE
+M:	Djalal Harouni <tixxdz@gmail.com>
+L:	kernel-hardening@lists.openwall.com
+L:	linux-security-module@vger.kernel.org
+S:	Supported
+F:	security/modautorestrict/
+
 SENSABLE PHANTOM
 M:	Jiri Slaby <jirislaby@gmail.com>
 S:	Maintained
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ca19cdb..679800c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1950,6 +1950,11 @@ void __init loadpin_add_hooks(void);
 #else
 static inline void loadpin_add_hooks(void) { };
 #endif
+#ifdef CONFIG_SECURITY_MODAUTORESTRICT
+extern void modautorestrict_init(void);
+#else
+static inline void __init modautorestrict_init(void) { }
+#endif
 
 /*
  * Per "struct task_struct" security blob is managed using index numbers.
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index a8d0759..e561ca3 100644
--- a/include/uapi/linux/prctl.h
+++ b/include/uapi/linux/prctl.h
@@ -197,4 +197,9 @@ struct prctl_mm_map {
 # define PR_CAP_AMBIENT_LOWER		3
 # define PR_CAP_AMBIENT_CLEAR_ALL	4
 
+/* Control ModAutoRestrict LSM options */
+#define PR_MOD_AUTO_RESTRICT_OPTS	48
+# define PR_SET_MOD_AUTO_RESTRICT	1
+# define PR_GET_MOD_AUTO_RESTRICT	2
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/security/Kconfig b/security/Kconfig
index 3ff1bf9..1e181c3 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -204,6 +204,7 @@ source security/tomoyo/Kconfig
 source security/apparmor/Kconfig
 source security/loadpin/Kconfig
 source security/yama/Kconfig
+source security/modautorestrict/Kconfig
 
 source security/integrity/Kconfig
 
diff --git a/security/Makefile b/security/Makefile
index f2d71cd..4c120f7 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -2,13 +2,14 @@
 # Makefile for the kernel security code
 #
 
-obj-$(CONFIG_KEYS)			+= keys/
-subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
-subdir-$(CONFIG_SECURITY_SMACK)		+= smack
-subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
-subdir-$(CONFIG_SECURITY_APPARMOR)	+= apparmor
-subdir-$(CONFIG_SECURITY_YAMA)		+= yama
-subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
+obj-$(CONFIG_KEYS)				+= keys/
+subdir-$(CONFIG_SECURITY_SELINUX)		+= selinux
+subdir-$(CONFIG_SECURITY_SMACK)			+= smack
+subdir-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo
+subdir-$(CONFIG_SECURITY_APPARMOR)		+= apparmor
+subdir-$(CONFIG_SECURITY_YAMA)			+= yama
+subdir-$(CONFIG_SECURITY_LOADPIN)		+= loadpin
+subdir-$(CONFIG_SECURITY_MODAUTORESTRICT)	+= modautorestrict
 
 # always enable default capabilities
 obj-y					+= commoncap.o
@@ -24,6 +25,7 @@ obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/
 obj-$(CONFIG_SECURITY_APPARMOR)		+= apparmor/
 obj-$(CONFIG_SECURITY_YAMA)		+= yama/
 obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
+obj-$(CONFIG_SECURITY_MODAUTORESTRICT)	+= modautorestrict/
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
 
 # Object integrity file lists
diff --git a/security/modautorestrict/Kconfig b/security/modautorestrict/Kconfig
new file mode 100644
index 0000000..9678106
--- /dev/null
+++ b/security/modautorestrict/Kconfig
@@ -0,0 +1,15 @@
+config SECURITY_MODAUTORESTRICT
+	bool "Automatic Module Loading Restriction"
+	depends on SECURITY
+	default n
+	help
+	  This selects ModAutoRestrict Linux Security Module which applies
+          restrictions on automatic module loading operations. If this
+          option is selected, a request to use a kernel feature that is
+          implemented by an unloaded module will first have to satisfy the
+          access rules of MODAUTORESTRICT. Furthermore, this allows system
+          administrators or sandbox mechanisms to prevent loading unneeded
+          modules.
+          Further information can be found in Documentation/security/ModAutoRestrict.txt.
+
+	  If you are unsure how to answer this question, answer N.
diff --git a/security/modautorestrict/Makefile b/security/modautorestrict/Makefile
new file mode 100644
index 0000000..a0656a5
--- /dev/null
+++ b/security/modautorestrict/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_SECURITY_MODAUTORESTRICT) := modautorestrict.o
+
+modautorestrict-y := modauto_lsm.o
diff --git a/security/modautorestrict/modauto_lsm.c b/security/modautorestrict/modauto_lsm.c
new file mode 100644
index 0000000..b3a83b8e
--- /dev/null
+++ b/security/modautorestrict/modauto_lsm.c
@@ -0,0 +1,372 @@
+/*
+ * ModAutoRestrict Linux Security Module
+ *
+ * Author: Djalal Harouni
+ *
+ * Copyright (C) 2017 Djalal Harouni
+ * Copyright (C) 2017 Endocode AG.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2, as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/errno.h>
+#include <linux/lsm_hooks.h>
+#include <linux/prctl.h>
+#include <linux/refcount.h>
+#include <linux/types.h>
+#include <linux/sched/mm.h>
+#include <linux/sched/task.h>
+#include <linux/sysctl.h>
+
+enum {
+	MOD_AUTOLOAD_ALLOWED	= 0,
+	MOD_AUTOLOAD_PRIVILEGED	= 1,
+	MOD_AUTOLOAD_DENIED	= 2,
+};
+
+struct modautoload_task {
+	bool usage;
+	u8 flags;
+};
+
+static int autoload_restrict;
+
+static int zero;
+static int max_autoload_restrict = MOD_AUTOLOAD_DENIED;
+
+/* Index number of per "struct task_struct" blob for ModAutoRestrict. */
+u16 modautorestrict_task_security_index __ro_after_init;
+
+static inline int modautoload_task_set_flag(struct modautoload_task *modtask,
+					    unsigned long value)
+{
+	int ret = 0;
+
+	if (value > MOD_AUTOLOAD_DENIED)
+		ret = -EINVAL;
+	else if (modtask->flags > value)
+		ret = -EPERM;
+	else if (modtask->flags < value)
+		modtask->flags = value;
+
+	return ret;
+}
+
+static inline struct modautoload_task *modautoload_task_security(struct task_struct *tsk)
+{
+	struct modautoload_task *modtask;
+
+	modtask = task_security(tsk, modautorestrict_task_security_index);
+	if (modtask->usage)
+		return modtask;
+
+	return NULL;
+}
+
+static inline struct modautoload_task *init_modautoload_task(struct task_struct *tsk,
+							     unsigned long flags)
+{
+	struct modautoload_task *modtask;
+
+	modtask = task_security(tsk, modautorestrict_task_security_index);
+
+	modtask->flags = (u8)flags;
+	modtask->usage = true;
+
+	return modtask;
+}
+
+static inline void clear_modautoload_task(struct task_struct *tsk)
+{
+	struct modautoload_task *modtask;
+
+	modtask = modautoload_task_security(tsk);
+	if (modtask) {
+		modtask->usage = false;
+		modtask->flags = MOD_AUTOLOAD_ALLOWED;
+	}
+}
+
+/*
+ * Return 0 if CAP_SYS_MODULE or if CAP_NET_ADMIN and the module is
+ * a netdev-%s module. Otherwise -EPERM is returned.
+ */
+static int modautoload_privileged_access(const char *name)
+{
+	int ret = -EPERM;
+
+	if (capable(CAP_SYS_MODULE))
+		ret = 0;
+	else if (name && strstr(name, "netdev-") && capable(CAP_NET_ADMIN))
+		ret = 0;
+
+	return ret;
+}
+
+static int modautoload_sysctl_perm(unsigned long op, const char *name)
+{
+	int ret = -EINVAL;
+	struct mm_struct *mm = NULL;
+
+	if (op != PR_GET_MOD_AUTO_RESTRICT)
+		return ret;
+
+	switch (autoload_restrict) {
+	case MOD_AUTOLOAD_ALLOWED:
+		ret = 0;
+		break;
+	case MOD_AUTOLOAD_PRIVILEGED:
+		/*
+		 * Are we allowed to sleep here ?
+		 * Also improve this check here
+		 */
+		ret = -EPERM;
+		mm = get_task_mm(current);
+		if (mm) {
+			ret = modautoload_privileged_access(name);
+			mmput(mm);
+		}
+		break;
+	case MOD_AUTOLOAD_DENIED:
+		ret = -EPERM;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int modautoload_task_perm(struct modautoload_task *mtask,
+				 char *kmod_name)
+{
+	int ret = -EINVAL;
+
+	switch (mtask->flags) {
+	case MOD_AUTOLOAD_ALLOWED:
+		ret = 0;
+		break;
+	case MOD_AUTOLOAD_PRIVILEGED:
+		ret = modautoload_privileged_access(kmod_name);
+		break;
+	case MOD_AUTOLOAD_DENIED:
+		ret = -EPERM;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+/* Set the given option in a modautorestrict task */
+static int modautoload_set_op_value(struct task_struct *tsk,
+				    unsigned long value)
+{
+	int ret = -EINVAL;
+	struct modautoload_task *modtask;
+
+	if (value > MOD_AUTOLOAD_DENIED)
+		return ret;
+
+	modtask = modautoload_task_security(tsk);
+	if (!modtask) {
+		modtask = init_modautoload_task(tsk, value);
+		return 0;
+	}
+
+	return modautoload_task_set_flag(modtask, value);
+}
+
+static int modautoload_get_op_value(struct task_struct *tsk)
+{
+	struct modautoload_task *modtask;
+
+	modtask = modautoload_task_security(tsk);
+	if (!modtask)
+		return -EINVAL;
+
+	return modtask->flags;
+}
+
+/* Copy modautorestrict context from parent to child */
+int modautoload_task_alloc(struct task_struct *tsk, unsigned long clone_flags)
+{
+	struct modautoload_task *modparent;
+
+	modparent = modautoload_task_security(current);
+
+	/* Parent has a modautorestrict context */
+	if (modparent)
+		init_modautoload_task(tsk, modparent->flags);
+
+	return 0;
+}
+
+/*
+ * Return 0 on success, -error on error.  -ENOSYS is returned when modautorestrict
+ * does not handle the given option, or -EINVAL if the passed arguments are not
+ * valid.
+ */
+int modautoload_task_prctl(int option, unsigned long arg2, unsigned long arg3,
+			   unsigned long arg4, unsigned long arg5)
+{
+	int ret = -EINVAL;
+	struct task_struct *myself = current;
+
+	if (option != PR_MOD_AUTO_RESTRICT_OPTS)
+		return -ENOSYS;
+
+	get_task_struct(myself);
+
+	switch (arg2) {
+	case PR_SET_MOD_AUTO_RESTRICT:
+		if (arg4 || arg5)
+			goto out;
+
+		ret = modautoload_set_op_value(myself, arg3);
+		break;
+	case PR_GET_MOD_AUTO_RESTRICT:
+		if (arg3 || arg4 || arg5)
+			goto out;
+
+		ret = modautoload_get_op_value(myself);
+		break;
+	default:
+		break;
+	}
+
+out:
+	put_task_struct(myself);
+	return ret;
+}
+
+void modautoload_task_free(struct task_struct *tsk)
+{
+	clear_modautoload_task(tsk);
+}
+
+/*
+ * TODO:
+ * if this is covered entirely by CAP_SYS_MODULE then we should removed it.
+ */
+static int modautoload_kernel_module_file(struct file *file)
+{
+	int ret = 0;
+	struct modautoload_task *modtask;
+	struct task_struct *myself = current;
+
+	/* First check if the task allows that */
+	modtask = modautoload_task_security(myself);
+	if (modtask) {
+		ret = modautoload_task_perm(modtask, NULL);
+		if (ret < 0)
+			return ret;
+	}
+
+	return modautoload_sysctl_perm(PR_GET_MOD_AUTO_RESTRICT, NULL);
+}
+
+static int modautoload_kernel_module_request(char *kmod_name)
+{
+	int ret = 0;
+	struct modautoload_task *modtask;
+	struct task_struct *myself = current;
+
+	/* First check if the task allows that */
+	modtask = modautoload_task_security(myself);
+	if (modtask) {
+		ret = modautoload_task_perm(modtask, kmod_name);
+		if (ret < 0)
+			return ret;
+	}
+
+	return modautoload_sysctl_perm(PR_GET_MOD_AUTO_RESTRICT, kmod_name);
+}
+
+/*
+ * TODO:
+ * if this is covered entirely by CAP_SYS_MODULE then we should removed it.
+ */
+static int modautoload_kernel_read_file(struct file *file,
+					enum kernel_read_file_id id)
+{
+	int ret = 0;
+
+	switch (id) {
+	case READING_MODULE:
+		ret = modautoload_kernel_module_file(file);
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static struct security_hook_list modautoload_hooks[] = {
+	LSM_HOOK_INIT(kernel_module_request, modautoload_kernel_module_request),
+	LSM_HOOK_INIT(kernel_read_file, modautoload_kernel_read_file),
+	LSM_HOOK_INIT(task_alloc, modautoload_task_alloc),
+	LSM_HOOK_INIT(task_prctl, modautoload_task_prctl),
+	LSM_HOOK_INIT(task_free, modautoload_task_free),
+};
+
+#ifdef CONFIG_SYSCTL
+static int modautoload_dointvec_minmax(struct ctl_table *table, int write,
+				       void __user *buffer, size_t *lenp,
+				       loff_t *ppos)
+{
+	struct ctl_table table_copy;
+
+	if (write && !capable(CAP_SYS_MODULE))
+		return -EPERM;
+
+	table_copy = *table;
+	if (*(int *)table_copy.data == *(int *)table_copy.extra2)
+		table_copy.extra1 = table_copy.extra2;
+
+	return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
+}
+
+struct ctl_path modautoload_sysctl_path[] = {
+	{ .procname = "kernel", },
+	{ .procname = "modautorestrict", },
+	{ }
+};
+
+static struct ctl_table modautoload_sysctl_table[] = {
+	{
+		.procname       = "autoload",
+		.data           = &autoload_restrict,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = modautoload_dointvec_minmax,
+		.extra1         = &zero,
+		.extra2         = &max_autoload_restrict,
+	},
+	{ }
+};
+
+static void __init modautoload_init_sysctl(void)
+{
+	if (!register_sysctl_paths(modautoload_sysctl_path, modautoload_sysctl_table))
+		panic("modautorestrict: sysctl registration failed.\n");
+}
+#else
+static inline void modautoload_init_sysctl(void) { }
+#endif /* CONFIG_SYSCTL */
+
+void __init modautorestrict_init(void)
+{
+	modautorestrict_task_security_index =
+		security_reserve_task_blob_index(sizeof(struct modautoload_task));
+	security_add_hooks(modautoload_hooks,
+			   ARRAY_SIZE(modautoload_hooks), "modautorestrict");
+
+	modautoload_init_sysctl();
+	pr_info("ModAutoRestrict LSM:  Initialized\n");
+}
diff --git a/security/security.c b/security/security.c
index 4dc6bca..d8852fe 100644
--- a/security/security.c
+++ b/security/security.c
@@ -70,6 +70,7 @@ int __init security_init(void)
 	capability_add_hooks();
 	yama_add_hooks();
 	loadpin_add_hooks();
+	modautorestrict_init();
 
 	/*
 	 * Load all the remaining security modules.
-- 
2.10.2

[PATCH RFC v2 3/3] Documentation: add ModAutoRestrict LSM documentation

[Djalal Harouni]

Cc: Andy Lutomirski <luto@kernel.org>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
---
 Documentation/security/00-INDEX            |  2 +
 Documentation/security/ModAutoRestrict.txt | 77 ++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)
 create mode 100644 Documentation/security/ModAutoRestrict.txt

diff --git a/Documentation/security/00-INDEX b/Documentation/security/00-INDEX
index 45c82fd..35dbdf0 100644
--- a/Documentation/security/00-INDEX
+++ b/Documentation/security/00-INDEX
@@ -24,3 +24,5 @@ tomoyo.txt
 	- documentation on the TOMOYO Linux Security Module.
 IMA-templates.txt
 	- documentation on the template management mechanism for IMA.
+ModAutoRestrict.txt
+        - documentation on the ModAutoRestrict Linux Security Module.
diff --git a/Documentation/security/ModAutoRestrict.txt b/Documentation/security/ModAutoRestrict.txt
new file mode 100644
index 0000000..47acae8
--- /dev/null
+++ b/Documentation/security/ModAutoRestrict.txt
@@ -0,0 +1,77 @@
+ModAutoRestrict is a Linux Security Module that applies restrictions on
+automatic module loading operations. This is selectable at build-time
+with CONFIG_SECURITY_MODAUTORESTRICT, and can be controlled at run-time
+through sysctls in /proc/sys/kernel/modautorestrict/autoload or as a
+per-process setting via a prctl() interface.
+
+===========================================
+
+A userspace request to use a kernel feature that is implemented by modules
+that are not loaded may trigger the module auto-load feature to load
+these modules in order to satisfy userspace. However as today's Linux use
+cases cover embedded systems to containers where applications are running
+in their own separate environments, reducing or preventing operations
+that may affect external environments is an important constraint.
+Therefore, we need a way to control if automatic module loading is
+allowed or which applications are allowed to trigger the module
+auto-load feature.
+
+The ModAutoRestrict LSM allows system administrators or sandbox
+mechanisms to control the module auto-load feature and prevent loading
+unneeded modules or abuse the interface.
+
+The settings can be applied globally using a sysctl interface which
+completes the core kernel interface "modules_disable".
+
+The feature is also available as a prctl() interface. This allows to
+apply restrictions when sandboxing processes. On embedded Linux systems,
+or containers where only some containers/processes should have the
+right privileges to load modules, this allows to restrict those
+processes from inserting modules. Only privileged processes can be
+allowed to perform so. A more restrictive access can be applied where
+the module autoload feature is completely disabled.
+In this schema the access rules are per-process and inherited by
+children created by fork(2) and clone(2), and preserved across execve(2).
+
+Interface:
+
+*) The per-process prctl() settings are:
+
+ prctl(PR_MOD_AUTO_RESTRICT_OPTS, PR_SET_MOD_AUTO_RESTRICT, value, 0, 0)
+
+ Where value means:
+
+ 0 - Classic module auto-load permissions, nothing changes.
+
+ 1 - The current process must have CAP_SYS_MODULE to be able to
+     auto-load modules. CAP_NET_ADMIN should allow to auto-load
+     modules with a 'netdev-%s' alias.
+
+ 2 - Current process can not auto-load modules. Once set, this prctl
+     value can not be changed.
+
+ The per-process value may only be increased, never decreased, thus ensuring
+ that once applied, processes can never relaxe their setting.
+
+*) The global sysctl setting can be set by writting an integer value to
+   '/proc/sys/kernel/modautorestrict/autoload'
+
+ The valid values are:
+
+ 0 - Classic module auto-load permissions, nothing changes.
+
+ 1 - Processes must have CAP_SYS_MODULE to be able to auto-load modules.
+     CAP_NET_ADMIN should allow to auto-load modules with a 'netdev-%s'
+     alias.
+
+ 2 - Processes can not auto-load modules. Once set, this sysctl value
+     can not be changed.
+
+*) Access rules:
+   First the prctl() settings are checked, if the access is not denied
+   then the global sysctl settings are checked.
+
+
+The original idea and inspiration is from grsecurity 'GRKERNSEC_MODHARDEN'.
+
+==========================================================================
-- 
2.10.2

Re: [PATCH RFC v2 2/3] security: add the ModAutoRestrict Linux Security Module

[Casey Schaufler]

On 4/9/2017 3:42 AM, Djalal Harouni wrote:
> This adds the ModAutoRestrict Linux Security Module. The new module
> is a stackable LSM that has been tested with Yama and SELinux, all
> the three modules running.
>
> The module applies restrictions on automatic module loading operations.
> If it is selected, every request to use a kernel feature that is implemented
> by modules that are not loaded will first have to satisfy the access rules
> of ModAutoRestrict LSM. If the access is authorized then the module will
> be automatically loaded. Furthermore, this allows system administrators
> or sandbox mechanisms to prevent loading unneeded modules or abuse the
> interface.
>
> The settings can be applied globally using a sysctl interface which
> completes the core kernel interface "modules_disable" that works only
> on two modes: allow or deny, ignoring that module loading can be either
> an explicit operation or an implicit one via automatic loading. The
> CAP_SYS_MODULE capability can be used to restrict explicit operations,
> however implicit module loading is in general not subject to permission
> checks which allows unprivileged to insert modules.
>
> Using the new ModAutoRestrict settings allow to control if implicit
> operations are allowed or denied.
> This behaviour was inspired from grsecurity's GRKERNSEC_MODHARDEN option.
>
> The feature is also available as a prctl() interface. This allows to
> apply restrictions when sandboxing processes. On embedded Linux systems,
> or containers where only some containers/processes should have the
> right privileges to load modules, this allows to restrict those
> processes from inserting modules through the autoload feature, only
> privileged processes can be allowed to perform so. A more restrictive
> access can be applied where the module autoload feature is completely
> disabled.
> In this schema the access rules are per-process and inherited by
> children created by fork(2) and clone(2), and preserved across execve(2).
>
> Current interface:
>
> *) The per-process prctl() settings are:
>
>  prctl(PR_MOD_AUTO_RESTRICT_OPTS, PR_SET_MOD_AUTO_RESTRICT, value, 0, 0)
>
>  Where value means:
>
>  0 - Classic module auto-load permissions, nothing changes.
>
>  1 - The current process must have CAP_SYS_MODULE to be able to
>      auto-load modules. CAP_NET_ADMIN should allow to auto-load
>      modules with a 'netdev-%s' alias.
>
>  2 - Current process can not auto-load modules. Once set, this prctl
>      value can not be changed.
>
>  The per-process value may only be increased, never decreased, thus ensuring
>  that once applied, processes can never relaxe their setting.
>
> *) The global sysctl setting can be set by writting an integer value to
>    '/proc/sys/kernel/modautorestrict/autoload'
>
>  The valid values are:
>
>  0 - Classic module auto-load permissions, nothing changes.
>
>  1 - Processes must have CAP_SYS_MODULE to be able to auto-load modules.
>      CAP_NET_ADMIN should allow to auto-load modules with a 'netdev-%s'
>      alias.
>
>  2 - Processes can not auto-load modules. Once set, this sysctl value
>      can not be changed.
>
> *) Access rules:
>    First the prctl() settings are checked, if the access is not denied
>    then the global sysctl settings are checked.
>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> ---
>  MAINTAINERS                            |   7 +
>  include/linux/lsm_hooks.h              |   5 +
>  include/uapi/linux/prctl.h             |   5 +
>  security/Kconfig                       |   1 +
>  security/Makefile                      |  16 +-
>  security/modautorestrict/Kconfig       |  15 ++
>  security/modautorestrict/Makefile      |   3 +
>  security/modautorestrict/modauto_lsm.c | 372 +++++++++++++++++++++++++++++++++
>  security/security.c                    |   1 +
>  9 files changed, 418 insertions(+), 7 deletions(-)
>  create mode 100644 security/modautorestrict/Kconfig
>  create mode 100644 security/modautorestrict/Makefile
>  create mode 100644 security/modautorestrict/modauto_lsm.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c45c02b..38d17cd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11326,6 +11326,13 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git yama/tip
>  S:	Supported
>  F:	security/yama/
>  
> +MODAUTORESTRICT SECURITY MODULE
> +M:	Djalal Harouni <tixxdz@gmail.com>
> +L:	kernel-hardening@lists.openwall.com
> +L:	linux-security-module@vger.kernel.org
> +S:	Supported
> +F:	security/modautorestrict/
> +
>  SENSABLE PHANTOM
>  M:	Jiri Slaby <jirislaby@gmail.com>
>  S:	Maintained
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index ca19cdb..679800c 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1950,6 +1950,11 @@ void __init loadpin_add_hooks(void);
>  #else
>  static inline void loadpin_add_hooks(void) { };
>  #endif
> +#ifdef CONFIG_SECURITY_MODAUTORESTRICT
> +extern void modautorestrict_init(void);
> +#else
> +static inline void __init modautorestrict_init(void) { }
> +#endif
>  
>  /*
>   * Per "struct task_struct" security blob is managed using index numbers.
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index a8d0759..e561ca3 100644
> --- a/include/uapi/linux/prctl.h
> +++ b/include/uapi/linux/prctl.h
> @@ -197,4 +197,9 @@ struct prctl_mm_map {
>  # define PR_CAP_AMBIENT_LOWER		3
>  # define PR_CAP_AMBIENT_CLEAR_ALL	4
>  
> +/* Control ModAutoRestrict LSM options */
> +#define PR_MOD_AUTO_RESTRICT_OPTS	48
> +# define PR_SET_MOD_AUTO_RESTRICT	1
> +# define PR_GET_MOD_AUTO_RESTRICT	2
> +
>  #endif /* _LINUX_PRCTL_H */
> diff --git a/security/Kconfig b/security/Kconfig
> index 3ff1bf9..1e181c3 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -204,6 +204,7 @@ source security/tomoyo/Kconfig
>  source security/apparmor/Kconfig
>  source security/loadpin/Kconfig
>  source security/yama/Kconfig
> +source security/modautorestrict/Kconfig
>  
>  source security/integrity/Kconfig
>  
> diff --git a/security/Makefile b/security/Makefile
> index f2d71cd..4c120f7 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -2,13 +2,14 @@
>  # Makefile for the kernel security code
>  #
>  
> -obj-$(CONFIG_KEYS)			+= keys/
> -subdir-$(CONFIG_SECURITY_SELINUX)	+= selinux
> -subdir-$(CONFIG_SECURITY_SMACK)		+= smack
> -subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
> -subdir-$(CONFIG_SECURITY_APPARMOR)	+= apparmor
> -subdir-$(CONFIG_SECURITY_YAMA)		+= yama
> -subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
> +obj-$(CONFIG_KEYS)				+= keys/
> +subdir-$(CONFIG_SECURITY_SELINUX)		+= selinux
> +subdir-$(CONFIG_SECURITY_SMACK)			+= smack
> +subdir-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo
> +subdir-$(CONFIG_SECURITY_APPARMOR)		+= apparmor
> +subdir-$(CONFIG_SECURITY_YAMA)			+= yama
> +subdir-$(CONFIG_SECURITY_LOADPIN)		+= loadpin
> +subdir-$(CONFIG_SECURITY_MODAUTORESTRICT)	+= modautorestrict
>  
>  # always enable default capabilities
>  obj-y					+= commoncap.o
> @@ -24,6 +25,7 @@ obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/
>  obj-$(CONFIG_SECURITY_APPARMOR)		+= apparmor/
>  obj-$(CONFIG_SECURITY_YAMA)		+= yama/
>  obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
> +obj-$(CONFIG_SECURITY_MODAUTORESTRICT)	+= modautorestrict/
>  obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
>  
>  # Object integrity file lists
> diff --git a/security/modautorestrict/Kconfig b/security/modautorestrict/Kconfig
> new file mode 100644
> index 0000000..9678106
> --- /dev/null
> +++ b/security/modautorestrict/Kconfig
> @@ -0,0 +1,15 @@
> +config SECURITY_MODAUTORESTRICT
> +	bool "Automatic Module Loading Restriction"
> +	depends on SECURITY
> +	default n
> +	help
> +	  This selects ModAutoRestrict Linux Security Module which applies
> +          restrictions on automatic module loading operations. If this
> +          option is selected, a request to use a kernel feature that is
> +          implemented by an unloaded module will first have to satisfy the
> +          access rules of MODAUTORESTRICT. Furthermore, this allows system
> +          administrators or sandbox mechanisms to prevent loading unneeded
> +          modules.
> +          Further information can be found in Documentation/security/ModAutoRestrict.txt.
> +
> +	  If you are unsure how to answer this question, answer N.
> diff --git a/security/modautorestrict/Makefile b/security/modautorestrict/Makefile
> new file mode 100644
> index 0000000..a0656a5
> --- /dev/null
> +++ b/security/modautorestrict/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_SECURITY_MODAUTORESTRICT) := modautorestrict.o
> +
> +modautorestrict-y := modauto_lsm.o
> diff --git a/security/modautorestrict/modauto_lsm.c b/security/modautorestrict/modauto_lsm.c
> new file mode 100644
> index 0000000..b3a83b8e
> --- /dev/null
> +++ b/security/modautorestrict/modauto_lsm.c
> @@ -0,0 +1,372 @@
> +/*
> + * ModAutoRestrict Linux Security Module
> + *
> + * Author: Djalal Harouni
> + *
> + * Copyright (C) 2017 Djalal Harouni
> + * Copyright (C) 2017 Endocode AG.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2, as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/lsm_hooks.h>
> +#include <linux/prctl.h>
> +#include <linux/refcount.h>
> +#include <linux/types.h>
> +#include <linux/sched/mm.h>
> +#include <linux/sched/task.h>
> +#include <linux/sysctl.h>
> +
> +enum {
> +	MOD_AUTOLOAD_ALLOWED	= 0,
> +	MOD_AUTOLOAD_PRIVILEGED	= 1,
> +	MOD_AUTOLOAD_DENIED	= 2,
> +};
> +
> +struct modautoload_task {
> +	bool usage;
> +	u8 flags;
> +};
> +
> +static int autoload_restrict;
> +
> +static int zero;
> +static int max_autoload_restrict = MOD_AUTOLOAD_DENIED;
> +
> +/* Index number of per "struct task_struct" blob for ModAutoRestrict. */
> +u16 modautorestrict_task_security_index __ro_after_init;
> +
> +static inline int modautoload_task_set_flag(struct modautoload_task *modtask,
> +					    unsigned long value)
> +{
> +	int ret = 0;
> +
> +	if (value > MOD_AUTOLOAD_DENIED)
> +		ret = -EINVAL;
> +	else if (modtask->flags > value)
> +		ret = -EPERM;
> +	else if (modtask->flags < value)
> +		modtask->flags = value;
> +
> +	return ret;
> +}
> +
> +static inline struct modautoload_task *modautoload_task_security(struct task_struct *tsk)
> +{
> +	struct modautoload_task *modtask;
> +
> +	modtask = task_security(tsk, modautorestrict_task_security_index);
> +	if (modtask->usage)
> +		return modtask;
> +
> +	return NULL;
> +}
> +
> +static inline struct modautoload_task *init_modautoload_task(struct task_struct *tsk,
> +							     unsigned long flags)
> +{
> +	struct modautoload_task *modtask;
> +
> +	modtask = task_security(tsk, modautorestrict_task_security_index);
> +
> +	modtask->flags = (u8)flags;

I don't think you want to do this cast.

> +	modtask->usage = true;
> +
> +	return modtask;
> +}
> +
> +static inline void clear_modautoload_task(struct task_struct *tsk)
> +{
> +	struct modautoload_task *modtask;
> +
> +	modtask = modautoload_task_security(tsk);
> +	if (modtask) {
> +		modtask->usage = false;
> +		modtask->flags = MOD_AUTOLOAD_ALLOWED;
> +	}
> +}
> +
> +/*
> + * Return 0 if CAP_SYS_MODULE or if CAP_NET_ADMIN and the module is
> + * a netdev-%s module. Otherwise -EPERM is returned.
> + */
> +static int modautoload_privileged_access(const char *name)
> +{
> +	int ret = -EPERM;
> +
> +	if (capable(CAP_SYS_MODULE))
> +		ret = 0;
> +	else if (name && strstr(name, "netdev-") && capable(CAP_NET_ADMIN))
> +		ret = 0;
> +
> +	return ret;
> +}
> +
> +static int modautoload_sysctl_perm(unsigned long op, const char *name)
> +{
> +	int ret = -EINVAL;
> +	struct mm_struct *mm = NULL;
> +
> +	if (op != PR_GET_MOD_AUTO_RESTRICT)
> +		return ret;
> +
> +	switch (autoload_restrict) {
> +	case MOD_AUTOLOAD_ALLOWED:
> +		ret = 0;
> +		break;
> +	case MOD_AUTOLOAD_PRIVILEGED:
> +		/*
> +		 * Are we allowed to sleep here ?
> +		 * Also improve this check here
> +		 */
> +		ret = -EPERM;
> +		mm = get_task_mm(current);
> +		if (mm) {
> +			ret = modautoload_privileged_access(name);
> +			mmput(mm);
> +		}
> +		break;
> +	case MOD_AUTOLOAD_DENIED:
> +		ret = -EPERM;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int modautoload_task_perm(struct modautoload_task *mtask,
> +				 char *kmod_name)
> +{
> +	int ret = -EINVAL;
> +
> +	switch (mtask->flags) {
> +	case MOD_AUTOLOAD_ALLOWED:
> +		ret = 0;
> +		break;
> +	case MOD_AUTOLOAD_PRIVILEGED:
> +		ret = modautoload_privileged_access(kmod_name);
> +		break;
> +	case MOD_AUTOLOAD_DENIED:
> +		ret = -EPERM;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +/* Set the given option in a modautorestrict task */
> +static int modautoload_set_op_value(struct task_struct *tsk,
> +				    unsigned long value)
> +{
> +	int ret = -EINVAL;
> +	struct modautoload_task *modtask;
> +
> +	if (value > MOD_AUTOLOAD_DENIED)
> +		return ret;
> +
> +	modtask = modautoload_task_security(tsk);
> +	if (!modtask) {
> +		modtask = init_modautoload_task(tsk, value);
> +		return 0;
> +	}
> +
> +	return modautoload_task_set_flag(modtask, value);
> +}
> +
> +static int modautoload_get_op_value(struct task_struct *tsk)
> +{
> +	struct modautoload_task *modtask;
> +
> +	modtask = modautoload_task_security(tsk);
> +	if (!modtask)
> +		return -EINVAL;
> +
> +	return modtask->flags;
> +}
> +
> +/* Copy modautorestrict context from parent to child */
> +int modautoload_task_alloc(struct task_struct *tsk, unsigned long clone_flags)
> +{
> +	struct modautoload_task *modparent;
> +
> +	modparent = modautoload_task_security(current);
> +
> +	/* Parent has a modautorestrict context */
> +	if (modparent)
> +		init_modautoload_task(tsk, modparent->flags);
> +
> +	return 0;
> +}
> +
> +/*
> + * Return 0 on success, -error on error.  -ENOSYS is returned when modautorestrict
> + * does not handle the given option, or -EINVAL if the passed arguments are not
> + * valid.
> + */
> +int modautoload_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> +			   unsigned long arg4, unsigned long arg5)
> +{
> +	int ret = -EINVAL;
> +	struct task_struct *myself = current;
> +
> +	if (option != PR_MOD_AUTO_RESTRICT_OPTS)
> +		return -ENOSYS;
> +
> +	get_task_struct(myself);
> +
> +	switch (arg2) {
> +	case PR_SET_MOD_AUTO_RESTRICT:
> +		if (arg4 || arg5)
> +			goto out;
> +
> +		ret = modautoload_set_op_value(myself, arg3);
> +		break;
> +	case PR_GET_MOD_AUTO_RESTRICT:
> +		if (arg3 || arg4 || arg5)
> +			goto out;
> +
> +		ret = modautoload_get_op_value(myself);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +out:
> +	put_task_struct(myself);
> +	return ret;
> +}
> +
> +void modautoload_task_free(struct task_struct *tsk)
> +{
> +	clear_modautoload_task(tsk);
> +}
> +
> +/*
> + * TODO:
> + * if this is covered entirely by CAP_SYS_MODULE then we should removed it.
> + */
> +static int modautoload_kernel_module_file(struct file *file)
> +{
> +	int ret = 0;
> +	struct modautoload_task *modtask;
> +	struct task_struct *myself = current;
> +
> +	/* First check if the task allows that */
> +	modtask = modautoload_task_security(myself);
> +	if (modtask) {
> +		ret = modautoload_task_perm(modtask, NULL);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return modautoload_sysctl_perm(PR_GET_MOD_AUTO_RESTRICT, NULL);
> +}
> +
> +static int modautoload_kernel_module_request(char *kmod_name)
> +{
> +	int ret = 0;
> +	struct modautoload_task *modtask;
> +	struct task_struct *myself = current;
> +
> +	/* First check if the task allows that */
> +	modtask = modautoload_task_security(myself);
> +	if (modtask) {
> +		ret = modautoload_task_perm(modtask, kmod_name);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return modautoload_sysctl_perm(PR_GET_MOD_AUTO_RESTRICT, kmod_name);
> +}
> +
> +/*
> + * TODO:
> + * if this is covered entirely by CAP_SYS_MODULE then we should removed it.
> + */
> +static int modautoload_kernel_read_file(struct file *file,
> +					enum kernel_read_file_id id)
> +{
> +	int ret = 0;
> +
> +	switch (id) {
> +	case READING_MODULE:
> +		ret = modautoload_kernel_module_file(file);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static struct security_hook_list modautoload_hooks[] = {
> +	LSM_HOOK_INIT(kernel_module_request, modautoload_kernel_module_request),
> +	LSM_HOOK_INIT(kernel_read_file, modautoload_kernel_read_file),
> +	LSM_HOOK_INIT(task_alloc, modautoload_task_alloc),
> +	LSM_HOOK_INIT(task_prctl, modautoload_task_prctl),
> +	LSM_HOOK_INIT(task_free, modautoload_task_free),
> +};
> +
> +#ifdef CONFIG_SYSCTL
> +static int modautoload_dointvec_minmax(struct ctl_table *table, int write,
> +				       void __user *buffer, size_t *lenp,
> +				       loff_t *ppos)
> +{
> +	struct ctl_table table_copy;
> +
> +	if (write && !capable(CAP_SYS_MODULE))
> +		return -EPERM;
> +
> +	table_copy = *table;
> +	if (*(int *)table_copy.data == *(int *)table_copy.extra2)

While it's probably doing what you want, I find this
sort of casting disturbing.

> +		table_copy.extra1 = table_copy.extra2;
> +
> +	return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
> +}
> +
> +struct ctl_path modautoload_sysctl_path[] = {
> +	{ .procname = "kernel", },
> +	{ .procname = "modautorestrict", },
> +	{ }
> +};
> +
> +static struct ctl_table modautoload_sysctl_table[] = {
> +	{
> +		.procname       = "autoload",
> +		.data           = &autoload_restrict,
> +		.maxlen         = sizeof(int),
> +		.mode           = 0644,
> +		.proc_handler   = modautoload_dointvec_minmax,
> +		.extra1         = &zero,
> +		.extra2         = &max_autoload_restrict,
> +	},
> +	{ }
> +};
> +
> +static void __init modautoload_init_sysctl(void)
> +{
> +	if (!register_sysctl_paths(modautoload_sysctl_path, modautoload_sysctl_table))
> +		panic("modautorestrict: sysctl registration failed.\n");
> +}
> +#else
> +static inline void modautoload_init_sysctl(void) { }
> +#endif /* CONFIG_SYSCTL */
> +
> +void __init modautorestrict_init(void)
> +{
> +	modautorestrict_task_security_index =
> +		security_reserve_task_blob_index(sizeof(struct modautoload_task));
> +	security_add_hooks(modautoload_hooks,
> +			   ARRAY_SIZE(modautoload_hooks), "modautorestrict");
> +
> +	modautoload_init_sysctl();
> +	pr_info("ModAutoRestrict LSM:  Initialized\n");
> +}
> diff --git a/security/security.c b/security/security.c
> index 4dc6bca..d8852fe 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -70,6 +70,7 @@ int __init security_init(void)
>  	capability_add_hooks();
>  	yama_add_hooks();
>  	loadpin_add_hooks();
> +	modautorestrict_init();

This should be modautorestrict_add_hooks() if this were
a "minor" module, but as it's using a blob it is a "major"
module. Either way, this is not right.

>  
>  	/*
>  	 * Load all the remaining security modules.

Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Casey Schaufler]

On 4/9/2017 3:42 AM, Djalal Harouni wrote:
> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>
> Since several modules are planning to use per "struct task_struct" blob,
> we need a layer for isolating it. Therefore, this patch introduces per LSM
> module per "struct task_struct" blob.
>
> It would be possible to remember location in security_hook_heads.task_alloc
> list and undo up to the corresponding security_hook_heads.task_free list
> when task_alloc hook failed. But this patch calls all task_free hooks
> without checking whether the corresponding task_alloc hook was called
> since most modules should be able to handle this correctly.
>
> How to calculate amount of needed bytes and allocate memory for initial
> task is a chicken-or-egg syndrome. We need to know how many bytes needs
> to be allocated for initial task's security blobs before security_init()
> is called. But security_reserve_task_blob_index() which calculates amount
> of needed bytes is called from security_init(). We will need to split
> module registration into three steps. The first step is call
> security_reserve_task_blob_index() on all modules which should be
> activated. The second step is allocate memory for the initial task's
> security blob. The third step is actually activate all modules which
> should be activated.
>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

While I appreciate your enthusiasm, I would really like
to see a mechanism for managing all of the blobs as being
potentially shared rather than just the task blob. With
infrastructure blob management being the general case we
could stack any set of existing modules that does not
include both SELinux and Smack. (AppArmor is threatening
to join that set, but we're still waiting on that) I
think it's a bad idea to go ahead with one implementation
for task blobs without taking care of the others.

> ---
>  include/linux/lsm_hooks.h | 36 +++++++++++++++++++++++++++++++++++-
>  security/security.c       | 37 ++++++++++++++++++++++++++++++++++++-
>  2 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 080f34e..ca19cdb 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -27,6 +27,7 @@
>  #include <linux/security.h>
>  #include <linux/init.h>
>  #include <linux/rculist.h>
> +#include <linux/sched.h> /* "struct task_struct" */
>  
>  /**
>   * Security hooks for program execution operations.
> @@ -536,7 +537,10 @@
>   * @task_alloc:
>   *	@task task being allocated.
>   *	@clone_flags contains the flags indicating what should be shared.
> - *	Handle allocation of task-related resources.
> + *	Handle allocation of task-related resources. Note that task_free is
> + *	called even if task_alloc failed. This means that all task_free users
> + *	have to be prepared for task_free being called without corresponding
> + *	task_alloc call.
>   *	Returns a zero on success, negative values on failure.
>   * @task_free:
>   *	@task task about to be freed.
> @@ -1947,4 +1951,34 @@ void __init loadpin_add_hooks(void);
>  static inline void loadpin_add_hooks(void) { };
>  #endif
>  
> +/*
> + * Per "struct task_struct" security blob is managed using index numbers.
> + *
> + * Any user who wants to use per "struct task_struct" security blob reserves an
> + * index number using security_reserve_task_blob_index() before calling
> + * security_add_hooks().
> + *
> + * security_reserve_task_blob_index() returns an index number which has to be
> + * passed to task_security() by that user when fetching security blob of given
> + * "struct task_struct" for that user.
> + *
> + * Security blob for newly allocated "struct task_struct" is allocated and
> + * initialized with 0 inside security_task_alloc(), before calling each user's
> + * task_alloc hook. Be careful with task_free hook, for security_task_free()
> + * can be called when calling each user's task_alloc hook failed.
> + *
> + * Note that security_reserve_task_blob_index() uses "u16". It is not a good
> + * idea to directly reserve large amount. Sum of reserved amount by all users
> + * should be less than PAGE_SIZE bytes, for per "struct task_struct" security
> + * blob is allocated for each "struct task_struct" using kcalloc().
> + */
> +extern u16 __init security_reserve_task_blob_index(const u16 size);
> +static inline void *task_security(const struct task_struct *task,
> +				  const u16 index)
> +{
> +	unsigned long *p = task->security;
> +
> +	return p + index;
> +}
> +
>  #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/security/security.c b/security/security.c
> index 549bddc..4dc6bca 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -32,6 +32,7 @@
>  /* Maximum number of letters for an LSM name string */
>  #define SECURITY_NAME_MAX	10
>  
> +static u16 lsm_max_per_task_blob_index __ro_after_init;
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init;
>  char *lsm_names;
>  /* Boot-time LSM user choice */
> @@ -75,6 +76,15 @@ int __init security_init(void)
>  	 */
>  	do_security_initcalls();
>  
> +	/*
> +	 * Allocate per "struct task_struct" blob for initial task.
> +	 * Initialization is done later by each module which needs it.
> +	 */
> +	if (lsm_max_per_task_blob_index)
> +		current->security = kcalloc(lsm_max_per_task_blob_index,
> +					    sizeof(unsigned long),
> +					    GFP_KERNEL | __GFP_NOFAIL);
> +
>  	return 0;
>  }
>  
> @@ -86,6 +96,15 @@ static int __init choose_lsm(char *str)
>  }
>  __setup("security=", choose_lsm);
>  
> +u16 __init security_reserve_task_blob_index(const u16 size)
> +{
> +	const u16 index = lsm_max_per_task_blob_index;
> +	u16 requested = DIV_ROUND_UP(size, sizeof(unsigned long));
> +
> +	lsm_max_per_task_blob_index += requested;
> +	return index;
> +}
> +
>  static int lsm_append(char *new, char **result)
>  {
>  	char *cp;
> @@ -939,12 +958,28 @@ int security_task_create(unsigned long clone_flags)
>  
>  int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
>  {
> -	return call_int_hook(task_alloc, 0, task, clone_flags);
> +	int rc;
> +	const unsigned long index = lsm_max_per_task_blob_index;
> +
> +	if (index) {
> +		task->security = kcalloc(index, sizeof(unsigned long),
> +					 GFP_KERNEL);
> +		if (unlikely(!task->security))
> +			return -ENOMEM;
> +	}
> +	rc = call_int_hook(task_alloc, 0, task, clone_flags);
> +	if (unlikely(rc))
> +		security_task_free(task);
> +	return rc;
>  }
>  
>  void security_task_free(struct task_struct *task)
>  {
>  	call_void_hook(task_free, task);
> +	if (lsm_max_per_task_blob_index) {
> +		kfree(task->security);
> +		task->security = NULL;
> +	}
>  }
>  
>  int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)

Re: [PATCH RFC v2 2/3] security: add the ModAutoRestrict Linux Security Module

[Djalal Harouni]

On Mon, Apr 10, 2017 at 5:42 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/9/2017 3:42 AM, Djalal Harouni wrote:
[...]
>> +
>> +static inline struct modautoload_task *init_modautoload_task(struct task_struct *tsk,
>> +                                                          unsigned long flags)
>> +{
>> +     struct modautoload_task *modtask;
>> +
>> +     modtask = task_security(tsk, modautorestrict_task_security_index);
>> +
>> +     modtask->flags = (u8)flags;
>
> I don't think you want to do this cast.

Will fix it. Thanks!


[...]
>> +
>> +#ifdef CONFIG_SYSCTL
>> +static int modautoload_dointvec_minmax(struct ctl_table *table, int write,
>> +                                    void __user *buffer, size_t *lenp,
>> +                                    loff_t *ppos)
>> +{
>> +     struct ctl_table table_copy;
>> +
>> +     if (write && !capable(CAP_SYS_MODULE))
>> +             return -EPERM;
>> +
>> +     table_copy = *table;
>> +     if (*(int *)table_copy.data == *(int *)table_copy.extra2)
>
> While it's probably doing what you want, I find this
> sort of casting disturbing.

Ok will try to improve it.


>> +             table_copy.extra1 = table_copy.extra2;
>> +
>> +     return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>> +}
>> +
>> +struct ctl_path modautoload_sysctl_path[] = {
>> +     { .procname = "kernel", },
>> +     { .procname = "modautorestrict", },
>> +     { }
>> +};
>> +
>> +static struct ctl_table modautoload_sysctl_table[] = {
>> +     {
>> +             .procname       = "autoload",
>> +             .data           = &autoload_restrict,
>> +             .maxlen         = sizeof(int),
>> +             .mode           = 0644,
>> +             .proc_handler   = modautoload_dointvec_minmax,
>> +             .extra1         = &zero,
>> +             .extra2         = &max_autoload_restrict,
>> +     },
>> +     { }
>> +};
>> +
>> +static void __init modautoload_init_sysctl(void)
>> +{
>> +     if (!register_sysctl_paths(modautoload_sysctl_path, modautoload_sysctl_table))
>> +             panic("modautorestrict: sysctl registration failed.\n");
>> +}
>> +#else
>> +static inline void modautoload_init_sysctl(void) { }
>> +#endif /* CONFIG_SYSCTL */
>> +
>> +void __init modautorestrict_init(void)
>> +{
>> +     modautorestrict_task_security_index =
>> +             security_reserve_task_blob_index(sizeof(struct modautoload_task));
>> +     security_add_hooks(modautoload_hooks,
>> +                        ARRAY_SIZE(modautoload_hooks), "modautorestrict");
>> +
>> +     modautoload_init_sysctl();
>> +     pr_info("ModAutoRestrict LSM:  Initialized\n");
>> +}
>> diff --git a/security/security.c b/security/security.c
>> index 4dc6bca..d8852fe 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -70,6 +70,7 @@ int __init security_init(void)
>>       capability_add_hooks();
>>       yama_add_hooks();
>>       loadpin_add_hooks();
>> +     modautorestrict_init();
>
> This should be modautorestrict_add_hooks() if this were
> a "minor" module, but as it's using a blob it is a "major"
> module. Either way, this is not right.

Do you mean that if I'm using a blob, it should go with the rest LSMs
in do_security_initcalls() ?

>>
>>       /*
>>        * Load all the remaining security modules.
>

Thanks for the comments!


-- 
tixxdz

Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Djalal Harouni]

On Mon, Apr 10, 2017 at 5:50 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/9/2017 3:42 AM, Djalal Harouni wrote:
>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>
>> Since several modules are planning to use per "struct task_struct" blob,
>> we need a layer for isolating it. Therefore, this patch introduces per LSM
>> module per "struct task_struct" blob.
>>
>> It would be possible to remember location in security_hook_heads.task_alloc
>> list and undo up to the corresponding security_hook_heads.task_free list
>> when task_alloc hook failed. But this patch calls all task_free hooks
>> without checking whether the corresponding task_alloc hook was called
>> since most modules should be able to handle this correctly.
>>
>> How to calculate amount of needed bytes and allocate memory for initial
>> task is a chicken-or-egg syndrome. We need to know how many bytes needs
>> to be allocated for initial task's security blobs before security_init()
>> is called. But security_reserve_task_blob_index() which calculates amount
>> of needed bytes is called from security_init(). We will need to split
>> module registration into three steps. The first step is call
>> security_reserve_task_blob_index() on all modules which should be
>> activated. The second step is allocate memory for the initial task's
>> security blob. The third step is actually activate all modules which
>> should be activated.
>>
>> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>
> While I appreciate your enthusiasm, I would really like
> to see a mechanism for managing all of the blobs as being
> potentially shared rather than just the task blob. With
> infrastructure blob management being the general case we
> could stack any set of existing modules that does not
> include both SELinux and Smack. (AppArmor is threatening
> to join that set, but we're still waiting on that) I

Yes! most of the other kernel maintainers I discussed the feature with
did not even believe or knew that LSM stacking is possible! Some other
kernel frameworks are being replaced with new ones (I'm not sure if
the old ones were perfect!)... but what I'm trying to say is that we
should make it easy for dynamic LSM plugins model so they can explore
the interface, and maybe after some time the whole framework will even
be replaced with a better one...
Thanks to your effort and others we may achieve it, but I don't think
that delaying features for a perfect implementation is the best way.
During linuxcon 2016 Europe you said that there should be a new kind
of LSMs, that's why I think we should make it easy for that to happen.

> think it's a bad idea to go ahead with one implementation
> for task blobs without taking care of the others.

Ok. Sorry if I missed this, but could you please point me why this
could be a bad idea ?

As I understand it, these are internal details that could be replaced.
My first implementation used resizable concurrent hashtables that are
heavily used in the networking code and it worked, and I easily
converted the module to use Tetsuo's approach since I didn't see any
objection for it, and it continued to work. Maybe the point should be
*which* LSM should use the task->security and how ? The
ModAutoRestrict module is a simple LSM which could easily work with
any provided solution.

That being said, I could convert the module back and stick with
rhashtables for now since it does not conflict with any module and it
works well. I would like to avoid same history where Apparmor, Smack
and TOMOYO all suffered to get mainlined, even Yama due to some
requests...  Then I can convert the module back to use the same LSM
infrastructure if you maintainers think that how it should go, that's
totally fine by me. Yama internally could use the same task blob but
it is avoiding conflicts by using lists to manage its internal data,
that's the same design with ModAutoRestrict and rhashtables.

Thank you for the comment!

-- 
tixxdz

Re: [PATCH RFC v2 2/3] security: add the ModAutoRestrict Linux Security Module

[Casey Schaufler]

On 4/10/2017 11:27 AM, Djalal Harouni wrote:
> On Mon, Apr 10, 2017 at 5:42 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/9/2017 3:42 AM, Djalal Harouni wrote:
> [...]
>>> +
>>> +static inline struct modautoload_task *init_modautoload_task(struct task_struct *tsk,
>>> +                                                          unsigned long flags)
>>> +{
>>> +     struct modautoload_task *modtask;
>>> +
>>> +     modtask = task_security(tsk, modautorestrict_task_security_index);
>>> +
>>> +     modtask->flags = (u8)flags;
>> I don't think you want to do this cast.
> Will fix it. Thanks!
>
>
> [...]
>>> +
>>> +#ifdef CONFIG_SYSCTL
>>> +static int modautoload_dointvec_minmax(struct ctl_table *table, int write,
>>> +                                    void __user *buffer, size_t *lenp,
>>> +                                    loff_t *ppos)
>>> +{
>>> +     struct ctl_table table_copy;
>>> +
>>> +     if (write && !capable(CAP_SYS_MODULE))
>>> +             return -EPERM;
>>> +
>>> +     table_copy = *table;
>>> +     if (*(int *)table_copy.data == *(int *)table_copy.extra2)
>> While it's probably doing what you want, I find this
>> sort of casting disturbing.
> Ok will try to improve it.
>
>
>>> +             table_copy.extra1 = table_copy.extra2;
>>> +
>>> +     return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>>> +}
>>> +
>>> +struct ctl_path modautoload_sysctl_path[] = {
>>> +     { .procname = "kernel", },
>>> +     { .procname = "modautorestrict", },
>>> +     { }
>>> +};
>>> +
>>> +static struct ctl_table modautoload_sysctl_table[] = {
>>> +     {
>>> +             .procname       = "autoload",
>>> +             .data           = &autoload_restrict,
>>> +             .maxlen         = sizeof(int),
>>> +             .mode           = 0644,
>>> +             .proc_handler   = modautoload_dointvec_minmax,
>>> +             .extra1         = &zero,
>>> +             .extra2         = &max_autoload_restrict,
>>> +     },
>>> +     { }
>>> +};
>>> +
>>> +static void __init modautoload_init_sysctl(void)
>>> +{
>>> +     if (!register_sysctl_paths(modautoload_sysctl_path, modautoload_sysctl_table))
>>> +             panic("modautorestrict: sysctl registration failed.\n");
>>> +}
>>> +#else
>>> +static inline void modautoload_init_sysctl(void) { }
>>> +#endif /* CONFIG_SYSCTL */
>>> +
>>> +void __init modautorestrict_init(void)
>>> +{
>>> +     modautorestrict_task_security_index =
>>> +             security_reserve_task_blob_index(sizeof(struct modautoload_task));
>>> +     security_add_hooks(modautoload_hooks,
>>> +                        ARRAY_SIZE(modautoload_hooks), "modautorestrict");
>>> +
>>> +     modautoload_init_sysctl();
>>> +     pr_info("ModAutoRestrict LSM:  Initialized\n");
>>> +}
>>> diff --git a/security/security.c b/security/security.c
>>> index 4dc6bca..d8852fe 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -70,6 +70,7 @@ int __init security_init(void)
>>>       capability_add_hooks();
>>>       yama_add_hooks();
>>>       loadpin_add_hooks();
>>> +     modautorestrict_init();
>> This should be modautorestrict_add_hooks() if this were
>> a "minor" module, but as it's using a blob it is a "major"
>> module. Either way, this is not right.
> Do you mean that if I'm using a blob, it should go with the rest LSMs
> in do_security_initcalls() ?

Right. Today you have coincidental non-interference because
no one else is using the task blob. As you're aware, TOMOYO
is going to start using it, and I believe the AppArmor has
plans for it as well. There are parts of the Smack cred blob
that should probably go in the task blob as they aren't used
in access decisions. I haven't looked closely enough, but that's
possible for SELinux, too. So even though it's a new blob, the
major/minor rules apply.

>
>>>       /*
>>>        * Load all the remaining security modules.
> Thanks for the comments!
>
>

Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Casey Schaufler]

On 4/10/2017 11:30 AM, Djalal Harouni wrote:
> On Mon, Apr 10, 2017 at 5:50 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/9/2017 3:42 AM, Djalal Harouni wrote:
>>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>>>
>>> Since several modules are planning to use per "struct task_struct" blob,
>>> we need a layer for isolating it. Therefore, this patch introduces per LSM
>>> module per "struct task_struct" blob.
>>>
>>> It would be possible to remember location in security_hook_heads.task_alloc
>>> list and undo up to the corresponding security_hook_heads.task_free list
>>> when task_alloc hook failed. But this patch calls all task_free hooks
>>> without checking whether the corresponding task_alloc hook was called
>>> since most modules should be able to handle this correctly.
>>>
>>> How to calculate amount of needed bytes and allocate memory for initial
>>> task is a chicken-or-egg syndrome. We need to know how many bytes needs
>>> to be allocated for initial task's security blobs before security_init()
>>> is called. But security_reserve_task_blob_index() which calculates amount
>>> of needed bytes is called from security_init(). We will need to split
>>> module registration into three steps. The first step is call
>>> security_reserve_task_blob_index() on all modules which should be
>>> activated. The second step is allocate memory for the initial task's
>>> security blob. The third step is actually activate all modules which
>>> should be activated.
>>>
>>> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> While I appreciate your enthusiasm, I would really like
>> to see a mechanism for managing all of the blobs as being
>> potentially shared rather than just the task blob. With
>> infrastructure blob management being the general case we
>> could stack any set of existing modules that does not
>> include both SELinux and Smack. (AppArmor is threatening
>> to join that set, but we're still waiting on that) I
> Yes! most of the other kernel maintainers I discussed the feature with
> did not even believe or knew that LSM stacking is possible! Some other
> kernel frameworks are being replaced with new ones (I'm not sure if
> the old ones were perfect!)... but what I'm trying to say is that we
> should make it easy for dynamic LSM plugins model so they can explore
> the interface, and maybe after some time the whole framework will even
> be replaced with a better one...

I'm not committing to dynamic modules, but I am working
to make sure I don't do anything to prevent someone else
from doing it in the future. There are a good number of
people who find the notion of dynamic security policy
disturbing.

> Thanks to your effort and others we may achieve it, but I don't think
> that delaying features for a perfect implementation is the best way.
> During linuxcon 2016 Europe you said that there should be a new kind
> of LSMs, that's why I think we should make it easy for that to happen.

I agree that it's better to use a work-around today then to
let the shortcomings of the existing infrastructure stop you
from getting your job done.

>
>> think it's a bad idea to go ahead with one implementation
>> for task blobs without taking care of the others.
> Ok. Sorry if I missed this, but could you please point me why this
> could be a bad idea ?

It's a whole lot easier to maintain one sort of blob
management then a different kind for each blob. It would
be lots cleaner to have a single call that tells the
infrastructure how much space you need for all your blobs
than a separate interface for each.

> As I understand it, these are internal details that could be replaced.
> My first implementation used resizable concurrent hashtables that are
> heavily used in the networking code and it worked, and I easily
> converted the module to use Tetsuo's approach since I didn't see any
> objection for it, and it continued to work. Maybe the point should be
> *which* LSM should use the task->security and how ? The
> ModAutoRestrict module is a simple LSM which could easily work with
> any provided solution.

So I see.

> That being said, I could convert the module back and stick with
> rhashtables for now since it does not conflict with any module and it
> works well. I would like to avoid same history where Apparmor, Smack
> and TOMOYO all suffered to get mainlined, even Yama due to some
> requests...  Then I can convert the module back to use the same LSM
> infrastructure if you maintainers think that how it should go, that's
> totally fine by me. Yama internally could use the same task blob but
> it is avoiding conflicts by using lists to manage its internal data,
> that's the same design with ModAutoRestrict and rhashtables.

I think that would be the prudent approach. There is still
the possibility that blob sharing (or full stacking, if you
prefer) won't be accepted any time soon.

> Thank you for the comment!
>

Re: [PATCH RFC v2 2/3] security: add the ModAutoRestrict Linux Security Module

[Djalal Harouni]

On Mon, Apr 10, 2017 at 9:04 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/10/2017 11:27 AM, Djalal Harouni wrote:
>> On Mon, Apr 10, 2017 at 5:42 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 4/9/2017 3:42 AM, Djalal Harouni wrote:
[...]

>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -70,6 +70,7 @@ int __init security_init(void)
>>>>       capability_add_hooks();
>>>>       yama_add_hooks();
>>>>       loadpin_add_hooks();
>>>> +     modautorestrict_init();
>>> This should be modautorestrict_add_hooks() if this were
>>> a "minor" module, but as it's using a blob it is a "major"
>>> module. Either way, this is not right.
>> Do you mean that if I'm using a blob, it should go with the rest LSMs
>> in do_security_initcalls() ?
>
> Right. Today you have coincidental non-interference because
> no one else is using the task blob. As you're aware, TOMOYO
> is going to start using it, and I believe the AppArmor has
> plans for it as well. There are parts of the Smack cred blob
> that should probably go in the task blob as they aren't used
> in access decisions. I haven't looked closely enough, but that's
> possible for SELinux, too. So even though it's a new blob, the
> major/minor rules apply.
>

Ok, point taken.

Thanks!

-- 
tixxdz

Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Djalal Harouni]

On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/10/2017 11:30 AM, Djalal Harouni wrote:
>> On Mon, Apr 10, 2017 at 5:50 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 4/9/2017 3:42 AM, Djalal Harouni wrote:
>>>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[...]

>>> While I appreciate your enthusiasm, I would really like
>>> to see a mechanism for managing all of the blobs as being
>>> potentially shared rather than just the task blob. With
>>> infrastructure blob management being the general case we
>>> could stack any set of existing modules that does not
>>> include both SELinux and Smack. (AppArmor is threatening
>>> to join that set, but we're still waiting on that) I
>> Yes! most of the other kernel maintainers I discussed the feature with
>> did not even believe or knew that LSM stacking is possible! Some other
>> kernel frameworks are being replaced with new ones (I'm not sure if
>> the old ones were perfect!)... but what I'm trying to say is that we
>> should make it easy for dynamic LSM plugins model so they can explore
>> the interface, and maybe after some time the whole framework will even
>> be replaced with a better one...
>
> I'm not committing to dynamic modules, but I am working
> to make sure I don't do anything to prevent someone else
> from doing it in the future. There are a good number of
> people who find the notion of dynamic security policy
> disturbing.

I understand, though that with today's container recycling use cases
some of it may make sense. Anyway you are doing the real work, you
know better.


>> Thanks to your effort and others we may achieve it, but I don't think
>> that delaying features for a perfect implementation is the best way.
>> During linuxcon 2016 Europe you said that there should be a new kind
>> of LSMs, that's why I think we should make it easy for that to happen.
>
> I agree that it's better to use a work-around today then to
> let the shortcomings of the existing infrastructure stop you
> from getting your job done.

Indeed!


>>
>>> think it's a bad idea to go ahead with one implementation
>>> for task blobs without taking care of the others.
>> Ok. Sorry if I missed this, but could you please point me why this
>> could be a bad idea ?
>
> It's a whole lot easier to maintain one sort of blob
> management then a different kind for each blob. It would
> be lots cleaner to have a single call that tells the
> infrastructure how much space you need for all your blobs
> than a separate interface for each.

I perfectly agree here. With Tetsuo patch it was easy to add a
stackable LSM, with a consistent approach that would be even better.
But as you note that should not be a blocker to upstream new stackable
LSMs.


>> As I understand it, these are internal details that could be replaced.
>> My first implementation used resizable concurrent hashtables that are
>> heavily used in the networking code and it worked, and I easily
>> converted the module to use Tetsuo's approach since I didn't see any
>> objection for it, and it continued to work. Maybe the point should be
>> *which* LSM should use the task->security and how ? The
>> ModAutoRestrict module is a simple LSM which could easily work with
>> any provided solution.
>
> So I see.
>
>> That being said, I could convert the module back and stick with
>> rhashtables for now since it does not conflict with any module and it
>> works well. I would like to avoid same history where Apparmor, Smack
>> and TOMOYO all suffered to get mainlined, even Yama due to some
>> requests...  Then I can convert the module back to use the same LSM
>> infrastructure if you maintainers think that how it should go, that's
>> totally fine by me. Yama internally could use the same task blob but
>> it is avoiding conflicts by using lists to manage its internal data,
>> that's the same design with ModAutoRestrict and rhashtables.
>
> I think that would be the prudent approach. There is still
> the possibility that blob sharing (or full stacking, if you
> prefer) won't be accepted any time soon.

Ok Casey! I will wait for more feedback, and if other maintainers do
not object, I will convert it back to rhashtables in next iterations
making sure that it should be simple to convert later to a blob
sharing mechanism.

Thanks again for the comments!


-- 
tixxdz

Re: [PATCH RFC v2 0/3] security: Add ModAutoRestrict LSM

[Kees Cook]

On Sun, Apr 9, 2017 at 3:42 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> Hi List,
>
> This is RFC v2 of the Module auto-loading restriction feature. The
> module has been renamed ModAutoRestrict LSM.
>
> This RFC is a work in progress update.
>
> There are still minor things to fix which are listed in the TODO
> section. Also I used Tetsuo approach of stacking task->security, since
> now that the task_security_alloc() hook is in linux-security/next I took
> advantage of it and switched to use that hook and applied Tetsuo
> task->security per LSM blob on top of it. I can also switch to Casey
> approach. However, since these are internal implementation details which
> are not exposed, I do not think that this is a big issue. Thanks for the
> work now it is clear that we are able to easily stack new LSMs.
>
> Patches are against linux-security/next HEAD 622f6e3265707eb
>
> ==============
>
> ModAutoRestrict is a Linux Security Module that applies restrictions on
> automatic module loading operations. This is selectable at build-time
> with CONFIG_SECURITY_MODAUTORESTRICT, and can be controlled at run-time
> through sysctls in /proc/sys/kernel/modautorestrict/autoload or as a
> per-process setting via a prctl() interface.
>
> A userspace request to use a kernel feature that is implemented by modules
> that are not loaded may trigger the module auto-load feature to load
> these modules in order to satisfy userspace. However as today's Linux use
> cases cover embedded systems to containers where applications are running
> in their own separate environments, reducing or preventing operations
> that may affect external environments is an important constraint.
> Therefore, we need a way to control if automatic module loading is
> allowed or which applications are allowed to trigger the module
> auto-load feature.

It might be worth looking through the last several years of kernel
CVEs to find all the ones that would have been stopped with a feature
like this. There are many examples lately: both DCCP (CVE-2017-6074)
and n_hldc (CVE-2017-2636) come to mind this year alone.

> The ModAutoRestrict LSM allows system administrators or sandbox
> mechanisms to control the module auto-load feature and prevent loading
> unneeded modules or abuse the interface.
>
> The settings can be applied globally using a sysctl interface which
> completes the core kernel interface "modules_disable".

Typo: complements

Perhaps mention why this is a complement (i.e. modules_disable is ALL
modules, and modautorestrict is just auto-loaded, not explicitly
loaded).

>
> The feature is also available as a prctl() interface. This allows to
> apply restrictions when sandboxing processes. On embedded Linux systems,
> or containers where only some containers/processes should have the
> right privileges to load modules, this allows to restrict those
> processes from inserting modules. Only privileged processes can be
> allowed to perform so. A more restrictive access can be applied where
> the module autoload feature is completely disabled.
> In this schema the access rules are per-process and inherited by
> children created by fork(2) and clone(2), and preserved across execve(2).
>
> Interface:
>
> *) The per-process prctl() settings are:
>
>  prctl(PR_MOD_AUTO_RESTRICT_OPTS, PR_SET_MOD_AUTO_RESTRICT, value, 0, 0)
>
>  Where value means:
>
>  0 - Classic module auto-load permissions, nothing changes.
>
>  1 - The current process must have CAP_SYS_MODULE to be able to
>      auto-load modules. CAP_NET_ADMIN should allow to auto-load
>      modules with a 'netdev-%s' alias.
>
>  2 - Current process can not auto-load modules. Once set, this prctl
>      value can not be changed.
>
>  The per-process value may only be increased, never decreased, thus ensuring
>  that once applied, processes can never relaxe their setting.

Typo: relax

> *) The global sysctl setting can be set by writting an integer value to
>    '/proc/sys/kernel/modautorestrict/autoload'

I wonder if this should just be named
/proc/sys/kernel/modules_autoload (to look more like
modules_disabled)?

>  The valid values are:
>
>  0 - Classic module auto-load permissions, nothing changes.
>
>  1 - Processes must have CAP_SYS_MODULE to be able to auto-load modules.
>      CAP_NET_ADMIN should allow to auto-load modules with a 'netdev-%s'
>      alias.
>
>  2 - Processes can not auto-load modules. Once set, this sysctl value
>      can not be changed.
>
> *) Access rules:
>    First the prctl() settings are checked, if the access is not denied
>    then the global sysctl settings are checked.
>
> The original idea and inspiration is from grsecurity 'GRKERNSEC_MODHARDEN'.
>
>
> The sample code here can be used to test the feature:
> https://gist.githubusercontent.com/tixxdz/39a583358f04d40b4d3e5571f95c075b/raw/7fb416285412891e2637fba149da930fe0898356/modautorestrict_test.c
>
> # TODO list:
> *) Confirm the struct task_struct->security stacking mechanism.

If we can't settle on a way to do this, perhaps this LSM should just
live in the core kernel, similar to modules_disabled? We do already
have the hooks to implement it with an LSM, but I'd hate to block this
feature just because we can't solve task_struct sharing.

Maybe add a new bit like no_new_privs? (Though I guess you'd need 2
bits, so maybe a full field like no_new_privs used to be.)

> *) Add a logging mechanism.
> *) Remove the use of security_kernel_read_file hook. Use only
>    security_kernel_module_request and make sure that we cover all cases.
> *) Convert documentation to .rst

Another TODO item could be to add output to /proc/$pid/status as done
for seccomp and no_new_privs? Though perhaps only if this is a core
kernel change...

> # Changes since v1:
> *) Renamed module to ModAutoRestrict
> *) Improved documentation to explicity refer to module autoloading.
> *) Switched to use the new task_security_alloc() hook.
> *) Switched from rhash tables to use task->security since it is in
>    linux-security/next branch now.
> *) Check all parameters passed to prctl() syscall.
> *) Many other bug fixes and documentation improvements.
>
> All Kees Cook comments handled, except for the removing of
> security_kernel_read_file which I will do in next iteration when
> I make sure that I did not miss something. Thank you!
>
>
> Tetsuo Handa (1):
>         LSM: Allow per LSM module per "struct task_struct" blob
>
> Djalal Harouni (2):
>         security: add the ModAutoRestrict Linux Security Module
>         Documentation: add ModAutoRestrict LSM documentation
>
>
>  Documentation/security/00-INDEX            |   2 +
>  Documentation/security/ModAutoRestrict.txt |  77 ++++++
>  MAINTAINERS                                |   7 +
>  include/linux/lsm_hooks.h                  |  41 +++-
>  include/uapi/linux/prctl.h                 |   5 +
>  security/Kconfig                           |   1 +
>  security/Makefile                          |  16 +-
>  security/modautorestrict/Kconfig           |  15 ++
>  security/modautorestrict/Makefile          |   3 +
>  security/modautorestrict/modauto_lsm.c     | 372 +++++++++++++++++++++++++++++
>  security/security.c                        |  38 ++-
>  11 files changed, 568 insertions(+), 9 deletions(-)

I'd love to see this restriction added; I'm sick of adding stuff to
modprobe.d blacklist files after-the-fact...

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Kees Cook]

On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> I think that would be the prudent approach. There is still
>> the possibility that blob sharing (or full stacking, if you
>> prefer) won't be accepted any time soon.
>
> Ok Casey! I will wait for more feedback, and if other maintainers do
> not object, I will convert it back to rhashtables in next iterations
> making sure that it should be simple to convert later to a blob
> sharing mechanism.

Would it be possible just to add a single field to task_struct if this
LSM is built in? I feel like rhashtables is a huge overhead when a
single field is all that's needed.

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Casey Schaufler]

On 4/10/2017 9:43 PM, Kees Cook wrote:
> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> I think that would be the prudent approach. There is still
>>> the possibility that blob sharing (or full stacking, if you
>>> prefer) won't be accepted any time soon.
>> Ok Casey! I will wait for more feedback, and if other maintainers do
>> not object, I will convert it back to rhashtables in next iterations
>> making sure that it should be simple to convert later to a blob
>> sharing mechanism.
> Would it be possible just to add a single field to task_struct if this
> LSM is built in? I feel like rhashtables is a huge overhead when a
> single field is all that's needed.

Special casing the task_struct based on which modules
are compiled in would work, but I'm under the impression
that there's a strong desire to keep to one pointer for
security module information in the major structures.

The code for generalizing shared blobs isn't that hard,
and y'all have seen it many times. It would be perfectly
safe to convert the task, cred, inode and such blobs to
be infrastructure managed right now. That wouldn't mean
that all the stacking issues (e.g. audit and networking)
would be addressed, or that all combinations of modules
would work (i.e. no SELinux+Smack) but it would clear
the way for this case. And Yama could use a blob if it
wanted to.

>
> -Kees
>

Re: [kernel-hardening] Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Kees Cook]

On Tue, Apr 11, 2017 at 12:54 PM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
> On 4/10/2017 9:43 PM, Kees Cook wrote:
>> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> I think that would be the prudent approach. There is still
>>>> the possibility that blob sharing (or full stacking, if you
>>>> prefer) won't be accepted any time soon.
>>> Ok Casey! I will wait for more feedback, and if other maintainers do
>>> not object, I will convert it back to rhashtables in next iterations
>>> making sure that it should be simple to convert later to a blob
>>> sharing mechanism.
>> Would it be possible just to add a single field to task_struct if this
>> LSM is built in? I feel like rhashtables is a huge overhead when a
>> single field is all that's needed.
>
> Special casing the task_struct based on which modules
> are compiled in would work, but I'm under the impression
> that there's a strong desire to keep to one pointer for
> security module information in the major structures.

Right, I meant as far as keeping this patch set unblocked by the other one...

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH RFC v2 0/3] security: Add ModAutoRestrict LSM

[Djalal Harouni]

On Tue, Apr 11, 2017 at 6:23 AM, Kees Cook <keescook@chromium.org> wrote:
> On Sun, Apr 9, 2017 at 3:42 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
[...]

>> A userspace request to use a kernel feature that is implemented by modules
>> that are not loaded may trigger the module auto-load feature to load
>> these modules in order to satisfy userspace. However as today's Linux use
>> cases cover embedded systems to containers where applications are running
>> in their own separate environments, reducing or preventing operations
>> that may affect external environments is an important constraint.
>> Therefore, we need a way to control if automatic module loading is
>> allowed or which applications are allowed to trigger the module
>> auto-load feature.
>
> It might be worth looking through the last several years of kernel
> CVEs to find all the ones that would have been stopped with a feature
> like this. There are many examples lately: both DCCP (CVE-2017-6074)
> and n_hldc (CVE-2017-2636) come to mind this year alone.

You are right, will do it.


>> The ModAutoRestrict LSM allows system administrators or sandbox
>> mechanisms to control the module auto-load feature and prevent loading
>> unneeded modules or abuse the interface.
>>
>> The settings can be applied globally using a sysctl interface which
>> completes the core kernel interface "modules_disable".
>
> Typo: complements
>
> Perhaps mention why this is a complement (i.e. modules_disable is ALL
> modules, and modautorestrict is just auto-loaded, not explicitly
> loaded).

Ok will fix it.


>>
>> The feature is also available as a prctl() interface. This allows to
>> apply restrictions when sandboxing processes. On embedded Linux systems,
>> or containers where only some containers/processes should have the
>> right privileges to load modules, this allows to restrict those
>> processes from inserting modules. Only privileged processes can be
>> allowed to perform so. A more restrictive access can be applied where
>> the module autoload feature is completely disabled.
>> In this schema the access rules are per-process and inherited by
>> children created by fork(2) and clone(2), and preserved across execve(2).
>>
>> Interface:
>>
>> *) The per-process prctl() settings are:
>>
>>  prctl(PR_MOD_AUTO_RESTRICT_OPTS, PR_SET_MOD_AUTO_RESTRICT, value, 0, 0)
>>
>>  Where value means:
>>
>>  0 - Classic module auto-load permissions, nothing changes.
>>
>>  1 - The current process must have CAP_SYS_MODULE to be able to
>>      auto-load modules. CAP_NET_ADMIN should allow to auto-load
>>      modules with a 'netdev-%s' alias.
>>
>>  2 - Current process can not auto-load modules. Once set, this prctl
>>      value can not be changed.
>>
>>  The per-process value may only be increased, never decreased, thus ensuring
>>  that once applied, processes can never relaxe their setting.
>
> Typo: relax
>
>> *) The global sysctl setting can be set by writting an integer value to
>>    '/proc/sys/kernel/modautorestrict/autoload'
>
> I wonder if this should just be named
> /proc/sys/kernel/modules_autoload (to look more like
> modules_disabled)?

Indeed that's better, noted.


>>  The valid values are:
>>
>>  0 - Classic module auto-load permissions, nothing changes.
>>
>>  1 - Processes must have CAP_SYS_MODULE to be able to auto-load modules.
>>      CAP_NET_ADMIN should allow to auto-load modules with a 'netdev-%s'
>>      alias.
>>
>>  2 - Processes can not auto-load modules. Once set, this sysctl value
>>      can not be changed.
>>
>> *) Access rules:
>>    First the prctl() settings are checked, if the access is not denied
>>    then the global sysctl settings are checked.
>>
>> The original idea and inspiration is from grsecurity 'GRKERNSEC_MODHARDEN'.
>>
>>
>> The sample code here can be used to test the feature:
>> https://gist.githubusercontent.com/tixxdz/39a583358f04d40b4d3e5571f95c075b/raw/7fb416285412891e2637fba149da930fe0898356/modautorestrict_test.c
>>
>> # TODO list:
>> *) Confirm the struct task_struct->security stacking mechanism.
>
> If we can't settle on a way to do this, perhaps this LSM should just
> live in the core kernel, similar to modules_disabled? We do already
> have the hooks to implement it with an LSM, but I'd hate to block this
> feature just because we can't solve task_struct sharing.
>
> Maybe add a new bit like no_new_privs? (Though I guess you'd need 2
> bits, so maybe a full field like no_new_privs used to be.)

I can go with that too. The feature is really useful! and after
thinking about it, it could also be in task and continue to use LSM
hooks, but implemented more like the capability security module, this
way it is more of a core kernel feature than a proper LSM on its own.


>> *) Add a logging mechanism.
>> *) Remove the use of security_kernel_read_file hook. Use only
>>    security_kernel_module_request and make sure that we cover all cases.
>> *) Convert documentation to .rst
>
> Another TODO item could be to add output to /proc/$pid/status as done
> for seccomp and no_new_privs? Though perhaps only if this is a core
> kernel change...

Ok, noted.

Thank you for the review!

-- 
tixxdz

Re: [kernel-hardening] Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Djalal Harouni]

On Tue, Apr 11, 2017 at 9:54 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/10/2017 9:43 PM, Kees Cook wrote:
>> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> I think that would be the prudent approach. There is still
>>>> the possibility that blob sharing (or full stacking, if you
>>>> prefer) won't be accepted any time soon.
>>> Ok Casey! I will wait for more feedback, and if other maintainers do
>>> not object, I will convert it back to rhashtables in next iterations
>>> making sure that it should be simple to convert later to a blob
>>> sharing mechanism.
>> Would it be possible just to add a single field to task_struct if this
>> LSM is built in? I feel like rhashtables is a huge overhead when a
>> single field is all that's needed.
>
> Special casing the task_struct based on which modules
> are compiled in would work, but I'm under the impression
> that there's a strong desire to keep to one pointer for
> security module information in the major structures.
>
> The code for generalizing shared blobs isn't that hard,
> and y'all have seen it many times. It would be perfectly
> safe to convert the task, cred, inode and such blobs to
> be infrastructure managed right now. That wouldn't mean
> that all the stacking issues (e.g. audit and networking)
> would be addressed, or that all combinations of modules
> would work (i.e. no SELinux+Smack) but it would clear
> the way for this case. And Yama could use a blob if it
> wanted to.

I've been thinking about this again, so my patches did not convert
creds, inodes etc, I don't have a use case for them now. My use case
was for task->security and I re-used the simple approach. I can't
offer a solution for other blobs since I'm not familiar with their
context nor the different use cases. I checked TOMOYO and that was
easy, but I can't check all of them. I agreed that I may use
rhashtables but as Kees pointed out this may introduce overheads and
extra memory, where the task->security for this ModAutoProtect LSM
will only require an extra sizeof(unsigned long) per-task. Also again
the problem is not in this proposed Module, the problem is in modules
that can't be stacked together.

I am bringing this, since maybe after we manage to merge this, and if
Kees agree I may send another set of patches for Yama to enable the
same per-task context, this enables containers but also allows later
in systemd-logind sessions to set it where all inferiors inside the
user sessions are protected by default, not only some apps or special
desktops but for all. So I will hit the same problem again where to
put it? You said that the code that generalize the blobs isn't that
hard, but also in a previous response that it may not be accepted...
so I will try to converge the task->security blob to be more like the
infrastructure that you are proposing, then resubmit and maybe we will
enable these modules that are stackable by default.

Is this reasonable ?


Thanks!

-- 
tixxdz

Re: [kernel-hardening] Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Djalal Harouni]

On Tue, Apr 11, 2017 at 6:43 AM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> I think that would be the prudent approach. There is still
>>> the possibility that blob sharing (or full stacking, if you
>>> prefer) won't be accepted any time soon.
>>
>> Ok Casey! I will wait for more feedback, and if other maintainers do
>> not object, I will convert it back to rhashtables in next iterations
>> making sure that it should be simple to convert later to a blob
>> sharing mechanism.
>
> Would it be possible just to add a single field to task_struct if this
> LSM is built in? I feel like rhashtables is a huge overhead when a
> single field is all that's needed.

Well, yes rhashtables can have an overhead especially when reclaiming
memory back, I could not identify a way how to separate tables unless
we use cgroups as an ID. Anyway this of course could be added in
task_struct and updated to work like the capability security hooks
rather than a proper LSM with its own name. But as noted in the other
response, we may need task->security field for Yama anyway. I'm open
to suggestion ? I may try to converge the task->security blob with
what Casey is proposing and see! otherwise fallback to task_struct as
a last resort!

Thanks!

-- 
tixxdz

Re: [kernel-hardening] Re: [PATCH RFC v2 1/3] LSM: Allow per LSM module per "struct task_struct" blob.

[Casey Schaufler]

On 4/12/2017 9:22 AM, Djalal Harouni wrote:
> On Tue, Apr 11, 2017 at 6:43 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote:
>>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> I think that would be the prudent approach. There is still
>>>> the possibility that blob sharing (or full stacking, if you
>>>> prefer) won't be accepted any time soon.
>>> Ok Casey! I will wait for more feedback, and if other maintainers do
>>> not object, I will convert it back to rhashtables in next iterations
>>> making sure that it should be simple to convert later to a blob
>>> sharing mechanism.
>> Would it be possible just to add a single field to task_struct if this
>> LSM is built in? I feel like rhashtables is a huge overhead when a
>> single field is all that's needed.
> Well, yes rhashtables can have an overhead especially when reclaiming
> memory back, I could not identify a way how to separate tables unless
> we use cgroups as an ID. Anyway this of course could be added in
> task_struct and updated to work like the capability security hooks
> rather than a proper LSM with its own name. But as noted in the other
> response, we may need task->security field for Yama anyway. I'm open
> to suggestion ? I may try to converge the task->security blob with
> what Casey is proposing and see! otherwise fallback to task_struct as
> a last resort!
>
> Thanks!

I can present a patch set based on my existing stacking
work that includes the move from module based memory
management to infrastructure memory management but pretty
well stops there. There will be changes to Kconfig to allow
stacking of everything except SELinux and Smack, because
that's the only combination with other conficts. Well,
there's /proc/attr, but I'd include the module specific
subdirectories, too. That would allow landlock to come in
and TOMOYO (and potentially YAMA) to use/share the task
blob. I see this as taking down a barrier rather than
otherwise pointless infrastructure change.