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(a)redhat.com&gt; > > 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(a)redhat.com&gt; > Based-on-idea-by: Peter Moody <pmoody(a)google.com&gt; 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(a)google.com > Signed-off-by: Eric Paris <eparis(a)redhat.com&gt; > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; 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