Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753067AbbHGGZU (ORCPT ); Fri, 7 Aug 2015 02:25:20 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36989 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751129AbbHGGZS (ORCPT ); Fri, 7 Aug 2015 02:25:18 -0400 Date: Fri, 7 Aug 2015 02:25:14 -0400 From: Richard Guy Briggs To: Paul Moore Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org, sgrubb@redhat.com, eparis@redhat.com, peter@hda3.com Subject: Re: [PATCH V9 2/3] audit: implement audit by executable Message-ID: <20150807062411.GB12116@madcap2.tricolour.ca> References: <7d151a4fc35f52c187a383a3648ac9fc7dd2f361.1438801342.git.rgb@redhat.com> <42326522.RDQEUGoptm@sifl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <42326522.RDQEUGoptm@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: 12745 Lines: 339 On 15/08/06, Paul Moore wrote: > On Wednesday, August 05, 2015 04:29:37 PM Richard Guy Briggs wrote: > > This adds the ability audit the actions of a not-yet-running process. > > > > This patch implements the ability to filter on the executable path. Instead > > of just hard coding the ino and dev of the executable we care about at the > > moment the rule is inserted into the kernel, use the new audit_fsnotify > > infrastructure to manage this dynamically. This means that if the filename > > does not yet exist but the containing directory does, or if the inode in > > question is unlinked and creat'd (aka updated) the rule will just continue > > to work. If the containing directory is moved or deleted or the filesystem > > is unmounted, the rule is deleted automatically. A future enhancement > > would be to have the rule survive across directory disruptions. > > > > This is a heavily modified version of a patch originally submitted by Eric > > Paris with some ideas from Peter Moody. > > > > Cc: Peter Moody > > Cc: Eric Paris > > Signed-off-by: Richard Guy Briggs > > --- > > include/linux/audit.h | 1 + > > include/uapi/linux/audit.h | 5 +++- > > kernel/audit.h | 4 +++ > > kernel/audit_tree.c | 2 + > > kernel/audit_watch.c | 31 +++++++++++++++++++++++++ > > kernel/auditfilter.c | 53 ++++++++++++++++++++++++++++++++++++++++- > > kernel/auditsc.c | 3 ++ > > 7 files changed, 97 insertions(+), 2 deletions(-) > > Merged, although some more minor whitespace tweaks were necessary for > checkpatch. On a related note, if you're not running ./scripts/checlpatch.pl > on your patches before sending them out, I would recommend it. It isn't > perfect, but it can catch some silly things that we all do from time to time. No excuses... I have been running it pretty regularly and got lazy and distracted with patch revisions. I can't say I agree with the no space before closing round parenthesis due to legibility, but will comply. > Also, one last thing. It is pretty late in the -rcX cycle to merge these two > patches, but considering that we've been talking about these for a while, I'm > reasonably okay merging them. In the future, if it isn't in audit#next by the > time -rc5 is released, it isn't going to make the merge window. I've been quite aware of that looming merge window... This feature has been iterating for a while, so there are no big surprises. I was aiming for earlier. :) > > diff --git a/include/linux/audit.h b/include/linux/audit.h > > index c2e7e3a..aee456f 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -59,6 +59,7 @@ struct audit_krule { > > struct audit_field *inode_f; /* quick access to an inode field */ > > struct audit_watch *watch; /* associated watch */ > > struct audit_tree *tree; /* associated watched tree */ > > + struct audit_fsnotify_mark *exe; > > struct list_head rlist; /* entry in audit_{watch,tree}.rules list */ > > struct list_head list; /* for AUDIT_LIST* purposes only */ > > u64 prio; > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > index 971df22..e2ca600 100644 > > --- a/include/uapi/linux/audit.h > > +++ b/include/uapi/linux/audit.h > > @@ -266,6 +266,7 @@ > > #define AUDIT_OBJ_UID 109 > > #define AUDIT_OBJ_GID 110 > > #define AUDIT_FIELD_COMPARE 111 > > +#define AUDIT_EXE 112 > > > > #define AUDIT_ARG0 200 > > #define AUDIT_ARG1 (AUDIT_ARG0+1) > > @@ -324,8 +325,10 @@ enum { > > > > #define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001 > > #define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002 > > +#define AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH 0x00000004 > > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \ > > - AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME) > > + AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \ > > + AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH ) > > > > /* deprecated: AUDIT_VERSION_* */ > > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL > > diff --git a/kernel/audit.h b/kernel/audit.h > > index 46d10dd..dadf86a 100644 > > --- a/kernel/audit.h > > +++ b/kernel/audit.h > > @@ -277,6 +277,8 @@ extern char *audit_mark_path(struct audit_fsnotify_mark > > *mark); extern void audit_remove_mark(struct audit_fsnotify_mark > > *audit_mark); extern void audit_remove_mark_rule(struct audit_krule > > *krule); > > extern int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned > > long ino, dev_t dev); +extern int audit_dupe_exe(struct audit_krule *new, > > struct audit_krule *old); +extern int audit_exe_compare(struct task_struct > > *tsk, struct audit_fsnotify_mark *mark); > > > > #else > > #define audit_put_watch(w) {} > > @@ -292,6 +294,8 @@ extern int audit_mark_compare(struct audit_fsnotify_mark > > *mark, unsigned long in #define audit_remove_mark(m) > > #define audit_remove_mark_rule(k) > > #define audit_mark_compare(m, i, d) 0 > > +#define audit_exe_compare(t, m) (-EINVAL) > > +#define audit_dupe_exe(n, o) (-EINVAL) > > #endif /* CONFIG_AUDIT_WATCH */ > > > > #ifdef CONFIG_AUDIT_TREE > > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c > > index b0f9877..94ecdab 100644 > > --- a/kernel/audit_tree.c > > +++ b/kernel/audit_tree.c > > @@ -479,6 +479,8 @@ static void kill_rules(struct audit_tree *tree) > > if (rule->tree) { > > /* not a half-baked one */ > > audit_tree_log_remove_rule(rule); > > + if (entry->rule.exe) > > + audit_remove_mark(entry->rule.exe); > > rule->tree = NULL; > > list_del_rcu(&entry->list); > > list_del(&entry->rule.list); > > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c > > index c668bfc..1255dbf 100644 > > --- a/kernel/audit_watch.c > > +++ b/kernel/audit_watch.c > > @@ -312,6 +312,8 @@ static void audit_update_watch(struct audit_parent > > *parent, list_replace(&oentry->rule.list, > > &nentry->rule.list); > > } > > + if (oentry->rule.exe) > > + audit_remove_mark(oentry->rule.exe); > > > > audit_watch_log_rule_change(r, owatch, "updated_rules"); > > > > @@ -342,6 +344,8 @@ static void audit_remove_parent_watches(struct > > audit_parent *parent) list_for_each_entry_safe(r, nextr, &w->rules, rlist) > > { > > e = container_of(r, struct audit_entry, rule); > > audit_watch_log_rule_change(r, w, "remove_rule"); > > + if (e->rule.exe) > > + audit_remove_mark(e->rule.exe); > > list_del(&r->rlist); > > list_del(&r->list); > > list_del_rcu(&e->list); > > @@ -514,3 +518,30 @@ static int __init audit_watch_init(void) > > return 0; > > } > > device_initcall(audit_watch_init); > > + > > +int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old) > > +{ > > + struct audit_fsnotify_mark *audit_mark; > > + char *pathname; > > + > > + pathname = kstrdup(audit_mark_path(old->exe), GFP_KERNEL); > > + if (!pathname) > > + return -ENOMEM; > > + > > + audit_mark = audit_alloc_mark(new, pathname, strlen(pathname)); > > + if (IS_ERR(audit_mark)) { > > + kfree(pathname); > > + return PTR_ERR(audit_mark); > > + } > > + new->exe = audit_mark; > > + > > + return 0; > > +} > > + > > +int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark > > *mark) +{ > > + unsigned long ino = tsk->mm->exe_file->f_inode->i_ino; > > + dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev; > > + > > + return audit_mark_compare(mark, ino, dev); > > +} > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > index 3d99196..c662638 100644 > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c > > @@ -405,6 +405,12 @@ static int audit_field_valid(struct audit_entry *entry, > > struct audit_field *f) if (f->val > AUDIT_MAX_FIELD_COMPARE) > > return -EINVAL; > > break; > > + case AUDIT_EXE: > > + if (f->op != Audit_equal) > > + return -EINVAL; > > + if (entry->rule.listnr != AUDIT_FILTER_EXIT) > > + return -EINVAL; > > + break; > > }; > > return 0; > > } > > @@ -419,6 +425,7 @@ static struct audit_entry *audit_data_to_entry(struct > > audit_rule_data *data, size_t remain = datasz - sizeof(struct > > audit_rule_data); > > int i; > > char *str; > > + struct audit_fsnotify_mark *audit_mark; > > > > entry = audit_to_entry_common(data); > > if (IS_ERR(entry)) > > @@ -539,6 +546,24 @@ static struct audit_entry *audit_data_to_entry(struct > > audit_rule_data *data, entry->rule.buflen += f->val; > > entry->rule.filterkey = str; > > break; > > + case AUDIT_EXE: > > + if (entry->rule.exe || f->val > PATH_MAX) > > + goto exit_free; > > + str = audit_unpack_string(&bufp, &remain, f->val); > > + if (IS_ERR(str)) { > > + err = PTR_ERR(str); > > + goto exit_free; > > + } > > + entry->rule.buflen += f->val; > > + > > + audit_mark = audit_alloc_mark(&entry->rule, str, f->val); > > + if (IS_ERR(audit_mark)) { > > + kfree(str); > > + err = PTR_ERR(audit_mark); > > + goto exit_free; > > + } > > + entry->rule.exe = audit_mark; > > + break; > > } > > } > > > > @@ -551,6 +576,8 @@ exit_nofree: > > exit_free: > > if (entry->rule.tree) > > audit_put_tree(entry->rule.tree); /* that's the temporary one */ > > + if (entry->rule.exe) > > + audit_remove_mark(entry->rule.exe); /* that's the template one */ > > audit_free_rule(entry); > > return ERR_PTR(err); > > } > > @@ -615,6 +642,10 @@ static struct audit_rule_data > > *audit_krule_to_data(struct audit_krule *krule) data->buflen += > > data->values[i] = > > audit_pack_string(&bufp, krule->filterkey); > > break; > > + case AUDIT_EXE: > > + data->buflen += data->values[i] = > > + audit_pack_string(&bufp, audit_mark_path(krule->exe)); > > + break; > > case AUDIT_LOGINUID_SET: > > if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) { > > data->fields[i] = AUDIT_LOGINUID; > > @@ -678,6 +709,12 @@ static int audit_compare_rule(struct audit_krule *a, > > struct audit_krule *b) if (strcmp(a->filterkey, b->filterkey)) > > return 1; > > break; > > + case AUDIT_EXE: > > + /* both paths exist based on above type compare */ > > + if (strcmp(audit_mark_path(a->exe), > > + audit_mark_path(b->exe))) > > + return 1; > > + break; > > case AUDIT_UID: > > case AUDIT_EUID: > > case AUDIT_SUID: > > @@ -799,8 +836,14 @@ struct audit_entry *audit_dupe_rule(struct audit_krule > > *old) err = -ENOMEM; > > else > > new->filterkey = fk; > > + break; > > + case AUDIT_EXE: > > + err = audit_dupe_exe(new, old); > > + break; > > } > > if (err) { > > + if (new->exe) > > + audit_remove_mark(new->exe); > > audit_free_rule(entry); > > return ERR_PTR(err); > > } > > @@ -963,6 +1006,9 @@ int audit_del_rule(struct audit_entry *entry) > > if (e->rule.tree) > > audit_remove_tree_rule(&e->rule); > > > > + if (e->rule.exe) > > + audit_remove_mark_rule(&e->rule); > > + > > #ifdef CONFIG_AUDITSYSCALL > > if (!dont_count) > > audit_n_rules--; > > @@ -1067,8 +1113,11 @@ int audit_rule_change(int type, __u32 portid, int > > seq, void *data, WARN_ON(1); > > } > > > > - if (err || type == AUDIT_DEL_RULE) > > + if (err || type == AUDIT_DEL_RULE) { > > + if (entry->rule.exe) > > + audit_remove_mark(entry->rule.exe); > > audit_free_rule(entry); > > + } > > > > return err; > > } > > @@ -1360,6 +1409,8 @@ static int update_lsm_rule(struct audit_krule *r) > > return 0; > > > > nentry = audit_dupe_rule(r); > > + if (entry->rule.exe) > > + audit_remove_mark(entry->rule.exe); > > if (IS_ERR(nentry)) { > > /* save the first error encountered for the > > * return value */ > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 701ea5c..e9bac2b 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -466,6 +466,9 @@ 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_UID: > > result = audit_uid_comparator(cred->uid, f->op, f->uid); > > break; > > -- > paul moore > security @ redhat > - 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/