Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758654AbbGQPdX (ORCPT ); Fri, 17 Jul 2015 11:33:23 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40097 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757859AbbGQPdV (ORCPT ); Fri, 17 Jul 2015 11:33:21 -0400 Date: Fri, 17 Jul 2015 11:33:17 -0400 From: Richard Guy Briggs To: Paul Moore Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org, Eric Paris , sgrubb@redhat.com, pmoody@google.com Subject: Re: [PATCH V6 1/4] audit: implement audit by executable Message-ID: <20150717153317.GH32473@madcap2.tricolour.ca> References: <1935273.GTuYxx5kcM@sifl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1935273.GTuYxx5kcM@sifl> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10346 Lines: 307 On 15/07/16, Paul Moore wrote: > On Tuesday, July 14, 2015 11:50:23 AM Richard Guy Briggs wrote: > > From: Eric Paris > > > > 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 > > Based-on-idea-by: Peter Moody > > 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@google.com > > Signed-off-by: Eric Paris > > Signed-off-by: Richard Guy Briggs > > 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#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 > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -71,6 +72,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -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 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/