diff mbox series

[RFC,1/6] security/fbfam: Add a Kconfig to enable the fbfam feature

Message ID 20200910202107.3799376-2-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>

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

Comments

Jann Horn Sept. 10, 2020, 9:21 p.m. UTC | #1
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.
Kees Cook Sept. 10, 2020, 11:18 p.m. UTC | #2
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.
John Wood Sept. 17, 2020, 5:32 p.m. UTC | #3
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
John Wood Sept. 17, 2020, 6:40 p.m. UTC | #4
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
Kees Cook Sept. 17, 2020, 10:05 p.m. UTC | #5
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"?
John Wood Sept. 18, 2020, 2:50 p.m. UTC | #6
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 mbox series

Patch

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.