diff mbox series

[RFC,5/6] security/fbfam: Detect a fork brute force attack

Message ID 20200910202107.3799376-6-keescook@chromium.org
State New
Headers show
Series Fork brute force attack mitigation (fbfam) | expand

Commit Message

Kees Cook Sept. 10, 2020, 8:21 p.m. UTC
From: John Wood <john.wood@gmx.com>

To detect a fork brute force attack it is necessary to compute the
crashing rate of the application. This calculation is performed in each
fatal fail of a task, or in other words, when a core dump is triggered.
If this rate shows that the application is crashing quickly, there is a
clear signal that an attack is happening.

Since the crashing rate is computed in milliseconds per fault, if this
rate goes under a certain threshold a warning is triggered.

Signed-off-by: John Wood <john.wood@gmx.com>
---
 fs/coredump.c          |  2 ++
 include/fbfam/fbfam.h  |  2 ++
 security/fbfam/fbfam.c | 39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+)

Comments

Jann Horn Sept. 10, 2020, 9:10 p.m. UTC | #1
On Thu, Sep 10, 2020 at 10:22 PM Kees Cook <keescook@chromium.org> wrote:
> To detect a fork brute force attack it is necessary to compute the
> crashing rate of the application. This calculation is performed in each
> fatal fail of a task, or in other words, when a core dump is triggered.
> If this rate shows that the application is crashing quickly, there is a
> clear signal that an attack is happening.
>
> Since the crashing rate is computed in milliseconds per fault, if this
> rate goes under a certain threshold a warning is triggered.
[...]
> +/**
> + * fbfam_handle_attack() - Fork brute force attack detection.
> + * @signal: Signal number that causes the core dump.
> + *
> + * The crashing rate of an application is computed in milliseconds per fault in
> + * each crash. So, if this rate goes under a certain threshold there is a clear
> + * signal that the application is crashing quickly. At this moment, a fork brute
> + * force attack is happening.
> + *
> + * Return: -EFAULT if the current task doesn't have statistical data. Zero
> + *         otherwise.
> + */
> +int fbfam_handle_attack(int signal)
> +{
> +       struct fbfam_stats *stats = current->fbfam_stats;
> +       u64 delta_jiffies, delta_time;
> +       u64 crashing_rate;
> +
> +       if (!stats)
> +               return -EFAULT;
> +
> +       if (!(signal == SIGILL || signal == SIGBUS || signal == SIGKILL ||
> +             signal == SIGSEGV || signal == SIGSYS))
> +               return 0;

As far as I can tell, you can never get here with SIGKILL, since
SIGKILL doesn't trigger core dumping and also isn't used by seccomp?

> +
> +       stats->faults += 1;

This is a data race. If you want to be able to increment a variable
that may be concurrently incremented by other tasks, use either
locking or the atomic_t helpers.

> +       delta_jiffies = get_jiffies_64() - stats->jiffies;
> +       delta_time = jiffies64_to_msecs(delta_jiffies);
> +       crashing_rate = delta_time / (u64)stats->faults;

Do I see this correctly, is this computing the total runtime of this
process hierarchy divided by the total number of faults seen in this
process hierarchy? If so, you may want to reconsider whether that's
really the behavior you want. For example, if I configure the minimum
period between crashes to be 30s (as is the default in the sysctl
patch), and I try to attack a server that has been running without any
crashes for a month, I'd instantly be able to crash around
30*24*60*60/30 = 86400 times before the detection kicks in. That seems
suboptimal.

(By the way, it kind of annoys me that you call it the "rate" when
it's actually the inverse of the rate. "Period" might be more
appropriate?)



> +       if (crashing_rate < (u64)sysctl_crashing_rate_threshold)
> +               pr_warn("fbfam: Fork brute force attack detected\n");
> +
> +       return 0;
> +}
> +
> --
> 2.25.1
>
Kees Cook Sept. 10, 2020, 11:49 p.m. UTC | #2
On Thu, Sep 10, 2020 at 01:21:06PM -0700, Kees Cook wrote:
> From: John Wood <john.wood@gmx.com>
> 
> To detect a fork brute force attack it is necessary to compute the
> crashing rate of the application. This calculation is performed in each
> fatal fail of a task, or in other words, when a core dump is triggered.
> If this rate shows that the application is crashing quickly, there is a
> clear signal that an attack is happening.
> 
> Since the crashing rate is computed in milliseconds per fault, if this
> rate goes under a certain threshold a warning is triggered.
> 
> Signed-off-by: John Wood <john.wood@gmx.com>
> ---
>  fs/coredump.c          |  2 ++
>  include/fbfam/fbfam.h  |  2 ++
>  security/fbfam/fbfam.c | 39 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/fs/coredump.c b/fs/coredump.c
> index 76e7c10edfc0..d4ba4e1828d5 100644
> --- a/fs/coredump.c
> +++ b/fs/coredump.c
> @@ -51,6 +51,7 @@
>  #include "internal.h"
>  
>  #include <trace/events/sched.h>
> +#include <fbfam/fbfam.h>
>  
>  int core_uses_pid;
>  unsigned int core_pipe_limit;
> @@ -825,6 +826,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
>  fail_creds:
>  	put_cred(cred);
>  fail:
> +	fbfam_handle_attack(siginfo->si_signo);

I don't think this is the right place for detecting a crash -- isn't
this only for the "dumping core" condition? In other words, don't you
want to do this in get_signal()'s "fatal" block? (i.e. very close to the
do_coredump, but without the "should I dump?" check?)

Hmm, but maybe I'm wrong? It looks like you're looking at noticing the
process taking a signal from SIG_KERNEL_COREDUMP_MASK ?

(Better yet: what are fatal conditions that do NOT match
SIG_KERNEL_COREDUMP_MASK, and should those be covered?)

Regardless, *this* looks like the only place without an LSM hook. And it
doesn't seem unreasonable to add one here. I assume it would probably
just take the siginfo pointer, which is also what you're checking.

e.g. for include/linux/lsm_hook_defs.h:

LSM_HOOK(int, 0, task_coredump, const kernel_siginfo_t *siginfo);


>  	return;
>  }
>  
> diff --git a/include/fbfam/fbfam.h b/include/fbfam/fbfam.h
> index 2cfe51d2b0d5..9ac8e33d8291 100644
> --- a/include/fbfam/fbfam.h
> +++ b/include/fbfam/fbfam.h
> @@ -12,10 +12,12 @@ extern struct ctl_table fbfam_sysctls[];
>  int fbfam_fork(struct task_struct *child);
>  int fbfam_execve(void);
>  int fbfam_exit(void);
> +int fbfam_handle_attack(int signal);
>  #else
>  static inline int fbfam_fork(struct task_struct *child) { return 0; }
>  static inline int fbfam_execve(void) { return 0; }
>  static inline int fbfam_exit(void) { return 0; }
> +static inline int fbfam_handle_attack(int signal) { return 0; }
>  #endif
>  
>  #endif /* _FBFAM_H_ */
> diff --git a/security/fbfam/fbfam.c b/security/fbfam/fbfam.c
> index 9be4639b72eb..3aa669e4ea51 100644
> --- a/security/fbfam/fbfam.c
> +++ b/security/fbfam/fbfam.c
> @@ -4,7 +4,9 @@
>  #include <linux/errno.h>
>  #include <linux/gfp.h>
>  #include <linux/jiffies.h>
> +#include <linux/printk.h>
>  #include <linux/refcount.h>
> +#include <linux/signal.h>
>  #include <linux/slab.h>
>  
>  /**
> @@ -172,3 +174,40 @@ int fbfam_exit(void)
>  	return 0;
>  }
>  
> +/**
> + * fbfam_handle_attack() - Fork brute force attack detection.
> + * @signal: Signal number that causes the core dump.
> + *
> + * The crashing rate of an application is computed in milliseconds per fault in
> + * each crash. So, if this rate goes under a certain threshold there is a clear
> + * signal that the application is crashing quickly. At this moment, a fork brute
> + * force attack is happening.
> + *
> + * Return: -EFAULT if the current task doesn't have statistical data. Zero
> + *         otherwise.
> + */
> +int fbfam_handle_attack(int signal)
> +{
> +	struct fbfam_stats *stats = current->fbfam_stats;
> +	u64 delta_jiffies, delta_time;
> +	u64 crashing_rate;
> +
> +	if (!stats)
> +		return -EFAULT;
> +
> +	if (!(signal == SIGILL || signal == SIGBUS || signal == SIGKILL ||
> +	      signal == SIGSEGV || signal == SIGSYS))
> +		return 0;

This will only be called for:

#define SIG_KERNEL_COREDUMP_MASK (\
        rt_sigmask(SIGQUIT)   |  rt_sigmask(SIGILL)    | \
        rt_sigmask(SIGTRAP)   |  rt_sigmask(SIGABRT)   | \
        rt_sigmask(SIGFPE)    |  rt_sigmask(SIGSEGV)   | \
        rt_sigmask(SIGBUS)    |  rt_sigmask(SIGSYS)    | \
        rt_sigmask(SIGXCPU)   |  rt_sigmask(SIGXFSZ)   | \
        SIGEMT_MASK                                    )

So you're skipping:

	SIGQUIT
	SIGTRAP
	SIGABRT
	SIGFPE
	SIGXCPU
	SIGXFSZ
	SIGEMT_MASK

I would include SIGABRT (e.g. glibc will call abort() for stack
canary, malloc, etc failures, which may indicate a mitigation has
fired).
Jann Horn Sept. 11, 2020, 12:01 a.m. UTC | #3
On Fri, Sep 11, 2020 at 1:49 AM Kees Cook <keescook@chromium.org> wrote:
> On Thu, Sep 10, 2020 at 01:21:06PM -0700, Kees Cook wrote:
> > From: John Wood <john.wood@gmx.com>
> >
> > To detect a fork brute force attack it is necessary to compute the
> > crashing rate of the application. This calculation is performed in each
> > fatal fail of a task, or in other words, when a core dump is triggered.
> > If this rate shows that the application is crashing quickly, there is a
> > clear signal that an attack is happening.
> >
> > Since the crashing rate is computed in milliseconds per fault, if this
> > rate goes under a certain threshold a warning is triggered.
> >
> > Signed-off-by: John Wood <john.wood@gmx.com>
> > ---
> >  fs/coredump.c          |  2 ++
> >  include/fbfam/fbfam.h  |  2 ++
> >  security/fbfam/fbfam.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 43 insertions(+)
> >
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 76e7c10edfc0..d4ba4e1828d5 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -51,6 +51,7 @@
> >  #include "internal.h"
> >
> >  #include <trace/events/sched.h>
> > +#include <fbfam/fbfam.h>
> >
> >  int core_uses_pid;
> >  unsigned int core_pipe_limit;
> > @@ -825,6 +826,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> >  fail_creds:
> >       put_cred(cred);
> >  fail:
> > +     fbfam_handle_attack(siginfo->si_signo);
>
> I don't think this is the right place for detecting a crash -- isn't
> this only for the "dumping core" condition? In other words, don't you
> want to do this in get_signal()'s "fatal" block? (i.e. very close to the
> do_coredump, but without the "should I dump?" check?)
>
> Hmm, but maybe I'm wrong? It looks like you're looking at noticing the
> process taking a signal from SIG_KERNEL_COREDUMP_MASK ?
>
> (Better yet: what are fatal conditions that do NOT match
> SIG_KERNEL_COREDUMP_MASK, and should those be covered?)
>
> Regardless, *this* looks like the only place without an LSM hook. And it
> doesn't seem unreasonable to add one here. I assume it would probably
> just take the siginfo pointer, which is also what you're checking.

Good point, making this an LSM might be a good idea.

> e.g. for include/linux/lsm_hook_defs.h:
>
> LSM_HOOK(int, 0, task_coredump, const kernel_siginfo_t *siginfo);

I guess it should probably be an LSM_RET_VOID hook? And since, as you
said, it's not really semantically about core dumping, maybe it should
be named task_fatal_signal or something like that.
John Wood Sept. 13, 2020, 4:56 p.m. UTC | #4
Hi,

On Fri, Sep 11, 2020 at 02:01:56AM +0200, Jann Horn wrote:
> On Fri, Sep 11, 2020 at 1:49 AM Kees Cook <keescook@chromium.org> wrote:
> > On Thu, Sep 10, 2020 at 01:21:06PM -0700, Kees Cook wrote:
> > > diff --git a/fs/coredump.c b/fs/coredump.c
> > > index 76e7c10edfc0..d4ba4e1828d5 100644
> > > --- a/fs/coredump.c
> > > +++ b/fs/coredump.c
> > > @@ -51,6 +51,7 @@
> > >  #include "internal.h"
> > >
> > >  #include <trace/events/sched.h>
> > > +#include <fbfam/fbfam.h>
> > >
> > >  int core_uses_pid;
> > >  unsigned int core_pipe_limit;
> > > @@ -825,6 +826,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> > >  fail_creds:
> > >       put_cred(cred);
> > >  fail:
> > > +     fbfam_handle_attack(siginfo->si_signo);
> >
> > I don't think this is the right place for detecting a crash -- isn't
> > this only for the "dumping core" condition? In other words, don't you
> > want to do this in get_signal()'s "fatal" block? (i.e. very close to the
> > do_coredump, but without the "should I dump?" check?)
> >
> > Hmm, but maybe I'm wrong? It looks like you're looking at noticing the
> > process taking a signal from SIG_KERNEL_COREDUMP_MASK ?
> >
> > (Better yet: what are fatal conditions that do NOT match
> > SIG_KERNEL_COREDUMP_MASK, and should those be covered?)
> >
> > Regardless, *this* looks like the only place without an LSM hook. And it
> > doesn't seem unreasonable to add one here. I assume it would probably
> > just take the siginfo pointer, which is also what you're checking.
>
> Good point, making this an LSM might be a good idea.
>
> > e.g. for include/linux/lsm_hook_defs.h:
> >
> > LSM_HOOK(int, 0, task_coredump, const kernel_siginfo_t *siginfo);
>
> I guess it should probably be an LSM_RET_VOID hook? And since, as you
> said, it's not really semantically about core dumping, maybe it should
> be named task_fatal_signal or something like that.

If I understand correctly you propose to add a new LSM hook without return
value and place it here:

diff --git a/kernel/signal.c b/kernel/signal.c
index a38b3edc6851..074492d23e98 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2751,6 +2751,8 @@ bool get_signal(struct ksignal *ksig)
                        do_coredump(&ksig->info);
                }

+               // Add the new LSM hook here
+
                /*
                 * Death signals, no core dump.
                 */

Thanks,
John Wood
John Wood Sept. 13, 2020, 5:54 p.m. UTC | #5
Hi,

On Thu, Sep 10, 2020 at 11:10:38PM +0200, Jann Horn wrote:
> On Thu, Sep 10, 2020 at 10:22 PM Kees Cook <keescook@chromium.org> wrote:
> > To detect a fork brute force attack it is necessary to compute the
> > crashing rate of the application. This calculation is performed in each
> > fatal fail of a task, or in other words, when a core dump is triggered.
> > If this rate shows that the application is crashing quickly, there is a
> > clear signal that an attack is happening.
> >
> > Since the crashing rate is computed in milliseconds per fault, if this
> > rate goes under a certain threshold a warning is triggered.
> [...]
> > +/**
> > + * fbfam_handle_attack() - Fork brute force attack detection.
> > + * @signal: Signal number that causes the core dump.
> > + *
> > + * The crashing rate of an application is computed in milliseconds per fault in
> > + * each crash. So, if this rate goes under a certain threshold there is a clear
> > + * signal that the application is crashing quickly. At this moment, a fork brute
> > + * force attack is happening.
> > + *
> > + * Return: -EFAULT if the current task doesn't have statistical data. Zero
> > + *         otherwise.
> > + */
> > +int fbfam_handle_attack(int signal)
> > +{
> > +       struct fbfam_stats *stats = current->fbfam_stats;
> > +       u64 delta_jiffies, delta_time;
> > +       u64 crashing_rate;
> > +
> > +       if (!stats)
> > +               return -EFAULT;
> > +
> > +       if (!(signal == SIGILL || signal == SIGBUS || signal == SIGKILL ||
> > +             signal == SIGSEGV || signal == SIGSYS))
> > +               return 0;
>
> As far as I can tell, you can never get here with SIGKILL, since
> SIGKILL doesn't trigger core dumping and also isn't used by seccomp?

Understood.

> > +
> > +       stats->faults += 1;
>
> This is a data race. If you want to be able to increment a variable
> that may be concurrently incremented by other tasks, use either
> locking or the atomic_t helpers.

Ok, I will correct this for the next version. Thanks.

> > +       delta_jiffies = get_jiffies_64() - stats->jiffies;
> > +       delta_time = jiffies64_to_msecs(delta_jiffies);
> > +       crashing_rate = delta_time / (u64)stats->faults;
>
> Do I see this correctly, is this computing the total runtime of this
> process hierarchy divided by the total number of faults seen in this
> process hierarchy? If so, you may want to reconsider whether that's
> really the behavior you want. For example, if I configure the minimum
> period between crashes to be 30s (as is the default in the sysctl
> patch), and I try to attack a server that has been running without any
> crashes for a month, I'd instantly be able to crash around
> 30*24*60*60/30 = 86400 times before the detection kicks in. That seems
> suboptimal.

You are right. This is not the behaviour we want. So, for the next
version it would be better to compute the crashing period as the time
between two faults, or the time between the execve call and the first
fault (first fault case).

However, I am afraid of a premature detection if a child process fails
twice in a short period.

So, I think it would be a good idea add a new sysctl to setup a
minimum number of faults before the time between faults starts to be
computed. And so, the attack detection only will be triggered if the
application crashes quickly but after a number of crashes.

What do you think?

>
> (By the way, it kind of annoys me that you call it the "rate" when
> it's actually the inverse of the rate. "Period" might be more
> appropriate?)

Yes, "period" it's more appropiate. Thanks for the clarification.

> > +       if (crashing_rate < (u64)sysctl_crashing_rate_threshold)
> > +               pr_warn("fbfam: Fork brute force attack detected\n");
> > +
> > +       return 0;
> > +}
> > +
> > --
> > 2.25.1
> >

Regards,
John Wood
Jann Horn Sept. 14, 2020, 7:39 p.m. UTC | #6
On Sun, Sep 13, 2020 at 6:56 PM John Wood <john.wood@gmx.com> wrote:
> On Fri, Sep 11, 2020 at 02:01:56AM +0200, Jann Horn wrote:
> > On Fri, Sep 11, 2020 at 1:49 AM Kees Cook <keescook@chromium.org> wrote:
> > > On Thu, Sep 10, 2020 at 01:21:06PM -0700, Kees Cook wrote:
> > > > diff --git a/fs/coredump.c b/fs/coredump.c
> > > > index 76e7c10edfc0..d4ba4e1828d5 100644
> > > > --- a/fs/coredump.c
> > > > +++ b/fs/coredump.c
> > > > @@ -51,6 +51,7 @@
> > > >  #include "internal.h"
> > > >
> > > >  #include <trace/events/sched.h>
> > > > +#include <fbfam/fbfam.h>
> > > >
> > > >  int core_uses_pid;
> > > >  unsigned int core_pipe_limit;
> > > > @@ -825,6 +826,7 @@ void do_coredump(const kernel_siginfo_t *siginfo)
> > > >  fail_creds:
> > > >       put_cred(cred);
> > > >  fail:
> > > > +     fbfam_handle_attack(siginfo->si_signo);
> > >
> > > I don't think this is the right place for detecting a crash -- isn't
> > > this only for the "dumping core" condition? In other words, don't you
> > > want to do this in get_signal()'s "fatal" block? (i.e. very close to the
> > > do_coredump, but without the "should I dump?" check?)
> > >
> > > Hmm, but maybe I'm wrong? It looks like you're looking at noticing the
> > > process taking a signal from SIG_KERNEL_COREDUMP_MASK ?
> > >
> > > (Better yet: what are fatal conditions that do NOT match
> > > SIG_KERNEL_COREDUMP_MASK, and should those be covered?)
> > >
> > > Regardless, *this* looks like the only place without an LSM hook. And it
> > > doesn't seem unreasonable to add one here. I assume it would probably
> > > just take the siginfo pointer, which is also what you're checking.
> >
> > Good point, making this an LSM might be a good idea.
> >
> > > e.g. for include/linux/lsm_hook_defs.h:
> > >
> > > LSM_HOOK(int, 0, task_coredump, const kernel_siginfo_t *siginfo);
> >
> > I guess it should probably be an LSM_RET_VOID hook? And since, as you
> > said, it's not really semantically about core dumping, maybe it should
> > be named task_fatal_signal or something like that.
>
> If I understand correctly you propose to add a new LSM hook without return
> value and place it here:
>
> diff --git a/kernel/signal.c b/kernel/signal.c
> index a38b3edc6851..074492d23e98 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2751,6 +2751,8 @@ bool get_signal(struct ksignal *ksig)
>                         do_coredump(&ksig->info);
>                 }
>
> +               // Add the new LSM hook here
> +
>                 /*
>                  * Death signals, no core dump.
>                  */

It should probably be in the "if (sig_kernel_coredump(signr)) {"
branch. And I'm not sure whether it should be before or after
do_coredump() - if you do it after do_coredump(), the hook will have
to wait until the core dump file has been written, which may take a
little bit of time.
Jann Horn Sept. 14, 2020, 7:42 p.m. UTC | #7
On Sun, Sep 13, 2020 at 7:55 PM John Wood <john.wood@gmx.com> wrote:
> On Thu, Sep 10, 2020 at 11:10:38PM +0200, Jann Horn wrote:
> > On Thu, Sep 10, 2020 at 10:22 PM Kees Cook <keescook@chromium.org> wrote:
> > > To detect a fork brute force attack it is necessary to compute the
> > > crashing rate of the application. This calculation is performed in each
> > > fatal fail of a task, or in other words, when a core dump is triggered.
> > > If this rate shows that the application is crashing quickly, there is a
> > > clear signal that an attack is happening.
> > >
> > > Since the crashing rate is computed in milliseconds per fault, if this
> > > rate goes under a certain threshold a warning is triggered.
[...]
> > > +       delta_jiffies = get_jiffies_64() - stats->jiffies;
> > > +       delta_time = jiffies64_to_msecs(delta_jiffies);
> > > +       crashing_rate = delta_time / (u64)stats->faults;
> >
> > Do I see this correctly, is this computing the total runtime of this
> > process hierarchy divided by the total number of faults seen in this
> > process hierarchy? If so, you may want to reconsider whether that's
> > really the behavior you want. For example, if I configure the minimum
> > period between crashes to be 30s (as is the default in the sysctl
> > patch), and I try to attack a server that has been running without any
> > crashes for a month, I'd instantly be able to crash around
> > 30*24*60*60/30 = 86400 times before the detection kicks in. That seems
> > suboptimal.
>
> You are right. This is not the behaviour we want. So, for the next
> version it would be better to compute the crashing period as the time
> between two faults, or the time between the execve call and the first
> fault (first fault case).
>
> However, I am afraid of a premature detection if a child process fails
> twice in a short period.
>
> So, I think it would be a good idea add a new sysctl to setup a
> minimum number of faults before the time between faults starts to be
> computed. And so, the attack detection only will be triggered if the
> application crashes quickly but after a number of crashes.
>
> What do you think?

You could keep a list of the timestamps of the last five crashes or
so, and then take action if the last five crashes happened within
(5-1)*crash_period_limit time.
John Wood Sept. 15, 2020, 5:36 p.m. UTC | #8
Hi,

On Mon, Sep 14, 2020 at 09:39:10PM +0200, Jann Horn wrote:
> On Sun, Sep 13, 2020 at 6:56 PM John Wood <john.wood@gmx.com> wrote:
> > On Fri, Sep 11, 2020 at 02:01:56AM +0200, Jann Horn wrote:
> > > On Fri, Sep 11, 2020 at 1:49 AM Kees Cook <keescook@chromium.org> wrote:
> > > > On Thu, Sep 10, 2020 at 01:21:06PM -0700, Kees Cook wrote:
> > > > [...]
> > > > I don't think this is the right place for detecting a crash -- isn't
> > > > this only for the "dumping core" condition? In other words, don't you
> > > > want to do this in get_signal()'s "fatal" block? (i.e. very close to the
> > > > do_coredump, but without the "should I dump?" check?)
> > > >
> > > > Hmm, but maybe I'm wrong? It looks like you're looking at noticing the
> > > > process taking a signal from SIG_KERNEL_COREDUMP_MASK ?
> > > >
> > > > (Better yet: what are fatal conditions that do NOT match
> > > > SIG_KERNEL_COREDUMP_MASK, and should those be covered?)
> > > >
> > > > Regardless, *this* looks like the only place without an LSM hook. And it
> > > > doesn't seem unreasonable to add one here. I assume it would probably
> > > > just take the siginfo pointer, which is also what you're checking.
> > >
> > > Good point, making this an LSM might be a good idea.
> > >
> > > > e.g. for include/linux/lsm_hook_defs.h:
> > > >
> > > > LSM_HOOK(int, 0, task_coredump, const kernel_siginfo_t *siginfo);
> > >
> > > I guess it should probably be an LSM_RET_VOID hook? And since, as you
> > > said, it's not really semantically about core dumping, maybe it should
> > > be named task_fatal_signal or something like that.
> >
> > If I understand correctly you propose to add a new LSM hook without return
> > value and place it here:
> >
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index a38b3edc6851..074492d23e98 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -2751,6 +2751,8 @@ bool get_signal(struct ksignal *ksig)
> >                         do_coredump(&ksig->info);
> >                 }
> >
> > +               // Add the new LSM hook here
> > +
> >                 /*
> >                  * Death signals, no core dump.
> >                  */
>
> It should probably be in the "if (sig_kernel_coredump(signr)) {"
> branch. And I'm not sure whether it should be before or after
> do_coredump() - if you do it after do_coredump(), the hook will have
> to wait until the core dump file has been written, which may take a
> little bit of time.

But if the LSM hook is placed in the "if (sig_kernel_coredump(signr)) {"
branch, then only the following signals will be passed to it.

SIGQUIT, SIGILL, SIGTRAP, SIGABRT, SIGFPE, SIGSEGV, SIGBUS, SIGSYS,
SIGXCPU, SIGXFSZ, SIGEMT

The above signals are extracted from SIG_KERNEL_COREDUMP_MASK macro, and
are only related to coredump.

So, if we add a new LSM hook (named task_fatal_signal) to detect a fatal
signal it would be better to place it just above the if statement.

diff --git a/kernel/signal.c b/kernel/signal.c
index a38b3edc6851..406af87f2f96 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2736,6 +2736,8 @@ bool get_signal(struct ksignal *ksig)
                 */
                current->flags |= PF_SIGNALED;

+               // Place the new LSM hook here
+
                if (sig_kernel_coredump(signr)) {
                        if (print_fatal_signals)
                                print_fatal_signal(ksig->info.si_signo);

This way all the fatal signals are caught and we also avoid the commented
delay if a core dump is necessary.

Thanks,
John Wood
John Wood Sept. 15, 2020, 6:44 p.m. UTC | #9
On Mon, Sep 14, 2020 at 09:42:37PM +0200, Jann Horn wrote:
> On Sun, Sep 13, 2020 at 7:55 PM John Wood <john.wood@gmx.com> wrote:
> > On Thu, Sep 10, 2020 at 11:10:38PM +0200, Jann Horn wrote:
> > > > +       delta_jiffies = get_jiffies_64() - stats->jiffies;
> > > > +       delta_time = jiffies64_to_msecs(delta_jiffies);
> > > > +       crashing_rate = delta_time / (u64)stats->faults;
> > >
> > > Do I see this correctly, is this computing the total runtime of this
> > > process hierarchy divided by the total number of faults seen in this
> > > process hierarchy? If so, you may want to reconsider whether that's
> > > really the behavior you want. For example, if I configure the minimum
> > > period between crashes to be 30s (as is the default in the sysctl
> > > patch), and I try to attack a server that has been running without any
> > > crashes for a month, I'd instantly be able to crash around
> > > 30*24*60*60/30 = 86400 times before the detection kicks in. That seems
> > > suboptimal.
> >
> > You are right. This is not the behaviour we want. So, for the next
> > version it would be better to compute the crashing period as the time
> > between two faults, or the time between the execve call and the first
> > fault (first fault case).
> >
> > However, I am afraid of a premature detection if a child process fails
> > twice in a short period.
> >
> > So, I think it would be a good idea add a new sysctl to setup a
> > minimum number of faults before the time between faults starts to be
> > computed. And so, the attack detection only will be triggered if the
> > application crashes quickly but after a number of crashes.
> >
> > What do you think?
>
> You could keep a list of the timestamps of the last five crashes or
> so, and then take action if the last five crashes happened within
> (5-1)*crash_period_limit time.

Ok, your proposed solution seems a more clever one. Anyway I think that a
new sysctl for fine tuning the number of timestamps would be needed.

Thanks,
John Wood
diff mbox series

Patch

diff --git a/fs/coredump.c b/fs/coredump.c
index 76e7c10edfc0..d4ba4e1828d5 100644
--- a/fs/coredump.c
+++ b/fs/coredump.c
@@ -51,6 +51,7 @@ 
 #include "internal.h"
 
 #include <trace/events/sched.h>
+#include <fbfam/fbfam.h>
 
 int core_uses_pid;
 unsigned int core_pipe_limit;
@@ -825,6 +826,7 @@  void do_coredump(const kernel_siginfo_t *siginfo)
 fail_creds:
 	put_cred(cred);
 fail:
+	fbfam_handle_attack(siginfo->si_signo);
 	return;
 }
 
diff --git a/include/fbfam/fbfam.h b/include/fbfam/fbfam.h
index 2cfe51d2b0d5..9ac8e33d8291 100644
--- a/include/fbfam/fbfam.h
+++ b/include/fbfam/fbfam.h
@@ -12,10 +12,12 @@  extern struct ctl_table fbfam_sysctls[];
 int fbfam_fork(struct task_struct *child);
 int fbfam_execve(void);
 int fbfam_exit(void);
+int fbfam_handle_attack(int signal);
 #else
 static inline int fbfam_fork(struct task_struct *child) { return 0; }
 static inline int fbfam_execve(void) { return 0; }
 static inline int fbfam_exit(void) { return 0; }
+static inline int fbfam_handle_attack(int signal) { return 0; }
 #endif
 
 #endif /* _FBFAM_H_ */
diff --git a/security/fbfam/fbfam.c b/security/fbfam/fbfam.c
index 9be4639b72eb..3aa669e4ea51 100644
--- a/security/fbfam/fbfam.c
+++ b/security/fbfam/fbfam.c
@@ -4,7 +4,9 @@ 
 #include <linux/errno.h>
 #include <linux/gfp.h>
 #include <linux/jiffies.h>
+#include <linux/printk.h>
 #include <linux/refcount.h>
+#include <linux/signal.h>
 #include <linux/slab.h>
 
 /**
@@ -172,3 +174,40 @@  int fbfam_exit(void)
 	return 0;
 }
 
+/**
+ * fbfam_handle_attack() - Fork brute force attack detection.
+ * @signal: Signal number that causes the core dump.
+ *
+ * The crashing rate of an application is computed in milliseconds per fault in
+ * each crash. So, if this rate goes under a certain threshold there is a clear
+ * signal that the application is crashing quickly. At this moment, a fork brute
+ * force attack is happening.
+ *
+ * Return: -EFAULT if the current task doesn't have statistical data. Zero
+ *         otherwise.
+ */
+int fbfam_handle_attack(int signal)
+{
+	struct fbfam_stats *stats = current->fbfam_stats;
+	u64 delta_jiffies, delta_time;
+	u64 crashing_rate;
+
+	if (!stats)
+		return -EFAULT;
+
+	if (!(signal == SIGILL || signal == SIGBUS || signal == SIGKILL ||
+	      signal == SIGSEGV || signal == SIGSYS))
+		return 0;
+
+	stats->faults += 1;
+
+	delta_jiffies = get_jiffies_64() - stats->jiffies;
+	delta_time = jiffies64_to_msecs(delta_jiffies);
+	crashing_rate = delta_time / (u64)stats->faults;
+
+	if (crashing_rate < (u64)sysctl_crashing_rate_threshold)
+		pr_warn("fbfam: Fork brute force attack detected\n");
+
+	return 0;
+}
+