| Message ID | 1486055094-4532-3-git-send-email-djalal@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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> >
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
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
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"); +}