| Message ID | 20200910202107.3799376-5-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fork brute force attack mitigation (fbfam) | expand |
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)
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 --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, + }, + { } +}; +