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

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:

...

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

...

/**

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

...

+/**

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

...

+/**

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

...

+/**

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

...

+/**

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

...

+/**

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

...

• 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