| Message ID | 20210307113031.11671-6-john.wood@gmx.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fork brute force attack mitigation | expand |
On Sun, Mar 07, 2021 at 12:30:28PM +0100, John Wood wrote: > In order to mitigate a brute force attack all the offending tasks involved > in the attack must be killed. In other words, it is necessary to kill all > the tasks that share the fork and/or exec statistical data related to the > attack. Moreover, if the attack happens through the fork system call, the > processes that have the same group_leader that the current task (the task > that has crashed) must be avoided since they are in the path to be killed. > > When the SIGKILL signal is sent to the offending tasks, the function > "brute_kill_offending_tasks" will be called in a recursive way from the > task_fatal_signal LSM hook due to a small crash period. So, to avoid kill > again the same tasks due to a recursive call of this function, it is > necessary to disable the attack detection for the involved hierarchies. > > To disable the attack detection, set to zero the last crash timestamp and > avoid to compute the application crash period in this case. > > Signed-off-by: John Wood <john.wood@gmx.com> > --- > security/brute/brute.c | 141 ++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 132 insertions(+), 9 deletions(-) > > diff --git a/security/brute/brute.c b/security/brute/brute.c > index 38e5e050964a..36a3286a02dd 100644 > --- a/security/brute/brute.c > +++ b/security/brute/brute.c > @@ -22,6 +22,7 @@ > #include <linux/math64.h> > #include <linux/netdevice.h> > #include <linux/path.h> > +#include <linux/pid.h> > #include <linux/printk.h> > #include <linux/refcount.h> > #include <linux/rwlock.h> > @@ -64,7 +65,7 @@ struct brute_cred { > * @lock: Lock to protect the brute_stats structure. > * @refc: Reference counter. > * @faults: Number of crashes. > - * @jiffies: Last crash timestamp. > + * @jiffies: Last crash timestamp. If zero, the attack detection is disabled. > * @period: Crash period's moving average. > * @saved_cred: Saved credentials. > * @network: Network activity flag. > @@ -571,6 +572,125 @@ static inline void print_fork_attack_running(void) > pr_warn("Fork brute force attack detected [%s]\n", current->comm); > } > > +/** > + * brute_disabled() - Test if the brute force attack detection is disabled. > + * @stats: Statistical data shared by all the fork hierarchy processes. > + * > + * The brute force attack detection enabling/disabling is based on the last > + * crash timestamp. A zero timestamp indicates that this feature is disabled. A > + * timestamp greater than zero indicates that the attack detection is enabled. > + * > + * The statistical data shared by all the fork hierarchy processes 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_fatal_signal hook. > + * > + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock > + * held. > + * Return: True if the brute force attack detection is disabled. False > + * otherwise. > + */ > +static bool brute_disabled(struct brute_stats *stats) > +{ > + bool disabled; > + > + spin_lock(&stats->lock); > + disabled = !stats->jiffies; > + spin_unlock(&stats->lock); > + > + return disabled; > +} > + > +/** > + * brute_disable() - Disable the brute force attack detection. > + * @stats: Statistical data shared by all the fork hierarchy processes. > + * > + * To disable the brute force attack detection it is only necessary to set the > + * last crash timestamp to zero. A zero timestamp indicates that this feature is > + * disabled. A timestamp greater than zero indicates that the attack detection > + * is enabled. > + * > + * The statistical data shared by all the fork hierarchy processes cannot be > + * NULL. > + * > + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock > + * and brute_stats::lock held. > + */ > +static inline void brute_disable(struct brute_stats *stats) > +{ > + stats->jiffies = 0; > +} > + > +/** > + * enum brute_attack_type - Brute force attack type. > + * @BRUTE_ATTACK_TYPE_FORK: Attack that happens through the fork system call. > + * @BRUTE_ATTACK_TYPE_EXEC: Attack that happens through the execve system call. > + */ > +enum brute_attack_type { > + BRUTE_ATTACK_TYPE_FORK, > + BRUTE_ATTACK_TYPE_EXEC, > +}; > + > +/** > + * brute_kill_offending_tasks() - Kill the offending tasks. > + * @attack_type: Brute force attack type. > + * @stats: Statistical data shared by all the fork hierarchy processes. > + * > + * When a brute force attack is detected all the offending tasks involved in the > + * attack must be killed. In other words, it is necessary to kill all the tasks > + * that share the same statistical data. Moreover, if the attack happens through > + * the fork system call, the processes that have the same group_leader that the > + * current task must be avoided since they are in the path to be killed. > + * > + * When the SIGKILL signal is sent to the offending tasks, this function will be > + * called again from the task_fatal_signal hook due to a small crash period. So, > + * to avoid kill again the same tasks due to a recursive call of this function, > + * it is necessary to disable the attack detection for this fork hierarchy. Hah. Interesting. I wonder if there is a better way to handle this. Hmm. > + * > + * The statistical data shared by all the fork hierarchy processes 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_fatal_signal hook. > + * > + * Context: Must be called with interrupts disabled and tasklist_lock and > + * brute_stats_ptr_lock held. > + */ > +static void brute_kill_offending_tasks(enum brute_attack_type attack_type, > + struct brute_stats *stats) > +{ > + struct task_struct *p; > + struct brute_stats **p_stats; > + > + spin_lock(&stats->lock); > + > + if (attack_type == BRUTE_ATTACK_TYPE_FORK && > + refcount_read(&stats->refc) == 1) { > + spin_unlock(&stats->lock); > + return; > + } refcount_read() isn't a safe way to check that there is only 1 reference. What's this trying to do? > + > + brute_disable(stats); > + spin_unlock(&stats->lock); > + > + for_each_process(p) { > + if (attack_type == BRUTE_ATTACK_TYPE_FORK && > + p->group_leader == current->group_leader) > + continue; > + > + p_stats = brute_stats_ptr(p); > + if (*p_stats != stats) > + continue; > + > + do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_PID); > + pr_warn_ratelimited("Offending process %d [%s] killed\n", > + p->pid, p->comm); > + } > +} > + > /** > * brute_manage_fork_attack() - Manage a fork brute force attack. > * @stats: Statistical data shared by all the fork hierarchy processes. > @@ -586,8 +706,8 @@ static inline void print_fork_attack_running(void) > * since the task_free hook can be called from an IRQ context during the > * execution of the task_fatal_signal hook. > * > - * Context: Must be called with interrupts disabled and brute_stats_ptr_lock > - * held. > + * Context: Must be called with interrupts disabled and tasklist_lock and > + * brute_stats_ptr_lock held. > * Return: The last crash timestamp before updating it. > */ > static u64 brute_manage_fork_attack(struct brute_stats *stats, u64 now) > @@ -595,8 +715,10 @@ static u64 brute_manage_fork_attack(struct brute_stats *stats, u64 now) > u64 last_fork_crash; > > last_fork_crash = brute_update_crash_period(stats, now); > - if (brute_attack_running(stats)) > + if (brute_attack_running(stats)) { > print_fork_attack_running(); > + brute_kill_offending_tasks(BRUTE_ATTACK_TYPE_FORK, stats); > + } > > return last_fork_crash; > } > @@ -783,8 +905,10 @@ static void brute_manage_exec_attack(struct brute_stats *stats, u64 now, > if (fork_period == exec_period) > return; > > - if (brute_attack_running(exec_stats)) > + if (brute_attack_running(exec_stats)) { > print_exec_attack_running(exec_stats); > + brute_kill_offending_tasks(BRUTE_ATTACK_TYPE_EXEC, exec_stats); > + } > } > > /** > @@ -900,10 +1024,9 @@ static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo) > read_lock(&tasklist_lock); > read_lock_irqsave(&brute_stats_ptr_lock, flags); > > - if (WARN(!*stats, "No statistical data\n")) > - goto unlock; > - > - if (!brute_threat_model_supported(siginfo, *stats)) > + if (WARN(!*stats, "No statistical data\n") || > + brute_disabled(*stats) || > + !brute_threat_model_supported(siginfo, *stats)) > goto unlock; > > last_fork_crash = brute_manage_fork_attack(*stats, now); > -- > 2.25.1 > -- Kees Cook
On Wed, Mar 17, 2021 at 09:04:15PM -0700, Kees Cook wrote: > On Sun, Mar 07, 2021 at 12:30:28PM +0100, John Wood wrote: > > +/** > > + * brute_kill_offending_tasks() - Kill the offending tasks. > > + * @attack_type: Brute force attack type. > > + * @stats: Statistical data shared by all the fork hierarchy processes. > > + * > > + * When a brute force attack is detected all the offending tasks involved in the > > + * attack must be killed. In other words, it is necessary to kill all the tasks > > + * that share the same statistical data. Moreover, if the attack happens through > > + * the fork system call, the processes that have the same group_leader that the > > + * current task must be avoided since they are in the path to be killed. > > + * > > + * When the SIGKILL signal is sent to the offending tasks, this function will be > > + * called again from the task_fatal_signal hook due to a small crash period. So, > > + * to avoid kill again the same tasks due to a recursive call of this function, > > + * it is necessary to disable the attack detection for this fork hierarchy. > > Hah. Interesting. I wonder if there is a better way to handle this. Hmm. If your comment is related to disable the detection: I think it's no problematic to disable the attack detection for this fork hierarchy since all theirs tasks will be removed. Also, I think that the disable mark can help in the path to use the wait*() functions to notify userspace that a task has been killed by the brute mitigation. Is a work in progress now. If your comment is related to kill all the tasks: In the previous version I have a useful discussion with Andi Kleen where a proposal to block the fork system call during a time was made. He explains me the cons of this method and proposes that if the mitigation works as now we can use the wait*() functions to notify userspace that the tasks has been killed by the brute mitigation. This way other problems related with the supervisors and respawned processes could be handled. Anyway, new points of view are also welcome. > > + * > > + * The statistical data shared by all the fork hierarchy processes 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_fatal_signal hook. > > + * > > + * Context: Must be called with interrupts disabled and tasklist_lock and > > + * brute_stats_ptr_lock held. > > + */ > > +static void brute_kill_offending_tasks(enum brute_attack_type attack_type, > > + struct brute_stats *stats) > > +{ > > + struct task_struct *p; > > + struct brute_stats **p_stats; > > + > > + spin_lock(&stats->lock); > > + > > + if (attack_type == BRUTE_ATTACK_TYPE_FORK && > > + refcount_read(&stats->refc) == 1) { > > + spin_unlock(&stats->lock); > > + return; > > + } > > refcount_read() isn't a safe way to check that there is only 1 > reference. What's this trying to do? If a fork brute force attack has been detected is due to a new fatal crash. Under this scenario, if there is only one reference of these stats, it is not necessary to kill any other tasks since the stats are not shared with another process. Moreover, if this task has failed in a fatal way, is in the path to be killed. So, no action is required. How can I make this check in a safe way? > > + > > + brute_disable(stats); > > + spin_unlock(&stats->lock); > > + > > + for_each_process(p) { > > + if (attack_type == BRUTE_ATTACK_TYPE_FORK && > > + p->group_leader == current->group_leader) > > + continue; > > + > > + p_stats = brute_stats_ptr(p); > > + if (*p_stats != stats) > > + continue; > > + > > + do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_PID); > > + pr_warn_ratelimited("Offending process %d [%s] killed\n", > > + p->pid, p->comm); > > + } > > +} Thanks, John Wood
On Sat, Mar 20, 2021 at 04:48:47PM +0100, John Wood wrote: > On Wed, Mar 17, 2021 at 09:04:15PM -0700, Kees Cook wrote: > > On Sun, Mar 07, 2021 at 12:30:28PM +0100, John Wood wrote: > > > +/** > > > + * brute_kill_offending_tasks() - Kill the offending tasks. > > > + * @attack_type: Brute force attack type. > > > + * @stats: Statistical data shared by all the fork hierarchy processes. > > > + * > > > + * When a brute force attack is detected all the offending tasks involved in the > > > + * attack must be killed. In other words, it is necessary to kill all the tasks > > > + * that share the same statistical data. Moreover, if the attack happens through > > > + * the fork system call, the processes that have the same group_leader that the > > > + * current task must be avoided since they are in the path to be killed. > > > + * > > > + * When the SIGKILL signal is sent to the offending tasks, this function will be > > > + * called again from the task_fatal_signal hook due to a small crash period. So, > > > + * to avoid kill again the same tasks due to a recursive call of this function, > > > + * it is necessary to disable the attack detection for this fork hierarchy. > > > > Hah. Interesting. I wonder if there is a better way to handle this. Hmm. > > If your comment is related to disable the detection: > > I think it's no problematic to disable the attack detection for this fork > hierarchy since all theirs tasks will be removed. Also, I think that the disable > mark can help in the path to use the wait*() functions to notify userspace that > a task has been killed by the brute mitigation. Is a work in progress now. > > If your comment is related to kill all the tasks: > > In the previous version I have a useful discussion with Andi Kleen where a > proposal to block the fork system call during a time was made. He explains me > the cons of this method and proposes that if the mitigation works as now we can > use the wait*() functions to notify userspace that the tasks has been killed > by the brute mitigation. This way other problems related with the supervisors > and respawned processes could be handled. > > Anyway, new points of view are also welcome. I was just amused by my realizing that the brute mitigation could trigger itself. I was just glad you had a comment about the situation -- I hadn't thought about that case yet. :) > > > > + * > > > + * The statistical data shared by all the fork hierarchy processes 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_fatal_signal hook. > > > + * > > > + * Context: Must be called with interrupts disabled and tasklist_lock and > > > + * brute_stats_ptr_lock held. > > > + */ > > > +static void brute_kill_offending_tasks(enum brute_attack_type attack_type, > > > + struct brute_stats *stats) > > > +{ > > > + struct task_struct *p; > > > + struct brute_stats **p_stats; > > > + > > > + spin_lock(&stats->lock); > > > + > > > + if (attack_type == BRUTE_ATTACK_TYPE_FORK && > > > + refcount_read(&stats->refc) == 1) { > > > + spin_unlock(&stats->lock); > > > + return; > > > + } > > > > refcount_read() isn't a safe way to check that there is only 1 > > reference. What's this trying to do? > > If a fork brute force attack has been detected is due to a new fatal crash. > Under this scenario, if there is only one reference of these stats, it is > not necessary to kill any other tasks since the stats are not shared with > another process. Moreover, if this task has failed in a fatal way, is in > the path to be killed. So, no action is required. > > How can I make this check in a safe way? I think you can just skip the optimization -- killing off threads isn't going to be a fast path. -Kees > > > > + > > > + brute_disable(stats); > > > + spin_unlock(&stats->lock); > > > + > > > + for_each_process(p) { > > > + if (attack_type == BRUTE_ATTACK_TYPE_FORK && > > > + p->group_leader == current->group_leader) > > > + continue; > > > + > > > + p_stats = brute_stats_ptr(p); > > > + if (*p_stats != stats) > > > + continue; > > > + > > > + do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_PID); > > > + pr_warn_ratelimited("Offending process %d [%s] killed\n", > > > + p->pid, p->comm); > > > + } > > > +} > > Thanks, > John Wood -- Kees Cook
diff --git a/security/brute/brute.c b/security/brute/brute.c index 38e5e050964a..36a3286a02dd 100644 --- a/security/brute/brute.c +++ b/security/brute/brute.c @@ -22,6 +22,7 @@ #include <linux/math64.h> #include <linux/netdevice.h> #include <linux/path.h> +#include <linux/pid.h> #include <linux/printk.h> #include <linux/refcount.h> #include <linux/rwlock.h> @@ -64,7 +65,7 @@ struct brute_cred { * @lock: Lock to protect the brute_stats structure. * @refc: Reference counter. * @faults: Number of crashes. - * @jiffies: Last crash timestamp. + * @jiffies: Last crash timestamp. If zero, the attack detection is disabled. * @period: Crash period's moving average. * @saved_cred: Saved credentials. * @network: Network activity flag. @@ -571,6 +572,125 @@ static inline void print_fork_attack_running(void) pr_warn("Fork brute force attack detected [%s]\n", current->comm); } +/** + * brute_disabled() - Test if the brute force attack detection is disabled. + * @stats: Statistical data shared by all the fork hierarchy processes. + * + * The brute force attack detection enabling/disabling is based on the last + * crash timestamp. A zero timestamp indicates that this feature is disabled. A + * timestamp greater than zero indicates that the attack detection is enabled. + * + * The statistical data shared by all the fork hierarchy processes 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_fatal_signal hook. + * + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock + * held. + * Return: True if the brute force attack detection is disabled. False + * otherwise. + */ +static bool brute_disabled(struct brute_stats *stats) +{ + bool disabled; + + spin_lock(&stats->lock); + disabled = !stats->jiffies; + spin_unlock(&stats->lock); + + return disabled; +} + +/** + * brute_disable() - Disable the brute force attack detection. + * @stats: Statistical data shared by all the fork hierarchy processes. + * + * To disable the brute force attack detection it is only necessary to set the + * last crash timestamp to zero. A zero timestamp indicates that this feature is + * disabled. A timestamp greater than zero indicates that the attack detection + * is enabled. + * + * The statistical data shared by all the fork hierarchy processes cannot be + * NULL. + * + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock + * and brute_stats::lock held. + */ +static inline void brute_disable(struct brute_stats *stats) +{ + stats->jiffies = 0; +} + +/** + * enum brute_attack_type - Brute force attack type. + * @BRUTE_ATTACK_TYPE_FORK: Attack that happens through the fork system call. + * @BRUTE_ATTACK_TYPE_EXEC: Attack that happens through the execve system call. + */ +enum brute_attack_type { + BRUTE_ATTACK_TYPE_FORK, + BRUTE_ATTACK_TYPE_EXEC, +}; + +/** + * brute_kill_offending_tasks() - Kill the offending tasks. + * @attack_type: Brute force attack type. + * @stats: Statistical data shared by all the fork hierarchy processes. + * + * When a brute force attack is detected all the offending tasks involved in the + * attack must be killed. In other words, it is necessary to kill all the tasks + * that share the same statistical data. Moreover, if the attack happens through + * the fork system call, the processes that have the same group_leader that the + * current task must be avoided since they are in the path to be killed. + * + * When the SIGKILL signal is sent to the offending tasks, this function will be + * called again from the task_fatal_signal hook due to a small crash period. So, + * to avoid kill again the same tasks due to a recursive call of this function, + * it is necessary to disable the attack detection for this fork hierarchy. + * + * The statistical data shared by all the fork hierarchy processes 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_fatal_signal hook. + * + * Context: Must be called with interrupts disabled and tasklist_lock and + * brute_stats_ptr_lock held. + */ +static void brute_kill_offending_tasks(enum brute_attack_type attack_type, + struct brute_stats *stats) +{ + struct task_struct *p; + struct brute_stats **p_stats; + + spin_lock(&stats->lock); + + if (attack_type == BRUTE_ATTACK_TYPE_FORK && + refcount_read(&stats->refc) == 1) { + spin_unlock(&stats->lock); + return; + } + + brute_disable(stats); + spin_unlock(&stats->lock); + + for_each_process(p) { + if (attack_type == BRUTE_ATTACK_TYPE_FORK && + p->group_leader == current->group_leader) + continue; + + p_stats = brute_stats_ptr(p); + if (*p_stats != stats) + continue; + + do_send_sig_info(SIGKILL, SEND_SIG_PRIV, p, PIDTYPE_PID); + pr_warn_ratelimited("Offending process %d [%s] killed\n", + p->pid, p->comm); + } +} + /** * brute_manage_fork_attack() - Manage a fork brute force attack. * @stats: Statistical data shared by all the fork hierarchy processes. @@ -586,8 +706,8 @@ static inline void print_fork_attack_running(void) * since the task_free hook can be called from an IRQ context during the * execution of the task_fatal_signal hook. * - * Context: Must be called with interrupts disabled and brute_stats_ptr_lock - * held. + * Context: Must be called with interrupts disabled and tasklist_lock and + * brute_stats_ptr_lock held. * Return: The last crash timestamp before updating it. */ static u64 brute_manage_fork_attack(struct brute_stats *stats, u64 now) @@ -595,8 +715,10 @@ static u64 brute_manage_fork_attack(struct brute_stats *stats, u64 now) u64 last_fork_crash; last_fork_crash = brute_update_crash_period(stats, now); - if (brute_attack_running(stats)) + if (brute_attack_running(stats)) { print_fork_attack_running(); + brute_kill_offending_tasks(BRUTE_ATTACK_TYPE_FORK, stats); + } return last_fork_crash; } @@ -783,8 +905,10 @@ static void brute_manage_exec_attack(struct brute_stats *stats, u64 now, if (fork_period == exec_period) return; - if (brute_attack_running(exec_stats)) + if (brute_attack_running(exec_stats)) { print_exec_attack_running(exec_stats); + brute_kill_offending_tasks(BRUTE_ATTACK_TYPE_EXEC, exec_stats); + } } /** @@ -900,10 +1024,9 @@ static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo) read_lock(&tasklist_lock); read_lock_irqsave(&brute_stats_ptr_lock, flags); - if (WARN(!*stats, "No statistical data\n")) - goto unlock; - - if (!brute_threat_model_supported(siginfo, *stats)) + if (WARN(!*stats, "No statistical data\n") || + brute_disabled(*stats) || + !brute_threat_model_supported(siginfo, *stats)) goto unlock; last_fork_crash = brute_manage_fork_attack(*stats, now);
In order to mitigate a brute force attack all the offending tasks involved in the attack must be killed. In other words, it is necessary to kill all the tasks that share the fork and/or exec statistical data related to the attack. Moreover, if the attack happens through the fork system call, the processes that have the same group_leader that the current task (the task that has crashed) must be avoided since they are in the path to be killed. When the SIGKILL signal is sent to the offending tasks, the function "brute_kill_offending_tasks" will be called in a recursive way from the task_fatal_signal LSM hook due to a small crash period. So, to avoid kill again the same tasks due to a recursive call of this function, it is necessary to disable the attack detection for the involved hierarchies. To disable the attack detection, set to zero the last crash timestamp and avoid to compute the application crash period in this case. Signed-off-by: John Wood <john.wood@gmx.com> --- security/brute/brute.c | 141 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 132 insertions(+), 9 deletions(-) -- 2.25.1