[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202103211038.99C87F12@keescook>
Date: Sun, 21 Mar 2021 11:01:03 -0700
From: Kees Cook <keescook@...omium.org>
To: John Wood <john.wood@....com>
Cc: 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 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:
> > On Sun, Mar 07, 2021 at 12:30:27PM +0100, John Wood wrote:
> > > +/**
> > > + * 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.)
> > > +/**
> > > + * 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?
> > > +
> > > + 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
> > > + 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
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
