Message-ID: <20150717204618.GB28281@madcap2.tricolour.ca>
Date:	Fri, 17 Jul 2015 16:46:18 -0400
From:	Richard Guy Briggs <rgb@...hat.com>
To:	Paul Moore <pmoore@...hat.com>
Cc:	peter@...3.com, linux-audit@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V6 1/4] audit: implement audit by executable

On 15/07/17, Paul Moore wrote:
> 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 ;)

>From Documentation/SubmittingPatches:
	11) Signed-off-by:
	12) Acked-by: and Cc:
	13) Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:

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

The description will need a complete overhaul, so it'll get fixed.

> > > > 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
> 
> --
> Linux-audit mailing list
> Linux-audit@...hat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- 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