| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150717153317.GH32473@madcap2.tricolour.ca>
Date: Fri, 17 Jul 2015 11:33:17 -0400
From: Richard Guy Briggs <rgb@...hat.com>
To: Paul Moore <pmoore@...hat.com>
Cc: linux-audit@...hat.com, linux-kernel@...r.kernel.org,
Eric Paris <eparis@...hat.com>, sgrubb@...hat.com,
pmoody@...gle.com
Subject: Re: [PATCH V6 1/4] audit: implement audit by executable
On 15/07/16, Paul Moore wrote:
> On Tuesday, July 14, 2015 11:50:23 AM Richard Guy Briggs wrote:
> > From: Eric Paris <eparis@...hat.com>
> >
> > This patch implements the ability to filter on the executable. It is
> > clearly incomplete! This patch adds the inode/dev of the executable at
> > the moment the rule is loaded. It does not update if the executable is
> > updated/moved/whatever. That should be added. But at this moment, this
> > patch works.
>
> This needs work. Either this patch is incomplete as the description says, in
> which case I'm not going to merge it, or the description is out of date and
> needs to be updated.
It is pretty close to the original, so the description is valid, however
combining this patch with 3/4 and 4/4 triggers a rewrite.
> If later patches in the series fix deficiencies in this patch (I haven't
> gotten past this description yet) then we should consider merging some of the
> patches together so they are more useful.
Ok, no problem. (More below...)
> > RGB: Explicitly declare prototypes as extern.
> > RGB: Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch,
> > lsm_field.
> >
> > Based-on-user-interface-by: Richard Guy Briggs <rgb@...hat.com>
> > Based-on-idea-by: Peter Moody <pmoody@...gle.com>
>
> I'm not fully up on the different patch metadata the cool kids are using these
> days, but I think it is okay to simply credit Peter in the patch description
> and omit the two lines above. Giving credit is important, but these metadata
> tags are a bit silly in my opinion.
Fair enough. If it doesn't need to be machine-readable, I'll merge it
into the patch description prose.
> > Cc: pmoody@...gle.com
> > Signed-off-by: Eric Paris <eparis@...hat.com>
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
>
> It has been a while since I've looked at Eric's original patch, but
> considering considering some of the work involved, I think it is reasonable to
> claim this patch as your own and credit Eric in the patch description.
I left it in his authorship since all I did was declare the prototypes
explicitly as external and renamed one funciton by one letter. I didn't
think that meritted claiming authorhship, but if I merge it as you
recommend and makes sense to me, there's no reason not to assume
authorship.
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index d641f9b..3aca24f 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -278,6 +286,30 @@ extern int audit_watch_compare(struct audit_watch
> > *watch, unsigned long ino, dev #define audit_watch_path(w) ""
> > #define audit_watch_compare(w, i, d) 0
> >
> > +static inline int audit_make_exe_rule(struct audit_krule *krule, char
> > *pathname, int len, u32 op)
> > +{
> > + return -EINVAL;
> > +}
> > +static inline void audit_remove_exe_rule(struct audit_krule *krule)
> > +{
> > + BUG();
> > + return 0;
> > +}
> > +static inline char *audit_exe_path(struct audit_exe *exe)
> > +{
> > + BUG();
> > + return "";
> > +}
> > +static inline int audit_dupe_exe(struct audit_krule *new, struct
> > audit_krule *old) +{
> > + BUG();
> > + return -EINVAL
> > +}
> > +static inline int audit_exe_compare(struct task_struct *tsk, struct
> > audit_exe *exe) +{
> > + BUG();
> > + return 0;
> > +}
> > #endif /* CONFIG_AUDIT_WATCH */
>
> Not a big fan of the BUG() calls in the stubs above, let's get rid of them.
Ok, that way I can more easily convert them to #defines.
> Yes, I know some other audit stubs are defined as BUG(), or similar, those
> aren't a good idea either, but they are already there ...
Ok, cleanup patch later...
> > diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
> > new file mode 100644
> > index 0000000..d4cc8b5
> > --- /dev/null
> > +++ b/kernel/audit_exe.c
> > @@ -0,0 +1,109 @@
> > +/* audit_exe.c -- filtering of audit events
> > + *
> > + * Copyright 2014-2015 Red Hat, Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/audit.h>
> > +#include <linux/mutex.h>
> > +#include <linux/fs.h>
> > +#include <linux/namei.h>
> > +#include <linux/slab.h>
> > +#include "audit.h"
> > +
> > +struct audit_exe {
> > + char *pathname;
> > + unsigned long ino;
> > + dev_t dev;
> > +};
> > +
> > +/* Translate a watch string to kernel respresentation. */
> > +int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len,
> > u32 op)
> > +{
> > + struct audit_exe *exe;
> > + struct path path;
> > + struct dentry *dentry;
> > + unsigned long ino;
> > + dev_t dev;
> > +
> > + if (pathname[0] != '/' || pathname[len-1] == '/')
> > + return -EINVAL;
> > +
> > + dentry = kern_path_locked(pathname, &path);
> > + if (IS_ERR(dentry))
> > + return PTR_ERR(dentry);
> > + mutex_unlock(&path.dentry->d_inode->i_mutex);
> > +
> > + if (!dentry->d_inode)
> > + return -ENOENT;
> > + dev = dentry->d_inode->i_sb->s_dev;
> > + ino = dentry->d_inode->i_ino;
> > + dput(dentry);
> > +
> > + exe = kmalloc(sizeof(*exe), GFP_KERNEL);
> > + if (!exe)
> > + return -ENOMEM;
> > + exe->ino = ino;
> > + exe->dev = dev;
> > + exe->pathname = pathname;
> > + krule->exe = exe;
>
> You don't need the dev and ino variables here, just move the kmalloc() to just
> after the dentry->d_inode check ... or put it after the sanity checks,
> although you'll have to be careful to free it on error.
I'll take a closer look. As referenced elsewhere, I agree a helper
function may be useful.
> > + return 0;
> > +}
> > +
> > +void audit_remove_exe_rule(struct audit_krule *krule)
> > +{
> > + struct audit_exe *exe;
> > +
> > + exe = krule->exe;
> > + krule->exe = NULL;
> > + kfree(exe->pathname);
> > + kfree(exe);
> > +}
>
> Not your fault, and nothing to change here, but I really hate how audit dups
> strings outside the rule creation functions, but frees the strings in the rule
> free routines; it's almost asking to be misused.
Again, cleanup patch later maybe...
> > +char *audit_exe_path(struct audit_exe *exe)
> > +{
> > + return exe->pathname;
> > +}
> > +
> > +int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old)
> > +{
> > + struct audit_exe *exe;
> > +
> > + exe = kmalloc(sizeof(*exe), GFP_KERNEL);
> > + if (!exe)
> > + return -ENOMEM;
> > +
> > + exe->pathname = kstrdup(old->exe->pathname, GFP_KERNEL);
> > + if (!exe->pathname) {
> > + kfree(exe);
> > + return -ENOMEM;
> > + }
> > +
> > + exe->ino = old->exe->ino;
> > + exe->dev = old->exe->dev;
> > + new->exe = exe;
> > +
> > + return 0;
> > +}
> > +
> > +int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe)
> > +{
> > + if (tsk->mm->exe_file->f_inode->i_ino != exe->ino)
> > + return 0;
> > + if (tsk->mm->exe_file->f_inode->i_sb->s_dev != exe->dev)
> > + return 0;
> > + return 1;
> > +}
>
> I suspect Eric put the above functions in a separate file to ease development
> (no need to work about messy porting as upstream moved on), but I see no
> reason why these functions couldn't be folded into auditfilter.c.
I thought it made sense where Eric put it. It somewhat parallelled the
watch and tree code. It might be tempting to put it in
audit_fsnotify.c, but I don't really want to overload that, since the
fsnotify code may be used to simpify the watch code in the future. When
we're done after 3/4, audit_exe.c is down to 50 lines...
Mind you, auditsc.c is a bit overloaded with stuff that doesn't
necessarily belong there...
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 9fb9d1c..bf745c7 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -48,6 +48,7 @@
> > #include <asm/types.h>
> > #include <linux/atomic.h>
> > #include <linux/fs.h>
> > +#include <linux/dcache.h>
> > #include <linux/namei.h>
> > #include <linux/mm.h>
> > #include <linux/export.h>
> > @@ -71,6 +72,7 @@
> > #include <linux/capability.h>
> > #include <linux/fs_struct.h>
> > #include <linux/compat.h>
> > +#include <linux/sched.h>
> > #include <linux/ctype.h>
> > #include <linux/string.h>
> > #include <uapi/linux/limits.h>
> > @@ -466,6 +468,20 @@ static int audit_filter_rules(struct task_struct *tsk,
> > result = audit_comparator(ctx->ppid, f->op, f->val);
> > }
> > break;
> > + case AUDIT_EXE:
> > + result = audit_exe_compare(tsk, rule->exe);
> > + break;
> > + case AUDIT_EXE_CHILDREN:
> > + {
> > + struct task_struct *ptsk;
> > + for (ptsk = tsk; ptsk->parent->pid > 0; ptsk =
> > find_task_by_vpid(ptsk->parent->pid)) {
> > + if (audit_exe_compare(ptsk, rule->exe)) {
> > + ++result;
> > + break;
> > + }
> > + }
> > + }
> > + break;
>
> I don't completely understand the point of AUDIT_EXE_CHILDREN filter, what
> problem are we trying to solve? It checks to see if there is an executable
> match starting with the current process and walking up the process' parents in
> the current pid namespace?
Say we want to monitor /usr/sbin/apache2 and all its spawned processes.
Set up a rule that uses AUDIT_EXE_CHILDREN with /usr/sbin/apache2, then
when it spawns a cgi running perl or php, those actions will be caught.
> Help me understand what this accomplishes, I'm a little tried right now and I
> just don't get it.
This was Peter Moody's idea and it made sense, so we kept it.
> paul moore
- RGB
--
Richard Guy Briggs <rbriggs@...hat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists