archive mirror
 help / color / mirror / Atom feed
From: John Wood <>
To: Kees Cook <>, Andi Kleen <>
Cc: John Wood <>, Jann Horn <>,
	Jonathan Corbet <>,
	James Morris <>,
	"Serge E. Hallyn" <>,
	Shuah Khan <>,
	Thomas Gleixner <>,
	Ingo Molnar <>, Borislav Petkov <>,, "H. Peter Anvin" <>,
	Arnd Bergmann <>,,
	Greg Kroah-Hartman <>,
	Randy Dunlap <>,
	Andrew Morton <>,,,,,,,
Subject: Re: [PATCH v8 0/8] Fork brute force attack mitigation
Date: Fri, 11 Jun 2021 17:41:23 +0200	[thread overview]
Message-ID: <20210611154123.GA3057@ubuntu> (raw)
In-Reply-To: <202106090951.8C1B5BAD@keescook>

On Wed, Jun 09, 2021 at 09:52:29AM -0700, Kees Cook wrote:
> On Tue, Jun 08, 2021 at 04:38:15PM -0700, Andi Kleen wrote:
> >
> > On 6/8/2021 4:19 PM, Kees Cook wrote:
> > > On Sat, Jun 05, 2021 at 05:03:57PM +0200, John Wood wrote:
> > > > [...]
> > > > the kselftest to avoid the detection ;) ). So, in this version, to track
> > > > all the statistical data (info related with application crashes), the
> > > > extended attributes feature for the executable files are used. The xattr is
> > > > also used to mark the executables as "not allowed" when an attack is
> > > > detected. Then, the execve system call rely on this flag to avoid following
> > > > executions of this file.
> > >
> > > I have some concerns about this being actually usable and not creating
> > > DoS situations. For example, let's say an attacker had found a hard-to-hit
> > > bug in "sudo", and starts brute forcing it. When the brute LSM notices,
> > > it'll make "sudo" unusable for the entire system, yes?
> > >
> > > And a reboot won't fix it, either, IIUC.
> > >
> > The whole point of the mitigation is to trade potential attacks against DOS.
> >
> > If you're worried about DOS the whole thing is not for you.
> Right, but there's no need to make a system unusable for everyone else.
> There's nothing here that relaxes the defense (i.e. stop spawning apache
> for 10 minutes). Writing it to disk with nothing that undoes it seems a
> bit too much. :)

Here I have merge the first reply.

> It seems like there is a need to track "user" running "prog", and have
> that be timed out. Are there use-cases here where that wouldn't be
> sufficient?

Ok, what do you think of the following proposal:

Add an uid_t field to the structure saved in the xattr. So this struct
contains now

faults: Number of crashes.
nsecs: Last crash timestamp as the number of nanoseconds in the
       International Atomic Time (TAI) reference.
period: Crash period's moving average.
flags: Statistics flags.
uid: User id not allowed to run the executable.

The logic would be the following:

1. faults, nsecs and period are updated in every crash and is a common info
   for all the users.
2. If the max number of faults is reached, it is "not allowed" to run the
   executable by any user. This condition blocks the file until root clear
   the xattr. No timeout.
3. When an attack is detected the uid of the user that is running the app
   is saved in the xattr and the executable is marked as "not allowed" to
   run by this user. The "not allowed" state has a timeout (more below).
4. When someone tries to run the executable, if his uid is different from
   the uid saved in the xattr, then the operation is "allowed".
5. When someone tries to run the executable, if his uid is equal to the
   uid saved in the xattr, then the operation is "not allowed". This user
   is banned for a timeout.
6. When someone tries to run the executable and the timeout has expired,
   the operation is "allowed" and the saved uid is removed.
7. If the executable crashes again when it is run by a user different from
   the one saved in the xattr (and the timeout has no expired), the file
   is marked as "not allowed" to run by any user. All users are banned for
   a timeout.

The timeout: I think there are two options here.

1. A fixed timeout set by a sysctl attribute.
2. A dynamic timeout calculated from the info stored in the xattr. The
   timeout would be the needed period to guarantee that when the app is
   run again and it crashes, the attack detection will not be triggered.
   To be more clear I expose the formulas:

   Mathematically the application crash period's EMA can be expressed as

   period_ema[i] = period[i] * weight + period_ema[i - 1] * (1 - weight)

   If we isolate period:

   period[i] = (period_ema[i] - period_ema[i - 1] * (1 - weight)) / weight

   Where period_ema[i] is the "crash_period_threshold", period_ema[i - 1]
   is the last period ema saved in the xattr and period[i] is the dynamic

As a final point. Possibly there are more cases but the logic would be the
one explained. I think that it is not necessary to save the uid for every
user that crashes the app nor the crashes info for every user. If more
than one user crashes the application, something "bad" is happening. So,
all users are banned for a timeout. This way the info saved in the xattr
has a fixed size and we prevent an attacker from abusing this size.

I hope this proposal can be enough. What do you think?

John Wood.

      reply	other threads:[~2021-06-11 15:43 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-05 15:03 [PATCH v8 0/8] Fork brute force attack mitigation John Wood
2021-06-05 15:03 ` [PATCH v8 1/8] security: Add LSM hook at the point where a task gets a fatal signal John Wood
2021-06-05 15:03 ` [PATCH v8 2/8] security/brute: Define a LSM and add sysctl attributes John Wood
2021-06-05 15:04 ` [PATCH v8 3/8] security/brute: Detect a brute force attack John Wood
2021-06-05 15:04 ` [PATCH v8 4/8] security/brute: Mitigate " John Wood
2021-06-05 15:04 ` [PATCH v8 5/8] security/brute: Notify to userspace "task killed" John Wood
2021-06-05 15:04 ` [PATCH v8 6/8] selftests/brute: Add tests for the Brute LSM John Wood
2021-06-05 15:04 ` [PATCH v8 7/8] Documentation: Add documentation " John Wood
2021-06-05 15:04 ` [PATCH v8 8/8] MAINTAINERS: Add a new entry " John Wood
2021-06-08 23:19 ` [PATCH v8 0/8] Fork brute force attack mitigation Kees Cook
2021-06-08 23:38   ` Andi Kleen
2021-06-09 16:52     ` Kees Cook
2021-06-11 15:41       ` John Wood [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210611154123.GA3057@ubuntu \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).