| Message ID | 20200910202107.3799376-2-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 10:21 PM Kees Cook <keescook@chromium.org> wrote: > From: John Wood <john.wood@gmx.com> > > Add a menu entry under "Security options" to enable the "Fork brute > force attack mitigation" feature. [...] > +config FBFAM Please give this a more descriptive name than FBFAM. Some name where, if a random kernel developer sees an "#ifdef" with that name in some random piece of kernel code, they immediately have a rough idea for what kind of feature this is. Perhaps something like THROTTLE_FORK_CRASHES. Or something else that is equally descriptive. > + bool "Fork brute force attack mitigation" > + default n "default n" is superfluous and should AFAIK be omitted. > + help > + This is a user defense that detects any fork brute force attack > + based on the application's crashing rate. When this measure is > + triggered the fork system call is blocked. This help text claims that the mitigation will block fork(), but patch 6/6 actually kills the process hierarchy.
On Thu, Sep 10, 2020 at 01:21:02PM -0700, Kees Cook wrote: > From: John Wood <john.wood@gmx.com> > > Add a menu entry under "Security options" to enable the "Fork brute > force attack mitigation" feature. > > Signed-off-by: John Wood <john.wood@gmx.com> > --- > security/Kconfig | 1 + > security/fbfam/Kconfig | 10 ++++++++++ > 2 files changed, 11 insertions(+) > create mode 100644 security/fbfam/Kconfig > > diff --git a/security/Kconfig b/security/Kconfig > index 7561f6f99f1d..00a90e25b8d5 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -290,6 +290,7 @@ config LSM > If unsure, leave this as the default. > > source "security/Kconfig.hardening" > +source "security/fbfam/Kconfig" Given the layout you've chosen and the interface you've got, I think this should just be treated like a regular LSM. > > endmenu > > diff --git a/security/fbfam/Kconfig b/security/fbfam/Kconfig > new file mode 100644 > index 000000000000..bbe7f6aad369 > --- /dev/null > +++ b/security/fbfam/Kconfig > @@ -0,0 +1,10 @@ > +# SPDX-License-Identifier: GPL-2.0 > +config FBFAM To jump on the bikeshed: how about just calling this FORK_BRUTE_FORCE_DETECTION or FORK_BRUTE, and the directory could be "brute", etc. "fbfam" doesn't tell anyone anything.
On Thu, Sep 10, 2020 at 11:21:58PM +0200, Jann Horn wrote: > On Thu, Sep 10, 2020 at 10:21 PM Kees Cook <keescook@chromium.org> wrote: > > From: John Wood <john.wood@gmx.com> > > > > Add a menu entry under "Security options" to enable the "Fork brute > > force attack mitigation" feature. > [...] > > +config FBFAM > > Please give this a more descriptive name than FBFAM. Some name where, > if a random kernel developer sees an "#ifdef" with that name in some > random piece of kernel code, they immediately have a rough idea for > what kind of feature this is. > > Perhaps something like THROTTLE_FORK_CRASHES. Or something else that > is equally descriptive. Ok, understood. This will be fixed for the next version. Thanks. > > + bool "Fork brute force attack mitigation" > > + default n > > "default n" is superfluous and should AFAIK be omitted. Ok. I will remove it. Thanks. > > + help > > + This is a user defense that detects any fork brute force attack > > + based on the application's crashing rate. When this measure is > > + triggered the fork system call is blocked. > > This help text claims that the mitigation will block fork(), but patch > 6/6 actually kills the process hierarchy. Sorry, it's a mistake. It was the first idea but finally the implementation changed and this description not was modified. Apologies. It will be fixed for the next version. Thanks, John Wood
Hi, On Thu, Sep 10, 2020 at 04:18:08PM -0700, Kees Cook wrote: > On Thu, Sep 10, 2020 at 01:21:02PM -0700, Kees Cook wrote: > > From: John Wood <john.wood@gmx.com> > > > > Add a menu entry under "Security options" to enable the "Fork brute > > force attack mitigation" feature. > > > > Signed-off-by: John Wood <john.wood@gmx.com> > > --- > > security/Kconfig | 1 + > > security/fbfam/Kconfig | 10 ++++++++++ > > 2 files changed, 11 insertions(+) > > create mode 100644 security/fbfam/Kconfig > > > > diff --git a/security/Kconfig b/security/Kconfig > > index 7561f6f99f1d..00a90e25b8d5 100644 > > --- a/security/Kconfig > > +++ b/security/Kconfig > > @@ -290,6 +290,7 @@ config LSM > > If unsure, leave this as the default. > > > > source "security/Kconfig.hardening" > > +source "security/fbfam/Kconfig" > > Given the layout you've chosen and the interface you've got, I think > this should just be treated like a regular LSM. Yes, throughout the review it seems the most appropiate is treat this feature as a regular LSM. Thanks. > > > > endmenu > > > > diff --git a/security/fbfam/Kconfig b/security/fbfam/Kconfig > > new file mode 100644 > > index 000000000000..bbe7f6aad369 > > --- /dev/null > > +++ b/security/fbfam/Kconfig > > @@ -0,0 +1,10 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +config FBFAM > > To jump on the bikeshed: how about just calling this > FORK_BRUTE_FORCE_DETECTION or FORK_BRUTE, and the directory could be > "brute", etc. "fbfam" doesn't tell anyone anything. Understood. But how about use the fbfam abbreviation in the code? Like as function name prefix, struct name prefix, ... It would be better to use a more descriptive name in this scenario? It is not clear to me. > -- > Kees Cook Thanks, John Wood
On Thu, Sep 17, 2020 at 08:40:06PM +0200, John Wood wrote: > Hi, > > On Thu, Sep 10, 2020 at 04:18:08PM -0700, Kees Cook wrote: > > On Thu, Sep 10, 2020 at 01:21:02PM -0700, Kees Cook wrote: > > > From: John Wood <john.wood@gmx.com> > > > > > > Add a menu entry under "Security options" to enable the "Fork brute > > > force attack mitigation" feature. > > > > > > Signed-off-by: John Wood <john.wood@gmx.com> > > > --- > > > security/Kconfig | 1 + > > > security/fbfam/Kconfig | 10 ++++++++++ > > > 2 files changed, 11 insertions(+) > > > create mode 100644 security/fbfam/Kconfig > > > > > > diff --git a/security/Kconfig b/security/Kconfig > > > index 7561f6f99f1d..00a90e25b8d5 100644 > > > --- a/security/Kconfig > > > +++ b/security/Kconfig > > > @@ -290,6 +290,7 @@ config LSM > > > If unsure, leave this as the default. > > > > > > source "security/Kconfig.hardening" > > > +source "security/fbfam/Kconfig" > > > > Given the layout you've chosen and the interface you've got, I think > > this should just be treated like a regular LSM. > > Yes, throughout the review it seems the most appropiate is treat > this feature as a regular LSM. Thanks. > > > > > > > endmenu > > > > > > diff --git a/security/fbfam/Kconfig b/security/fbfam/Kconfig > > > new file mode 100644 > > > index 000000000000..bbe7f6aad369 > > > --- /dev/null > > > +++ b/security/fbfam/Kconfig > > > @@ -0,0 +1,10 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +config FBFAM > > > > To jump on the bikeshed: how about just calling this > > FORK_BRUTE_FORCE_DETECTION or FORK_BRUTE, and the directory could be > > "brute", etc. "fbfam" doesn't tell anyone anything. > > Understood. But how about use the fbfam abbreviation in the code? Like as > function name prefix, struct name prefix, ... It would be better to use a > more descriptive name in this scenario? It is not clear to me. I don't feel too strongly, but I think having the CONFIG roughly match the directory name, roughly match the function prefixes should be best. Maybe call the directory and function prefix "brute"?
On Thu, Sep 17, 2020 at 03:05:18PM -0700, Kees Cook wrote: > On Thu, Sep 17, 2020 at 08:40:06PM +0200, John Wood wrote: > > > To jump on the bikeshed: how about just calling this > > > FORK_BRUTE_FORCE_DETECTION or FORK_BRUTE, and the directory could be > > > "brute", etc. "fbfam" doesn't tell anyone anything. > > > > Understood. But how about use the fbfam abbreviation in the code? Like as > > function name prefix, struct name prefix, ... It would be better to use a > > more descriptive name in this scenario? It is not clear to me. > > I don't feel too strongly, but I think having the CONFIG roughly match > the directory name, roughly match the function prefixes should be best. > Maybe call the directory and function prefix "brute"? Thanks for the clarification. > -- > Kees Cook Regards, John Wood
diff --git a/security/Kconfig b/security/Kconfig index 7561f6f99f1d..00a90e25b8d5 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -290,6 +290,7 @@ config LSM If unsure, leave this as the default. source "security/Kconfig.hardening" +source "security/fbfam/Kconfig" endmenu diff --git a/security/fbfam/Kconfig b/security/fbfam/Kconfig new file mode 100644 index 000000000000..bbe7f6aad369 --- /dev/null +++ b/security/fbfam/Kconfig @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0 +config FBFAM + bool "Fork brute force attack mitigation" + default n + help + This is a user defense that detects any fork brute force attack + based on the application's crashing rate. When this measure is + triggered the fork system call is blocked. + + If you are unsure how to answer this question, answer N.