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:

quoted

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

quoted

 /**
  * 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.

quoted

+/**
+ * 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.

quoted

+
+/**
+ * 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.

quoted

+/**
+ * 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.

quoted

+
+/**
+ * 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.

quoted

+/**
+ * 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.

quoted

+	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