Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 20 Mar 2021 16:46:48 +0100
From:   John Wood <john.wood@gmx.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     John Wood <john.wood@gmx.com>, Jann Horn <jannh@google.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        Jonathan Corbet <corbet@lwn.net>,
        James Morris <jmorris@namei.org>,
        Shuah Khan <shuah@kernel.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andi Kleen <ak@linux.intel.com>,
        kernel test robot <oliver.sang@intel.com>,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kselftest@vger.kernel.org,
        kernel-hardening@lists.openwall.com
Subject: Re: [PATCH v6 4/8] security/brute: Fine tuning the attack detection
Message-ID: <20210320154648.GC3023@ubuntu>
References: <20210307113031.11671-1-john.wood@gmx.com>
 <20210307113031.11671-5-john.wood@gmx.com>
 <202103171957.16C0560D@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202103171957.16C0560D@keescook>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 h=
ierarchy
> >   * 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 =3D bprm->file;
> > +	struct inode *inode;
> > +	umode_t mode;
> > +
> > +	if (!file)
> > +		return false;
> > +
> > +	inode =3D file->f_path.dentry->d_inode;
> > +	mode =3D 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 no=
w. 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 t=
he
> > + * bounds_crossed flag based on a previous network activity and the i=
s_setid
> > + * parameter.
> > + *
> > + * The statistics to be reset cannot be NULL.
> > + *
> > + * Context: Must be called with interrupts disabled and brute_stats_p=
tr_lock
> > + *          and brute_stats::lock held.
> > + */
> > +static void brute_reset_stats(struct brute_stats *stats, bool is_seti=
d)
> > +{
> > +	const struct cred *cred =3D current_cred();
> > +
> > +	stats->faults =3D 0;
> > +	stats->jiffies =3D get_jiffies_64();
> > +	stats->period =3D 0;
> > +	stats->saved_cred.uid =3D cred->uid;
> > +	stats->saved_cred.gid =3D cred->gid;
> > +	stats->saved_cred.suid =3D cred->suid;
> > +	stats->saved_cred.sgid =3D cred->sgid;
> > +	stats->saved_cred.euid =3D cred->euid;
> > +	stats->saved_cred.egid =3D cred->egid;
> > +	stats->saved_cred.fsuid =3D cred->fsuid;
> > +	stats->saved_cred.fsgid =3D cred->fsgid;
> > +	stats->bounds_crossed =3D 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_p=
tr_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 =3D current_cred();
> > +	bool priv_have_changed;
> > +
> > +	priv_have_changed =3D !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 suppo=
rted.
> > + * @siginfo: Contains the signal information.
> > + * @stats: Statistical data shared by all the fork hierarchy processe=
s.
> > + *
> > + * To avoid false positives during the attack detection it is necessa=
ry to
> > + * narrow the possible cases. Only the following scenarios are taken =
into
> > + * account:
> > + *
> > + * 1.- Launching (fork()/exec()) a setuid/setgid process repeatedly u=
ntil a
> > + *     desirable memory layout is got (e.g. Stack Clash).
> > + * 2.- Connecting to an exec()ing network daemon (e.g. xinetd) repeat=
edly 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 e=
xposing
> > + *     state to attack a sibling.
> > + * 4.- Connecting to a fork()ing network daemon (e.g. apache) repeate=
dly 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 acco=
unt with
> > + * the exception of the SIGABRT signal since the latter is used by gl=
ibc for
> > + * stack canary, malloc, etc failures, which may indicate that a miti=
gation 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_st=
ats::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_p=
tr_lock
> > + *          held.
> > + * Return: True if the threat model is supported. False otherwise.
> > + */
> > +static bool brute_threat_model_supported(const kernel_siginfo_t *sigi=
nfo,
> > +					 struct brute_stats *stats)
> > +{
> > +	bool bounds_crossed;
> > +
> > +	if (siginfo->si_signo =3D=3D SIGKILL && siginfo->si_code !=3D SIGABR=
T)
> > +		return false;
> > +
> > +	spin_lock(&stats->lock);
> > +	bounds_crossed =3D stats->bounds_crossed;
> > +	bounds_crossed =3D bounds_crossed || brute_priv_have_changed(stats);
> > +	stats->bounds_crossed =3D 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 s=
k_buff.
> > + * @skb: Contains the incoming network data.
> > + *
> > + * A previous step to detect that a network to local boundary has bee=
n 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 =3D 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 =3D true;
> > +	spin_unlock(&(*stats)->lock);
> > +	read_unlock_irqrestore(&brute_stats_ptr_lock, flags);
> > +	return 0;
> > +}

Thanks,
John Wood