Re: [PATCH v8 3/8] security/brute: Detect a brute force attack

* 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