On Tuesday, July 14, 2015 11:50:23 AM Richard Guy Briggs wrote: 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. 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. 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. 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 ... 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. 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. 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 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

On Friday, July 17, 2015 11:33:17 AM Richard Guy Briggs wrote: 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 ;) 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. 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. 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. Yeah ... maybe. World peace too. See, there ya go ... I don't see the point of adding a new file for 50 lines. 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. 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

This is to be used to audit by executable rules, but audit watches should be able to share this code eventually. At the moment the audit watch code is a lot more complex, that code only creates one fsnotify watch per parent directory. That 'audit_parent' in turn has a list of 'audit_watches' which contain the name, ino, dev of the specific object we care about. This just creates one fsnotify watch per object we care about. So if you watch 100 inodes in /etc this code will create 100 fsnotify watches on /etc. The audit_watch code will instead create 1 fsnotify watch on /etc (the audit_parent) and then 100 individual watches chained from that fsnotify mark. We should be able to convert the audit_watch code to do one fsnotify mark per watch and simplify things/remove a whole lot of code. After that conversion we should be able to convert the audit_fsnotify code to support that hierarchy if the optimization is necessary. Signed-off-by: Eric Paris <eparis(a)redhat.com&gt; RGB: Move the access to the entry for audit_match_signal() to the beginning of the function in case the entry found is the same one passed in. This will enable it to be used by audit_autoremove_mark_rule(). RGB: Rename several "watch" references to "mark". RGB: Rename audit_remove_rule() to audit_autoremove_mark_rule(). RGB: Put audit_alloc_mark() arguments in same order as watch, tree and inode. RGB: Remove space from audit log value text in audit_autoremove_mark_rule(). RGB: Explicitly declare prototypes as extern. RGB: Rename audit_remove_mark_rule() called from audit_mark_handle_event() to audit_autoremove_mark_rule() to avoid confusion with audit_remove_{watch,tree}_rule() usage. RGB: Add audit_remove_mark_rule() to provide similar interface as audit_remove_{watch,tree}_rule(). RGB: Simplify stubs to defines. RGB: Rename audit_free_fsnotify_mark() to audit_fsnotify_free_mark() in keeping with the naming convention of inotify_free_mark(), dnotify_free_mark(), fanotify_free_mark(), audit_watch_free_mark(). RGB: Return -ENOMEM rather than null in case of memory allocation failure for audit_mark. RGB: Rename audit_free_mark() to audit_mark_free() to avoid association with {i,d,fa}notify_free_mark() and audit_watch_free_mark(). Based-on-code-by: Eric Paris <eparis(a)redhat.com&gt; Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- kernel/Makefile | 2 +- kernel/audit.h | 22 ++++ kernel/audit_fsnotify.c | 253 +++++++++++++++++++++++++++++++++++++++++++++++ kernel/auditfilter.c | 5 +- 4 files changed, 279 insertions(+), 3 deletions(-) create mode 100644 kernel/audit_fsnotify.c diff --git a/kernel/Makefile b/kernel/Makefile index a7ea330..397109e 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -64,7 +64,7 @@ obj-$(CONFIG_SMP) += stop_machine.o obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o obj-$(CONFIG_AUDIT) += audit.o auditfilter.o obj-$(CONFIG_AUDITSYSCALL) += auditsc.o -obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_exe.o +obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_exe.o audit_fsnotify.o obj-$(CONFIG_AUDIT_TREE) += audit_tree.o obj-$(CONFIG_GCOV_KERNEL) += gcov/ obj-$(CONFIG_KPROBES) += kprobes.o diff --git a/kernel/audit.h b/kernel/audit.h index 3aca24f..491bd4b 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -50,6 +50,7 @@ enum audit_state { /* Rule lists */ struct audit_watch; +struct audit_fsnotify_mark; struct audit_exe; struct audit_tree; struct audit_chunk; @@ -253,6 +254,7 @@ struct audit_net { extern int selinux_audit_rule_update(void); extern struct mutex audit_filter_mutex; +extern int audit_del_rule(struct audit_entry *); extern void audit_free_rule_rcu(struct rcu_head *); extern struct list_head audit_filter_list[]; @@ -271,6 +273,12 @@ extern void audit_remove_watch_rule(struct audit_krule *krule); extern char *audit_watch_path(struct audit_watch *watch); extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t dev); +extern struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len); +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_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op); extern void audit_remove_exe_rule(struct audit_krule *krule); extern char *audit_exe_path(struct audit_exe *exe); @@ -286,6 +294,20 @@ extern int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe); #define audit_watch_path(w) "" #define audit_watch_compare(w, i, d) 0 +#define audit_alloc_mark(k, p, l) (ERR_PTR(-EINVAL)) +static inline char *audit_mark_path(struct audit_fsnotify_mark *mark) +{ + BUG(); + return ""; +} +#define audit_remove_mark(m) BUG() +#define audit_remove_mark_rule(k) BUG() +static inline int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev) +{ + BUG(); + return 0; +} + static inline int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len, u32 op) { return -EINVAL; diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c new file mode 100644 index 0000000..a4e7b16 --- /dev/null +++ b/kernel/audit_fsnotify.c @@ -0,0 +1,253 @@ +/* audit_fsnotify.c -- tracking inodes + * + * Copyright 2003-2009,2014-2015 Red Hat, Inc. + * Copyright 2005 Hewlett-Packard Development Company, L.P. + * Copyright 2005 IBM Corporation + * + * 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. + */ + +#include <linux/kernel.h> +#include <linux/audit.h> +#include <linux/kthread.h> +#include <linux/mutex.h> +#include <linux/fs.h> +#include <linux/fsnotify_backend.h> +#include <linux/namei.h> +#include <linux/netlink.h> +#include <linux/sched.h> +#include <linux/slab.h> +#include <linux/security.h> +#include "audit.h" + +/* + * this mark lives on the parent directory of the inode in question. + * but dev, ino, and path are about the child + */ +struct audit_fsnotify_mark { + dev_t dev; /* associated superblock device */ + unsigned long ino; /* associated inode number */ + char *path; /* insertion path */ + struct fsnotify_mark mark; /* fsnotify mark on the inode */ + struct audit_krule *rule; +}; + +/* fsnotify handle. */ +static struct fsnotify_group *audit_fsnotify_group; + +/* fsnotify events we care about. */ +#define AUDIT_FS_EVENTS (FS_MOVE | FS_CREATE | FS_DELETE | FS_DELETE_SELF |\ + FS_MOVE_SELF | FS_EVENT_ON_CHILD) + +static void audit_mark_free(struct audit_fsnotify_mark *audit_mark) +{ + kfree(audit_mark->path); + kfree(audit_mark); +} + +static void audit_fsnotify_free_mark(struct fsnotify_mark *mark) +{ + struct audit_fsnotify_mark *audit_mark; + + audit_mark = container_of(mark, struct audit_fsnotify_mark, mark); + audit_mark_free(audit_mark); +} + +#if 0 /* not sure if we need these... */ +static void audit_get_mark(struct audit_fsnotify_mark *audit_mark) +{ + if (likely(audit_mark)) + fsnotify_get_mark(&audit_mark->mark); +} + +static void audit_put_mark(struct audit_fsnotify_mark *audit_mark) +{ + if (likely(audit_mark)) + fsnotify_put_mark(&audit_mark->mark); +} +#endif + +char *audit_mark_path(struct audit_fsnotify_mark *mark) +{ + return mark->path; +} + +int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino, dev_t dev) +{ + if (mark->ino == (unsigned long)-1) + return 0; + return (mark->ino == ino) && (mark->dev == dev); +} + +struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pathname, int len) +{ + struct audit_fsnotify_mark *audit_mark; + struct path path; + struct dentry *dentry; + struct inode *inode; + unsigned long ino; + char *local_pathname; + dev_t dev; + int ret; + + if (pathname[0] != '/' || pathname[len-1] == '/') + return ERR_PTR(-EINVAL); + + dentry = kern_path_locked(pathname, &path); + if (IS_ERR(dentry)) + return (void *)dentry; /* returning an error */ + inode = path.dentry->d_inode; + mutex_unlock(&inode->i_mutex); + + if (!dentry->d_inode) { + ino = (unsigned long)-1; + dev = (unsigned)-1; + } else { + dev = dentry->d_inode->i_sb->s_dev; + ino = dentry->d_inode->i_ino; + } + + audit_mark = ERR_PTR(-ENOMEM); + local_pathname = kstrdup(pathname, GFP_KERNEL); + if (!local_pathname) + goto out; + + audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL); + if (unlikely(!audit_mark)) { + kfree(local_pathname); + audit_mark = ERR_PTR(-ENOMEM); + goto out; + } + + fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_mark); + audit_mark->mark.mask = AUDIT_FS_EVENTS; + audit_mark->path = local_pathname; + audit_mark->ino = ino; + audit_mark->dev = dev; + audit_mark->rule = krule; + + ret = fsnotify_add_mark(&audit_mark->mark, audit_fsnotify_group, inode, NULL, true); + if (ret < 0) { + audit_mark_free(audit_mark); + audit_mark = ERR_PTR(ret); + } +out: + dput(dentry); + path_put(&path); + return audit_mark; +} + +static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, char *op) +{ + struct audit_buffer *ab; + struct audit_krule *rule = audit_mark->rule; + if (!audit_enabled) + return; + ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE); + if (unlikely(!ab)) + return; + audit_log_format(ab, "auid=%u ses=%u op=", + from_kuid(&init_user_ns, audit_get_loginuid(current)), + audit_get_sessionid(current)); + audit_log_string(ab, op); + audit_log_format(ab, " path="); + audit_log_untrustedstring(ab, audit_mark->path); + audit_log_key(ab, rule->filterkey); + audit_log_format(ab, " list=%d res=1", rule->listnr); + audit_log_end(ab); +} + +static int audit_update_mark(struct audit_fsnotify_mark *audit_mark, + struct inode *inode) +{ + if (inode) { + audit_mark->dev = inode->i_sb->s_dev; + audit_mark->ino = inode->i_ino; + } else { + audit_mark->dev = (unsigned)-1; + audit_mark->ino = (unsigned long)-1; + } + return 0; +} + +void audit_remove_mark(struct audit_fsnotify_mark *audit_mark) +{ + fsnotify_destroy_mark(&audit_mark->mark, audit_fsnotify_group); + fsnotify_put_mark(&audit_mark->mark); +} + +void audit_remove_mark_rule(struct audit_krule *krule) +{ + struct audit_fsnotify_mark *mark = krule->exe; + + audit_remove_mark(mark); +} + +static void audit_autoremove_mark_rule(struct audit_fsnotify_mark *audit_mark) +{ + struct audit_krule *rule = audit_mark->rule; + struct audit_entry *entry = container_of(rule, struct audit_entry, rule); + + audit_mark_log_rule_change(audit_mark, "autoremove_rule"); + audit_del_rule(entry); +} + +/* Update mark data in audit rules based on fsnotify events. */ +static int audit_mark_handle_event(struct fsnotify_group *group, + struct inode *to_tell, + struct fsnotify_mark *inode_mark, + struct fsnotify_mark *vfsmount_mark, + u32 mask, void *data, int data_type, + const unsigned char *dname, u32 cookie) +{ + struct audit_fsnotify_mark *audit_mark; + struct inode *inode = NULL; + + audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark); + + BUG_ON(group != audit_fsnotify_group); + + switch (data_type) { + case (FSNOTIFY_EVENT_PATH): + inode = ((struct path *)data)->dentry->d_inode; + break; + case (FSNOTIFY_EVENT_INODE): + inode = (struct inode *)data; + break; + default: + BUG(); + return 0; + }; + + if (mask & (FS_CREATE|FS_MOVED_TO|FS_DELETE|FS_MOVED_FROM)) { + if (audit_compare_dname_path(dname, audit_mark->path, AUDIT_NAME_FULL)) + return 0; + audit_update_mark(audit_mark, inode); + } else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF)) + audit_autoremove_mark_rule(audit_mark); + + return 0; +} + +static const struct fsnotify_ops audit_mark_fsnotify_ops = { + .handle_event = audit_mark_handle_event, +}; + +static int __init audit_fsnotify_init(void) +{ + audit_fsnotify_group = fsnotify_alloc_group(&audit_mark_fsnotify_ops); + if (IS_ERR(audit_fsnotify_group)) { + audit_fsnotify_group = NULL; + audit_panic("cannot create audit fsnotify group"); + } + return 0; +} +device_initcall(audit_fsnotify_init); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 09041b2..bbb39ec 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -977,7 +977,7 @@ error: } /* Remove an existing rule from filterlist. */ -static inline int audit_del_rule(struct audit_entry *entry) +int audit_del_rule(struct audit_entry *entry) { struct audit_entry *e; struct audit_tree *tree = entry->rule.tree; @@ -985,6 +985,7 @@ static inline int audit_del_rule(struct audit_entry *entry) int ret = 0; #ifdef CONFIG_AUDITSYSCALL int dont_count = 0; + int match = audit_match_signal(entry); /* If either of these, don't count towards total */ if (entry->rule.listnr == AUDIT_FILTER_USER || @@ -1017,7 +1018,7 @@ static inline int audit_del_rule(struct audit_entry *entry) if (!dont_count) audit_n_rules--; - if (!audit_match_signal(entry)) + if (!match) audit_signals--; #endif mutex_unlock(&audit_filter_mutex); -- 1.7.1

On 15/07/16, Paul Moore wrote: Done. Or 1/2 as you have suggested. Cleaned them out... Uh, yes, ok. Agreed. Missed that. I found nothing obvious, but it does surprise me a bit too... It would make sense if there were two structs that both included ino and dev members to call a funciton/macro with pointers to the two structs, or even one struct and an ino dev tuple, but when the four are discreet, it starts to lose its utility... Yeah, I didn't like it either, that's why 4/4 exists... I used audit_watch_log_rule_change() and audit_tree_log_remove_rule() as references... I think it is fine. It should not be, if the code is correct. I have no reason to believe otherwise since we are the only callers and no case should trigger it. Oh dang, missed this one in the patchsets I just sent out. This too was a development debug aid. I don't expect this condition to be hit, but pr_err(...) would work fine here. I've split it out and added a note to the cover letter. Er, I should have included the removal of "static inline" in that split out patch... - RGB -- Richard Guy Briggs <rbriggs(a)redhat.com&gt; 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

On Tuesday, July 14, 2015 11:50:25 AM Richard Guy Briggs wrote: Ah, good, the fix that patch 1/4 was hoping for is finally happening :) Since we're not getting paid by the number of patches in a patch set, I think I would prefer to see the fsnotify implementation as the first patch and the original crappy exe filter patch merged with this fixup patch. No in depth comments on this particular patch, I skimmed it but frankly it makes much more sense to review this patch once you've merged the two. -- paul moore security @ redhat

Make this interface consistent with watch and filter key, avoiding the extra string copy and simply consume the new string pointer. Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- kernel/audit_exe.c | 8 ++++++-- kernel/audit_fsnotify.c | 9 +-------- kernel/auditfilter.c | 2 +- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c index 75ad4f2..09e4eb4 100644 --- a/kernel/audit_exe.c +++ b/kernel/audit_exe.c @@ -27,11 +27,15 @@ int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old) struct audit_fsnotify_mark *audit_mark; char *pathname; - pathname = audit_mark_path(old->exe); + 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)) + if (IS_ERR(audit_mark)) { + kfree(pathname); return PTR_ERR(audit_mark); + } new->exe = audit_mark; return 0; diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c index a4e7b16..e57e08a 100644 --- a/kernel/audit_fsnotify.c +++ b/kernel/audit_fsnotify.c @@ -94,7 +94,6 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa struct dentry *dentry; struct inode *inode; unsigned long ino; - char *local_pathname; dev_t dev; int ret; @@ -115,21 +114,15 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa ino = dentry->d_inode->i_ino; } - audit_mark = ERR_PTR(-ENOMEM); - local_pathname = kstrdup(pathname, GFP_KERNEL); - if (!local_pathname) - goto out; - audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL); if (unlikely(!audit_mark)) { - kfree(local_pathname); audit_mark = ERR_PTR(-ENOMEM); goto out; } fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_mark); audit_mark->mark.mask = AUDIT_FS_EVENTS; - audit_mark->path = local_pathname; + audit_mark->path = pathname; audit_mark->ino = ino; audit_mark->dev = dev; audit_mark->rule = krule; diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index f65c97f..f46ed69 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -559,8 +559,8 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, entry->rule.buflen += f->val; audit_mark = audit_alloc_mark(&entry->rule, str, f->val); - kfree(str); if (IS_ERR(audit_mark)) { + kfree(str); err = PTR_ERR(audit_mark); goto exit_free; } -- 1.7.1

On 15/07/16, Paul Moore wrote: I wanted to keep this patch seperate until it is well understood and accepted rather than mix it in. I'm fine merging it if you prefer. - RGB -- Richard Guy Briggs <rbriggs(a)redhat.com&gt; 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

I have to admit, I'm partial to not merging this (with the other patches). Changing object lifetimes in what i seem to remember is long standing code (auditfilter, not auditexe) seems to me like something we really would want to be git bisectable, not mushed with an unrelated feature addition. But it ain't my tree :) -Eric On Thu, 2015-07-16 at 22:01 -0400, Richard Guy Briggs wrote:

On 15/07/16, Paul Moore wrote: Correct. However, it aims to follow the approach used in watch and tree code, rather than making yet another copy. - RGB -- Richard Guy Briggs <rbriggs(a)redhat.com&gt; 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

On 15/07/16, Eric Paris wrote: So maybe even fixing this before applying the audit exe stuff has merit... - RGB -- Richard Guy Briggs <rbriggs(a)redhat.com&gt; 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