Message-ID: <1935273.GTuYxx5kcM@sifl>
Date:	Thu, 16 Jul 2015 21:18:43 -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 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.

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.

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

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

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

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

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

Help me understand what this accomplishes, I'm a little tried right now and I 
just don't get it.

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