From: Kees Cook <keescook@chromium.org>
To: John Wood <john.wood@gmx.com>
Cc: 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
Date: Sun, 21 Mar 2021 11:01:03 -0700 [thread overview]
Message-ID: <202103211038.99C87F12@keescook> (raw)
In-Reply-To: <20210320154648.GC3023@ubuntu>
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
next prev parent reply other threads:[~2021-03-21 18:01 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-07 11:30 [PATCH v6 0/8] Fork brute force attack mitigation John Wood
2021-03-07 11:30 ` [PATCH v6 1/8] security: Add LSM hook at the point where a task gets a fatal signal John Wood
2021-03-18 1:22 ` Kees Cook
2021-03-07 11:30 ` [PATCH v6 2/8] security/brute: Define a LSM and manage statistical data John Wood
2021-03-18 2:00 ` Kees Cook
2021-03-20 15:01 ` John Wood
2021-03-21 17:37 ` Kees Cook
2021-03-07 11:30 ` [PATCH v6 3/8] securtiy/brute: Detect a brute force attack John Wood
2021-03-18 2:57 ` Kees Cook
2021-03-20 15:34 ` John Wood
2021-03-21 18:28 ` Kees Cook
2021-03-21 15:01 ` John Wood
2021-03-21 18:45 ` Kees Cook
2021-03-22 18:32 ` John Wood
2021-03-07 11:30 ` [PATCH v6 4/8] security/brute: Fine tuning the attack detection John Wood
2021-03-18 4:00 ` Kees Cook
2021-03-20 15:46 ` John Wood
2021-03-21 18:01 ` Kees Cook [this message]
2021-03-07 11:30 ` [PATCH v6 5/8] security/brute: Mitigate a brute force attack John Wood
2021-03-18 4:04 ` Kees Cook
2021-03-20 15:48 ` John Wood
2021-03-21 18:06 ` Kees Cook
2021-03-07 11:30 ` [PATCH v6 6/8] selftests/brute: Add tests for the Brute LSM John Wood
2021-03-18 4:08 ` Kees Cook
2021-03-20 15:49 ` John Wood
2021-03-07 11:30 ` [PATCH v6 7/8] Documentation: Add documentation " John Wood
2021-03-18 4:10 ` Kees Cook
2021-03-20 15:50 ` John Wood
2021-03-21 18:50 ` Jonathan Corbet
2021-03-26 15:41 ` John Wood
2021-03-07 11:30 ` [PATCH v6 8/8] MAINTAINERS: Add a new entry " John Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202103211038.99C87F12@keescook \
--to=keescook@chromium.org \
--cc=ak@linux.intel.com \
--cc=corbet@lwn.net \
--cc=gregkh@linuxfoundation.org \
--cc=jannh@google.com \
--cc=jmorris@namei.org \
--cc=john.wood@gmx.com \
--cc=kernel-hardening@lists.openwall.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=oliver.sang@intel.com \
--cc=rdunlap@infradead.org \
--cc=serge@hallyn.com \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).