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

[Alexander Lobakin <alobakin@pm.me>]

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