Date:	Fri, 17 Jul 2015 14:24:49 -0400
From:	Paul Moore <pmoore@...hat.com>
To:	Richard Guy Briggs <rgb@...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 Friday, July 17, 2015 11:33:17 AM Richard Guy Briggs wrote:
> 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.

You could do a "based on" or similar tag if you want.  I'm honestly not sure 
what the official tags are beyond signed-off, acked, and reviewed.  Those are 
the only ones I really care about anyway ;)

It isn't something I care enough about to reject a patch over, I figured you 
were going to need to do some respin work anyway so I wanted to mention it.

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

Much later.  It isn't something I worry much about, I just don't want to see 
us going crazy with BUG assertions in new code.

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

Once again, not something I would worry about for this patchset, let's just 
get this code fixed up and merged.  It was more of an observation that I see a 
lot of kernel audit structures storing and comparing dev/inode information and 
each way is slightly different ... I *love* consistency in code to an 
unhealthy level, so this bugs me a bit more than it probably should.

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

Yeah ... maybe.  World peace too.

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

See, there ya go ... I don't see the point of adding a new file for 50 lines.

> Mind you, auditsc.c is a bit overloaded with stuff that doesn't
> necessarily belong there...

At some point we'll clean things up a bit - most likely as part of the 
upcoming audit rework - but it doesn't need to happen with this patchset.

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

I suppose my confusion is that I see the exe filtering as very similar to our 
PID filtering, the big difference is that we allow you to match on an on-disk 
binary and not a running process.  We don't currently have a PID_CHILDREN 
filter and presumably no one has asked for it.  Yes we do have PPID, but it 
only goes up one level, e.g. it's PPID and not PPPPPPPPID, not like the 
potentially nasty loop we've got with EXE_CHILDREN.

I'm not super excited about the loop so I'd really like to hear how this is 
solving an actual user need and not just a "hey, wouldn't it be cool?" 
feature.

-- 
paul moore
security @ redhat

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