linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
@ 2021-07-01 23:55 Alexander Lobakin
  2021-07-02 14:59 ` John Wood
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Lobakin @ 2021-07-01 23:55 UTC (permalink / raw)
  To: John Wood
  Cc: Alexander Lobakin, Kees Cook, Jann Horn, Jonathan Corbet,
	James Morris, Serge E. Hallyn, Shuah Khan, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann,
	Andi Kleen, valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
	Andrew Morton, linux-doc, linux-kernel, linux-security-module,
	linux-kselftest, linux-arch, linux-hardening, kernel-hardening

Hi,

From: John Wood <john.wood@gmx.com>
Date: Sat, 5 Jun 2021 17:04:00 +0200

> For a correct management of a fork brute force attack it is necessary to
> track all the information related to the application crashes. To do so,
> use the extended attributes (xattr) of the executable files and define a
> statistical data structure to hold all the necessary information shared
> by all the fork hierarchy processes. This info is the number of crashes,
> the last crash timestamp and the crash period's moving average.
>
> The same can be achieved using a pointer to the fork hierarchy
> statistical data held by the task_struct structure. But this has an
> important drawback: a brute force attack that happens through the execve
> system call losts the faults info since these statistics are freed when
> the fork hierarchy disappears. Using this method makes not possible to
> manage this attack type that can be successfully treated using extended
> attributes.
>
> Also, to avoid false positives during the attack detection it is
> necessary to narrow the possible cases. So, only the following scenarios
> are taken into account:
>
> 1.- Launching (fork()/exec()) a setuid/setgid process repeatedly until a
>     desirable memory layout is got (e.g. Stack Clash).
> 2.- Connecting to an exec()ing network daemon (e.g. xinetd) repeatedly
>     until a desirable memory layout is got (e.g. what CTFs do for simple
>     network service).
> 3.- Launching processes without exec() (e.g. Android Zygote) and
>     exposing state to attack a sibling.
> 4.- Connecting to a fork()ing network daemon (e.g. apache) repeatedly
>     until the previously shared memory layout of all the other children
>     is exposed (e.g. kind of related to HeartBleed).
>
> In each case, a privilege boundary has been crossed:
>
> Case 1: setuid/setgid process
> Case 2: network to local
> Case 3: privilege changes
> Case 4: network to local
>
> To mark that a privilege boundary has been crossed it is only necessary
> to create a new stats for the executable file via the extended attribute
> and only if it has no previous statistical data. This is done using four
> different LSM hooks, one per privilege boundary:
>
> setuid/setgid process --> bprm_creds_from_file hook (based on secureexec
>                           flag).
> network to local -------> socket_accept hook (taking into account only
>                           external connections).
> privilege changes ------> task_fix_setuid and task_fix_setgid hooks.
>
> To detect a brute force attack it is necessary that the executable file
> statistics be updated in every fatal crash and the most important data
> to update is the application crash period. To do so, use the new
> "task_fatal_signal" LSM hook added in a previous step.
>
> The application crash period must be a value that is not prone to change
> due to spurious data and follows the real crash period. So, to compute
> it, the exponential moving average (EMA) is used.
>
> Based on the updated statistics two different attacks can be handled. A
> slow brute force attack that is detected if the maximum number of faults
> per fork hierarchy is reached and a fast brute force attack that is
> detected if the application crash period falls below a certain
> threshold.
>
> Moreover, only the signals delivered by the kernel are taken into
> account with the exception of the SIGABRT signal since the latter is
> used by glibc for stack canary, malloc, etc failures, which may indicate
> that a mitigation has been triggered.
>
> Signed-off-by: John Wood <john.wood@gmx.com>
>
> <snip>
>
> +static int brute_get_xattr_stats(struct dentry *dentry, struct inode *inode,
> +				 struct brute_stats *stats)
> +{
> +	int rc;
> +	struct brute_raw_stats raw_stats;
> +
> +	rc = __vfs_getxattr(dentry, inode, XATTR_NAME_BRUTE, &raw_stats,
> +			    sizeof(raw_stats));
> +	if (rc < 0)
> +		return rc;
> +
> +	stats->faults = le32_to_cpu(raw_stats.faults);
> +	stats->nsecs = le64_to_cpu(raw_stats.nsecs);
> +	stats->period = le64_to_cpu(raw_stats.period);
> +	stats->flags = raw_stats.flags;
> +	return 0;
> +}
>
> <snip>
>
> +static int brute_task_execve(struct linux_binprm *bprm, struct file *file)
> +{
> +	struct dentry *dentry = file_dentry(bprm->file);
> +	struct inode *inode = file_inode(bprm->file);
> +	struct brute_stats stats;
> +	int rc;
> +
> +	inode_lock(inode);
> +	rc = brute_get_xattr_stats(dentry, inode, &stats);
> +	if (WARN_ON_ONCE(rc && rc != -ENODATA))
> +		goto unlock;

I think I caught a problem here. Have you tested this with
initramfs?

According to init/do_mount.c's
init_rootfs()/rootfs_init_fs_context(), when `root=` cmdline
parameter is not empty, kernel creates rootfs of type ramfs
(tmpfs otherwise).
The thing about ramfs is that it doesn't support xattrs.

I'm running this v8 on a regular PC with initramfs and having
`root=` in cmdline, and Brute doesn't allow the kernel to run
any init processes (/init, /sbin/init, ...) with err == -95
(-EOPNOTSUPP) -- I'm getting a

WARNING: CPU: 0 PID: 173 at brute_task_execve+0x15d/0x200
<snip>
Failed to execute /init (error -95)

and so on (and a panic at the end).

If I omit `root=` from cmdline, then the kernel runs init process
just fine -- I guess because initramfs is then placed inside tmpfs
with xattr support.

As for me, this ramfs/tmpfs selection based on `root=` presence
is ridiculous and I don't see or know any reasons behind that.
But that's another story, and ramfs might be not the only one
system without xattr support.
I think Brute should have a fallback here, e.g. it could simply
ignore files from xattr-incapable filesystems instead of such
WARNING splats and stuff.

> +
> +	if (rc == -ENODATA && bprm->secureexec) {
> +		brute_reset_stats(&stats);
> +		rc = brute_set_xattr_stats(dentry, inode, &stats);
> +		if (WARN_ON_ONCE(rc))
> +			goto unlock;
> +	}
> +
> +	rc = 0;
> +unlock:
> +	inode_unlock(inode);
> +	return rc;
> +}
> +
>
> <snip>

Thanks,
Al


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
  2021-07-01 23:55 [PATCH v8 3/8] security/brute: Detect a brute force attack Alexander Lobakin
@ 2021-07-02 14:59 ` John Wood
  2021-07-02 17:08   ` Alexander Lobakin
  0 siblings, 1 reply; 7+ messages in thread
From: John Wood @ 2021-07-02 14:59 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: John Wood, Kees Cook, Jann Horn, Jonathan Corbet, James Morris,
	Serge E. Hallyn, Shuah Khan, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann, Andi Kleen,
	valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
	Andrew Morton, linux-doc, linux-kernel, linux-security-module,
	linux-kselftest, linux-arch, linux-hardening, kernel-hardening

Hi,

On Thu, Jul 01, 2021 at 11:55:14PM +0000, Alexander Lobakin wrote:
> Hi,
>
> From: John Wood <john.wood@gmx.com>
> Date: Sat, 5 Jun 2021 17:04:00 +0200
>
> > +static int brute_task_execve(struct linux_binprm *bprm, struct file *file)
> > +{
> > +	struct dentry *dentry = file_dentry(bprm->file);
> > +	struct inode *inode = file_inode(bprm->file);
> > +	struct brute_stats stats;
> > +	int rc;
> > +
> > +	inode_lock(inode);
> > +	rc = brute_get_xattr_stats(dentry, inode, &stats);
> > +	if (WARN_ON_ONCE(rc && rc != -ENODATA))
> > +		goto unlock;
>
> I think I caught a problem here. Have you tested this with
> initramfs?

No, it has not been tested with initramfs :(

> According to init/do_mount.c's
> init_rootfs()/rootfs_init_fs_context(), when `root=` cmdline
> parameter is not empty, kernel creates rootfs of type ramfs
> (tmpfs otherwise).
> The thing about ramfs is that it doesn't support xattrs.

It is a known issue that systems without xattr support are not
suitable for Brute (there are a note in the documentation).
However, the purpose is not to panic the system :(

> I'm running this v8 on a regular PC with initramfs and having
> `root=` in cmdline, and Brute doesn't allow the kernel to run
> any init processes (/init, /sbin/init, ...) with err == -95
> (-EOPNOTSUPP) -- I'm getting a
>
> WARNING: CPU: 0 PID: 173 at brute_task_execve+0x15d/0x200
> <snip>
> Failed to execute /init (error -95)
>
> and so on (and a panic at the end).
>
> If I omit `root=` from cmdline, then the kernel runs init process
> just fine -- I guess because initramfs is then placed inside tmpfs
> with xattr support.
>
> As for me, this ramfs/tmpfs selection based on `root=` presence
> is ridiculous and I don't see or know any reasons behind that.
> But that's another story, and ramfs might be not the only one
> system without xattr support.
> I think Brute should have a fallback here, e.g. it could simply
> ignore files from xattr-incapable filesystems instead of such
> WARNING splats and stuff.

Ok, it seems reasonable to me: if the file system doesn't support
xattr, but Brute is enabled, Brute will do nothing and the system
will work normally.

I will work on it for the next version.
Thanks for the feedback.

John Wood

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
  2021-07-02 14:59 ` John Wood
@ 2021-07-02 17:08   ` Alexander Lobakin
  2021-07-03 10:59     ` John Wood
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Lobakin @ 2021-07-02 17:08 UTC (permalink / raw)
  To: John Wood
  Cc: Alexander Lobakin, Kees Cook, Jann Horn, Jonathan Corbet,
	James Morris, Serge E. Hallyn, Shuah Khan, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann,
	Andi Kleen, valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
	Andrew Morton, linux-doc, linux-kernel, linux-security-module,
	linux-kselftest, linux-arch, linux-hardening, kernel-hardening

From: John Wood <john.wood@gmx.com>
Date: Fri, 2 Jul 2021 16:59:54 +0200

> Hi,
>
> On Thu, Jul 01, 2021 at 11:55:14PM +0000, Alexander Lobakin wrote:
> > Hi,
> >
> > From: John Wood <john.wood@gmx.com>
> > Date: Sat, 5 Jun 2021 17:04:00 +0200
> >
> > > +static int brute_task_execve(struct linux_binprm *bprm, struct file *file)
> > > +{
> > > +	struct dentry *dentry = file_dentry(bprm->file);
> > > +	struct inode *inode = file_inode(bprm->file);
> > > +	struct brute_stats stats;
> > > +	int rc;
> > > +
> > > +	inode_lock(inode);
> > > +	rc = brute_get_xattr_stats(dentry, inode, &stats);
> > > +	if (WARN_ON_ONCE(rc && rc != -ENODATA))
> > > +		goto unlock;
> >
> > I think I caught a problem here. Have you tested this with
> > initramfs?
>
> No, it has not been tested with initramfs :(
>
> > According to init/do_mount.c's
> > init_rootfs()/rootfs_init_fs_context(), when `root=` cmdline
> > parameter is not empty, kernel creates rootfs of type ramfs
> > (tmpfs otherwise).
> > The thing about ramfs is that it doesn't support xattrs.
>
> It is a known issue that systems without xattr support are not
> suitable for Brute (there are a note in the documentation).
> However, the purpose is not to panic the system :(
>
> > I'm running this v8 on a regular PC with initramfs and having
> > `root=` in cmdline, and Brute doesn't allow the kernel to run
> > any init processes (/init, /sbin/init, ...) with err == -95
> > (-EOPNOTSUPP) -- I'm getting a
> >
> > WARNING: CPU: 0 PID: 173 at brute_task_execve+0x15d/0x200
> > <snip>
> > Failed to execute /init (error -95)
> >
> > and so on (and a panic at the end).
> >
> > If I omit `root=` from cmdline, then the kernel runs init process
> > just fine -- I guess because initramfs is then placed inside tmpfs
> > with xattr support.
> >
> > As for me, this ramfs/tmpfs selection based on `root=` presence
> > is ridiculous and I don't see or know any reasons behind that.
> > But that's another story, and ramfs might be not the only one
> > system without xattr support.
> > I think Brute should have a fallback here, e.g. it could simply
> > ignore files from xattr-incapable filesystems instead of such
> > WARNING splats and stuff.
>
> Ok, it seems reasonable to me: if the file system doesn't support
> xattr, but Brute is enabled, Brute will do nothing and the system
> will work normally.

On the other hand, it leaves a potentional window for attackers to
perform brute force from xattr-incapable filesystems. So at the end
of the day I think that the current implementation (a strong
rejection of such filesystems) is way more secure than having
a fallback I proposed.

I'm planning to make a patch which will eliminate such weird rootfs
type selection and just always use more feature-rich tmpfs if it's
compiled in. So, as an alternative, you could add it to your series
as a preparatory change and just add a Kconfig dependency on
CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
without messing with any fallbacks at all.
What do you think?

> I will work on it for the next version.
> Thanks for the feedback.
>
> John Wood

Thanks,
Al


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
  2021-07-02 17:08   ` Alexander Lobakin
@ 2021-07-03 10:59     ` John Wood
  2021-07-04 14:01       ` John Wood
  0 siblings, 1 reply; 7+ messages in thread
From: John Wood @ 2021-07-03 10:59 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: John Wood, Kees Cook, Jann Horn, Jonathan Corbet, James Morris,
	Serge E. Hallyn, Shuah Khan, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann, Andi Kleen,
	valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
	Andrew Morton, linux-doc, linux-kernel, linux-security-module,
	linux-kselftest, linux-arch, linux-hardening, kernel-hardening

Hi,

On Fri, Jul 02, 2021 at 05:08:09PM +0000, Alexander Lobakin wrote:
>
> On the other hand, it leaves a potentional window for attackers to
> perform brute force from xattr-incapable filesystems. So at the end
> of the day I think that the current implementation (a strong
> rejection of such filesystems) is way more secure than having
> a fallback I proposed.

I've been thinking more about this: that the Brute LSM depends on xattr
support and I don't like this part. I want that brute force attacks can
be detected and mitigated on every system (with minimal dependencies).
So, now I am working in a solution without this drawback. I have some
ideas but I need to work on it.

> I'm planning to make a patch which will eliminate such weird rootfs
> type selection and just always use more feature-rich tmpfs if it's
> compiled in. So, as an alternative, you could add it to your series
> as a preparatory change and just add a Kconfig dependency on
> CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
> without messing with any fallbacks at all.
> What do you think?

Great. But I hope this patch will not be necessary for Brute LSM :)

Thanks,
John Wood

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
  2021-07-03 10:59     ` John Wood
@ 2021-07-04 14:01       ` John Wood
  2021-07-05 12:52         ` Alexander Lobakin
  0 siblings, 1 reply; 7+ messages in thread
From: John Wood @ 2021-07-04 14:01 UTC (permalink / raw)
  To: Alexander Lobakin
  Cc: John Wood, Kees Cook, Jann Horn, Jonathan Corbet, James Morris,
	Serge E. Hallyn, Shuah Khan, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann, Andi Kleen,
	valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
	Andrew Morton, linux-doc, linux-kernel, linux-security-module,
	linux-kselftest, linux-arch, linux-hardening, kernel-hardening

On Sat, Jul 03, 2021 at 12:59:28PM +0200, John Wood wrote:
> Hi,
>
> On Fri, Jul 02, 2021 at 05:08:09PM +0000, Alexander Lobakin wrote:
> >
> > On the other hand, it leaves a potentional window for attackers to
> > perform brute force from xattr-incapable filesystems. So at the end
> > of the day I think that the current implementation (a strong
> > rejection of such filesystems) is way more secure than having
> > a fallback I proposed.
>
> I've been thinking more about this: that the Brute LSM depends on xattr
> support and I don't like this part. I want that brute force attacks can
> be detected and mitigated on every system (with minimal dependencies).
> So, now I am working in a solution without this drawback. I have some
> ideas but I need to work on it.

I have been coding and testing a bit my ideas but:

Trying to track the applications faults info using kernel memory ends up
in an easy to abuse system (denied of service due to large amount of memory
in use) :(

So, I continue with the v8 idea: xattr to track application crashes info.

> > I'm planning to make a patch which will eliminate such weird rootfs
> > type selection and just always use more feature-rich tmpfs if it's
> > compiled in. So, as an alternative, you could add it to your series
> > as a preparatory change and just add a Kconfig dependency on
> > CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
> > without messing with any fallbacks at all.
> > What do you think?
>
> Great. But I hope this patch will not be necessary for Brute LSM :)

My words are no longer valid ;)

Thanks,
John Wood

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v8 3/8] security/brute: Detect a brute force attack
  2021-07-04 14:01       ` John Wood
@ 2021-07-05 12:52         ` Alexander Lobakin
  0 siblings, 0 replies; 7+ messages in thread
From: Alexander Lobakin @ 2021-07-05 12:52 UTC (permalink / raw)
  To: John Wood
  Cc: Alexander Lobakin, Kees Cook, Jann Horn, Jonathan Corbet,
	James Morris, Serge E. Hallyn, Shuah Khan, Thomas Gleixner,
	Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann,
	Andi Kleen, valdis.kletnieks, Greg Kroah-Hartman, Randy Dunlap,
	Andrew Morton, linux-doc, linux-kernel, linux-security-module,
	linux-kselftest, linux-arch, linux-hardening, kernel-hardening

From: John Wood <john.wood@gmx.com>
Date: Sun, 4 Jul 2021 16:01:08 +0200

> On Sat, Jul 03, 2021 at 12:59:28PM +0200, John Wood wrote:
> > Hi,
> >
> > On Fri, Jul 02, 2021 at 05:08:09PM +0000, Alexander Lobakin wrote:
> > >
> > > On the other hand, it leaves a potentional window for attackers to
> > > perform brute force from xattr-incapable filesystems. So at the end
> > > of the day I think that the current implementation (a strong
> > > rejection of such filesystems) is way more secure than having
> > > a fallback I proposed.
> >
> > I've been thinking more about this: that the Brute LSM depends on xattr
> > support and I don't like this part. I want that brute force attacks can
> > be detected and mitigated on every system (with minimal dependencies).
> > So, now I am working in a solution without this drawback. I have some
> > ideas but I need to work on it.
>
> I have been coding and testing a bit my ideas but:
>
> Trying to track the applications faults info using kernel memory ends up
> in an easy to abuse system (denied of service due to large amount of memor=
> y
> in use) :(
>
> So, I continue with the v8 idea: xattr to track application crashes info.
>
> > > I'm planning to make a patch which will eliminate such weird rootfs
> > > type selection and just always use more feature-rich tmpfs if it's
> > > compiled in. So, as an alternative, you could add it to your series
> > > as a preparatory change and just add a Kconfig dependency on
> > > CONFIG_TMPFS && CONFIG_TMPFS_XATTR to CONFIG_SECURITY_FORK_BRUTE
> > > without messing with any fallbacks at all.
> > > What do you think?
> >
> > Great. But I hope this patch will not be necessary for Brute LSM :)
>
> My words are no longer valid ;)

Ok, so here's the patch that prefers tmpfs for rootfs over ramfs
if it's built-in (which is true for 99% of systems): [0]

For now it hasn't been reviewed by anyone yet, will see. I'm running
my system with this patch for several days already and there were no
issues with rootfs or Brute so far.

[0] https://lore.kernel.org/lkml/20210702233727.21301-1-alobakin@pm.me/

> Thanks,
> John Wood

Thanks,
Al


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v8 3/8] security/brute: Detect a brute force attack
  2021-06-05 15:03 [PATCH v8 0/8] Fork brute force attack mitigation John Wood
@ 2021-06-05 15:04 ` John Wood
  0 siblings, 0 replies; 7+ messages in thread
From: John Wood @ 2021-06-05 15:04 UTC (permalink / raw)
  To: Kees Cook, Jann Horn, Jonathan Corbet, James Morris,
	Serge E. Hallyn, Shuah Khan, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, H. Peter Anvin, Arnd Bergmann
  Cc: John Wood, Andi Kleen, valdis.kletnieks, Greg Kroah-Hartman,
	Randy Dunlap, Andrew Morton, linux-doc, linux-kernel,
	linux-security-module, linux-kselftest, linux-arch,
	linux-hardening, kernel-hardening

For a correct management of a fork brute force attack it is necessary to
track all the information related to the application crashes. To do so,
use the extended attributes (xattr) of the executable files and define a
statistical data structure to hold all the necessary information shared
by all the fork hierarchy processes. This info is the number of crashes,
the last crash timestamp and the crash period's moving average.

The same can be achieved using a pointer to the fork hierarchy
statistical data held by the task_struct structure. But this has an
important drawback: a brute force attack that happens through the execve
system call losts the faults info since these statistics are freed when
the fork hierarchy disappears. Using this method makes not possible to
manage this attack type that can be successfully treated using extended
attributes.

Also, to avoid false positives during the attack detection it is
necessary to narrow the possible cases. So, only the following scenarios
are taken into account:

1.- Launching (fork()/exec()) a setuid/setgid process repeatedly until a
    desirable memory layout is got (e.g. Stack Clash).
2.- Connecting to an exec()ing network daemon (e.g. xinetd) repeatedly
    until a desirable memory layout is got (e.g. what CTFs do for simple
    network service).
3.- Launching processes without exec() (e.g. Android Zygote) and
    exposing state to attack a sibling.
4.- Connecting to a fork()ing network daemon (e.g. apache) repeatedly
    until the previously shared memory layout of all the other children
    is exposed (e.g. kind of related to HeartBleed).

In each case, a privilege boundary has been crossed:

Case 1: setuid/setgid process
Case 2: network to local
Case 3: privilege changes
Case 4: network to local

To mark that a privilege boundary has been crossed it is only necessary
to create a new stats for the executable file via the extended attribute
and only if it has no previous statistical data. This is done using four
different LSM hooks, one per privilege boundary:

setuid/setgid process --> bprm_creds_from_file hook (based on secureexec
                          flag).
network to local -------> socket_accept hook (taking into account only
                          external connections).
privilege changes ------> task_fix_setuid and task_fix_setgid hooks.

To detect a brute force attack it is necessary that the executable file
statistics be updated in every fatal crash and the most important data
to update is the application crash period. To do so, use the new
"task_fatal_signal" LSM hook added in a previous step.

The application crash period must be a value that is not prone to change
due to spurious data and follows the real crash period. So, to compute
it, the exponential moving average (EMA) is used.

Based on the updated statistics two different attacks can be handled. A
slow brute force attack that is detected if the maximum number of faults
per fork hierarchy is reached and a fast brute force attack that is
detected if the application crash period falls below a certain
threshold.

Moreover, only the signals delivered by the kernel are taken into
account with the exception of the SIGABRT signal since the latter is
used by glibc for stack canary, malloc, etc failures, which may indicate
that a mitigation has been triggered.

Signed-off-by: John Wood <john.wood@gmx.com>
---
 include/uapi/linux/xattr.h |   3 +
 security/brute/brute.c     | 500 +++++++++++++++++++++++++++++++++++++
 2 files changed, 503 insertions(+)

diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
index 9463db2dfa9d..ce1c8497dceb 100644
--- a/include/uapi/linux/xattr.h
+++ b/include/uapi/linux/xattr.h
@@ -76,6 +76,9 @@
 #define XATTR_CAPS_SUFFIX "capability"
 #define XATTR_NAME_CAPS XATTR_SECURITY_PREFIX XATTR_CAPS_SUFFIX

+#define XATTR_BRUTE_SUFFIX "brute"
+#define XATTR_NAME_BRUTE XATTR_SECURITY_PREFIX XATTR_BRUTE_SUFFIX
+
 #define XATTR_POSIX_ACL_ACCESS  "posix_acl_access"
 #define XATTR_NAME_POSIX_ACL_ACCESS XATTR_SYSTEM_PREFIX XATTR_POSIX_ACL_ACCESS
 #define XATTR_POSIX_ACL_DEFAULT  "posix_acl_default"
diff --git a/security/brute/brute.c b/security/brute/brute.c
index 0edb89a58ab0..03bebfd1ed1f 100644
--- a/security/brute/brute.c
+++ b/security/brute/brute.c
@@ -2,8 +2,85 @@

 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+#include <linux/binfmts.h>
 #include <linux/lsm_hooks.h>
 #include <linux/sysctl.h>
+#include <linux/xattr.h>
+#include <net/ipv6.h>
+#include <net/sock.h>
+
+/**
+ * struct brute_stats - Fork brute force attack statistics.
+ * @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 as a whole.
+ * @not_allowed: Not allowed executable file flag.
+ * @unused: Remaining unused flags.
+ *
+ * This structure holds the statistical data shared by all the fork hierarchy
+ * processes.
+ */
+struct brute_stats {
+	u32 faults;
+	u64 nsecs;
+	u64 period;
+	union {
+		u8 flags;
+		struct {
+			u8 not_allowed : 1;
+			u8 unused : 7;
+		};
+	};
+};
+
+/**
+ * struct brute_raw_stats - Raw fork brute force attack statistics.
+ * @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.
+ *
+ * This structure holds the statistical data on disk as an extended attribute.
+ * Since the filesystems on which extended attributes are stored might also be
+ * used on architectures with a different byte order and machine word size, care
+ * should be taken to store attribute values in an architecture-independent
+ * format.
+ */
+struct brute_raw_stats {
+	__le32 faults;
+	__le64 nsecs;
+	__le64 period;
+	u8 flags;
+} __packed;
+
+/**
+ * brute_get_current_exe_file() - Get the current task's executable file.
+ *
+ * Since all the kernel threads associated with a task share the same executable
+ * file, get the thread group leader's executable file.
+ *
+ * Context: The file must be released via fput().
+ * Return: NULL if the current task has no associated executable file. A pointer
+ *         to the executable file otherwise.
+ */
+static struct file *brute_get_current_exe_file(void)
+{
+	struct task_struct *task = current;
+	struct file *exe_file;
+
+	rcu_read_lock();
+	if (!thread_group_leader(task))
+		task = rcu_dereference(task->group_leader);
+	get_task_struct(task);
+	rcu_read_unlock();
+
+	exe_file = get_task_exe_file(task);
+	put_task_struct(task);
+	return exe_file;
+}

 /**
  * DOC: brute_ema_weight_numerator
@@ -19,6 +96,18 @@ static unsigned int brute_ema_weight_numerator __read_mostly = 7;
  */
 static unsigned int brute_ema_weight_denominator __read_mostly = 10;

+/**
+ * brute_mul_by_ema_weight() - Multiply by EMA weight.
+ * @value: Value to multiply by EMA weight.
+ *
+ * Return: The result of the multiplication operation.
+ */
+static inline u64 brute_mul_by_ema_weight(u64 value)
+{
+	return mul_u64_u32_div(value, brute_ema_weight_numerator,
+			       brute_ema_weight_denominator);
+}
+
 /**
  * DOC: brute_max_faults
  *
@@ -30,6 +119,56 @@ static unsigned int brute_ema_weight_denominator __read_mostly = 10;
  */
 static unsigned int brute_max_faults __read_mostly = 200;

+/**
+ * brute_update_crash_period() - Update the application crash period.
+ * @stats: Statistics that hold the application crash period to update. Cannot
+ *         be NULL.
+ *
+ * The application crash period must be a value that is not prone to change due
+ * to spurious data and follows the real crash period. So, to compute it, the
+ * exponential moving average (EMA) is used.
+ *
+ * This kind of average defines a weight (between 0 and 1) for the new value to
+ * add and applies the remainder of the weight to the current average value.
+ * This way, some spurious data will not excessively modify the average and only
+ * if the new values are persistent, the moving average will tend towards them.
+ *
+ * Mathematically the application crash period's EMA can be expressed as
+ * follows:
+ *
+ * period_ema = period * weight + period_ema * (1 - weight)
+ *
+ * If the operations are applied:
+ *
+ * period_ema = period * weight + period_ema - period_ema * weight
+ *
+ * If the operands are ordered:
+ *
+ * period_ema = period_ema - period_ema * weight + period * weight
+ *
+ * Finally, this formula can be written as follows:
+ *
+ * period_ema -= period_ema * weight;
+ * period_ema += period * weight;
+ */
+static void brute_update_crash_period(struct brute_stats *stats)
+{
+	u64 current_period;
+	u64 now = ktime_get_clocktai_ns();
+
+	if (stats->faults >= (u32)brute_max_faults)
+		return;
+
+	if (stats->nsecs) {
+		current_period = now > stats->nsecs ? now - stats->nsecs : 0;
+		stats->period -= brute_mul_by_ema_weight(stats->period);
+		stats->period += brute_mul_by_ema_weight(current_period);
+	}
+
+	stats->nsecs = now;
+	stats->faults += 1;
+}
+
 /**
  * DOC: brute_min_faults
  *
@@ -51,6 +190,365 @@ static unsigned int brute_min_faults __read_mostly = 5;
  */
 static unsigned int brute_crash_period_threshold __read_mostly = 30;

+/**
+ * brute_attack_running() - Test if a brute force attack is happening.
+ * @stats: Statistical data shared by all the fork hierarchy processes. Cannot
+ *         be NULL.
+ *
+ * The decision if a brute force attack is running is based on the statistical
+ * data shared by all the fork hierarchy processes.
+ *
+ * There are two types of brute force attacks that can be detected using the
+ * statistical data. The first one is a slow brute force attack that is detected
+ * if the maximum number of faults per fork hierarchy is reached. The second
+ * type is a fast brute force attack that is detected if the application crash
+ * period falls below a certain threshold.
+ *
+ * Moreover, it is important to note that no attacks will be detected until a
+ * minimum number of faults have occurred. This allows to have a trend in the
+ * crash period when the EMA is used.
+ *
+ * Return: True if a brute force attack is happening. False otherwise.
+ */
+static bool brute_attack_running(const struct brute_stats *stats)
+{
+	u64 threshold;
+
+	if (stats->faults < (u32)brute_min_faults)
+		return false;
+
+	if (stats->faults >= (u32)brute_max_faults)
+		return true;
+
+	threshold = (u64)brute_crash_period_threshold * (u64)NSEC_PER_SEC;
+	return stats->period < threshold;
+}
+
+/**
+ * brute_print_attack_running() - Warn about a fork brute force attack.
+ */
+static inline void brute_print_attack_running(void)
+{
+	pr_warn("fork brute force attack detected [pid %d: %s]\n", current->pid,
+		current->comm);
+}
+
+/**
+ * brute_get_xattr_stats() - Get the stats from an extended attribute.
+ * @dentry: The dentry of the file to get the extended attribute.
+ * @inode: The inode of the file to get the extended attribute.
+ * @stats: The stats where to store the info obtained from the extended
+ *         attribute. Cannot be NULL.
+ *
+ * Return: An error code if it is not possible to get the statistical data. Zero
+ *         otherwise.
+ */
+static int brute_get_xattr_stats(struct dentry *dentry, struct inode *inode,
+				 struct brute_stats *stats)
+{
+	int rc;
+	struct brute_raw_stats raw_stats;
+
+	rc = __vfs_getxattr(dentry, inode, XATTR_NAME_BRUTE, &raw_stats,
+			    sizeof(raw_stats));
+	if (rc < 0)
+		return rc;
+
+	stats->faults = le32_to_cpu(raw_stats.faults);
+	stats->nsecs = le64_to_cpu(raw_stats.nsecs);
+	stats->period = le64_to_cpu(raw_stats.period);
+	stats->flags = raw_stats.flags;
+	return 0;
+}
+
+/**
+ * brute_set_xattr_stats() - Set the stats to an extended attribute.
+ * @dentry: The dentry of the file to set the extended attribute.
+ * @inode: The inode of the file to set the extended attribute.
+ * @stats: The stats from where to extract the info to set the extended attribute.
+ *         Cannot be NULL.
+ *
+ * Return: An error code if it is not possible to set the statistical data. Zero
+ *         otherwise.
+ */
+static int brute_set_xattr_stats(struct dentry *dentry, struct inode *inode,
+				 const struct brute_stats *stats)
+{
+	struct brute_raw_stats raw_stats;
+
+	raw_stats.faults = cpu_to_le32(stats->faults);
+	raw_stats.nsecs = cpu_to_le64(stats->nsecs);
+	raw_stats.period = cpu_to_le64(stats->period);
+	raw_stats.flags = stats->flags;
+
+	return __vfs_setxattr(&init_user_ns, dentry, inode, XATTR_NAME_BRUTE,
+			      &raw_stats, sizeof(raw_stats), 0);
+}
+
+/**
+ * brute_update_xattr_stats() - Update the stats of a file.
+ * @file: The file that holds the statistical data to update. Cannot be NULL.
+ *
+ * For a correct management of a fork brute force attack it is only necessary to
+ * update the statistics and test if an attack is happening based on these data.
+ * It is important to note that if the file has no stats nothing is updated nor
+ * created. This way, the scenario where an application has not crossed any
+ * privilege boundary is avoided since the existence of the extended attribute
+ * denotes the crossing of bounds.
+ */
+static void brute_update_xattr_stats(const struct file *file)
+{
+	struct dentry *dentry = file_dentry(file);
+	struct inode *inode = file_inode(file);
+	struct brute_stats stats;
+	int rc;
+
+	inode_lock(inode);
+	rc = brute_get_xattr_stats(dentry, inode, &stats);
+	WARN_ON_ONCE(rc && rc != -ENODATA);
+	if (rc) {
+		inode_unlock(inode);
+		return;
+	}
+
+	brute_update_crash_period(&stats);
+	if (brute_attack_running(&stats)) {
+		brute_print_attack_running();
+		stats.not_allowed = true;
+	}
+
+	rc = brute_set_xattr_stats(dentry, inode, &stats);
+	WARN_ON_ONCE(rc);
+	inode_unlock(inode);
+}
+
+/**
+ * brute_reset_stats() - Reset the statistical data.
+ * @stats: Statistics to be reset. Cannot be NULL.
+ */
+static inline void brute_reset_stats(struct brute_stats *stats)
+{
+	memset(stats, 0, sizeof(*stats));
+}
+
+/**
+ * brute_new_xattr_stats() - New statistics for a file.
+ * @file: The file in which to create the new statistical data. Cannot be NULL.
+ *
+ * Only if the file has no statistical data create it. This function will be
+ * called to mark that a privilege boundary has been crossed so, if new stats
+ * are required, they do not contain any useful data. The existence of the
+ * extended attribute denotes the crossing of privilege bounds.
+ *
+ * Return: An error code if it is not possible to get or set the statistical
+ *         data. Zero otherwise.
+ */
+static int brute_new_xattr_stats(const struct file *file)
+{
+	struct dentry *dentry = file_dentry(file);
+	struct inode *inode = file_inode(file);
+	struct brute_stats stats;
+	int rc;
+
+	inode_lock(inode);
+	rc = brute_get_xattr_stats(dentry, inode, &stats);
+	if (rc && rc != -ENODATA)
+		goto unlock;
+
+	if (rc == -ENODATA) {
+		brute_reset_stats(&stats);
+		rc = brute_set_xattr_stats(dentry, inode, &stats);
+		if (rc)
+			goto unlock;
+	}
+
+unlock:
+	inode_unlock(inode);
+	return rc;
+}
+
+/**
+ * brute_current_new_xattr_stats() - New stats for the current task's exe file.
+ *
+ * Return: An error code if it is not possible to get or set the statistical
+ *         data. Zero otherwise.
+ */
+static int brute_current_new_xattr_stats(void)
+{
+	struct file *exe_file;
+	int rc;
+
+	exe_file = brute_get_current_exe_file();
+	if (WARN_ON_ONCE(!exe_file))
+		return -ENOENT;
+
+	rc = brute_new_xattr_stats(exe_file);
+	WARN_ON_ONCE(rc);
+	fput(exe_file);
+	return rc;
+}
+
+/**
+ * brute_signal_from_user() - Test if a signal is coming from userspace.
+ * @siginfo: Contains the signal information.
+ *
+ * To avoid false positives during the attack detection it is necessary to
+ * narrow the possible cases. So, only the signals delivered by the kernel are
+ * taken into account with the exception of the SIGABRT signal since the latter
+ * is used by glibc for stack canary, malloc, etc failures, which may indicate
+ * that a mitigation has been triggered.
+ *
+ * Return: True if the signal is coming from usersapce. False otherwise.
+ */
+static inline bool brute_signal_from_user(const kernel_siginfo_t *siginfo)
+{
+	return siginfo->si_signo == SIGKILL && siginfo->si_code != SIGABRT;
+}
+
+/**
+ * brute_task_fatal_signal() - Target for the task_fatal_signal hook.
+ * @siginfo: Contains the signal information.
+ *
+ * To detect a brute force attack it is necessary, as a first step, to test in
+ * every fatal crash if the signal is delibered by the kernel. If so, update the
+ * statistics and act based on these data.
+ */
+static void brute_task_fatal_signal(const kernel_siginfo_t *siginfo)
+{
+	struct file *exe_file;
+
+	if (brute_signal_from_user(siginfo))
+		return;
+
+	exe_file = brute_get_current_exe_file();
+	if (WARN_ON_ONCE(!exe_file))
+		return;
+
+	brute_update_xattr_stats(exe_file);
+	fput(exe_file);
+}
+
+/**
+ * brute_task_execve() - Target for the bprm_creds_from_file hook.
+ * @bprm: Contains the linux_binprm structure.
+ * @file: Binary that will be executed without an interpreter.
+ *
+ * This hook is useful to mark that a privilege boundary (setuid/setgid process)
+ * has been crossed. This is done based on the "secureexec" flag.
+ *
+ * To be defensive return an error code if it is not possible to get or set the
+ * stats using an extended attribute since this blocks the execution of the
+ * file. This scenario is treated as an attack.
+ *
+ * It is important to note that here the brute_new_xattr_stats function could be
+ * used with a previous test of the secureexec flag. However it is better to use
+ * the basic xattr functions since in a future commit a test if the execution is
+ * allowed (via the brute_stats::not_allowed flag) will be necessary. This way,
+ * the stats of the file will be get only once.
+ *
+ * Return: An error code if it is not possible to get or set the statistical
+ *         data. Zero otherwise.
+ */
+static int brute_task_execve(struct linux_binprm *bprm, struct file *file)
+{
+	struct dentry *dentry = file_dentry(bprm->file);
+	struct inode *inode = file_inode(bprm->file);
+	struct brute_stats stats;
+	int rc;
+
+	inode_lock(inode);
+	rc = brute_get_xattr_stats(dentry, inode, &stats);
+	if (WARN_ON_ONCE(rc && rc != -ENODATA))
+		goto unlock;
+
+	if (rc == -ENODATA && bprm->secureexec) {
+		brute_reset_stats(&stats);
+		rc = brute_set_xattr_stats(dentry, inode, &stats);
+		if (WARN_ON_ONCE(rc))
+			goto unlock;
+	}
+
+	rc = 0;
+unlock:
+	inode_unlock(inode);
+	return rc;
+}
+
+/**
+ * brute_task_change_priv() - Target for the task_fix_setid hooks.
+ * @new: The set of credentials that will be installed.
+ * @old: The set of credentials that are being replaced.
+ * @flags: Contains one of the LSM_SETID_* values.
+ *
+ * This hook is useful to mark that a privilege boundary (privilege changes) has
+ * been crossed.
+ *
+ * Return: An error code if it is not possible to get or set the statistical
+ *         data. Zero otherwise.
+ */
+static int brute_task_change_priv(struct cred *new, const struct cred *old, int flags)
+{
+	return brute_current_new_xattr_stats();
+}
+
+#ifdef CONFIG_IPV6
+/**
+ * brute_local_ipv6_rcv_saddr() - Test if an ipv6 rcv_saddr is local.
+ * @sk: The sock that contains the ipv6 address.
+ *
+ * Return: True if the ipv6 rcv_saddr is local. False otherwise.
+ */
+static inline bool brute_local_ipv6_rcv_saddr(const struct sock *sk)
+{
+	return ipv6_addr_equal(&sk->sk_v6_rcv_saddr, &in6addr_loopback);
+}
+#else
+static inline bool brute_local_ipv6_rcv_saddr(const struct sock *sk)
+{
+	return false;
+}
+#endif /* CONFIG_IPV6 */
+
+#ifdef CONFIG_SECURITY_NETWORK
+/**
+ * brute_socket_accept() - Target for the socket_accept hook.
+ * @sock: Contains the listening socket structure.
+ * @newsock: Contains the newly created server socket for connection.
+ *
+ * This hook is useful to mark that a privilege boundary (network to local) has
+ * been crossed. This is done only if the listening socket accepts external
+ * connections. The sockets for inter-process communication (IPC) and those that
+ * are listening on loopback addresses are not taken into account.
+ *
+ * Return: An error code if it is not possible to get or set the statistical
+ *         data. Zero otherwise.
+ */
+static int brute_socket_accept(struct socket *sock, struct socket *newsock)
+{
+	struct sock *sk = sock->sk;
+
+	if (sk->sk_family == AF_UNIX || sk->sk_family == AF_NETLINK ||
+	    sk->sk_rcv_saddr == htonl(INADDR_LOOPBACK) ||
+	    brute_local_ipv6_rcv_saddr(sk))
+		return 0;
+
+	return brute_current_new_xattr_stats();
+}
+#endif /* CONFIG_SECURITY_NETWORK */
+
+/*
+ * brute_hooks - Targets for the LSM's hooks.
+ */
+static struct security_hook_list brute_hooks[] __lsm_ro_after_init = {
+	LSM_HOOK_INIT(task_fatal_signal, brute_task_fatal_signal),
+	LSM_HOOK_INIT(bprm_creds_from_file, brute_task_execve),
+	LSM_HOOK_INIT(task_fix_setuid, brute_task_change_priv),
+	LSM_HOOK_INIT(task_fix_setgid, brute_task_change_priv),
+#ifdef CONFIG_SECURITY_NETWORK
+	LSM_HOOK_INIT(socket_accept, brute_socket_accept),
+#endif
+};
+
 #ifdef CONFIG_SYSCTL
 static unsigned int uint_max = UINT_MAX;
 #define SYSCTL_UINT_MAX (&uint_max)
@@ -137,6 +635,8 @@ static inline void brute_init_sysctl(void) { }
 static int __init brute_init(void)
 {
 	pr_info("becoming mindful\n");
+	security_add_hooks(brute_hooks, ARRAY_SIZE(brute_hooks),
+			   KBUILD_MODNAME);
 	brute_init_sysctl();
 	return 0;
 }
--
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-07-05 12:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-01 23:55 [PATCH v8 3/8] security/brute: Detect a brute force attack Alexander Lobakin
2021-07-02 14:59 ` John Wood
2021-07-02 17:08   ` Alexander Lobakin
2021-07-03 10:59     ` John Wood
2021-07-04 14:01       ` John Wood
2021-07-05 12:52         ` Alexander Lobakin
  -- strict thread matches above, loose matches on Subject: below --
2021-06-05 15:03 [PATCH v8 0/8] Fork brute force attack mitigation John Wood
2021-06-05 15:04 ` [PATCH v8 3/8] security/brute: Detect a brute force attack John Wood

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).