| Message ID | 20210307113031.11671-3-john.wood@gmx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fork brute force attack mitigation | expand |
On Sun, Mar 07, 2021 at 12:30:25PM +0100, John Wood wrote: > Add a new Kconfig file to define a menu entry under "Security options" > to enable the "Fork brute force attack detection and mitigation" > feature. > > For a correct management of a fork brute force attack it is necessary > that all the tasks hold statistical data. The same statistical data > needs to be shared between all the tasks that hold the same memory > contents or in other words, between all the tasks that have been forked > without any execve call. So, define a statistical data structure to hold > all the necessary information shared by all the fork hierarchy > processes. This info is basically the number of crashes, the last crash > timestamp and the crash period's moving average. > > When a forked task calls the execve system call, the memory contents are > set with new values. So, in this scenario the parent's statistical data > no need to be shared. Instead, a new statistical data structure must be > allocated to start a new hierarchy. > > The statistical data that is shared between all the fork hierarchy > processes needs to be freed when this hierarchy disappears. > > So, based in all the previous information define a LSM with three hooks > to manage all the commented cases. These hooks are "task_alloc" to do > the fork management, "bprm_committing_creds" to do the execve management > and "task_free" to release the resources. > > Also, add to the task_struct's security blob the pointer to the > statistical data. This way, all the tasks will have access to this > information. > > Signed-off-by: John Wood <john.wood@gmx.com> > --- > security/Kconfig | 11 +- > security/Makefile | 4 + > security/brute/Kconfig | 12 ++ > security/brute/Makefile | 2 + > security/brute/brute.c | 257 ++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 281 insertions(+), 5 deletions(-) > create mode 100644 security/brute/Kconfig > create mode 100644 security/brute/Makefile > create mode 100644 security/brute/brute.c > > diff --git a/security/Kconfig b/security/Kconfig > index 7561f6f99f1d..204bb311b1f1 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -240,6 +240,7 @@ source "security/safesetid/Kconfig" > source "security/lockdown/Kconfig" > > source "security/integrity/Kconfig" > +source "security/brute/Kconfig" > > choice > prompt "First legacy 'major LSM' to be initialized" > @@ -277,11 +278,11 @@ endchoice > > config LSM > string "Ordered list of enabled LSMs" > - default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK > - default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR > - default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO > - default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC > - default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" > + default "brute,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK > + default "brute,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR > + default "brute,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO > + default "brute,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC > + default "brute,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" It probably doesn't matter much, but I think brute should be added between lockdown and yama. > help > A comma-separated list of LSMs, in initialization order. > Any LSMs left off this list will be ignored. This can be > diff --git a/security/Makefile b/security/Makefile > index 3baf435de541..1236864876da 100644 > --- a/security/Makefile > +++ b/security/Makefile > @@ -36,3 +36,7 @@ obj-$(CONFIG_BPF_LSM) += bpf/ > # Object integrity file lists > subdir-$(CONFIG_INTEGRITY) += integrity > obj-$(CONFIG_INTEGRITY) += integrity/ > + > +# Object brute file lists > +subdir-$(CONFIG_SECURITY_FORK_BRUTE) += brute > +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute/ I don't think subdir is needed here? I think you can use obj-... like loadpin, etc. > diff --git a/security/brute/Kconfig b/security/brute/Kconfig > new file mode 100644 > index 000000000000..1bd2df1e2dec > --- /dev/null > +++ b/security/brute/Kconfig > @@ -0,0 +1,12 @@ > +# SPDX-License-Identifier: GPL-2.0 > +config SECURITY_FORK_BRUTE > + bool "Fork brute force attack detection and mitigation" > + depends on SECURITY > + help > + This is an LSM that stops any fork brute force attack against > + vulnerable userspace processes. The detection method is based on > + the application crash period and as a mitigation procedure all the > + offending tasks are killed. Like capabilities, this security module > + stacks with other LSMs. I'm not sure the stacking needs mentioning, but okay. :) > + > + If you are unsure how to answer this question, answer N. > diff --git a/security/brute/Makefile b/security/brute/Makefile > new file mode 100644 > index 000000000000..d3f233a132a9 > --- /dev/null > +++ b/security/brute/Makefile > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute.o > diff --git a/security/brute/brute.c b/security/brute/brute.c > new file mode 100644 > index 000000000000..99d099e45112 > --- /dev/null > +++ b/security/brute/brute.c > @@ -0,0 +1,257 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <asm/current.h> Why is this needed? > +#include <linux/bug.h> > +#include <linux/compiler.h> > +#include <linux/errno.h> > +#include <linux/gfp.h> > +#include <linux/init.h> > +#include <linux/jiffies.h> > +#include <linux/kernel.h> > +#include <linux/lsm_hooks.h> > +#include <linux/printk.h> > +#include <linux/refcount.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > +#include <linux/spinlock.h> > +#include <linux/types.h> > + > +/** > + * struct brute_stats - Fork brute force attack statistics. > + * @lock: Lock to protect the brute_stats structure. > + * @refc: Reference counter. > + * @faults: Number of crashes. > + * @jiffies: Last crash timestamp. > + * @period: Crash period's moving average. > + * > + * This structure holds the statistical data shared by all the fork hierarchy > + * processes. > + */ > +struct brute_stats { > + spinlock_t lock; > + refcount_t refc; > + unsigned char faults; > + u64 jiffies; > + u64 period; > +}; I assume the max-255 "faults" will be explained... why is this so small? > + > +/* > + * brute_blob_sizes - LSM blob sizes. > + * > + * To share statistical data among all the fork hierarchy processes, define a > + * pointer to the brute_stats structure as a part of the task_struct's security > + * blob. > + */ > +static struct lsm_blob_sizes brute_blob_sizes __lsm_ro_after_init = { > + .lbs_task = sizeof(struct brute_stats *), > +}; > + > +/** > + * brute_stats_ptr() - Get the pointer to the brute_stats structure. > + * @task: Task that holds the statistical data. > + * > + * Return: A pointer to a pointer to the brute_stats structure. > + */ > +static inline struct brute_stats **brute_stats_ptr(struct task_struct *task) > +{ > + return task->security + brute_blob_sizes.lbs_task; > +} > + > +/** > + * brute_new_stats() - Allocate a new statistics structure. > + * > + * If the allocation is successful the reference counter is set to one to > + * indicate that there will be one task that points to this structure. Also, the > + * last crash timestamp is set to now. This way, it is possible to compute the > + * application crash period at the first fault. > + * > + * Return: NULL if the allocation fails. A pointer to the new allocated > + * statistics structure if it success. > + */ > +static struct brute_stats *brute_new_stats(void) > +{ > + struct brute_stats *stats; > + > + stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL); > + if (!stats) > + return NULL; Since this is tied to process creation, I think it might make sense to have a dedicated kmem cache for this (instead of using the "generic" kmalloc). See kmem_cache_{create,*alloc,free} > + > + spin_lock_init(&stats->lock); > + refcount_set(&stats->refc, 1); > + stats->faults = 0; > + stats->jiffies = get_jiffies_64(); > + stats->period = 0; And either way, I'd recommend using the "z" variant of the allocator (kmem_cache_zalloc, kzalloc) to pre-zero everything (and then you can drop the "= 0" lines here). > + > + return stats; > +} > + > +/** > + * brute_share_stats() - Share the statistical data between processes. > + * @src: Source of statistics to be shared. > + * @dst: Destination of statistics to be shared. > + * > + * Copy the src's pointer to the statistical data structure to the dst's pointer > + * to the same structure. Since there is a new process that shares the same > + * data, increase the reference counter. The src's pointer cannot be NULL. > + * > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > + * since the task_free hook can be called from an IRQ context during the > + * execution of the task_alloc hook. > + */ > +static void brute_share_stats(struct brute_stats *src, > + struct brute_stats **dst) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&src->lock, flags); > + refcount_inc(&src->refc); > + *dst = src; > + spin_unlock_irqrestore(&src->lock, flags); > +} > + > +/** > + * brute_task_alloc() - Target for the task_alloc hook. > + * @task: Task being allocated. > + * @clone_flags: Contains the flags indicating what should be shared. > + * > + * For a correct management of a fork brute force attack it is necessary that > + * all the tasks hold statistical data. The same statistical data needs to be > + * shared between all the tasks that hold the same memory contents or in other > + * words, between all the tasks that have been forked without any execve call. > + * > + * To ensure this, if the current task doesn't have statistical data when forks, > + * it is mandatory to allocate a new statistics structure and share it between > + * this task and the new one being allocated. Otherwise, share the statistics > + * that the current task already has. > + * > + * Return: -ENOMEM if the allocation of the new statistics structure fails. Zero > + * otherwise. > + */ > +static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags) > +{ > + struct brute_stats **stats, **p_stats; > + > + stats = brute_stats_ptr(task); > + p_stats = brute_stats_ptr(current); > + > + if (likely(*p_stats)) { > + brute_share_stats(*p_stats, stats); > + return 0; > + } > + > + *stats = brute_new_stats(); > + if (!*stats) > + return -ENOMEM; > + > + brute_share_stats(*stats, p_stats); > + return 0; > +} During the task_alloc hook, aren't both "current" and "task" already immutable (in the sense that no lock needs to be held for brute_share_stats())? And what is the case where brute_stats_ptr(current) returns NULL? > + > +/** > + * brute_task_execve() - Target for the bprm_committing_creds hook. > + * @bprm: Points to the linux_binprm structure. > + * > + * When a forked task calls the execve system call, the memory contents are set > + * with new values. So, in this scenario the parent's statistical data no need > + * to be shared. Instead, a new statistical data structure must be allocated to > + * start a new hierarchy. This condition is detected when the statistics > + * reference counter holds a value greater than or equal to two (a fork always > + * sets the statistics reference counter to a minimum of two since the parent > + * and the child task are sharing the same data). > + * > + * However, if the execve function is called immediately after another execve > + * call, althought the memory contents are reset, there is no need to allocate > + * a new statistical data structure. This is possible because at this moment > + * only one task (the task that calls the execve function) points to the data. > + * In this case, the previous allocation is used but the statistics are reset. > + * > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > + * since the task_free hook can be called from an IRQ context during the > + * execution of the bprm_committing_creds hook. > + */ > +static void brute_task_execve(struct linux_binprm *bprm) > +{ > + struct brute_stats **stats; > + unsigned long flags; > + > + stats = brute_stats_ptr(current); > + if (WARN(!*stats, "No statistical data\n")) > + return; > + > + spin_lock_irqsave(&(*stats)->lock, flags); > + > + if (!refcount_dec_not_one(&(*stats)->refc)) { > + /* execve call after an execve call */ > + (*stats)->faults = 0; > + (*stats)->jiffies = get_jiffies_64(); > + (*stats)->period = 0; > + spin_unlock_irqrestore(&(*stats)->lock, flags); > + return; > + } > + > + /* execve call after a fork call */ > + spin_unlock_irqrestore(&(*stats)->lock, flags); > + *stats = brute_new_stats(); > + WARN(!*stats, "Cannot allocate statistical data\n"); > +} I don't think any of this locking is needed -- you're always operating on "current", so its brute_stats will always be valid. > + > +/** > + * brute_task_free() - Target for the task_free hook. > + * @task: Task about to be freed. > + * > + * The statistical data that is shared between all the fork hierarchy processes > + * needs to be freed when this hierarchy disappears. > + * > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > + * since the task_free hook can be called from an IRQ context during the > + * execution of the task_free hook. > + */ > +static void brute_task_free(struct task_struct *task) > +{ > + struct brute_stats **stats; > + unsigned long flags; > + bool refc_is_zero; > + > + stats = brute_stats_ptr(task); > + if (WARN(!*stats, "No statistical data\n")) > + return; > + > + spin_lock_irqsave(&(*stats)->lock, flags); > + refc_is_zero = refcount_dec_and_test(&(*stats)->refc); > + spin_unlock_irqrestore(&(*stats)->lock, flags); > + > + if (refc_is_zero) { > + kfree(*stats); > + *stats = NULL; > + } > +} Same thing -- this is what dec_and_test is for: it's atomic, so no locking needed. > + > +/* > + * brute_hooks - Targets for the LSM's hooks. > + */ > +static struct security_hook_list brute_hooks[] __lsm_ro_after_init = { > + LSM_HOOK_INIT(task_alloc, brute_task_alloc), > + LSM_HOOK_INIT(bprm_committing_creds, brute_task_execve), > + LSM_HOOK_INIT(task_free, brute_task_free), > +}; > + > +/** > + * brute_init() - Initialize the brute LSM. > + * > + * Return: Always returns zero. > + */ > +static int __init brute_init(void) > +{ > + pr_info("Brute initialized\n"); > + security_add_hooks(brute_hooks, ARRAY_SIZE(brute_hooks), > + KBUILD_MODNAME); > + return 0; > +} > + > +DEFINE_LSM(brute) = { > + .name = KBUILD_MODNAME, > + .init = brute_init, > + .blobs = &brute_blob_sizes, > +}; > -- > 2.25.1 > -- Kees Cook
Hi, First of all thanks for the review. More info and questions inline. On Wed, Mar 17, 2021 at 07:00:56PM -0700, Kees Cook wrote: > On Sun, Mar 07, 2021 at 12:30:25PM +0100, John Wood wrote: > > > > config LSM > > string "Ordered list of enabled LSMs" > > - default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK > > - default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR > > - default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO > > - default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC > > - default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" > > It probably doesn't matter much, but I think brute should be added > between lockdown and yama. What is the rationale for the stacking order (in relation with brute and lockdown)? > > diff --git a/security/Makefile b/security/Makefile > > index 3baf435de541..1236864876da 100644 > > --- a/security/Makefile > > +++ b/security/Makefile > > @@ -36,3 +36,7 @@ obj-$(CONFIG_BPF_LSM) += bpf/ > > # Object integrity file lists > > subdir-$(CONFIG_INTEGRITY) += integrity > > obj-$(CONFIG_INTEGRITY) += integrity/ > > + > > +# Object brute file lists > > +subdir-$(CONFIG_SECURITY_FORK_BRUTE) += brute > > +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute/ > > I don't think subdir is needed here? I think you can use obj-... like > loadpin, etc. loadpin also uses subdir just like selinux, smack, tomoyo, etc.. So, why is it not necessary for brute? > > +#include <asm/current.h> > > Why is this needed? IIUC, the "current" macro is defined in this header. I try to include the appropiate header for every macro and function used. > > +/** > > + * struct brute_stats - Fork brute force attack statistics. > > + * @lock: Lock to protect the brute_stats structure. > > + * @refc: Reference counter. > > + * @faults: Number of crashes. > > + * @jiffies: Last crash timestamp. > > + * @period: Crash period's moving average. > > + * > > + * This structure holds the statistical data shared by all the fork hierarchy > > + * processes. > > + */ > > +struct brute_stats { > > + spinlock_t lock; > > + refcount_t refc; > > + unsigned char faults; > > + u64 jiffies; > > + u64 period; > > +}; > > I assume the max-255 "faults" will be explained... why is this so small? If a brute force attack is running slowly for a long time, the application crash period's EMA is not suitable for the detection. This type of attack must be detected using a maximum number of faults. In this case, the BRUTE_MAX_FAULTS is defined as 200. > > [...] > > +static struct brute_stats *brute_new_stats(void) > > +{ > > + struct brute_stats *stats; > > + > > + stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL); > > + if (!stats) > > + return NULL; > > Since this is tied to process creation, I think it might make sense to > have a dedicated kmem cache for this (instead of using the "generic" > kmalloc). See kmem_cache_{create,*alloc,free} Thanks, I will work on it for the next version. > > > + > > + spin_lock_init(&stats->lock); > > + refcount_set(&stats->refc, 1); > > + stats->faults = 0; > > + stats->jiffies = get_jiffies_64(); > > + stats->period = 0; > > And either way, I'd recommend using the "z" variant of the allocator > (kmem_cache_zalloc, kzalloc) to pre-zero everything (and then you can > drop the "= 0" lines here). Understood. > > > + > > + return stats; > > +} > > + > > +/** > > + * brute_share_stats() - Share the statistical data between processes. > > + * @src: Source of statistics to be shared. > > + * @dst: Destination of statistics to be shared. > > + * > > + * Copy the src's pointer to the statistical data structure to the dst's pointer > > + * to the same structure. Since there is a new process that shares the same > > + * data, increase the reference counter. The src's pointer cannot be NULL. > > + * > > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > > + * since the task_free hook can be called from an IRQ context during the > > + * execution of the task_alloc hook. > > + */ > > +static void brute_share_stats(struct brute_stats *src, > > + struct brute_stats **dst) > > +{ > > + unsigned long flags; > > + > > + spin_lock_irqsave(&src->lock, flags); > > + refcount_inc(&src->refc); > > + *dst = src; > > + spin_unlock_irqrestore(&src->lock, flags); > > +} > > + > > +/** > > + * brute_task_alloc() - Target for the task_alloc hook. > > + * @task: Task being allocated. > > + * @clone_flags: Contains the flags indicating what should be shared. > > + * > > + * For a correct management of a fork brute force attack it is necessary that > > + * all the tasks hold statistical data. The same statistical data needs to be > > + * shared between all the tasks that hold the same memory contents or in other > > + * words, between all the tasks that have been forked without any execve call. > > + * > > + * To ensure this, if the current task doesn't have statistical data when forks, > > + * it is mandatory to allocate a new statistics structure and share it between > > + * this task and the new one being allocated. Otherwise, share the statistics > > + * that the current task already has. > > + * > > + * Return: -ENOMEM if the allocation of the new statistics structure fails. Zero > > + * otherwise. > > + */ > > +static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags) > > +{ > > + struct brute_stats **stats, **p_stats; > > + > > + stats = brute_stats_ptr(task); > > + p_stats = brute_stats_ptr(current); > > + > > + if (likely(*p_stats)) { > > + brute_share_stats(*p_stats, stats); > > + return 0; > > + } > > + > > + *stats = brute_new_stats(); > > + if (!*stats) > > + return -ENOMEM; > > + > > + brute_share_stats(*stats, p_stats); > > + return 0; > > +} > > During the task_alloc hook, aren't both "current" and "task" already > immutable (in the sense that no lock needs to be held for > brute_share_stats())? I will work on it. > And what is the case where brute_stats_ptr(current) returns NULL? Sorry, but I don't understand what you are trying to explain me. brute_stats_ptr(current) returns a pointer to a pointer. So, I think your question is: What's the purpose of the "if (likely(*p_stats))" check? If it is the case, this check is to guarantee that all the tasks have statistical data. If some task has been allocated prior the brute LSM initialization, this task doesn't have stats. So, with this check all the tasks that fork have stats. > > + > > +/** > > + * brute_task_execve() - Target for the bprm_committing_creds hook. > > + * @bprm: Points to the linux_binprm structure. > > + * > > + * When a forked task calls the execve system call, the memory contents are set > > + * with new values. So, in this scenario the parent's statistical data no need > > + * to be shared. Instead, a new statistical data structure must be allocated to > > + * start a new hierarchy. This condition is detected when the statistics > > + * reference counter holds a value greater than or equal to two (a fork always > > + * sets the statistics reference counter to a minimum of two since the parent > > + * and the child task are sharing the same data). > > + * > > + * However, if the execve function is called immediately after another execve > > + * call, althought the memory contents are reset, there is no need to allocate > > + * a new statistical data structure. This is possible because at this moment > > + * only one task (the task that calls the execve function) points to the data. > > + * In this case, the previous allocation is used but the statistics are reset. > > + * > > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > > + * since the task_free hook can be called from an IRQ context during the > > + * execution of the bprm_committing_creds hook. > > + */ > > +static void brute_task_execve(struct linux_binprm *bprm) > > +{ > > + struct brute_stats **stats; > > + unsigned long flags; > > + > > + stats = brute_stats_ptr(current); > > + if (WARN(!*stats, "No statistical data\n")) > > + return; > > + > > + spin_lock_irqsave(&(*stats)->lock, flags); > > + > > + if (!refcount_dec_not_one(&(*stats)->refc)) { > > + /* execve call after an execve call */ > > + (*stats)->faults = 0; > > + (*stats)->jiffies = get_jiffies_64(); > > + (*stats)->period = 0; > > + spin_unlock_irqrestore(&(*stats)->lock, flags); > > + return; > > + } > > + > > + /* execve call after a fork call */ > > + spin_unlock_irqrestore(&(*stats)->lock, flags); > > + *stats = brute_new_stats(); > > + WARN(!*stats, "Cannot allocate statistical data\n"); > > +} > > I don't think any of this locking is needed -- you're always operating > on "current", so its brute_stats will always be valid. But another process (that share the same stats) could be modifying this concurrently. Scenario 1: cpu 1 writes stats and cpu 2 writes stats. Scenario 2: cpu 1 writes stats, then IRQ on the same cpu writes stats. I think it is possible. So AFAIK we need locking. Sorry if I am wrong. > > + > > +/** > > + * brute_task_free() - Target for the task_free hook. > > + * @task: Task about to be freed. > > + * > > + * The statistical data that is shared between all the fork hierarchy processes > > + * needs to be freed when this hierarchy disappears. > > + * > > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > > + * since the task_free hook can be called from an IRQ context during the > > + * execution of the task_free hook. > > + */ > > +static void brute_task_free(struct task_struct *task) > > +{ > > + struct brute_stats **stats; > > + unsigned long flags; > > + bool refc_is_zero; > > + > > + stats = brute_stats_ptr(task); > > + if (WARN(!*stats, "No statistical data\n")) > > + return; > > + > > + spin_lock_irqsave(&(*stats)->lock, flags); > > + refc_is_zero = refcount_dec_and_test(&(*stats)->refc); > > + spin_unlock_irqrestore(&(*stats)->lock, flags); > > + > > + if (refc_is_zero) { > > + kfree(*stats); > > + *stats = NULL; > > + } > > +} > > Same thing -- this is what dec_and_test is for: it's atomic, so no > locking needed. Ok, in this case I can see that the locking is not necessary due to the stats::refc is atomic. But in the previous case, faults, jiffies and period are not atomic. So I think the lock is necessary. If not, what am I missing? Thanks, John Wood
On Sat, Mar 20, 2021 at 04:01:53PM +0100, John Wood wrote: > Hi, > First of all thanks for the review. More info and questions inline. > > On Wed, Mar 17, 2021 at 07:00:56PM -0700, Kees Cook wrote: > > On Sun, Mar 07, 2021 at 12:30:25PM +0100, John Wood wrote: > > > > > > config LSM > > > string "Ordered list of enabled LSMs" > > > - default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK > > > - default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR > > > - default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO > > > - default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC > > > - default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" > > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK > > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR > > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO > > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC > > > + default "brute,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" > > > > It probably doesn't matter much, but I think brute should be added > > between lockdown and yama. > > What is the rationale for the stacking order (in relation with brute and > lockdown)? lockdown has some very early hooks, so leaving it at the front seems organizationally correct to me. It doesn't really matter, though, so perhaps we should just alphabetize them, but that's for another day. > > > > diff --git a/security/Makefile b/security/Makefile > > > index 3baf435de541..1236864876da 100644 > > > --- a/security/Makefile > > > +++ b/security/Makefile > > > @@ -36,3 +36,7 @@ obj-$(CONFIG_BPF_LSM) += bpf/ > > > # Object integrity file lists > > > subdir-$(CONFIG_INTEGRITY) += integrity > > > obj-$(CONFIG_INTEGRITY) += integrity/ > > > + > > > +# Object brute file lists > > > +subdir-$(CONFIG_SECURITY_FORK_BRUTE) += brute > > > +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute/ > > > > I don't think subdir is needed here? I think you can use obj-... like > > loadpin, etc. > > loadpin also uses subdir just like selinux, smack, tomoyo, etc.. So, why > is it not necessary for brute? Oops, yes, my mistake. I didn't look at the Makefile as a whole. I will adjust my suggestion as: please split subdir and obj as done by the other LSMs (integrity should be fixed to do the same, but that doesn't need to be part of this series). > > > > +#include <asm/current.h> > > > > Why is this needed? > > IIUC, the "current" macro is defined in this header. I try to include the > appropiate header for every macro and function used. The common approach is actually to minimize the number of explicit headers so that if header files includes need to be changed, they only need to be changed internally instead of everywhere in the kernel. Please find an appropriately minimal set of headers to include. > > > > +/** > > > + * struct brute_stats - Fork brute force attack statistics. > > > + * @lock: Lock to protect the brute_stats structure. > > > + * @refc: Reference counter. > > > + * @faults: Number of crashes. > > > + * @jiffies: Last crash timestamp. > > > + * @period: Crash period's moving average. > > > + * > > > + * This structure holds the statistical data shared by all the fork hierarchy > > > + * processes. > > > + */ > > > +struct brute_stats { > > > + spinlock_t lock; > > > + refcount_t refc; > > > + unsigned char faults; > > > + u64 jiffies; > > > + u64 period; > > > +}; > > > > I assume the max-255 "faults" will be explained... why is this so small? > > If a brute force attack is running slowly for a long time, the application > crash period's EMA is not suitable for the detection. This type of attack > must be detected using a maximum number of faults. In this case, the > BRUTE_MAX_FAULTS is defined as 200. Okay, so given the choise of BRUTE_MAX_FAULTS, you limited the storage size? I guess I worry about this somehow wrapping around easily. Given the struct has padding due to the u8 storage, it seems like just using int would be fine too. > > > +static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags) > > > +{ > > > + struct brute_stats **stats, **p_stats; > > > + > > > + stats = brute_stats_ptr(task); > > > + p_stats = brute_stats_ptr(current); > > > + > > > + if (likely(*p_stats)) { > > > + brute_share_stats(*p_stats, stats); > > > + return 0; > > > + } > > > + > > > + *stats = brute_new_stats(); > > > + if (!*stats) > > > + return -ENOMEM; > > > + > > > + brute_share_stats(*stats, p_stats); > > > + return 0; > > > +} > > > > During the task_alloc hook, aren't both "current" and "task" already > > immutable (in the sense that no lock needs to be held for > > brute_share_stats())? > > I will work on it. > > > And what is the case where brute_stats_ptr(current) returns NULL? > > Sorry, but I don't understand what you are trying to explain me. > brute_stats_ptr(current) returns a pointer to a pointer. So, I think > your question is: What's the purpose of the "if (likely(*p_stats))" > check? If it is the case, this check is to guarantee that all the tasks > have statistical data. If some task has been allocated prior the brute > LSM initialization, this task doesn't have stats. So, with this check > all the tasks that fork have stats. Thank you for figuring out my poorly-worded question. :) Yes, I was curious about the "if (likely(*p_stats))". It seems like it shouldn't be possible for a process to lack a stats allocation: the LSMs get initialized before processes. If you wanted to be defensive, I would have expected: if (WARN_ON_ONCE(!*p_stats)) return -ENOMEM; or something (brute should be able to count on the kernel internals behaving here: you're not expecting any path where this could happen). > > > > + > > > +/** > > > + * brute_task_execve() - Target for the bprm_committing_creds hook. > > > + * @bprm: Points to the linux_binprm structure. > > > + * > > > + * When a forked task calls the execve system call, the memory contents are set > > > + * with new values. So, in this scenario the parent's statistical data no need > > > + * to be shared. Instead, a new statistical data structure must be allocated to > > > + * start a new hierarchy. This condition is detected when the statistics > > > + * reference counter holds a value greater than or equal to two (a fork always > > > + * sets the statistics reference counter to a minimum of two since the parent > > > + * and the child task are sharing the same data). > > > + * > > > + * However, if the execve function is called immediately after another execve > > > + * call, althought the memory contents are reset, there is no need to allocate > > > + * a new statistical data structure. This is possible because at this moment > > > + * only one task (the task that calls the execve function) points to the data. > > > + * In this case, the previous allocation is used but the statistics are reset. > > > + * > > > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > > > + * since the task_free hook can be called from an IRQ context during the > > > + * execution of the bprm_committing_creds hook. > > > + */ > > > +static void brute_task_execve(struct linux_binprm *bprm) > > > +{ > > > + struct brute_stats **stats; > > > + unsigned long flags; > > > + > > > + stats = brute_stats_ptr(current); > > > + if (WARN(!*stats, "No statistical data\n")) > > > + return; > > > + > > > + spin_lock_irqsave(&(*stats)->lock, flags); > > > + > > > + if (!refcount_dec_not_one(&(*stats)->refc)) { > > > + /* execve call after an execve call */ > > > + (*stats)->faults = 0; > > > + (*stats)->jiffies = get_jiffies_64(); > > > + (*stats)->period = 0; > > > + spin_unlock_irqrestore(&(*stats)->lock, flags); > > > + return; > > > + } > > > + > > > + /* execve call after a fork call */ > > > + spin_unlock_irqrestore(&(*stats)->lock, flags); > > > + *stats = brute_new_stats(); > > > + WARN(!*stats, "Cannot allocate statistical data\n"); > > > +} > > > > I don't think any of this locking is needed -- you're always operating > > on "current", so its brute_stats will always be valid. > > But another process (that share the same stats) could be modifying this > concurrently. > > Scenario 1: cpu 1 writes stats and cpu 2 writes stats. > Scenario 2: cpu 1 writes stats, then IRQ on the same cpu writes stats. > > I think it is possible. So AFAIK we need locking. Sorry if I am wrong. Maybe I'm misunderstanding, but even your comments on the function say that the zeroing path is there to avoid a new allocation, since only 1 thread has access to that "stats". (i.e. no locking needed), and in the other path, a new stats is allocated (no locking needed). What are the kernel execution paths you see where you'd need locking here? > > > +/** > > > + * brute_task_free() - Target for the task_free hook. > > > + * @task: Task about to be freed. > > > + * > > > + * The statistical data that is shared between all the fork hierarchy processes > > > + * needs to be freed when this hierarchy disappears. > > > + * > > > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock > > > + * since the task_free hook can be called from an IRQ context during the > > > + * execution of the task_free hook. > > > + */ > > > +static void brute_task_free(struct task_struct *task) > > > +{ > > > + struct brute_stats **stats; > > > + unsigned long flags; > > > + bool refc_is_zero; > > > + > > > + stats = brute_stats_ptr(task); > > > + if (WARN(!*stats, "No statistical data\n")) > > > + return; > > > + > > > + spin_lock_irqsave(&(*stats)->lock, flags); > > > + refc_is_zero = refcount_dec_and_test(&(*stats)->refc); > > > + spin_unlock_irqrestore(&(*stats)->lock, flags); > > > + > > > + if (refc_is_zero) { > > > + kfree(*stats); > > > + *stats = NULL; > > > + } > > > +} > > > > Same thing -- this is what dec_and_test is for: it's atomic, so no > > locking needed. > > Ok, in this case I can see that the locking is not necessary due to the > stats::refc is atomic. But in the previous case, faults, jiffies and > period are not atomic. So I think the lock is necessary. If not, what am > I missing? I thought the code had established that there could only be a single stats holder for that code, so no locking. Maybe I misunderstood? -- Kees Cook
diff --git a/security/Kconfig b/security/Kconfig index 7561f6f99f1d..204bb311b1f1 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -240,6 +240,7 @@ source "security/safesetid/Kconfig" source "security/lockdown/Kconfig" source "security/integrity/Kconfig" +source "security/brute/Kconfig" choice prompt "First legacy 'major LSM' to be initialized" @@ -277,11 +278,11 @@ endchoice config LSM string "Ordered list of enabled LSMs" - default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK - default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR - default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO - default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC - default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" + default "brute,lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK + default "brute,lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR + default "brute,lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO + default "brute,lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC + default "brute,lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf" help A comma-separated list of LSMs, in initialization order. Any LSMs left off this list will be ignored. This can be diff --git a/security/Makefile b/security/Makefile index 3baf435de541..1236864876da 100644 --- a/security/Makefile +++ b/security/Makefile @@ -36,3 +36,7 @@ obj-$(CONFIG_BPF_LSM) += bpf/ # Object integrity file lists subdir-$(CONFIG_INTEGRITY) += integrity obj-$(CONFIG_INTEGRITY) += integrity/ + +# Object brute file lists +subdir-$(CONFIG_SECURITY_FORK_BRUTE) += brute +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute/ diff --git a/security/brute/Kconfig b/security/brute/Kconfig new file mode 100644 index 000000000000..1bd2df1e2dec --- /dev/null +++ b/security/brute/Kconfig @@ -0,0 +1,12 @@ +# SPDX-License-Identifier: GPL-2.0 +config SECURITY_FORK_BRUTE + bool "Fork brute force attack detection and mitigation" + depends on SECURITY + help + This is an LSM that stops any fork brute force attack against + vulnerable userspace processes. The detection method is based on + the application crash period and as a mitigation procedure all the + offending tasks are killed. Like capabilities, this security module + stacks with other LSMs. + + If you are unsure how to answer this question, answer N. diff --git a/security/brute/Makefile b/security/brute/Makefile new file mode 100644 index 000000000000..d3f233a132a9 --- /dev/null +++ b/security/brute/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_SECURITY_FORK_BRUTE) += brute.o diff --git a/security/brute/brute.c b/security/brute/brute.c new file mode 100644 index 000000000000..99d099e45112 --- /dev/null +++ b/security/brute/brute.c @@ -0,0 +1,257 @@ +// SPDX-License-Identifier: GPL-2.0 + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <asm/current.h> +#include <linux/bug.h> +#include <linux/compiler.h> +#include <linux/errno.h> +#include <linux/gfp.h> +#include <linux/init.h> +#include <linux/jiffies.h> +#include <linux/kernel.h> +#include <linux/lsm_hooks.h> +#include <linux/printk.h> +#include <linux/refcount.h> +#include <linux/sched.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/types.h> + +/** + * struct brute_stats - Fork brute force attack statistics. + * @lock: Lock to protect the brute_stats structure. + * @refc: Reference counter. + * @faults: Number of crashes. + * @jiffies: Last crash timestamp. + * @period: Crash period's moving average. + * + * This structure holds the statistical data shared by all the fork hierarchy + * processes. + */ +struct brute_stats { + spinlock_t lock; + refcount_t refc; + unsigned char faults; + u64 jiffies; + u64 period; +}; + +/* + * brute_blob_sizes - LSM blob sizes. + * + * To share statistical data among all the fork hierarchy processes, define a + * pointer to the brute_stats structure as a part of the task_struct's security + * blob. + */ +static struct lsm_blob_sizes brute_blob_sizes __lsm_ro_after_init = { + .lbs_task = sizeof(struct brute_stats *), +}; + +/** + * brute_stats_ptr() - Get the pointer to the brute_stats structure. + * @task: Task that holds the statistical data. + * + * Return: A pointer to a pointer to the brute_stats structure. + */ +static inline struct brute_stats **brute_stats_ptr(struct task_struct *task) +{ + return task->security + brute_blob_sizes.lbs_task; +} + +/** + * brute_new_stats() - Allocate a new statistics structure. + * + * If the allocation is successful the reference counter is set to one to + * indicate that there will be one task that points to this structure. Also, the + * last crash timestamp is set to now. This way, it is possible to compute the + * application crash period at the first fault. + * + * Return: NULL if the allocation fails. A pointer to the new allocated + * statistics structure if it success. + */ +static struct brute_stats *brute_new_stats(void) +{ + struct brute_stats *stats; + + stats = kmalloc(sizeof(struct brute_stats), GFP_KERNEL); + if (!stats) + return NULL; + + spin_lock_init(&stats->lock); + refcount_set(&stats->refc, 1); + stats->faults = 0; + stats->jiffies = get_jiffies_64(); + stats->period = 0; + + return stats; +} + +/** + * brute_share_stats() - Share the statistical data between processes. + * @src: Source of statistics to be shared. + * @dst: Destination of statistics to be shared. + * + * Copy the src's pointer to the statistical data structure to the dst's pointer + * to the same structure. Since there is a new process that shares the same + * data, increase the reference counter. The src's pointer cannot be NULL. + * + * It's mandatory to disable interrupts before acquiring the brute_stats::lock + * since the task_free hook can be called from an IRQ context during the + * execution of the task_alloc hook. + */ +static void brute_share_stats(struct brute_stats *src, + struct brute_stats **dst) +{ + unsigned long flags; + + spin_lock_irqsave(&src->lock, flags); + refcount_inc(&src->refc); + *dst = src; + spin_unlock_irqrestore(&src->lock, flags); +} + +/** + * brute_task_alloc() - Target for the task_alloc hook. + * @task: Task being allocated. + * @clone_flags: Contains the flags indicating what should be shared. + * + * For a correct management of a fork brute force attack it is necessary that + * all the tasks hold statistical data. The same statistical data needs to be + * shared between all the tasks that hold the same memory contents or in other + * words, between all the tasks that have been forked without any execve call. + * + * To ensure this, if the current task doesn't have statistical data when forks, + * it is mandatory to allocate a new statistics structure and share it between + * this task and the new one being allocated. Otherwise, share the statistics + * that the current task already has. + * + * Return: -ENOMEM if the allocation of the new statistics structure fails. Zero + * otherwise. + */ +static int brute_task_alloc(struct task_struct *task, unsigned long clone_flags) +{ + struct brute_stats **stats, **p_stats; + + stats = brute_stats_ptr(task); + p_stats = brute_stats_ptr(current); + + if (likely(*p_stats)) { + brute_share_stats(*p_stats, stats); + return 0; + } + + *stats = brute_new_stats(); + if (!*stats) + return -ENOMEM; + + brute_share_stats(*stats, p_stats); + return 0; +} + +/** + * brute_task_execve() - Target for the bprm_committing_creds hook. + * @bprm: Points to the linux_binprm structure. + * + * When a forked task calls the execve system call, the memory contents are set + * with new values. So, in this scenario the parent's statistical data no need + * to be shared. Instead, a new statistical data structure must be allocated to + * start a new hierarchy. This condition is detected when the statistics + * reference counter holds a value greater than or equal to two (a fork always + * sets the statistics reference counter to a minimum of two since the parent + * and the child task are sharing the same data). + * + * However, if the execve function is called immediately after another execve + * call, althought the memory contents are reset, there is no need to allocate + * a new statistical data structure. This is possible because at this moment + * only one task (the task that calls the execve function) points to the data. + * In this case, the previous allocation is used but the statistics are reset. + * + * It's mandatory to disable interrupts before acquiring the brute_stats::lock + * since the task_free hook can be called from an IRQ context during the + * execution of the bprm_committing_creds hook. + */ +static void brute_task_execve(struct linux_binprm *bprm) +{ + struct brute_stats **stats; + unsigned long flags; + + stats = brute_stats_ptr(current); + if (WARN(!*stats, "No statistical data\n")) + return; + + spin_lock_irqsave(&(*stats)->lock, flags); + + if (!refcount_dec_not_one(&(*stats)->refc)) { + /* execve call after an execve call */ + (*stats)->faults = 0; + (*stats)->jiffies = get_jiffies_64(); + (*stats)->period = 0; + spin_unlock_irqrestore(&(*stats)->lock, flags); + return; + } + + /* execve call after a fork call */ + spin_unlock_irqrestore(&(*stats)->lock, flags); + *stats = brute_new_stats(); + WARN(!*stats, "Cannot allocate statistical data\n"); +} + +/** + * brute_task_free() - Target for the task_free hook. + * @task: Task about to be freed. + * + * The statistical data that is shared between all the fork hierarchy processes + * needs to be freed when this hierarchy disappears. + * + * It's mandatory to disable interrupts before acquiring the brute_stats::lock + * since the task_free hook can be called from an IRQ context during the + * execution of the task_free hook. + */ +static void brute_task_free(struct task_struct *task) +{ + struct brute_stats **stats; + unsigned long flags; + bool refc_is_zero; + + stats = brute_stats_ptr(task); + if (WARN(!*stats, "No statistical data\n")) + return; + + spin_lock_irqsave(&(*stats)->lock, flags); + refc_is_zero = refcount_dec_and_test(&(*stats)->refc); + spin_unlock_irqrestore(&(*stats)->lock, flags); + + if (refc_is_zero) { + kfree(*stats); + *stats = NULL; + } +} + +/* + * brute_hooks - Targets for the LSM's hooks. + */ +static struct security_hook_list brute_hooks[] __lsm_ro_after_init = { + LSM_HOOK_INIT(task_alloc, brute_task_alloc), + LSM_HOOK_INIT(bprm_committing_creds, brute_task_execve), + LSM_HOOK_INIT(task_free, brute_task_free), +}; + +/** + * brute_init() - Initialize the brute LSM. + * + * Return: Always returns zero. + */ +static int __init brute_init(void) +{ + pr_info("Brute initialized\n"); + security_add_hooks(brute_hooks, ARRAY_SIZE(brute_hooks), + KBUILD_MODNAME); + return 0; +} + +DEFINE_LSM(brute) = { + .name = KBUILD_MODNAME, + .init = brute_init, + .blobs = &brute_blob_sizes, +};
Add a new Kconfig file to define a menu entry under "Security options" to enable the "Fork brute force attack detection and mitigation" feature. For a correct management of a fork brute force attack it is necessary that all the tasks hold statistical data. The same statistical data needs to be shared between all the tasks that hold the same memory contents or in other words, between all the tasks that have been forked without any execve call. So, define a statistical data structure to hold all the necessary information shared by all the fork hierarchy processes. This info is basically the number of crashes, the last crash timestamp and the crash period's moving average. When a forked task calls the execve system call, the memory contents are set with new values. So, in this scenario the parent's statistical data no need to be shared. Instead, a new statistical data structure must be allocated to start a new hierarchy. The statistical data that is shared between all the fork hierarchy processes needs to be freed when this hierarchy disappears. So, based in all the previous information define a LSM with three hooks to manage all the commented cases. These hooks are "task_alloc" to do the fork management, "bprm_committing_creds" to do the execve management and "task_free" to release the resources. Also, add to the task_struct's security blob the pointer to the statistical data. This way, all the tasks will have access to this information. Signed-off-by: John Wood <john.wood@gmx.com> --- security/Kconfig | 11 +- security/Makefile | 4 + security/brute/Kconfig | 12 ++ security/brute/Makefile | 2 + security/brute/brute.c | 257 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 281 insertions(+), 5 deletions(-) create mode 100644 security/brute/Kconfig create mode 100644 security/brute/Makefile create mode 100644 security/brute/brute.c -- 2.25.1