Message-ID: <20210320154648.GC3023@ubuntu>
Date:   Sat, 20 Mar 2021 16:46:48 +0100
From:   John Wood <john.wood@....com>
To:     Kees Cook <keescook@...omium.org>
Cc:     John Wood <john.wood@....com>, Jann Horn <jannh@...gle.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Jonathan Corbet <corbet@....net>,
        James Morris <jmorris@...ei.org>,
        Shuah Khan <shuah@...nel.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andi Kleen <ak@...ux.intel.com>,
        kernel test robot <oliver.sang@...el.com>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kselftest@...r.kernel.org,
        kernel-hardening@...ts.openwall.com
Subject: Re: [PATCH v6 4/8] security/brute: Fine tuning the attack detection

On Wed, Mar 17, 2021 at 09:00:51PM -0700, Kees Cook wrote:
> On Sun, Mar 07, 2021 at 12:30:27PM +0100, John Wood wrote:
> >  #include <asm/current.h>
> > +#include <asm/rwonce.h>
> > +#include <asm/siginfo.h>
> > +#include <asm/signal.h>
> > +#include <linux/binfmts.h>
> >  #include <linux/bug.h>
> >  #include <linux/compiler.h>
> > +#include <linux/cred.h>
> > +#include <linux/dcache.h>
> >  #include <linux/errno.h>
> > +#include <linux/fs.h>
> >  #include <linux/gfp.h>
> > +#include <linux/if.h>
> >  #include <linux/init.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/kernel.h>
> >  #include <linux/lsm_hooks.h>
> >  #include <linux/math64.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/path.h>
> >  #include <linux/printk.h>
> >  #include <linux/refcount.h>
> >  #include <linux/rwlock.h>
> > @@ -19,9 +29,35 @@
> >  #include <linux/sched.h>
> >  #include <linux/sched/signal.h>
> >  #include <linux/sched/task.h>
> > +#include <linux/signal.h>
> > +#include <linux/skbuff.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/stat.h>
> >  #include <linux/types.h>
> > +#include <linux/uidgid.h>
>
> This is really a LOT of includes. Are you sure all of these are
> explicitly needed?

I try to add the needed header for every macro and function used. If
there is a better method to do it I will apply it. Thanks.

> >  /**
> >   * struct brute_stats - Fork brute force attack statistics.
> > @@ -30,6 +66,9 @@
> >   * @faults: Number of crashes.
> >   * @jiffies: Last crash timestamp.
> >   * @period: Crash period's moving average.
> > + * @saved_cred: Saved credentials.
> > + * @network: Network activity flag.
> > + * @bounds_crossed: Privilege bounds crossed flag.
> >   *
> >   * This structure holds the statistical data shared by all the fork hierarchy
> >   * processes.
> > @@ -40,6 +79,9 @@ struct brute_stats {
> >  	unsigned char faults;
> >  	u64 jiffies;
> >  	u64 period;
> > +	struct brute_cred saved_cred;
> > +	unsigned char network : 1;
> > +	unsigned char bounds_crossed : 1;
>
> If you really want to keep faults a "char", I would move these bools
> after "faults" to avoid adding more padding.

Understood. Thanks.

> > +/**
> > + * brute_is_setid() - Test if the executable file has the setid flags set.
> > + * @bprm: Points to the linux_binprm structure.
> > + *
> > + * Return: True if the executable file has the setid flags set. False otherwise.
> > + */
> > +static bool brute_is_setid(const struct linux_binprm *bprm)
> > +{
> > +	struct file *file = bprm->file;
> > +	struct inode *inode;
> > +	umode_t mode;
> > +
> > +	if (!file)
> > +		return false;
> > +
> > +	inode = file->f_path.dentry->d_inode;
> > +	mode = inode->i_mode;
> > +
> > +	return !!(mode & (S_ISUID | S_ISGID));
> > +}
>
> Oh, er, no, this should not reinvent the wheel. You just want to know if
> creds got elevated, so you want bprm->secureexec; this gets correctly
> checked in cap_bprm_creds_from_file().

Ok, I will work on it for the next version.

> > +
> > +/**
> > + * brute_reset_stats() - Reset the statistical data.
> > + * @stats: Statistics to be reset.
> > + * @is_setid: The executable file has the setid flags set.
> > + *
> > + * Reset the faults and period and set the last crash timestamp to now. This
> > + * way, it is possible to compute the application crash period at the next
> > + * fault. Also, save the credentials of the current task and update the
> > + * bounds_crossed flag based on a previous network activity and the is_setid
> > + * parameter.
> > + *
> > + * The statistics to be reset cannot be NULL.
> > + *
> > + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock
> > + *          and brute_stats::lock held.
> > + */
> > +static void brute_reset_stats(struct brute_stats *stats, bool is_setid)
> > +{
> > +	const struct cred *cred = current_cred();
> > +
> > +	stats->faults = 0;
> > +	stats->jiffies = get_jiffies_64();
> > +	stats->period = 0;
> > +	stats->saved_cred.uid = cred->uid;
> > +	stats->saved_cred.gid = cred->gid;
> > +	stats->saved_cred.suid = cred->suid;
> > +	stats->saved_cred.sgid = cred->sgid;
> > +	stats->saved_cred.euid = cred->euid;
> > +	stats->saved_cred.egid = cred->egid;
> > +	stats->saved_cred.fsuid = cred->fsuid;
> > +	stats->saved_cred.fsgid = cred->fsgid;
> > +	stats->bounds_crossed = stats->network || is_setid;
> > +}
>
> I would include brute_reset_stats() in the first patch (and add to it as
> needed). To that end, it can start with a memset(stats, 0, sizeof(*stats));

So, need all the struct fields to be introduced in the initial patch?
Even if they are not needed in the initial patch? I'm confused.

> > +/**
> > + * brute_priv_have_changed() - Test if the privileges have changed.
> > + * @stats: Statistics that hold the saved credentials.
> > + *
> > + * The privileges have changed if the credentials of the current task are
> > + * different from the credentials saved in the statistics structure.
> > + *
> > + * The statistics that hold the saved credentials cannot be NULL.
> > + *
> > + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock
> > + *          and brute_stats::lock held.
> > + * Return: True if the privileges have changed. False otherwise.
> > + */
> > +static bool brute_priv_have_changed(struct brute_stats *stats)
> > +{
> > +	const struct cred *cred = current_cred();
> > +	bool priv_have_changed;
> > +
> > +	priv_have_changed = !uid_eq(stats->saved_cred.uid, cred->uid) ||
> > +		!gid_eq(stats->saved_cred.gid, cred->gid) ||
> > +		!uid_eq(stats->saved_cred.suid, cred->suid) ||
> > +		!gid_eq(stats->saved_cred.sgid, cred->sgid) ||
> > +		!uid_eq(stats->saved_cred.euid, cred->euid) ||
> > +		!gid_eq(stats->saved_cred.egid, cred->egid) ||
> > +		!uid_eq(stats->saved_cred.fsuid, cred->fsuid) ||
> > +		!gid_eq(stats->saved_cred.fsgid, cred->fsgid);
> > +
> > +	return priv_have_changed;
> > +}
>
> This should just be checked from bprm->secureexec, which is valid by the
> time you get to the bprm_committing_creds hook. You can just save the
> value to your stats struct instead of re-interrogating current_cred,
> etc.

Ok. Thanks.

> > +
> > +/**
> > + * brute_threat_model_supported() - Test if the threat model is supported.
> > + * @siginfo: Contains the signal information.
> > + * @stats: Statistical data shared by all the fork hierarchy processes.
> > + *
> > + * To avoid false positives during the attack detection it is necessary to
> > + * narrow the possible cases. 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
> > + *
> > + * Also, 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.
> > + *
> > + * The signal information and the statistical data shared by all the fork
> > + * hierarchy processes cannot be NULL.
> > + *
> > + * It's mandatory to disable interrupts before acquiring the brute_stats::lock
> > + * since the task_free hook can be called from an IRQ context during the
> > + * execution of the task_fatal_signal hook.
> > + *
> > + * Context: Must be called with interrupts disabled and brute_stats_ptr_lock
> > + *          held.
> > + * Return: True if the threat model is supported. False otherwise.
> > + */
> > +static bool brute_threat_model_supported(const kernel_siginfo_t *siginfo,
> > +					 struct brute_stats *stats)
> > +{
> > +	bool bounds_crossed;
> > +
> > +	if (siginfo->si_signo == SIGKILL && siginfo->si_code != SIGABRT)
> > +		return false;
> > +
> > +	spin_lock(&stats->lock);
> > +	bounds_crossed = stats->bounds_crossed;
> > +	bounds_crossed = bounds_crossed || brute_priv_have_changed(stats);
> > +	stats->bounds_crossed = bounds_crossed;
> > +	spin_unlock(&stats->lock);
> > +
> > +	return bounds_crossed;
> > +}
>
> I think this logic can be done with READ_ONCE()s and moved directly into
> brute_task_fatal_signal().

Thanks. I will work on locking.

> >
> > +/**
> > + * brute_network() - Target for the socket_sock_rcv_skb hook.
> > + * @sk: Contains the sock (not socket) associated with the incoming sk_buff.
> > + * @skb: Contains the incoming network data.
> > + *
> > + * A previous step to detect that a network to local boundary has been crossed
> > + * is to detect if there is network activity. To do this, it is only necessary
> > + * to check if there are data packets received from a network device other than
> > + * loopback.
> > + *
> > + * It's mandatory to disable interrupts before acquiring brute_stats_ptr_lock
> > + * and brute_stats::lock since the task_free hook can be called from an IRQ
> > + * context during the execution of the socket_sock_rcv_skb hook.
> > + *
> > + * Return: -EFAULT if the current task doesn't have statistical data. Zero
> > + *         otherwise.
> > + */
> > +static int brute_network(struct sock *sk, struct sk_buff *skb)
> > +{
> > +	struct brute_stats **stats;
> > +	unsigned long flags;
> > +
> > +	if (!skb->dev || (skb->dev->flags & IFF_LOOPBACK))
> > +		return 0;
> > +
> > +	stats = brute_stats_ptr(current);
>
> Uhh, is "current" valid here? I actually don't know this hook very well.

I think so, but I will try to study it. Thanks for noted this.

> > +	read_lock_irqsave(&brute_stats_ptr_lock, flags);
> > +
> > +	if (!*stats) {
> > +		read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> > +		return -EFAULT;
> > +	}
> > +
> > +	spin_lock(&(*stats)->lock);
> > +	(*stats)->network = true;
> > +	spin_unlock(&(*stats)->lock);
> > +	read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> > +	return 0;
> > +}

Thanks,
John Wood

Powered by blists - more mailing lists