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