Re: [PATCH v6 4/8] security/brute: Fine tuning the attack detection

On Sat, Mar 20, 2021 at 04:46:48PM +0100, John Wood wrote:

On Wed, Mar 17, 2021 at 09:00:51PM -0700, Kees Cook wrote:

quoted

On Sun, Mar 07, 2021 at 12:30:27PM +0100, John Wood wrote:

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.

No, I meant try to introduce as much infrastructure as possible early in
the series. In this case, I was suggesting having introduced
brute_reset_stats() at the start, so that in this patch you'd just be
adding the new fields to the function. (Instead of both adding new
fields and changing the execution pattern.)

quoted

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;

I wonder if you need to also ignore netlink, unix sockets, etc, or does
the IFF_LOOPBACK cover those too?

quoted

quoted

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

I think you might need to track the mapping of task to sock via
security_socket_post_create(), security_socket_accept(),
and/or security_socket_connect()?

Perhaps just mark it once with security_socket_post_create(), instead of
running a hook on every incoming network packet, too?

-Kees

quoted

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

-- 
Kees Cook