[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202103171823.E7F64A593@keescook>
Date: Wed, 17 Mar 2021 19:00:56 -0700
From: Kees Cook <keescook@...omium.org>
To: John Wood <john.wood@....com>
Cc: Jann Horn <jannh@...gle.com>, Randy Dunlap <rdunlap@...radead.org>,
Jonathan Corbet <corbet@....net>, James Morris <jmorris@...ei.org>,
Shuah Khan <shuah@...nel.org>, "Serge E. Hallyn" <serge@...lyn.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Andi Kleen <ak@...ux.intel.com>,
kernel test robot <oliver.sang@...el.com>,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org,
linux-kselftest@...r.kernel.org,
kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v6 2/8] security/brute: Define a LSM and manage
statistical data
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@....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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
