[RFC/PATCH 0/3] introduce Timgad LSM

[RFC/PATCH 0/3] introduce Timgad LSM

[Djalal Harouni]

From: Djalal Harouni <tixxdz@gmail.com>

Hi list,

This RFC introduces Timgad a Linux Security Module that adds restrictions
on module load and unload operations. The original idea and inspiration is
from grsecurity 'GRKERNSEC_MODHARDEN'. However this was adapted to fit
more as an LSM and also to fit today's Linux containers/embedded use cases.

Originally I had this code as part of Linux Yama module, however after
some discussions with Kees Cook mainainer of Yama, he proposed that I
should split the code on its own module and send it for discussion.

The module supports a system-wide security protection but also a per
processes/containers one using prctl() interface.

The module is selectable at build-time with CONFIG_SECURITY_TIMGAD, and can be
controlled at run-time through sysctls in /proc/sys/kernel/timgad/ or
prctl() interface. The prctl(2) settings are inherited by children created by
fork(2) and clone(2), and preserved across execve(2).


(The following is not fully implemented, and it still has bugs, as
 this is a preliminary RFC, the permission details, and sure other
 things have to be discussed before we proceed further, any positive
 feedback is welcome).


*) The per-process prctl() settings are:
    prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)

    Where value means:

0 - Classic module load and unload permissions, nothing changes.

1 - The current process must have CAP_SYS_MODULE to be able to load and
    unload modules. CAP_NET_ADMIN should allow the current process to
    load and unload only netdev aliased modules, not implemented

2 - Current process can not loaded nor unloaded modules.


*) The sysctl settings (writable only with CAP_SYS_MODULE) are:
        /proc/sys/kernel/timgad/module_restrict

0 - Classic module load and unload permissions, nothing changes.

1 - Only processes with CAP_SYS_MODULE should be able to load and
    unload modules. Processes with CAP_NET_ADMIN should be able to
    load and unload only netdev aliased modules, this is
    currently not implemented...

    (This should be improved)

2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
    cannot be changed.

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


Patches tested against 4.10-rc6 (next didn't boot for me).

The sample code here can be used to test the feature:
https://gist.github.com/tixxdz/f7d1eadd4728b6aec00b8c2438411b15
https://gist.githubusercontent.com/tixxdz/f7d1eadd4728b6aec00b8c2438411b15/raw/07452c73ea23b930d5a2ab9578ac53bf406a91a2/timgad_test.c


Patches:
  [1/3] security: add the security_task_copy() hook
  [2/3] security: Add the Timgad module
  [3/3] doc: add Timgad LSM documentation

TODO list:
*) Change the name of the module ?

*) Benchmark and improve rhash table.

*) Support unload and module_finit logic

*) Separate rhash tables by first task that sets the flags, childs go in.
   Then if we have multiple tables schedule next one on the global
   default one. This helps containers case and avoids stretches.

*) Make sure that task_struct are not re-used between put_ref() and
   the free work.

-- 
2.5.5

[RFC/PATCH 1/3] security: add the security_task_copy() hook

[Djalal Harouni]

From: Djalal Harouni <tixxdz@gmail.com>

This hook is needed by Timgad LSM. Timgad uses a sysctl and a
per-process prctl() interface to control which processes are allowed to
load and unload modules. However to achieve that we need new hooks to
save the context of tasks.

Sadly there is no way, if we want to add new LSMs we are faced with two
choices:

1) Either fight and collide we other major LSMs since all of them
   they do save their context on the same shared security structs.

2) If we want our module to be a minor stackable LSM, that can be used
   easily without any pain, we have to find a new way to store our context.

Our use case is more in the seccomp logic, we want to add or fix a gap
that seccomp can not handle. At the same time we want to keep the
interface as easy as possible for userspace sandboxing. After some
years it seems like a good easy security feature for userspace should
apply at the same time when we apply: seccomp filters and the
no_new_privs flag.

To achieve the above we add the security_task_copy() hook that allows us
to clone the Timgad context of parent into child task_struct.

The security hook can also be used by new LSMs after the child task has
done some initialization, this way they won't clash with the major LSMs.
The situation is not really well, this hook allows us to introduce a
stackable LSM that can be easily used with all other LSMs.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
---
 include/linux/lsm_hooks.h | 2 ++
 include/linux/security.h  | 6 ++++++
 kernel/fork.c             | 4 ++++
 security/security.c       | 6 ++++++
 4 files changed, 18 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 558adfa..b37e35e 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1479,6 +1479,7 @@ union security_list_options {
 	int (*file_open)(struct file *file, const struct cred *cred);
 
 	int (*task_create)(unsigned long clone_flags);
+	int (*task_copy)(struct task_struct *task);
 	void (*task_free)(struct task_struct *task);
 	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
 	void (*cred_free)(struct cred *cred);
@@ -1745,6 +1746,7 @@ struct security_hook_heads {
 	struct list_head file_receive;
 	struct list_head file_open;
 	struct list_head task_create;
+	struct list_head task_copy;
 	struct list_head task_free;
 	struct list_head cred_alloc_blank;
 	struct list_head cred_free;
diff --git a/include/linux/security.h b/include/linux/security.h
index c2125e9..748058e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -305,6 +305,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
 int security_file_receive(struct file *file);
 int security_file_open(struct file *file, const struct cred *cred);
 int security_task_create(unsigned long clone_flags);
+int security_task_copy(struct task_struct *task);
 void security_task_free(struct task_struct *task);
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
 void security_cred_free(struct cred *cred);
@@ -857,6 +858,11 @@ static inline int security_task_create(unsigned long clone_flags)
 	return 0;
 }
 
+static inline int security_task_copy(struct task_struct *task)
+{
+	return 0;
+}
+
 static inline void security_task_free(struct task_struct *task)
 { }
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 11c5c8a..81c29d8 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1746,6 +1746,10 @@ static __latent_entropy struct task_struct *copy_process(
 	if (retval)
 		goto bad_fork_free_pid;
 
+	retval = security_task_copy(p);
+	if (retval)
+		goto bad_fork_cancel_cgroup;
+
 	/*
 	 * Make it visible to the rest of the system, but dont wake it up yet.
 	 * Need tasklist lock for parent etc handling!
diff --git a/security/security.c b/security/security.c
index f825304..5c699c8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -892,6 +892,11 @@ int security_task_create(unsigned long clone_flags)
 	return call_int_hook(task_create, 0, clone_flags);
 }
 
+int security_task_copy(struct task_struct *task)
+{
+	return call_int_hook(task_copy, 0, task);
+}
+
 void security_task_free(struct task_struct *task)
 {
 	call_void_hook(task_free, task);
@@ -1731,6 +1736,7 @@ struct security_hook_heads security_hook_heads = {
 	.file_receive =	LIST_HEAD_INIT(security_hook_heads.file_receive),
 	.file_open =	LIST_HEAD_INIT(security_hook_heads.file_open),
 	.task_create =	LIST_HEAD_INIT(security_hook_heads.task_create),
+	.task_copy =    LIST_HEAD_INIT(security_hook_heads.task_copy),
 	.task_free =	LIST_HEAD_INIT(security_hook_heads.task_free),
 	.cred_alloc_blank =
 		LIST_HEAD_INIT(security_hook_heads.cred_alloc_blank),
-- 
2.5.5

[RFC/PATCH 2/3] security: Add the Timgad module

[Djalal Harouni]

From: Djalal Harouni <tixxdz@gmail.com>

This adds the Timgad module. Timgad allows to apply restrictions on
which task is allowed to load or unload kernel modules. Auto-load module
feature is also handled. The settings can also be applied globally using
a sysctl interface, this allows to complete the core kernel interface
"modules_disable" which has only two modes: allow globally or deny
globally.

The feature is useful for sandboxing, embedded systems and Linux
containers where only some containers/processes that have the
right privileges are allowed to load/unload modules. Unprivileged
processes should not be able to load/unload modules nor trigger the
module auto-load feature. This behaviour was inspired from grsecurity's
GRKERNSEC_MODHARDEN option.

However I still did not complete the check since this has to be
discussed first, so any bug here is not from grsecurity, but my bugs and
on purpose. As this is a preliminary RFC these points are not settled,
discussion has to happen on what should be the best behaviour and what
checks should be in place. Currently the settings:

Timgad module can be controled using a global sysctl setting:
   /proc/sys/kernel/timgad/module_restrict

Or using the prctl() interface:
   prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)

*) The per-process prctl() settings are:
    prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)

    Where value means:

0 - Classic module load and unload permissions, nothing changes.

1 - The current process must have CAP_SYS_MODULE to be able to load and
    unload modules. CAP_NET_ADMIN should allow the current process to
    load and unload only netdev aliased modules, not implemented

2 - Current process can not loaded nor unloaded modules.

*) sysctl interface supports the followin values:

0 - Classic module load and unload permissions, nothing changes.

1 - Only privileged processes with CAP_SYS_MODULE should be able to load and
    unload modules.

    To be added: processes with CAP_NET_ADMIN should be able to
    load and unload only netdev aliased modules, this is currently not
    supported. Other checks for real root without CAP_SYS_MODULE ? ...

    (This should be improved)

2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
    cannot be changed.

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

As said I will update the permission checks later, this is a preliminary
RFC.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
---
 include/linux/lsm_hooks.h     |   5 +
 include/uapi/linux/prctl.h    |   5 +
 security/Kconfig              |   1 +
 security/Makefile             |   2 +
 security/security.c           |   1 +
 security/timgad/Kconfig       |  10 ++
 security/timgad/Makefile      |   3 +
 security/timgad/timgad_core.c | 306 +++++++++++++++++++++++++++++++++++++++
 security/timgad/timgad_core.h |  53 +++++++
 security/timgad/timgad_lsm.c  | 327 ++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 713 insertions(+)
 create mode 100644 security/timgad/Kconfig
 create mode 100644 security/timgad/Makefile
 create mode 100644 security/timgad/timgad_core.c
 create mode 100644 security/timgad/timgad_core.h
 create mode 100644 security/timgad/timgad_lsm.c

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index b37e35e..6b83aaa 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1935,5 +1935,10 @@ void __init loadpin_add_hooks(void);
 #else
 static inline void loadpin_add_hooks(void) { };
 #endif
+#ifdef CONFIG_SECURITY_TIMGAD
+extern void __init timgad_add_hooks(void);
+#else
+static inline void __init timgad_add_hooks(void) { }
+#endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
index a8d0759..6d80eed 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 Timgad LSM options */
+#define PR_TIMGAD_OPTS			48
+# define PR_TIMGAD_SET_MOD_RESTRICT	1
+# define PR_TIMGAD_GET_MOD_RESTRICT	2
+
 #endif /* _LINUX_PRCTL_H */
diff --git a/security/Kconfig b/security/Kconfig
index 118f454..e6d6128 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -164,6 +164,7 @@ source security/tomoyo/Kconfig
 source security/apparmor/Kconfig
 source security/loadpin/Kconfig
 source security/yama/Kconfig
+source security/timgad/Kconfig
 
 source security/integrity/Kconfig
 
diff --git a/security/Makefile b/security/Makefile
index f2d71cd..6a8127e 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
 subdir-$(CONFIG_SECURITY_APPARMOR)	+= apparmor
 subdir-$(CONFIG_SECURITY_YAMA)		+= yama
 subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
+subdir-$(CONFIG_SECURITY_TIMGAD)	+= timgad
 
 # always enable default capabilities
 obj-y					+= commoncap.o
@@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_APPARMOR)		+= apparmor/
 obj-$(CONFIG_SECURITY_YAMA)		+= yama/
 obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
+obj-$(CONFIG_SECURITY_TIMGAD)		+= timgad/
 
 # Object integrity file lists
 subdir-$(CONFIG_INTEGRITY)		+= integrity
diff --git a/security/security.c b/security/security.c
index 5c699c8..c6c9349 100644
--- a/security/security.c
+++ b/security/security.c
@@ -61,6 +61,7 @@ int __init security_init(void)
 	capability_add_hooks();
 	yama_add_hooks();
 	loadpin_add_hooks();
+	timgad_add_hooks();
 
 	/*
 	 * Load all the remaining security modules.
diff --git a/security/timgad/Kconfig b/security/timgad/Kconfig
new file mode 100644
index 0000000..93ecdb6
--- /dev/null
+++ b/security/timgad/Kconfig
@@ -0,0 +1,10 @@
+config SECURITY_TIMGAD
+	bool "TIMGAD support"
+	depends on SECURITY
+	default n
+	help
+	  This selects TIMGAD, which applies restrictions on load and unload
+          modules operations. Further information can be found in
+          Documentation/security/Timgad.txt.
+
+	  If you are unsure how to answer this question, answer N.
diff --git a/security/timgad/Makefile b/security/timgad/Makefile
new file mode 100644
index 0000000..dca0441
--- /dev/null
+++ b/security/timgad/Makefile
@@ -0,0 +1,3 @@
+obj-$(CONFIG_SECURITY_TIMGAD) := timgad.o
+
+timgad-y := timgad_core.o timgad_lsm.o
diff --git a/security/timgad/timgad_core.c b/security/timgad/timgad_core.c
new file mode 100644
index 0000000..fa35965
--- /dev/null
+++ b/security/timgad/timgad_core.c
@@ -0,0 +1,306 @@
+/*
+ * Timgad 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/ctype.h>
+#include <linux/errno.h>
+#include <linux/list.h>
+#include <linux/prctl.h>
+#include <linux/rhashtable.h>
+#include <linux/sched.h>
+#include <linux/security.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+enum {
+	TIMGAD_TASK_INITIALIZED = 0,	/* Initialized, not active */
+	TIMGAD_TASK_ACTIVE = 1,		/* Linked in hash, active */
+	TIMGAD_TASK_INVALID = -1,	/* Scheduled to be removed from hash */
+};
+
+struct timgad_task_map {
+	unsigned long key_addr;
+};
+
+struct timgad_task {
+	struct rhash_head t_rhash_head;	/* timgad task hash node */
+	unsigned long key;
+
+	atomic_t usage;
+	u32 flags;
+
+	struct work_struct clean_work;
+};
+
+static struct rhashtable timgad_tasks_table;
+static DEFINE_SPINLOCK(timgad_tasks_lock);
+
+static inline int _cmp_timgad_task(struct rhashtable_compare_arg *arg,
+				   const void *obj)
+{
+	const struct timgad_task_map *tmap = arg->key;
+	const struct timgad_task *ttask = obj;
+
+	if (ttask->key != tmap->key_addr)
+		return 1;
+
+	/* Did we hit an entry that was invalidated ? */
+	if (atomic_read(&ttask->usage) == TIMGAD_TASK_INVALID)
+		return 1;
+
+	return 0;
+}
+
+/* TODO: optimize me */
+static const struct rhashtable_params timgad_tasks_hash_params = {
+	.nelem_hint = 512,
+	.head_offset = offsetof(struct timgad_task, t_rhash_head),
+	.key_offset = offsetof(struct timgad_task, key),
+	.key_len = sizeof(unsigned long),
+	.max_size = 8192,
+	.min_size = 256,
+	.obj_cmpfn = _cmp_timgad_task,
+	.automatic_shrinking = true,
+};
+
+int timgad_tasks_init(void)
+{
+	return rhashtable_init(&timgad_tasks_table, &timgad_tasks_hash_params);
+}
+
+void timgad_tasks_clean(void)
+{
+	rhashtable_destroy(&timgad_tasks_table);
+}
+
+unsigned long read_timgad_task_flags(struct timgad_task *timgad_tsk)
+{
+	return timgad_tsk->flags;
+}
+
+static inline int new_timgad_task_flags(unsigned long op,
+					unsigned long used_flag,
+					unsigned long passed_flag,
+					unsigned long *new_flag)
+{
+	if (passed_flag < used_flag)
+		return -EPERM;
+
+	*new_flag = passed_flag;
+	return 0;
+}
+
+static inline int update_timgad_task_flags(struct timgad_task *timgad_tsk,
+					   unsigned long op,
+					   unsigned long new_flag)
+{
+	if (op != PR_TIMGAD_SET_MOD_RESTRICT)
+		return -EINVAL;
+
+	timgad_tsk->flags = new_flag;
+	return 0;
+}
+
+int is_timgad_task_op_set(struct timgad_task *timgad_tsk, unsigned long op,
+			  unsigned long *flag)
+{
+	if (op != PR_TIMGAD_SET_MOD_RESTRICT)
+		return -EINVAL;
+
+	*flag = timgad_tsk->flags;
+	return 0;
+}
+
+int timgad_task_set_op_flag(struct timgad_task *timgad_tsk, unsigned long op,
+			    unsigned long flag, unsigned long value)
+{
+	int ret;
+	unsigned long new_flag = 0;
+	unsigned long used_flag;
+
+	ret = is_timgad_task_op_set(timgad_tsk, op, &used_flag);
+	if (ret < 0)
+		return ret;
+
+	ret = new_timgad_task_flags(op, used_flag, flag, &new_flag);
+	if (ret < 0)
+		return ret;
+
+	/* Nothing to do if the flag did not change */
+	if (new_flag == used_flag)
+		return 0;
+
+	return update_timgad_task_flags(timgad_tsk, op, new_flag);
+}
+
+static struct timgad_task *__lookup_timgad_task(struct task_struct *tsk)
+{
+	struct timgad_task_map tmap = { .key_addr = (unsigned long)(uintptr_t)tsk };
+
+	return rhashtable_lookup_fast(&timgad_tasks_table, &tmap,
+				      timgad_tasks_hash_params);
+}
+
+static inline struct timgad_task *__get_timgad_task(struct timgad_task *timgad_tsk)
+{
+	if (atomic_inc_not_zero(&timgad_tsk->usage))
+		return timgad_tsk;
+
+	return NULL;
+}
+
+static inline void __put_timgad_task(struct timgad_task *timgad_tsk,
+				     bool *collect)
+{
+	/* First check if we have not been interrupted */
+	if (atomic_read(&timgad_tsk->usage) <= TIMGAD_TASK_INITIALIZED ||
+	    atomic_dec_and_test(&timgad_tsk->usage)) {
+		if (collect)
+			*collect = true;
+		/*
+		 * Invalidate entry as early as possible so we
+		 * do not collide
+		 */
+		atomic_set(&timgad_tsk->usage, TIMGAD_TASK_INVALID);
+	}
+}
+
+/* We do take a reference count */
+struct timgad_task *get_timgad_task(struct task_struct *tsk)
+{
+	struct timgad_task *ttask;
+
+	rcu_read_lock();
+	ttask = __lookup_timgad_task(tsk);
+	if (ttask)
+		ttask = __get_timgad_task(ttask);
+	rcu_read_unlock();
+
+	return ttask;
+}
+
+void put_timgad_task(struct timgad_task *timgad_tsk, bool *collect)
+{
+	if (timgad_tsk)
+		__put_timgad_task(timgad_tsk, collect);
+}
+
+/*
+ * We return all timgad tasks that are not in the TIMGAD_TASK_INVALID state.
+ * We do not take reference count on timgad tasks here
+ */
+struct timgad_task *lookup_timgad_task(struct task_struct *tsk)
+{
+	struct timgad_task *ttask;
+
+	rcu_read_lock();
+	ttask = __lookup_timgad_task(tsk);
+	rcu_read_unlock();
+
+	return ttask;
+}
+
+static int insert_timgad_task(struct timgad_task *timgad_tsk)
+{
+	int ret;
+	struct timgad_task *ttask = timgad_tsk;
+
+	/* TODO: improve me */
+	if (unlikely(atomic_read(&timgad_tasks_table.nelems) >= INT_MAX))
+		return -ENOMEM;
+
+	atomic_set(&ttask->usage, TIMGAD_TASK_ACTIVE);
+	spin_lock(&timgad_tasks_lock);
+	ret = rhashtable_insert_fast(&timgad_tasks_table,
+				     &timgad_tsk->t_rhash_head,
+				     timgad_tasks_hash_params);
+	spin_unlock(&timgad_tasks_lock);
+	if (ret != 0 && ret != -EEXIST) {
+		atomic_set(&ttask->usage, TIMGAD_TASK_INITIALIZED);
+		return ret;
+	}
+
+	return 0;
+}
+
+void release_timgad_task(struct task_struct *tsk)
+{
+	struct timgad_task *ttask;
+	bool reclaim = false;
+
+	rcu_read_lock();
+	/* We do not take a ref count here */
+	ttask = __lookup_timgad_task(tsk);
+	if (ttask)
+		put_timgad_task(ttask, &reclaim);
+	rcu_read_unlock();
+
+	if (reclaim)
+		schedule_work(&ttask->clean_work);
+}
+
+static void reclaim_timgad_task(struct work_struct *work)
+{
+	struct timgad_task *ttask = container_of(work, struct timgad_task,
+						 clean_work);
+
+	WARN_ON(atomic_read(&ttask->usage) != TIMGAD_TASK_INVALID);
+
+	spin_lock(&timgad_tasks_lock);
+	rhashtable_remove_fast(&timgad_tasks_table, &ttask->t_rhash_head,
+			       timgad_tasks_hash_params);
+	spin_unlock(&timgad_tasks_lock);
+
+	kfree(ttask);
+}
+
+struct timgad_task *init_timgad_task(struct task_struct *tsk,
+				     unsigned long value)
+{
+	struct timgad_task *ttask;
+
+	ttask = kzalloc(sizeof(*ttask), GFP_KERNEL | __GFP_NOWARN);
+	if (ttask == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	ttask->key = (unsigned long)(uintptr_t)tsk;
+	ttask->flags = value;
+
+	atomic_set(&ttask->usage, TIMGAD_TASK_INITIALIZED);
+	INIT_WORK(&ttask->clean_work, reclaim_timgad_task);
+
+	return ttask;
+}
+
+/* On success, callers have to do put_timgad_task() */
+struct timgad_task *give_me_timgad_task(struct task_struct *tsk,
+					unsigned long value)
+{
+	int ret;
+	struct timgad_task *ttask;
+
+	ttask = init_timgad_task(tsk, value);
+	if (IS_ERR(ttask))
+		return ttask;
+
+	/* Mark it as active */
+	ret = insert_timgad_task(ttask);
+	if (ret) {
+		kfree(ttask);
+		return ERR_PTR(ret);
+	}
+
+	return ttask;
+}
diff --git a/security/timgad/timgad_core.h b/security/timgad/timgad_core.h
new file mode 100644
index 0000000..4096c39
--- /dev/null
+++ b/security/timgad/timgad_core.h
@@ -0,0 +1,53 @@
+/*
+ * Timgad 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.
+ *
+ */
+
+#define TIMGAD_MODULE_OFF	0x00000000
+#define TIMGAD_MODULE_STRICT	0x00000001
+#define TIMGAD_MODULE_NO_LOAD	0x00000002
+
+struct timgad_task;
+
+static inline int timgad_op_to_flag(unsigned long op,
+				    unsigned long value,
+				    unsigned long *flag)
+{
+	if (op != PR_TIMGAD_SET_MOD_RESTRICT || value > TIMGAD_MODULE_NO_LOAD)
+		return -EINVAL;
+
+	*flag = value;
+	return 0;
+}
+
+unsigned long read_timgad_task_flags(struct timgad_task *timgad_tsk);
+
+int timgad_task_set_op_flag(struct timgad_task *timgad_tsk,
+			    unsigned long op, unsigned long flag,
+			    unsigned long value);
+
+int is_timgad_task_op_set(struct timgad_task *timgad_tsk, unsigned long op,
+			  unsigned long *flag);
+
+struct timgad_task *get_timgad_task(struct task_struct *tsk);
+void put_timgad_task(struct timgad_task *timgad_tsk, bool *collect);
+struct timgad_task *lookup_timgad_task(struct task_struct *tsk);
+
+void release_timgad_task(struct task_struct *tsk);
+
+struct timgad_task *init_timgad_task(struct task_struct *tsk,
+				     unsigned long flag);
+struct timgad_task *give_me_timgad_task(struct task_struct *tsk,
+					unsigned long value);
+
+int timgad_tasks_init(void);
+void timgad_tasks_clean(void);
diff --git a/security/timgad/timgad_lsm.c b/security/timgad/timgad_lsm.c
new file mode 100644
index 0000000..809b7cf
--- /dev/null
+++ b/security/timgad/timgad_lsm.c
@@ -0,0 +1,327 @@
+/*
+ * Timgad 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/sysctl.h>
+#include <linux/prctl.h>
+#include <linux/types.h>
+
+#include "timgad_core.h"
+
+static int module_restrict;
+
+static int zero;
+static int max_module_restrict_scope = TIMGAD_MODULE_NO_LOAD;
+
+/* TODO:
+ *  complete permission check
+ *  inline function logic with the per-process if possible
+ */
+static int timgad_has_global_sysctl_perm(unsigned long op)
+{
+	int ret = -EINVAL;
+	struct mm_struct *mm = NULL;
+
+	if (op != PR_TIMGAD_GET_MOD_RESTRICT)
+		return ret;
+
+	switch (module_restrict) {
+	case TIMGAD_MODULE_OFF:
+		ret = 0;
+		break;
+	/* TODO: complete this and handle it later per task too */
+	case TIMGAD_MODULE_STRICT:
+		/*
+		 * Are we allowed to sleep here ?
+		 * Also improve this check here
+		 */
+		ret = -EPERM;
+		mm = get_task_mm(current);
+		if (mm) {
+			if (capable(CAP_SYS_MODULE))
+				ret = 0;
+			mmput(mm);
+		}
+		break;
+	case TIMGAD_MODULE_NO_LOAD:
+		ret = -EPERM;
+		break;
+	}
+
+	return ret;
+}
+
+/* TODO: simplify me and move me to timgad_core.c file */
+static int module_timgad_task_perm(struct timgad_task *timgad_tsk,
+				   char *kmod_name)
+{
+	int ret;
+	unsigned long flag = 0;
+
+	ret = is_timgad_task_op_set(timgad_tsk,
+				    PR_TIMGAD_SET_MOD_RESTRICT, &flag);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * TODO: complete me
+	 *    * Allow net modules only with CAP_NET_ADMIN and other cases...
+	 *    * Other exotic cases when set to STRICT should be denied...
+	 *    * Inline logic
+	 */
+	switch (flag) {
+	case TIMGAD_MODULE_OFF:
+		ret = 0;
+		break;
+	case TIMGAD_MODULE_STRICT:
+		if (!capable(CAP_SYS_MODULE))
+			ret = -EPERM;
+		else
+			ret = 0;
+		break;
+	case TIMGAD_MODULE_NO_LOAD:
+		ret = -EPERM;
+		break;
+	}
+
+	return ret;
+}
+
+/* Set the given option in a timgad task */
+static int timgad_set_op_value(struct task_struct *tsk,
+			       unsigned long op, unsigned long value)
+{
+	int ret = 0;
+	struct timgad_task *ttask;
+	unsigned long flag = 0;
+
+	ret = timgad_op_to_flag(op, value, &flag);
+	if (ret < 0)
+		return ret;
+
+	ttask = get_timgad_task(tsk);
+	if (!ttask) {
+		ttask = give_me_timgad_task(tsk, value);
+		if (IS_ERR(ttask))
+			return PTR_ERR(ttask);
+
+		return 0;
+	}
+
+	ret = timgad_task_set_op_flag(ttask, op, flag, value);
+
+	put_timgad_task(ttask, NULL);
+	return ret;
+}
+
+/* Get the given option from a timgad task */
+static int timgad_get_op_value(struct task_struct *tsk, unsigned long op)
+{
+	int ret = -EINVAL;
+	struct timgad_task *ttask;
+	unsigned long flag = 0;
+
+	ttask = get_timgad_task(tsk);
+	if (!ttask)
+		return ret;
+
+	ret = is_timgad_task_op_set(ttask, op, &flag);
+	put_timgad_task(ttask, NULL);
+
+	return ret < 0 ? ret : flag;
+}
+
+/* Copy Timgad context from parent to child */
+int timgad_task_copy(struct task_struct *tsk)
+{
+	int ret = 0;
+	struct timgad_task *tparent;
+	struct timgad_task *ttask;
+	unsigned long value = 0;
+
+	tparent = get_timgad_task(current);
+
+	/* Parent does not have a timgad context, nothing to do */
+	if (tparent == NULL)
+		return 0;
+
+	value = read_timgad_task_flags(tparent);
+
+	ttask = give_me_timgad_task(tsk, value);
+	if (IS_ERR(ttask))
+		ret = PTR_ERR(ttask);
+	else
+		ret = 0;
+
+	put_timgad_task(tparent, NULL);
+	return ret;
+}
+
+/*
+ * Return 0 on success, -error on error.  -EINVAL is returned when Timgad
+ * does not handle the given option.
+ */
+int timgad_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_TIMGAD_OPTS)
+		return 0;
+
+	get_task_struct(myself);
+
+	switch (arg2) {
+	case PR_TIMGAD_SET_MOD_RESTRICT:
+		ret = timgad_set_op_value(myself,
+					  PR_TIMGAD_SET_MOD_RESTRICT,
+					  arg3);
+		break;
+	case PR_TIMGAD_GET_MOD_RESTRICT:
+		ret = timgad_get_op_value(myself,
+					  PR_TIMGAD_SET_MOD_RESTRICT);
+		break;
+	}
+
+	put_task_struct(myself);
+	return ret;
+}
+
+/*
+ * Free the specific task attached resources
+ * task_free() can be called from interrupt context
+ */
+void timgad_task_free(struct task_struct *tsk)
+{
+	release_timgad_task(tsk);
+}
+
+static int timgad_kernel_module_file(struct file *file)
+{
+	int ret = 0;
+	struct timgad_task *ttask;
+	struct task_struct *myself = current;
+
+	/* First check if the task allows that */
+	ttask = get_timgad_task(myself);
+	if (ttask != NULL) {
+		ret = module_timgad_task_perm(ttask, NULL);
+		put_timgad_task(ttask, NULL);
+	}
+
+	if (ret < 0)
+		return ret;
+
+	return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
+}
+
+static int timgad_kernel_module_request(char *kmod_name)
+{
+	int ret = 0;
+	struct timgad_task *ttask;
+	struct task_struct *myself = current;
+
+	/* First check if the task allows that */
+	ttask = get_timgad_task(myself);
+	if (ttask != NULL) {
+		ret = module_timgad_task_perm(ttask, kmod_name);
+		put_timgad_task(ttask, NULL);
+	}
+
+	if (ret < 0)
+		return ret;
+
+	return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
+}
+
+static int timgad_kernel_read_file(struct file *file,
+				   enum kernel_read_file_id id)
+{
+	int ret = 0;
+
+	switch (id) {
+	case READING_MODULE:
+		ret = timgad_kernel_module_file(file);
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static struct security_hook_list timgad_hooks[] = {
+	LSM_HOOK_INIT(kernel_module_request, timgad_kernel_module_request),
+	LSM_HOOK_INIT(kernel_read_file, timgad_kernel_read_file),
+	LSM_HOOK_INIT(task_copy, timgad_task_copy),
+	LSM_HOOK_INIT(task_prctl, timgad_task_prctl),
+	LSM_HOOK_INIT(task_free, timgad_task_free),
+};
+
+#ifdef CONFIG_SYSCTL
+static int timgad_mod_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 timgad_sysctl_path[] = {
+	{ .procname = "kernel", },
+	{ .procname = "timgad", },
+	{ }
+};
+
+static struct ctl_table timgad_sysctl_table[] = {
+	{
+		.procname       = "module_restrict",
+		.data           = &module_restrict,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = timgad_mod_dointvec_minmax,
+		.extra1         = &zero,
+		.extra2         = &max_module_restrict_scope,
+	},
+	{ }
+};
+
+static void __init timgad_init_sysctl(void)
+{
+	if (!register_sysctl_paths(timgad_sysctl_path, timgad_sysctl_table))
+		panic("Timgad: sysctl registration failed.\n");
+}
+#else
+static inline void timgad_init_sysctl(void) { }
+#endif /* CONFIG_SYSCTL */
+
+void __init timgad_add_hooks(void)
+{
+	pr_info("Timgad: becoming mindful.\n");
+	security_add_hooks(timgad_hooks, ARRAY_SIZE(timgad_hooks));
+	timgad_init_sysctl();
+
+	if (timgad_tasks_init())
+		panic("Timgad: tasks initialization failed.\n");
+}
-- 
2.5.5

[RFC/PATCH 3/3] doc: add Timgad LSM documentation

[Djalal Harouni]

From: Djalal Harouni <tixxdz@gmail.com>

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
---
 Documentation/security/Timgad.txt | 61 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/security/Timgad.txt

diff --git a/Documentation/security/Timgad.txt b/Documentation/security/Timgad.txt
new file mode 100644
index 0000000..1ae1e7c
--- /dev/null
+++ b/Documentation/security/Timgad.txt
@@ -0,0 +1,61 @@
+Timgad is a Linux Security Module that collects per process and
+system-wide security protections that are not handled by the core kernel
+itself.
+
+This is selectable at build-time with CONFIG_SECURITY_TIMGAD, and can be
+controlled at run-time through sysctls in /proc/sys/kernel/timgad:
+or prctl() interface.
+
+
+- module_restrict
+
+==============================================================
+
+Linux containers need robust settings to control if modules are allowed to
+be loaded or unloaded globally or per process/container policy.
+Automatic loading of kernel modules using the "auto-load" feature is also
+covered.
+
+This adds global sysctl settings to indicate if modules are allowed
+to be loaded or unloaded, at same time it also supports a
+per-process/container settings based on prctl(2) interface. The prctl(2)
+settings are inherited by children created by fork(2) and clone(2), and
+preserved across execve(2).
+
+
+*) The per-process prctl() settings are:
+   prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
+
+   Where value means:
+
+0 - Classic module load and unload permissions, nothing changes.
+
+1 - The current process must have CAP_SYS_MODULE to be able to load and
+    unload modules. CAP_NET_ADMIN should allow the current process to
+    load and unload only netdev aliased modules.
+
+2 - Current process can not loaded nor unloaded modules.
+
+
+*) The sysctl settings (writable only with CAP_SYS_MODULE) are:
+   /proc/sys/kernel/timgad/module_restrict
+
+0 - Classic module load and unload permissions, nothing changes.
+
+1 - Only processes with CAP_SYS_MODULE should be able to load and
+    unload modules. Processes with CAP_NET_ADMIN should be able to
+    load and unload only netdev aliased modules.
+
+2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
+    cannot be changed.
+
+
+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.5.5

Re: [RFC/PATCH 2/3] security: Add the Timgad module

[James Morris]

On Thu, 2 Feb 2017, Djalal Harouni wrote:

> *) The per-process prctl() settings are:
>     prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
> 
>     Where value means:
> 
> 0 - Classic module load and unload permissions, nothing changes.
> 
> 1 - The current process must have CAP_SYS_MODULE to be able to load and
>     unload modules. CAP_NET_ADMIN should allow the current process to
>     load and unload only netdev aliased modules, not implemented
> 
> 2 - Current process can not loaded nor unloaded modules.
> 
> *) sysctl interface supports the followin values:
> 
> 0 - Classic module load and unload permissions, nothing changes.
> 
> 1 - Only privileged processes with CAP_SYS_MODULE should be able to load and
>     unload modules.
> 
>     To be added: processes with CAP_NET_ADMIN should be able to
>     load and unload only netdev aliased modules, this is currently not
>     supported. Other checks for real root without CAP_SYS_MODULE ? ...
> 
>     (This should be improved)
> 
> 2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
>     cannot be changed.

How is this different to just using CAP_SYS_MODULE?

-- 
James Morris
<jmorris@namei.org>

Re: [RFC/PATCH 1/3] security: add the security_task_copy() hook

[Tetsuo Handa]

Djalal Harouni wrote:
> To achieve the above we add the security_task_copy() hook that allows us
> to clone the Timgad context of parent into child task_struct.
> 
> The security hook can also be used by new LSMs after the child task has
> done some initialization, this way they won't clash with the major LSMs.
> The situation is not really well, this hook allows us to introduce a
> stackable LSM that can be easily used with all other LSMs.

We are already planning to revive security_task_alloc() hook (probably in Linux 4.12)
( news://news.gmane.org:119/201701101958.JAD43709.OtJSOQFVFOLHMF@I-love.SAKURA.ne.jp ).
Is security_task_alloc() called too early for your case?

(Well, we want to configure http archive like marc.info ?)

Re: [RFC/PATCH 2/3] security: Add the Timgad module

[Djalal Harouni]

Hi James,

On Fri, Feb 3, 2017 at 2:02 AM, James Morris <jmorris@namei.org> wrote:
> On Thu, 2 Feb 2017, Djalal Harouni wrote:
>
>> *) The per-process prctl() settings are:
>>     prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
>>
>>     Where value means:
>>
>> 0 - Classic module load and unload permissions, nothing changes.
>>
>> 1 - The current process must have CAP_SYS_MODULE to be able to load and
>>     unload modules. CAP_NET_ADMIN should allow the current process to
>>     load and unload only netdev aliased modules, not implemented
>>
>> 2 - Current process can not loaded nor unloaded modules.
>>
>> *) sysctl interface supports the followin values:
>>
>> 0 - Classic module load and unload permissions, nothing changes.
>>
>> 1 - Only privileged processes with CAP_SYS_MODULE should be able to load and
>>     unload modules.
>>
>>     To be added: processes with CAP_NET_ADMIN should be able to
>>     load and unload only netdev aliased modules, this is currently not
>>     supported. Other checks for real root without CAP_SYS_MODULE ? ...
>>
>>     (This should be improved)
>>
>> 2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
>>     cannot be changed.
>
> How is this different to just using CAP_SYS_MODULE?

Using only CAP_SYS_MODULE is not sufficient on its own, first we have
the following:

* Some modules can be auto-loaded without any privileges:
  tun, all the tunneling modules, maybe some md drivers, some crypto,
some device drivers... (long list)

* Other network modules require CAP_NET_ADMIN and netdev module prefix.

* Some socket types require CAP_SYS_ADMIN before loading the
corresponding protocol module: phonet sockets...

* Some other operations related to ioctl require CAP_SYS_ADMIN

* Filesystem prefixed modules can be auto-loaded inside user
namespaces, this was discussed in the past, and seems everyone agreed
that there is no harm.

* All the situations where a module requests another module or an
external symbol.

Then comes the rest, manual module loading operations which require
CAP_SYS_MODULE.

Of course all of these features are must have for a usable system.
However as Linux covers lot of use cases, there are situations where
offering users more granularity would be better, restricting the
ability of unprivileged to stack dozens of modules and expose them in
a container/embedded world IMO is worth it. Also CAP_NET_ADMIN is
pretty useful on its own for some services, but at same time maybe we
don't want it to translate to CAP_SYS_MODULE on that node or for that
container.

This is a first RFC to seek feedback, permission checks should be
adapted to cover what everyone think is reasonable. I think that we
definitely need a way to have a usable system/processes but also be
able to restrict *only* a set of processes from auto inserting modules
for various reasons (security, using other net protocols...) We are
already using seccomp, however it does not cover those cases.

Btw do you think it is acceptable to add the security_task_copy() hook
in the first patch ?

The whole approach is based on that hook, otherwise I don't see
another easy way and we have to clash with all other LSMs. This one
make it so easy to stack more minor LSMs.

Thanks!


> --
> James Morris
> <jmorris@namei.org>
>



-- 
tixxdz

Re: [RFC/PATCH 1/3] security: add the security_task_copy() hook

[Djalal Harouni]

Hi Tetsuo,

On Mon, Feb 6, 2017 at 11:49 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Djalal Harouni wrote:
>> To achieve the above we add the security_task_copy() hook that allows us
>> to clone the Timgad context of parent into child task_struct.
>>
>> The security hook can also be used by new LSMs after the child task has
>> done some initialization, this way they won't clash with the major LSMs.
>> The situation is not really well, this hook allows us to introduce a
>> stackable LSM that can be easily used with all other LSMs.
>
> We are already planning to revive security_task_alloc() hook (probably in Linux 4.12)
> ( news://news.gmane.org:119/201701101958.JAD43709.OtJSOQFVFOLHMF@I-love.SAKURA.ne.jp ).
> Is security_task_alloc() called too early for your case?

Hmm, didn't know about it, thank you!

Yes that seems the same, to have a per-task vars or context.

I'm reading http://www.spinics.net/linux/fedora/linux-security-module/msg17004.html

For my specific use case I'm still not sure. I was thinking that since
this is LSM maybe it would be useful to only add one hook after all
the copy_*() functions that also copy the context, maybe another LSM
will find it useful and may want to check the context of namespaces,
fs (copy_namespaces(), copy_fs() ...). I mean this already happened to
me with security_task_create() which is too early, so maybe try to do
it right.

The other thing is cgroups cgroup_can_fork() better do it after which
avoids me to clean up. IMHO the best place is near or after
copy_seccomp() where we hold the lock, especially if there is an LSM
that wants to copy/apply the context on all other threads not only
current. Ultimately if we are copying stuff then maybe it should
happen as late as possible, and if we have to sleep I guess we can use
security_task_create() to allocate ?

What do you think ?

Thanks!

> (Well, we want to configure http archive like marc.info ?)



-- 
tixxdz

Re: [RFC/PATCH 1/3] security: add the security_task_copy() hook

[Djalal Harouni]

On Mon, Feb 6, 2017 at 11:49 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Djalal Harouni wrote:
>> To achieve the above we add the security_task_copy() hook that allows us
>> to clone the Timgad context of parent into child task_struct.
>>
>> The security hook can also be used by new LSMs after the child task has
>> done some initialization, this way they won't clash with the major LSMs.
>> The situation is not really well, this hook allows us to introduce a
>> stackable LSM that can be easily used with all other LSMs.
>
> We are already planning to revive security_task_alloc() hook (probably in Linux 4.12)
> ( news://news.gmane.org:119/201701101958.JAD43709.OtJSOQFVFOLHMF@I-love.SAKURA.ne.jp ).
> Is security_task_alloc() called too early for your case?
>
> (Well, we want to configure http archive like marc.info ?)

I found this marc.info
http://marc.info/?l=linux-security-module&m=129584883703846

Oups from 2011! your email pretty sum up all the approach that I took
:-) , and yes it was not that obvious... so I did it that way where
the data is kept in an external table. Also yes beside TOMOYO I also
need that hook for the various reasons that are listed in that thread.

Thanks!

-- 
tixxdz

Re: [RFC/PATCH 2/3] security: Add the Timgad module

[Kees Cook]

On Thu, Feb 2, 2017 at 9:04 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
> From: Djalal Harouni <tixxdz@gmail.com>
>
> This adds the Timgad module. Timgad allows to apply restrictions on
> which task is allowed to load or unload kernel modules. Auto-load module
> feature is also handled. The settings can also be applied globally using
> a sysctl interface, this allows to complete the core kernel interface
> "modules_disable" which has only two modes: allow globally or deny
> globally.

To bikeshed on the name: since this is a module loading restriction
LSM, perhaps something more descriptive: ModAutoRestrict or something
like that? (Yes, Yama is poorly named, but initially it was going to
be more than just ptrace restrictions...)

> The feature is useful for sandboxing, embedded systems and Linux
> containers where only some containers/processes that have the
> right privileges are allowed to load/unload modules. Unprivileged

I'd be explicit here and discuss _auto_loading of modules. (Otherwise
people quickly get confused about this vs CAP_SYS_MODULE.) You mention
auto-load later, but I think mentioning it first make this easier to
understand.

> processes should not be able to load/unload modules nor trigger the
> module auto-load feature. This behaviour was inspired from grsecurity's
> GRKERNSEC_MODHARDEN option.
>
> However I still did not complete the check since this has to be
> discussed first, so any bug here is not from grsecurity, but my bugs and
> on purpose. As this is a preliminary RFC these points are not settled,
> discussion has to happen on what should be the best behaviour and what
> checks should be in place. Currently the settings:
>
> Timgad module can be controled using a global sysctl setting:

typo: controlled

>    /proc/sys/kernel/timgad/module_restrict

If this becomes /proc/sys/kernel/mod_autoload_restrict/ then the flag
can just be "enabled". (e.g. see LoadPin LSM.)

> Or using the prctl() interface:
>    prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
>
> *) The per-process prctl() settings are:
>     prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)

Excellent, yes, please require the trailing zeros. :)

>     Where value means:
>
> 0 - Classic module load and unload permissions, nothing changes.

"Classic module auto-load permissions ..." Nothing can auto-unload as-is.

> 1 - The current process must have CAP_SYS_MODULE to be able to load and
>     unload modules. CAP_NET_ADMIN should allow the current process to
>     load and unload only netdev aliased modules, not implemented

"... to be able to auto-load modules." Same thing about unloading:
everything needs CAP_SYS_MODULE to unload a module.

> 2 - Current process can not loaded nor unloaded modules.

"... cannot auto-load modules."

>
> *) sysctl interface supports the followin values:
>
> 0 - Classic module load and unload permissions, nothing changes.
>
> 1 - Only privileged processes with CAP_SYS_MODULE should be able to load and
>     unload modules.
>
>     To be added: processes with CAP_NET_ADMIN should be able to
>     load and unload only netdev aliased modules, this is currently not
>     supported. Other checks for real root without CAP_SYS_MODULE ? ...
>
>     (This should be improved)
>
> 2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
>     cannot be changed.
>
> Rules:
> First the prctl() settings are checked, if the access is not denied
> then the global sysctl settings are checked.
>
> As said I will update the permission checks later, this is a preliminary
> RFC.
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> ---
>  include/linux/lsm_hooks.h     |   5 +
>  include/uapi/linux/prctl.h    |   5 +
>  security/Kconfig              |   1 +
>  security/Makefile             |   2 +
>  security/security.c           |   1 +
>  security/timgad/Kconfig       |  10 ++
>  security/timgad/Makefile      |   3 +
>  security/timgad/timgad_core.c | 306 +++++++++++++++++++++++++++++++++++++++
>  security/timgad/timgad_core.h |  53 +++++++
>  security/timgad/timgad_lsm.c  | 327 ++++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 713 insertions(+)
>  create mode 100644 security/timgad/Kconfig
>  create mode 100644 security/timgad/Makefile
>  create mode 100644 security/timgad/timgad_core.c
>  create mode 100644 security/timgad/timgad_core.h
>  create mode 100644 security/timgad/timgad_lsm.c
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index b37e35e..6b83aaa 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1935,5 +1935,10 @@ void __init loadpin_add_hooks(void);
>  #else
>  static inline void loadpin_add_hooks(void) { };
>  #endif
> +#ifdef CONFIG_SECURITY_TIMGAD
> +extern void __init timgad_add_hooks(void);
> +#else
> +static inline void __init timgad_add_hooks(void) { }
> +#endif
>
>  #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
> index a8d0759..6d80eed 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 Timgad LSM options */
> +#define PR_TIMGAD_OPTS                 48
> +# define PR_TIMGAD_SET_MOD_RESTRICT    1
> +# define PR_TIMGAD_GET_MOD_RESTRICT    2
> +
>  #endif /* _LINUX_PRCTL_H */
> diff --git a/security/Kconfig b/security/Kconfig
> index 118f454..e6d6128 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -164,6 +164,7 @@ source security/tomoyo/Kconfig
>  source security/apparmor/Kconfig
>  source security/loadpin/Kconfig
>  source security/yama/Kconfig
> +source security/timgad/Kconfig
>
>  source security/integrity/Kconfig
>
> diff --git a/security/Makefile b/security/Makefile
> index f2d71cd..6a8127e 100644
> --- a/security/Makefile
> +++ b/security/Makefile
> @@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
>  subdir-$(CONFIG_SECURITY_APPARMOR)     += apparmor
>  subdir-$(CONFIG_SECURITY_YAMA)         += yama
>  subdir-$(CONFIG_SECURITY_LOADPIN)      += loadpin
> +subdir-$(CONFIG_SECURITY_TIMGAD)       += timgad
>
>  # always enable default capabilities
>  obj-y                                  += commoncap.o
> @@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_APPARMOR)               += apparmor/
>  obj-$(CONFIG_SECURITY_YAMA)            += yama/
>  obj-$(CONFIG_SECURITY_LOADPIN)         += loadpin/
>  obj-$(CONFIG_CGROUP_DEVICE)            += device_cgroup.o
> +obj-$(CONFIG_SECURITY_TIMGAD)          += timgad/
>
>  # Object integrity file lists
>  subdir-$(CONFIG_INTEGRITY)             += integrity
> diff --git a/security/security.c b/security/security.c
> index 5c699c8..c6c9349 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -61,6 +61,7 @@ int __init security_init(void)
>         capability_add_hooks();
>         yama_add_hooks();
>         loadpin_add_hooks();
> +       timgad_add_hooks();
>
>         /*
>          * Load all the remaining security modules.
> diff --git a/security/timgad/Kconfig b/security/timgad/Kconfig
> new file mode 100644
> index 0000000..93ecdb6
> --- /dev/null
> +++ b/security/timgad/Kconfig
> @@ -0,0 +1,10 @@
> +config SECURITY_TIMGAD
> +       bool "TIMGAD support"
> +       depends on SECURITY
> +       default n
> +       help
> +         This selects TIMGAD, which applies restrictions on load and unload
> +          modules operations. Further information can be found in
> +          Documentation/security/Timgad.txt.

I'd select a capitalization and stick with it (timgad, Timgad,
TIMGAD?) modulo its renaming, etc.

> +
> +         If you are unsure how to answer this question, answer N.
> diff --git a/security/timgad/Makefile b/security/timgad/Makefile
> new file mode 100644
> index 0000000..dca0441
> --- /dev/null
> +++ b/security/timgad/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_SECURITY_TIMGAD) := timgad.o
> +
> +timgad-y := timgad_core.o timgad_lsm.o
> diff --git a/security/timgad/timgad_core.c b/security/timgad/timgad_core.c
> new file mode 100644
> index 0000000..fa35965
> --- /dev/null
> +++ b/security/timgad/timgad_core.c
> [...]

In the hopes of reusing the task stacking stuff from Casey, I'm going
to skip reading the above code at least for now. ;)

> diff --git a/security/timgad/timgad_core.h b/security/timgad/timgad_core.h
> new file mode 100644
> index 0000000..4096c39
> --- /dev/null
> +++ b/security/timgad/timgad_core.h
> @@ -0,0 +1,53 @@
> +/*
> + * Timgad 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.
> + *
> + */
> +
> +#define TIMGAD_MODULE_OFF      0x00000000
> +#define TIMGAD_MODULE_STRICT   0x00000001
> +#define TIMGAD_MODULE_NO_LOAD  0x00000002
> +
> +struct timgad_task;
> +
> +static inline int timgad_op_to_flag(unsigned long op,
> +                                   unsigned long value,
> +                                   unsigned long *flag)
> +{
> +       if (op != PR_TIMGAD_SET_MOD_RESTRICT || value > TIMGAD_MODULE_NO_LOAD)
> +               return -EINVAL;
> +
> +       *flag = value;
> +       return 0;
> +}
> +
> +unsigned long read_timgad_task_flags(struct timgad_task *timgad_tsk);
> +
> +int timgad_task_set_op_flag(struct timgad_task *timgad_tsk,
> +                           unsigned long op, unsigned long flag,
> +                           unsigned long value);
> +
> +int is_timgad_task_op_set(struct timgad_task *timgad_tsk, unsigned long op,
> +                         unsigned long *flag);
> +
> +struct timgad_task *get_timgad_task(struct task_struct *tsk);
> +void put_timgad_task(struct timgad_task *timgad_tsk, bool *collect);
> +struct timgad_task *lookup_timgad_task(struct task_struct *tsk);
> +
> +void release_timgad_task(struct task_struct *tsk);
> +
> +struct timgad_task *init_timgad_task(struct task_struct *tsk,
> +                                    unsigned long flag);
> +struct timgad_task *give_me_timgad_task(struct task_struct *tsk,
> +                                       unsigned long value);
> +
> +int timgad_tasks_init(void);
> +void timgad_tasks_clean(void);
> diff --git a/security/timgad/timgad_lsm.c b/security/timgad/timgad_lsm.c
> new file mode 100644
> index 0000000..809b7cf
> --- /dev/null
> +++ b/security/timgad/timgad_lsm.c
> @@ -0,0 +1,327 @@
> +/*
> + * Timgad 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/sysctl.h>
> +#include <linux/prctl.h>
> +#include <linux/types.h>
> +
> +#include "timgad_core.h"
> +
> +static int module_restrict;
> +
> +static int zero;
> +static int max_module_restrict_scope = TIMGAD_MODULE_NO_LOAD;
> +
> +/* TODO:
> + *  complete permission check
> + *  inline function logic with the per-process if possible
> + */
> +static int timgad_has_global_sysctl_perm(unsigned long op)
> +{
> +       int ret = -EINVAL;
> +       struct mm_struct *mm = NULL;
> +
> +       if (op != PR_TIMGAD_GET_MOD_RESTRICT)
> +               return ret;
> +
> +       switch (module_restrict) {
> +       case TIMGAD_MODULE_OFF:
> +               ret = 0;
> +               break;
> +       /* TODO: complete this and handle it later per task too */
> +       case TIMGAD_MODULE_STRICT:
> +               /*
> +                * Are we allowed to sleep here ?
> +                * Also improve this check here
> +                */
> +               ret = -EPERM;
> +               mm = get_task_mm(current);
> +               if (mm) {
> +                       if (capable(CAP_SYS_MODULE))
> +                               ret = 0;
> +                       mmput(mm);
> +               }

Why the mm check here?

> +               break;
> +       case TIMGAD_MODULE_NO_LOAD:
> +               ret = -EPERM;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +/* TODO: simplify me and move me to timgad_core.c file */
> +static int module_timgad_task_perm(struct timgad_task *timgad_tsk,
> +                                  char *kmod_name)
> +{
> +       int ret;
> +       unsigned long flag = 0;
> +
> +       ret = is_timgad_task_op_set(timgad_tsk,
> +                                   PR_TIMGAD_SET_MOD_RESTRICT, &flag);
> +       if (ret < 0)
> +               return ret;
> +
> +       /*
> +        * TODO: complete me
> +        *    * Allow net modules only with CAP_NET_ADMIN and other cases...
> +        *    * Other exotic cases when set to STRICT should be denied...
> +        *    * Inline logic
> +        */
> +       switch (flag) {
> +       case TIMGAD_MODULE_OFF:
> +               ret = 0;
> +               break;
> +       case TIMGAD_MODULE_STRICT:
> +               if (!capable(CAP_SYS_MODULE))
> +                       ret = -EPERM;
> +               else
> +                       ret = 0;
> +               break;
> +       case TIMGAD_MODULE_NO_LOAD:
> +               ret = -EPERM;
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +/* Set the given option in a timgad task */
> +static int timgad_set_op_value(struct task_struct *tsk,
> +                              unsigned long op, unsigned long value)
> +{
> +       int ret = 0;
> +       struct timgad_task *ttask;
> +       unsigned long flag = 0;
> +
> +       ret = timgad_op_to_flag(op, value, &flag);
> +       if (ret < 0)
> +               return ret;
> +
> +       ttask = get_timgad_task(tsk);
> +       if (!ttask) {
> +               ttask = give_me_timgad_task(tsk, value);
> +               if (IS_ERR(ttask))
> +                       return PTR_ERR(ttask);
> +
> +               return 0;
> +       }
> +
> +       ret = timgad_task_set_op_flag(ttask, op, flag, value);
> +
> +       put_timgad_task(ttask, NULL);
> +       return ret;
> +}
> +
> +/* Get the given option from a timgad task */
> +static int timgad_get_op_value(struct task_struct *tsk, unsigned long op)
> +{
> +       int ret = -EINVAL;
> +       struct timgad_task *ttask;
> +       unsigned long flag = 0;
> +
> +       ttask = get_timgad_task(tsk);
> +       if (!ttask)
> +               return ret;
> +
> +       ret = is_timgad_task_op_set(ttask, op, &flag);
> +       put_timgad_task(ttask, NULL);
> +
> +       return ret < 0 ? ret : flag;
> +}
> +
> +/* Copy Timgad context from parent to child */
> +int timgad_task_copy(struct task_struct *tsk)
> +{
> +       int ret = 0;
> +       struct timgad_task *tparent;
> +       struct timgad_task *ttask;
> +       unsigned long value = 0;
> +
> +       tparent = get_timgad_task(current);
> +
> +       /* Parent does not have a timgad context, nothing to do */
> +       if (tparent == NULL)
> +               return 0;
> +
> +       value = read_timgad_task_flags(tparent);
> +
> +       ttask = give_me_timgad_task(tsk, value);
> +       if (IS_ERR(ttask))
> +               ret = PTR_ERR(ttask);
> +       else
> +               ret = 0;
> +
> +       put_timgad_task(tparent, NULL);
> +       return ret;
> +}
> +
> +/*
> + * Return 0 on success, -error on error.  -EINVAL is returned when Timgad
> + * does not handle the given option.
> + */
> +int timgad_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_TIMGAD_OPTS)
> +               return 0;
> +

Actually enforce that arg4 and arg5 == 0 here, and -EINVAL if they
don't. This will let you expand in the future if you need to.

> +       get_task_struct(myself);
> +
> +       switch (arg2) {
> +       case PR_TIMGAD_SET_MOD_RESTRICT:
> +               ret = timgad_set_op_value(myself,
> +                                         PR_TIMGAD_SET_MOD_RESTRICT,
> +                                         arg3);
> +               break;
> +       case PR_TIMGAD_GET_MOD_RESTRICT:
> +               ret = timgad_get_op_value(myself,
> +                                         PR_TIMGAD_SET_MOD_RESTRICT);
> +               break;
> +       }
> +
> +       put_task_struct(myself);
> +       return ret;
> +}
> +
> +/*
> + * Free the specific task attached resources
> + * task_free() can be called from interrupt context
> + */
> +void timgad_task_free(struct task_struct *tsk)
> +{
> +       release_timgad_task(tsk);
> +}
> +
> +static int timgad_kernel_module_file(struct file *file)
> +{
> +       int ret = 0;
> +       struct timgad_task *ttask;
> +       struct task_struct *myself = current;
> +
> +       /* First check if the task allows that */
> +       ttask = get_timgad_task(myself);
> +       if (ttask != NULL) {
> +               ret = module_timgad_task_perm(ttask, NULL);
> +               put_timgad_task(ttask, NULL);
> +       }
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
> +}

This is the kernel_module_file hook, which is an explicit load of a
module by the current process. Is your intention to block
CAP_SYS_MODULE-capable processes from direct kernel module loading? If
so, I misunderstood part of the design here. I'd explicitly call it
out in the commit text, since direct loading and auto loading are
quite different, though I'd argue that you don't want to touch direct
loading privileges because that's already entirely covered by
CAP_SYS_MODULE....

> +static int timgad_kernel_module_request(char *kmod_name)
> +{
> +       int ret = 0;
> +       struct timgad_task *ttask;
> +       struct task_struct *myself = current;
> +
> +       /* First check if the task allows that */
> +       ttask = get_timgad_task(myself);
> +       if (ttask != NULL) {
> +               ret = module_timgad_task_perm(ttask, kmod_name);
> +               put_timgad_task(ttask, NULL);
> +       }
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
> +}

This is the heart of the auto-loading check...

> +static int timgad_kernel_read_file(struct file *file,
> +                                  enum kernel_read_file_id id)
> +{
> +       int ret = 0;
> +
> +       switch (id) {
> +       case READING_MODULE:
> +               ret = timgad_kernel_module_file(file);
> +               break;
> +       default:
> +               break;
> +       }
> +
> +       return ret;
> +}
> +
> +static struct security_hook_list timgad_hooks[] = {
> +       LSM_HOOK_INIT(kernel_module_request, timgad_kernel_module_request),
> +       LSM_HOOK_INIT(kernel_read_file, timgad_kernel_read_file),
> +       LSM_HOOK_INIT(task_copy, timgad_task_copy),
> +       LSM_HOOK_INIT(task_prctl, timgad_task_prctl),
> +       LSM_HOOK_INIT(task_free, timgad_task_free),
> +};
> +
> +#ifdef CONFIG_SYSCTL
> +static int timgad_mod_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;

Are you intentionally pinning to the highest setting (like Yama)? This
isn't mentioned in the commit message, and it doesn't seem like
something that matters here?

> +
> +       return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
> +}
> +
> +struct ctl_path timgad_sysctl_path[] = {
> +       { .procname = "kernel", },
> +       { .procname = "timgad", },
> +       { }
> +};
> +
> +static struct ctl_table timgad_sysctl_table[] = {
> +       {
> +               .procname       = "module_restrict",
> +               .data           = &module_restrict,
> +               .maxlen         = sizeof(int),
> +               .mode           = 0644,
> +               .proc_handler   = timgad_mod_dointvec_minmax,
> +               .extra1         = &zero,
> +               .extra2         = &max_module_restrict_scope,
> +       },
> +       { }
> +};
> +
> +static void __init timgad_init_sysctl(void)
> +{
> +       if (!register_sysctl_paths(timgad_sysctl_path, timgad_sysctl_table))
> +               panic("Timgad: sysctl registration failed.\n");
> +}
> +#else
> +static inline void timgad_init_sysctl(void) { }
> +#endif /* CONFIG_SYSCTL */
> +
> +void __init timgad_add_hooks(void)
> +{
> +       pr_info("Timgad: becoming mindful.\n");

That's Yama's catch-phrase. ;) Your LSM should say something else. :)

> +       security_add_hooks(timgad_hooks, ARRAY_SIZE(timgad_hooks));
> +       timgad_init_sysctl();
> +
> +       if (timgad_tasks_init())
> +               panic("Timgad: tasks initialization failed.\n");
> +}
> --
> 2.5.5
>

Sorry for the delay in review! It's been a busy week. :)

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [RFC/PATCH 2/3] security: Add the Timgad module

[Djalal Harouni]

On Sat, Feb 11, 2017 at 1:33 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 2, 2017 at 9:04 AM, Djalal Harouni <tixxdz@gmail.com> wrote:
>> From: Djalal Harouni <tixxdz@gmail.com>
>>
>> This adds the Timgad module. Timgad allows to apply restrictions on
>> which task is allowed to load or unload kernel modules. Auto-load module
>> feature is also handled. The settings can also be applied globally using
>> a sysctl interface, this allows to complete the core kernel interface
>> "modules_disable" which has only two modes: allow globally or deny
>> globally.
>
> To bikeshed on the name: since this is a module loading restriction
> LSM, perhaps something more descriptive: ModAutoRestrict or something
> like that? (Yes, Yama is poorly named, but initially it was going to
> be more than just ptrace restrictions...)

Yes, same thing here, I was also thinking maybe I can add an other
restriction here if necessary that apply using prctl() interface (I've
another case need to discuss it first). It would make little sense to
add another module for one prctl() use case and have to duplicate
bookkeeping of task<->security blob where there is already some
infrastructure. I also read your response and Casey's security blob
stacking work so yes lets see, otherwise I will update the name to
ModAutoRestrict or another one, no problem ;-)


>> The feature is useful for sandboxing, embedded systems and Linux
>> containers where only some containers/processes that have the
>> right privileges are allowed to load/unload modules. Unprivileged
>
> I'd be explicit here and discuss _auto_loading of modules. (Otherwise
> people quickly get confused about this vs CAP_SYS_MODULE.) You mention
> auto-load later, but I think mentioning it first make this easier to
> understand.

Ok.

>> processes should not be able to load/unload modules nor trigger the
>> module auto-load feature. This behaviour was inspired from grsecurity's
>> GRKERNSEC_MODHARDEN option.
>>
>> However I still did not complete the check since this has to be
>> discussed first, so any bug here is not from grsecurity, but my bugs and
>> on purpose. As this is a preliminary RFC these points are not settled,
>> discussion has to happen on what should be the best behaviour and what
>> checks should be in place. Currently the settings:
>>
>> Timgad module can be controled using a global sysctl setting:
>
> typo: controlled
>
>>    /proc/sys/kernel/timgad/module_restrict
>
> If this becomes /proc/sys/kernel/mod_autoload_restrict/ then the flag
> can just be "enabled". (e.g. see LoadPin LSM.)

I see but I guess we need at least 3 values.

>> Or using the prctl() interface:
>>    prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
>>
>> *) The per-process prctl() settings are:
>>     prctl(PR_TIMGAD_OPTS, PR_TIGMAD_SET_MOD_RESTRICT, value, 0, 0)
>
> Excellent, yes, please require the trailing zeros. :)
>
>>     Where value means:
>>
>> 0 - Classic module load and unload permissions, nothing changes.
>
> "Classic module auto-load permissions ..." Nothing can auto-unload as-is.
>
>> 1 - The current process must have CAP_SYS_MODULE to be able to load and
>>     unload modules. CAP_NET_ADMIN should allow the current process to
>>     load and unload only netdev aliased modules, not implemented
>
> "... to be able to auto-load modules." Same thing about unloading:
> everything needs CAP_SYS_MODULE to unload a module.
>
>> 2 - Current process can not loaded nor unloaded modules.
>
> "... cannot auto-load modules."

Ok.


>>
>> *) sysctl interface supports the followin values:
>>
>> 0 - Classic module load and unload permissions, nothing changes.
>>
>> 1 - Only privileged processes with CAP_SYS_MODULE should be able to load and
>>     unload modules.
>>
>>     To be added: processes with CAP_NET_ADMIN should be able to
>>     load and unload only netdev aliased modules, this is currently not
>>     supported. Other checks for real root without CAP_SYS_MODULE ? ...
>>
>>     (This should be improved)
>>
>> 2 - Modules can not be loaded nor unloaded. Once set, this sysctl value
>>      cannot be changed.
>>
>> Rules:
>> First the prctl() settings are checked, if the access is not denied
>> then the global sysctl settings are checked.
>>
>> As said I will update the permission checks later, this is a preliminary
>> RFC.
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>> ---
>>  include/linux/lsm_hooks.h     |   5 +
>>  include/uapi/linux/prctl.h    |   5 +
>>  security/Kconfig              |   1 +
>>  security/Makefile             |   2 +
>>  security/security.c           |   1 +
>>  security/timgad/Kconfig       |  10 ++
>>  security/timgad/Makefile      |   3 +
>>  security/timgad/timgad_core.c | 306 +++++++++++++++++++++++++++++++++++++++
>>  security/timgad/timgad_core.h |  53 +++++++
>>  security/timgad/timgad_lsm.c  | 327 ++++++++++++++++++++++++++++++++++++++++++
>>  10 files changed, 713 insertions(+)
>>  create mode 100644 security/timgad/Kconfig
>>  create mode 100644 security/timgad/Makefile
>>  create mode 100644 security/timgad/timgad_core.c
>>  create mode 100644 security/timgad/timgad_core.h
>>  create mode 100644 security/timgad/timgad_lsm.c
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index b37e35e..6b83aaa 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -1935,5 +1935,10 @@ void __init loadpin_add_hooks(void);
>>  #else
>>  static inline void loadpin_add_hooks(void) { };
>>  #endif
>> +#ifdef CONFIG_SECURITY_TIMGAD
>> +extern void __init timgad_add_hooks(void);
>> +#else
>> +static inline void __init timgad_add_hooks(void) { }
>> +#endif
>>
>>  #endif /* ! __LINUX_LSM_HOOKS_H */
>> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
>> index a8d0759..6d80eed 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 Timgad LSM options */
>> +#define PR_TIMGAD_OPTS                 48
>> +# define PR_TIMGAD_SET_MOD_RESTRICT    1
>> +# define PR_TIMGAD_GET_MOD_RESTRICT    2
>> +
>>  #endif /* _LINUX_PRCTL_H */
>> diff --git a/security/Kconfig b/security/Kconfig
>> index 118f454..e6d6128 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -164,6 +164,7 @@ source security/tomoyo/Kconfig
>>  source security/apparmor/Kconfig
>>  source security/loadpin/Kconfig
>>  source security/yama/Kconfig
>> +source security/timgad/Kconfig
>>
>>  source security/integrity/Kconfig
>>
>> diff --git a/security/Makefile b/security/Makefile
>> index f2d71cd..6a8127e 100644
>> --- a/security/Makefile
>> +++ b/security/Makefile
>> @@ -9,6 +9,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
>>  subdir-$(CONFIG_SECURITY_APPARMOR)     += apparmor
>>  subdir-$(CONFIG_SECURITY_YAMA)         += yama
>>  subdir-$(CONFIG_SECURITY_LOADPIN)      += loadpin
>> +subdir-$(CONFIG_SECURITY_TIMGAD)       += timgad
>>
>>  # always enable default capabilities
>>  obj-y                                  += commoncap.o
>> @@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_APPARMOR)               += apparmor/
>>  obj-$(CONFIG_SECURITY_YAMA)            += yama/
>>  obj-$(CONFIG_SECURITY_LOADPIN)         += loadpin/
>>  obj-$(CONFIG_CGROUP_DEVICE)            += device_cgroup.o
>> +obj-$(CONFIG_SECURITY_TIMGAD)          += timgad/
>>
>>  # Object integrity file lists
>>  subdir-$(CONFIG_INTEGRITY)             += integrity
>> diff --git a/security/security.c b/security/security.c
>> index 5c699c8..c6c9349 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -61,6 +61,7 @@ int __init security_init(void)
>>         capability_add_hooks();
>>         yama_add_hooks();
>>         loadpin_add_hooks();
>> +       timgad_add_hooks();
>>
>>         /*
>>          * Load all the remaining security modules.
>> diff --git a/security/timgad/Kconfig b/security/timgad/Kconfig
>> new file mode 100644
>> index 0000000..93ecdb6
>> --- /dev/null
>> +++ b/security/timgad/Kconfig
>> @@ -0,0 +1,10 @@
>> +config SECURITY_TIMGAD
>> +       bool "TIMGAD support"
>> +       depends on SECURITY
>> +       default n
>> +       help
>> +         This selects TIMGAD, which applies restrictions on load and unload
>> +          modules operations. Further information can be found in
>> +          Documentation/security/Timgad.txt.
>
> I'd select a capitalization and stick with it (timgad, Timgad,
> TIMGAD?) modulo its renaming, etc.

Noted.

>> +
>> +         If you are unsure how to answer this question, answer N.
>> diff --git a/security/timgad/Makefile b/security/timgad/Makefile
>> new file mode 100644
>> index 0000000..dca0441
>> --- /dev/null
>> +++ b/security/timgad/Makefile
>> @@ -0,0 +1,3 @@
>> +obj-$(CONFIG_SECURITY_TIMGAD) := timgad.o
>> +
>> +timgad-y := timgad_core.o timgad_lsm.o
>> diff --git a/security/timgad/timgad_core.c b/security/timgad/timgad_core.c
>> new file mode 100644
>> index 0000000..fa35965
>> --- /dev/null
>> +++ b/security/timgad/timgad_core.c
>> [...]
>
> In the hopes of reusing the task stacking stuff from Casey, I'm going
> to skip reading the above code at least for now. ;)

Ok :-) , also on another note a quick test with RCU readers, a lookup
on more than 15000 tasks with a timgad context:
    11.33%  ps           [kernel.kallsyms]        [k] lock_acquire
    10.72%  bash         [kernel.kallsyms]        [k]
page_check_address_transhuge
     8.11%  ps           [kernel.kallsyms]        [k]
_raw_spin_unlock_irqrestore
     6.52%  ps           [kernel.kallsyms]        [k] lock_release
     3.45%  ps           [kernel.kallsyms]        [k] lock_is_held_type
     ...
     0.01%  bash         [kernel.kallsyms]        [k] get_timgad_task
     0.01%  bash         [kernel.kallsyms]        [k] getname_flags
     0.01%  bash         [kernel.kallsyms]        [k] hashtab_search
     0.01%  bash         [kernel.kallsyms]        [k] iput


>> diff --git a/security/timgad/timgad_core.h b/security/timgad/timgad_core.h
>> new file mode 100644
>> index 0000000..4096c39
>> --- /dev/null
>> +++ b/security/timgad/timgad_core.h
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Timgad 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.
>> + *
>> + */
>> +
>> +#define TIMGAD_MODULE_OFF      0x00000000
>> +#define TIMGAD_MODULE_STRICT   0x00000001
>> +#define TIMGAD_MODULE_NO_LOAD  0x00000002
>> +
>> +struct timgad_task;
>> +
>> +static inline int timgad_op_to_flag(unsigned long op,
>> +                                   unsigned long value,
>> +                                   unsigned long *flag)
>> +{
>> +       if (op != PR_TIMGAD_SET_MOD_RESTRICT || value > TIMGAD_MODULE_NO_LOAD)
>> +               return -EINVAL;
>> +
>> +       *flag = value;
>> +       return 0;
>> +}
>> +
>> +unsigned long read_timgad_task_flags(struct timgad_task *timgad_tsk);
>> +
>> +int timgad_task_set_op_flag(struct timgad_task *timgad_tsk,
>> +                           unsigned long op, unsigned long flag,
>> +                           unsigned long value);
>> +
>> +int is_timgad_task_op_set(struct timgad_task *timgad_tsk, unsigned long op,
>> +                         unsigned long *flag);
>> +
>> +struct timgad_task *get_timgad_task(struct task_struct *tsk);
>> +void put_timgad_task(struct timgad_task *timgad_tsk, bool *collect);
>> +struct timgad_task *lookup_timgad_task(struct task_struct *tsk);
>> +
>> +void release_timgad_task(struct task_struct *tsk);
>> +
>> +struct timgad_task *init_timgad_task(struct task_struct *tsk,
>> +                                    unsigned long flag);
>> +struct timgad_task *give_me_timgad_task(struct task_struct *tsk,
>> +                                       unsigned long value);
>> +
>> +int timgad_tasks_init(void);
>> +void timgad_tasks_clean(void);
>> diff --git a/security/timgad/timgad_lsm.c b/security/timgad/timgad_lsm.c
>> new file mode 100644
>> index 0000000..809b7cf
>> --- /dev/null
>> +++ b/security/timgad/timgad_lsm.c
>> @@ -0,0 +1,327 @@
>> +/*
>> + * Timgad 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/sysctl.h>
>> +#include <linux/prctl.h>
>> +#include <linux/types.h>
>> +
>> +#include "timgad_core.h"
>> +
>> +static int module_restrict;
>> +
>> +static int zero;
>> +static int max_module_restrict_scope = TIMGAD_MODULE_NO_LOAD;
>> +
>> +/* TODO:
>> + *  complete permission check
>> + *  inline function logic with the per-process if possible
>> + */
>> +static int timgad_has_global_sysctl_perm(unsigned long op)
>> +{
>> +       int ret = -EINVAL;
>> +       struct mm_struct *mm = NULL;
>> +
>> +       if (op != PR_TIMGAD_GET_MOD_RESTRICT)
>> +               return ret;
>> +
>> +       switch (module_restrict) {
>> +       case TIMGAD_MODULE_OFF:
>> +               ret = 0;
>> +               break;
>> +       /* TODO: complete this and handle it later per task too */
>> +       case TIMGAD_MODULE_STRICT:
>> +               /*
>> +                * Are we allowed to sleep here ?
>> +                * Also improve this check here
>> +                */
>> +               ret = -EPERM;
>> +               mm = get_task_mm(current);
>> +               if (mm) {
>> +                       if (capable(CAP_SYS_MODULE))
>> +                               ret = 0;
>> +                       mmput(mm);
>> +               }
>
> Why the mm check here?

Due to lack of time, I could not check this so I just put this here in
case we have drivers pushed through a work job from a kthread... So to
not forget about it. I'm not sure about this, I will revisit it of
course.


>> +               break;
>> +       case TIMGAD_MODULE_NO_LOAD:
>> +               ret = -EPERM;
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/* TODO: simplify me and move me to timgad_core.c file */
>> +static int module_timgad_task_perm(struct timgad_task *timgad_tsk,
>> +                                  char *kmod_name)
>> +{
>> +       int ret;
>> +       unsigned long flag = 0;
>> +
>> +       ret = is_timgad_task_op_set(timgad_tsk,
>> +                                   PR_TIMGAD_SET_MOD_RESTRICT, &flag);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       /*
>> +        * TODO: complete me
>> +        *    * Allow net modules only with CAP_NET_ADMIN and other cases...
>> +        *    * Other exotic cases when set to STRICT should be denied...
>> +        *    * Inline logic
>> +        */
>> +       switch (flag) {
>> +       case TIMGAD_MODULE_OFF:
>> +               ret = 0;
>> +               break;
>> +       case TIMGAD_MODULE_STRICT:
>> +               if (!capable(CAP_SYS_MODULE))
>> +                       ret = -EPERM;
>> +               else
>> +                       ret = 0;
>> +               break;
>> +       case TIMGAD_MODULE_NO_LOAD:
>> +               ret = -EPERM;
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/* Set the given option in a timgad task */
>> +static int timgad_set_op_value(struct task_struct *tsk,
>> +                              unsigned long op, unsigned long value)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *ttask;
>> +       unsigned long flag = 0;
>> +
>> +       ret = timgad_op_to_flag(op, value, &flag);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ttask = get_timgad_task(tsk);
>> +       if (!ttask) {
>> +               ttask = give_me_timgad_task(tsk, value);
>> +               if (IS_ERR(ttask))
>> +                       return PTR_ERR(ttask);
>> +
>> +               return 0;
>> +       }
>> +
>> +       ret = timgad_task_set_op_flag(ttask, op, flag, value);
>> +
>> +       put_timgad_task(ttask, NULL);
>> +       return ret;
>> +}
>> +
>> +/* Get the given option from a timgad task */
>> +static int timgad_get_op_value(struct task_struct *tsk, unsigned long op)
>> +{
>> +       int ret = -EINVAL;
>> +       struct timgad_task *ttask;
>> +       unsigned long flag = 0;
>> +
>> +       ttask = get_timgad_task(tsk);
>> +       if (!ttask)
>> +               return ret;
>> +
>> +       ret = is_timgad_task_op_set(ttask, op, &flag);
>> +       put_timgad_task(ttask, NULL);
>> +
>> +       return ret < 0 ? ret : flag;
>> +}
>> +
>> +/* Copy Timgad context from parent to child */
>> +int timgad_task_copy(struct task_struct *tsk)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *tparent;
>> +       struct timgad_task *ttask;
>> +       unsigned long value = 0;
>> +
>> +       tparent = get_timgad_task(current);
>> +
>> +       /* Parent does not have a timgad context, nothing to do */
>> +       if (tparent == NULL)
>> +               return 0;
>> +
>> +       value = read_timgad_task_flags(tparent);
>> +
>> +       ttask = give_me_timgad_task(tsk, value);
>> +       if (IS_ERR(ttask))
>> +               ret = PTR_ERR(ttask);
>> +       else
>> +               ret = 0;
>> +
>> +       put_timgad_task(tparent, NULL);
>> +       return ret;
>> +}
>> +
>> +/*
>> + * Return 0 on success, -error on error.  -EINVAL is returned when Timgad
>> + * does not handle the given option.
>> + */
>> +int timgad_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_TIMGAD_OPTS)
>> +               return 0;
>> +
>
> Actually enforce that arg4 and arg5 == 0 here, and -EINVAL if they
> don't. This will let you expand in the future if you need to.

Ok, thanks will do.

>
>> +       get_task_struct(myself);
>> +
>> +       switch (arg2) {
>> +       case PR_TIMGAD_SET_MOD_RESTRICT:
>> +               ret = timgad_set_op_value(myself,
>> +                                         PR_TIMGAD_SET_MOD_RESTRICT,
>> +                                         arg3);
>> +               break;
>> +       case PR_TIMGAD_GET_MOD_RESTRICT:
>> +               ret = timgad_get_op_value(myself,
>> +                                         PR_TIMGAD_SET_MOD_RESTRICT);
>> +               break;
>> +       }
>> +
>> +       put_task_struct(myself);
>> +       return ret;
>> +}
>> +
>> +/*
>> + * Free the specific task attached resources
>> + * task_free() can be called from interrupt context
>> + */
>> +void timgad_task_free(struct task_struct *tsk)
>> +{
>> +       release_timgad_task(tsk);
>> +}
>> +
>> +static int timgad_kernel_module_file(struct file *file)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *ttask;
>> +       struct task_struct *myself = current;
>> +
>> +       /* First check if the task allows that */
>> +       ttask = get_timgad_task(myself);
>> +       if (ttask != NULL) {
>> +               ret = module_timgad_task_perm(ttask, NULL);
>> +               put_timgad_task(ttask, NULL);
>> +       }
>> +
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
>> +}
>
> This is the kernel_module_file hook, which is an explicit load of a
> module by the current process. Is your intention to block
> CAP_SYS_MODULE-capable processes from direct kernel module loading? If
> so, I misunderstood part of the design here. I'd explicitly call it
> out in the commit text, since direct loading and auto loading are
> quite different, though I'd argue that you don't want to touch direct
> loading privileges because that's already entirely covered by
> CAP_SYS_MODULE....

Makes sense, I'll update and confirm that we don't need this in case
we don't have context info about modprobe caller, otherwise we may
need this to fill the gap using a global sysctl policy, or maybe have
semantics based on module types and aliases... for auto-load, the
higher value will deny all auto-load operations.


>
>> +static int timgad_kernel_module_request(char *kmod_name)
>> +{
>> +       int ret = 0;
>> +       struct timgad_task *ttask;
>> +       struct task_struct *myself = current;
>> +
>> +       /* First check if the task allows that */
>> +       ttask = get_timgad_task(myself);
>> +       if (ttask != NULL) {
>> +               ret = module_timgad_task_perm(ttask, kmod_name);
>> +               put_timgad_task(ttask, NULL);
>> +       }
>> +
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       return timgad_has_global_sysctl_perm(PR_TIMGAD_GET_MOD_RESTRICT);
>> +}
>
> This is the heart of the auto-loading check...
>
>> +static int timgad_kernel_read_file(struct file *file,
>> +                                  enum kernel_read_file_id id)
>> +{
>> +       int ret = 0;
>> +
>> +       switch (id) {
>> +       case READING_MODULE:
>> +               ret = timgad_kernel_module_file(file);
>> +               break;
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +static struct security_hook_list timgad_hooks[] = {
>> +       LSM_HOOK_INIT(kernel_module_request, timgad_kernel_module_request),
>> +       LSM_HOOK_INIT(kernel_read_file, timgad_kernel_read_file),
>> +       LSM_HOOK_INIT(task_copy, timgad_task_copy),
>> +       LSM_HOOK_INIT(task_prctl, timgad_task_prctl),
>> +       LSM_HOOK_INIT(task_free, timgad_task_free),
>> +};
>> +
>> +#ifdef CONFIG_SYSCTL
>> +static int timgad_mod_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;
>
> Are you intentionally pinning to the highest setting (like Yama)? This
> isn't mentioned in the commit message, and it doesn't seem like
> something that matters here?


Yes, I forget to document that. Thanks!

>
>> +
>> +       return proc_dointvec_minmax(&table_copy, write, buffer, lenp, ppos);
>> +}
>> +
>> +struct ctl_path timgad_sysctl_path[] = {
>> +       { .procname = "kernel", },
>> +       { .procname = "timgad", },
>> +       { }
>> +};
>> +
>> +static struct ctl_table timgad_sysctl_table[] = {
>> +       {
>> +               .procname       = "module_restrict",
>> +               .data           = &module_restrict,
>> +               .maxlen         = sizeof(int),
>> +               .mode           = 0644,
>> +               .proc_handler   = timgad_mod_dointvec_minmax,
>> +               .extra1         = &zero,
>> +               .extra2         = &max_module_restrict_scope,
>> +       },
>> +       { }
>> +};
>> +
>> +static void __init timgad_init_sysctl(void)
>> +{
>> +       if (!register_sysctl_paths(timgad_sysctl_path, timgad_sysctl_table))
>> +               panic("Timgad: sysctl registration failed.\n");
>> +}
>> +#else
>> +static inline void timgad_init_sysctl(void) { }
>> +#endif /* CONFIG_SYSCTL */
>> +
>> +void __init timgad_add_hooks(void)
>> +{
>> +       pr_info("Timgad: becoming mindful.\n");
>
> That's Yama's catch-phrase. ;) Your LSM should say something else. :)

oups, hehe I hope I can find something!

>
>> +       security_add_hooks(timgad_hooks, ARRAY_SIZE(timgad_hooks));
>> +       timgad_init_sysctl();
>> +
>> +       if (timgad_tasks_init())
>> +               panic("Timgad: tasks initialization failed.\n");
>> +}
>> --
>> 2.5.5
>>
>
> Sorry for the delay in review! It's been a busy week. :)
>
> Thanks!

Thanks for the review Kees, I will revisit all this, fix based on
suggestions and update after next merge window. In mean time I'll
follow also Casey's stacking effort.

> -Kees
>
> --
> Kees Cook
> Pixel Security



-- 
tixxdz