diff mbox series

[RFC,4/6] security/fbfam: Add a new sysctl to control the crashing rate threshold

Message ID 20200910202107.3799376-5-keescook@chromium.org
State New, archived
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>

This is a previous step to add the detection feature.

A fork brute force attack will be detected when an application crashes
quickly. Since, a rate can be defined as a time per fault, add a new
sysctl to control the crashing rate threshold.

This way, each system can tune the detection's sensibility adjusting the
milliseconds per fault. So, if the application's crashing rate falls
under this threshold an attack will be detected. So, the higher this
value, the faster an attack will be detected.

Signed-off-by: John Wood <john.wood@gmx.com>
---
 include/fbfam/fbfam.h   |  4 ++++
 kernel/sysctl.c         |  9 +++++++++
 security/fbfam/Makefile |  1 +
 security/fbfam/fbfam.c  | 11 +++++++++++
 security/fbfam/sysctl.c | 20 ++++++++++++++++++++
 5 files changed, 45 insertions(+)
 create mode 100644 security/fbfam/sysctl.c

Comments

Kees Cook Sept. 10, 2020, 11:14 p.m. UTC | #1
On Thu, Sep 10, 2020 at 01:21:05PM -0700, Kees Cook wrote:
> From: John Wood <john.wood@gmx.com>
> 
> This is a previous step to add the detection feature.
> 
> A fork brute force attack will be detected when an application crashes
> quickly. Since, a rate can be defined as a time per fault, add a new
> sysctl to control the crashing rate threshold.
> 
> This way, each system can tune the detection's sensibility adjusting the
> milliseconds per fault. So, if the application's crashing rate falls
> under this threshold an attack will be detected. So, the higher this
> value, the faster an attack will be detected.
> 
> Signed-off-by: John Wood <john.wood@gmx.com>
> ---
>  include/fbfam/fbfam.h   |  4 ++++
>  kernel/sysctl.c         |  9 +++++++++
>  security/fbfam/Makefile |  1 +
>  security/fbfam/fbfam.c  | 11 +++++++++++
>  security/fbfam/sysctl.c | 20 ++++++++++++++++++++
>  5 files changed, 45 insertions(+)
>  create mode 100644 security/fbfam/sysctl.c
> 
> diff --git a/include/fbfam/fbfam.h b/include/fbfam/fbfam.h
> index b5b7d1127a52..2cfe51d2b0d5 100644
> --- a/include/fbfam/fbfam.h
> +++ b/include/fbfam/fbfam.h
> @@ -3,8 +3,12 @@
>  #define _FBFAM_H_
>  
>  #include <linux/sched.h>
> +#include <linux/sysctl.h>
>  
>  #ifdef CONFIG_FBFAM
> +#ifdef CONFIG_SYSCTL
> +extern struct ctl_table fbfam_sysctls[];
> +#endif

Instead of doing the extern and adding to sysctl.c, this can all be done
directly (dynamically) from the fbfam.c file instead.

>  int fbfam_fork(struct task_struct *child);
>  int fbfam_execve(void);
>  int fbfam_exit(void);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 09e70ee2332e..c3b4d737bef3 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -77,6 +77,8 @@
>  #include <linux/uaccess.h>
>  #include <asm/processor.h>
>  
> +#include <fbfam/fbfam.h>
> +
>  #ifdef CONFIG_X86
>  #include <asm/nmi.h>
>  #include <asm/stacktrace.h>
> @@ -2660,6 +2662,13 @@ static struct ctl_table kern_table[] = {
>  		.extra1		= SYSCTL_ZERO,
>  		.extra2		= SYSCTL_ONE,
>  	},
> +#endif
> +#ifdef CONFIG_FBFAM
> +	{
> +		.procname	= "fbfam",
> +		.mode		= 0555,
> +		.child		= fbfam_sysctls,
> +	},
>  #endif
>  	{ }
>  };
> diff --git a/security/fbfam/Makefile b/security/fbfam/Makefile
> index f4b9f0b19c44..b8d5751ecea4 100644
> --- a/security/fbfam/Makefile
> +++ b/security/fbfam/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_FBFAM) += fbfam.o
> +obj-$(CONFIG_SYSCTL) += sysctl.o
> diff --git a/security/fbfam/fbfam.c b/security/fbfam/fbfam.c
> index 0387f95f6408..9be4639b72eb 100644
> --- a/security/fbfam/fbfam.c
> +++ b/security/fbfam/fbfam.c
> @@ -7,6 +7,17 @@
>  #include <linux/refcount.h>
>  #include <linux/slab.h>
>  
> +/**
> + * sysctl_crashing_rate_threshold - Crashing rate threshold.
> + *
> + * The rate's units are in milliseconds per fault.
> + *
> + * A fork brute force attack will be detected if the application's crashing rate
> + * falls under this threshold. So, the higher this value, the faster an attack
> + * will be detected.
> + */
> +unsigned long sysctl_crashing_rate_threshold = 30000;

I would move the sysctls here, instead. (Also, the above should be
const.)

> +
>  /**
>   * struct fbfam_stats - Fork brute force attack mitigation statistics.
>   * @refc: Reference counter.
> diff --git a/security/fbfam/sysctl.c b/security/fbfam/sysctl.c
> new file mode 100644
> index 000000000000..430323ad8e9f
> --- /dev/null
> +++ b/security/fbfam/sysctl.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/sysctl.h>
> +
> +extern unsigned long sysctl_crashing_rate_threshold;
> +static unsigned long ulong_one = 1;
> +static unsigned long ulong_max = ULONG_MAX;
> +
> +struct ctl_table fbfam_sysctls[] = {
> +	{
> +		.procname	= "crashing_rate_threshold",
> +		.data		= &sysctl_crashing_rate_threshold,
> +		.maxlen		= sizeof(sysctl_crashing_rate_threshold),
> +		.mode		= 0644,
> +		.proc_handler	= proc_doulongvec_minmax,
> +		.extra1		= &ulong_one,
> +		.extra2		= &ulong_max,
> +	},
> +	{ }
> +};

I wouldn't bother splitting this into a separate file. (Just leave it in
fbfam.c)
John Wood Sept. 13, 2020, 2:33 p.m. UTC | #2
Hi, more inline.

On Thu, Sep 10, 2020 at 04:14:38PM -0700, Kees Cook wrote:
> > diff --git a/include/fbfam/fbfam.h b/include/fbfam/fbfam.h
> > index b5b7d1127a52..2cfe51d2b0d5 100644
> > --- a/include/fbfam/fbfam.h
> > +++ b/include/fbfam/fbfam.h
> > @@ -3,8 +3,12 @@
> >  #define _FBFAM_H_
> >
> >  #include <linux/sched.h>
> > +#include <linux/sysctl.h>
> >
> >  #ifdef CONFIG_FBFAM
> > +#ifdef CONFIG_SYSCTL
> > +extern struct ctl_table fbfam_sysctls[];
> > +#endif
>
> Instead of doing the extern and adding to sysctl.c, this can all be done
> directly (dynamically) from the fbfam.c file instead.

Like Yama do in the yama_init_sysctl() function? As a reference code.

> >  int fbfam_fork(struct task_struct *child);
> >  int fbfam_execve(void);
> >  int fbfam_exit(void);
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 09e70ee2332e..c3b4d737bef3 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -77,6 +77,8 @@
> >  #include <linux/uaccess.h>
> >  #include <asm/processor.h>
> >
> > +#include <fbfam/fbfam.h>
> > +
> >  #ifdef CONFIG_X86
> >  #include <asm/nmi.h>
> >  #include <asm/stacktrace.h>
> > @@ -2660,6 +2662,13 @@ static struct ctl_table kern_table[] = {
> >  		.extra1		= SYSCTL_ZERO,
> >  		.extra2		= SYSCTL_ONE,
> >  	},
> > +#endif
> > +#ifdef CONFIG_FBFAM
> > +	{
> > +		.procname	= "fbfam",
> > +		.mode		= 0555,
> > +		.child		= fbfam_sysctls,
> > +	},
> >  #endif
> >  	{ }
> >  };
> > diff --git a/security/fbfam/Makefile b/security/fbfam/Makefile
> > index f4b9f0b19c44..b8d5751ecea4 100644
> > --- a/security/fbfam/Makefile
> > +++ b/security/fbfam/Makefile
> > @@ -1,2 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  obj-$(CONFIG_FBFAM) += fbfam.o
> > +obj-$(CONFIG_SYSCTL) += sysctl.o
> > diff --git a/security/fbfam/fbfam.c b/security/fbfam/fbfam.c
> > index 0387f95f6408..9be4639b72eb 100644
> > --- a/security/fbfam/fbfam.c
> > +++ b/security/fbfam/fbfam.c
> > @@ -7,6 +7,17 @@
> >  #include <linux/refcount.h>
> >  #include <linux/slab.h>
> >
> > +/**
> > + * sysctl_crashing_rate_threshold - Crashing rate threshold.
> > + *
> > + * The rate's units are in milliseconds per fault.
> > + *
> > + * A fork brute force attack will be detected if the application's crashing rate
> > + * falls under this threshold. So, the higher this value, the faster an attack
> > + * will be detected.
> > + */
> > +unsigned long sysctl_crashing_rate_threshold = 30000;
>
> I would move the sysctls here, instead. (Also, the above should be
> const.)

If the above variable is const how the sysctl interface can modify it?
I think it would be better to declare it as __read_mostly instead. What
do you think?

unsigned long sysctl_crashing_rate_threshold __read_mostly = 30000;

> > +
> >  /**
> >   * struct fbfam_stats - Fork brute force attack mitigation statistics.
> >   * @refc: Reference counter.
> > diff --git a/security/fbfam/sysctl.c b/security/fbfam/sysctl.c
> > new file mode 100644
> > index 000000000000..430323ad8e9f
> > --- /dev/null
> > +++ b/security/fbfam/sysctl.c
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/sysctl.h>
> > +
> > +extern unsigned long sysctl_crashing_rate_threshold;
> > +static unsigned long ulong_one = 1;
> > +static unsigned long ulong_max = ULONG_MAX;
> > +
> > +struct ctl_table fbfam_sysctls[] = {
> > +	{
> > +		.procname	= "crashing_rate_threshold",
> > +		.data		= &sysctl_crashing_rate_threshold,
> > +		.maxlen		= sizeof(sysctl_crashing_rate_threshold),
> > +		.mode		= 0644,
> > +		.proc_handler	= proc_doulongvec_minmax,
> > +		.extra1		= &ulong_one,
> > +		.extra2		= &ulong_max,
> > +	},
> > +	{ }
> > +};
>
> I wouldn't bother splitting this into a separate file. (Just leave it in
> fbfam.c)
>
> --
> Kees Cook

Thanks,
John Wood
diff mbox series

Patch

diff --git a/include/fbfam/fbfam.h b/include/fbfam/fbfam.h
index b5b7d1127a52..2cfe51d2b0d5 100644
--- a/include/fbfam/fbfam.h
+++ b/include/fbfam/fbfam.h
@@ -3,8 +3,12 @@ 
 #define _FBFAM_H_
 
 #include <linux/sched.h>
+#include <linux/sysctl.h>
 
 #ifdef CONFIG_FBFAM
+#ifdef CONFIG_SYSCTL
+extern struct ctl_table fbfam_sysctls[];
+#endif
 int fbfam_fork(struct task_struct *child);
 int fbfam_execve(void);
 int fbfam_exit(void);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 09e70ee2332e..c3b4d737bef3 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -77,6 +77,8 @@ 
 #include <linux/uaccess.h>
 #include <asm/processor.h>
 
+#include <fbfam/fbfam.h>
+
 #ifdef CONFIG_X86
 #include <asm/nmi.h>
 #include <asm/stacktrace.h>
@@ -2660,6 +2662,13 @@  static struct ctl_table kern_table[] = {
 		.extra1		= SYSCTL_ZERO,
 		.extra2		= SYSCTL_ONE,
 	},
+#endif
+#ifdef CONFIG_FBFAM
+	{
+		.procname	= "fbfam",
+		.mode		= 0555,
+		.child		= fbfam_sysctls,
+	},
 #endif
 	{ }
 };
diff --git a/security/fbfam/Makefile b/security/fbfam/Makefile
index f4b9f0b19c44..b8d5751ecea4 100644
--- a/security/fbfam/Makefile
+++ b/security/fbfam/Makefile
@@ -1,2 +1,3 @@ 
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_FBFAM) += fbfam.o
+obj-$(CONFIG_SYSCTL) += sysctl.o
diff --git a/security/fbfam/fbfam.c b/security/fbfam/fbfam.c
index 0387f95f6408..9be4639b72eb 100644
--- a/security/fbfam/fbfam.c
+++ b/security/fbfam/fbfam.c
@@ -7,6 +7,17 @@ 
 #include <linux/refcount.h>
 #include <linux/slab.h>
 
+/**
+ * sysctl_crashing_rate_threshold - Crashing rate threshold.
+ *
+ * The rate's units are in milliseconds per fault.
+ *
+ * A fork brute force attack will be detected if the application's crashing rate
+ * falls under this threshold. So, the higher this value, the faster an attack
+ * will be detected.
+ */
+unsigned long sysctl_crashing_rate_threshold = 30000;
+
 /**
  * struct fbfam_stats - Fork brute force attack mitigation statistics.
  * @refc: Reference counter.
diff --git a/security/fbfam/sysctl.c b/security/fbfam/sysctl.c
new file mode 100644
index 000000000000..430323ad8e9f
--- /dev/null
+++ b/security/fbfam/sysctl.c
@@ -0,0 +1,20 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/sysctl.h>
+
+extern unsigned long sysctl_crashing_rate_threshold;
+static unsigned long ulong_one = 1;
+static unsigned long ulong_max = ULONG_MAX;
+
+struct ctl_table fbfam_sysctls[] = {
+	{
+		.procname	= "crashing_rate_threshold",
+		.data		= &sysctl_crashing_rate_threshold,
+		.maxlen		= sizeof(sysctl_crashing_rate_threshold),
+		.mode		= 0644,
+		.proc_handler	= proc_doulongvec_minmax,
+		.extra1		= &ulong_one,
+		.extra2		= &ulong_max,
+	},
+	{ }
+};
+