Re: [RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR

2018-06-01 22:05 GMT+02:00 Richard Guy Briggs <rgb(a)redhat.com&gt;: > On 2018-06-01 10:12, Ondrej Mosnacek wrote: >> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb(a)redhat.com&gt;: >> > On 2018-05-30 10:45, Ondrej Mosnacek wrote: >> >> This patch allows the AUDIR_DIR field to be used also with the exclude >> >> filter. >> >> >> >> Not-yet-signed-off-by: Ondrej Mosnacek <omosnace(a)redhat.com&gt; >> >> --- >> >> kernel/audit.c | 5 +++-- >> >> kernel/audit.h | 32 +++++++++++++++++++++++++++++++- >> >> kernel/audit_tree.c | 4 +++- >> >> kernel/auditfilter.c | 6 +++++- >> >> kernel/auditsc.c | 28 ---------------------------- >> >> 5 files changed, 42 insertions(+), 33 deletions(-) >> >> >> >> diff --git a/kernel/audit.c b/kernel/audit.c >> >> index e7478cb58079..aac5b6ecc11d 100644 >> >> --- a/kernel/audit.c >> >> +++ b/kernel/audit.c >> >> @@ -1333,7 +1333,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) >> >> if (!audit_enabled && msg_type != AUDIT_USER_AVC) >> >> return 0; >> >> >> >> - err = audit_filter(msg_type, AUDIT_FILTER_USER); >> >> + // FIXME: do we need to pass the context here? >> >> + err = audit_filter(msg_type, AUDIT_FILTER_USER, NULL); >> >> if (err == 1) { /* match or error */ >> >> err = 0; >> >> if (msg_type == AUDIT_USER_TTY) { >> >> @@ -1754,7 +1755,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, >> >> if (audit_initialized != AUDIT_INITIALIZED) >> >> return NULL; >> >> >> >> - if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE))) >> >> + if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE, ctx))) >> >> return NULL; >> >> >> >> /* NOTE: don't ever fail/sleep on these two conditions: >> >> diff --git a/kernel/audit.h b/kernel/audit.h >> >> index 214e14948370..43cfeba25802 100644 >> >> --- a/kernel/audit.h >> >> +++ b/kernel/audit.h >> >> @@ -324,13 +324,43 @@ extern void audit_kill_trees(struct list_head *list); >> >> #define audit_kill_trees(list) BUG() >> >> #endif >> >> >> >> +struct audit_tree_refs { >> >> + struct audit_tree_refs *next; >> >> + struct audit_chunk *c[31]; >> >> +}; >> >> + >> >> +/* A utility function to match tree refs: */ >> >> +static inline int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) >> >> +{ >> >> +#ifdef CONFIG_AUDIT_TREE >> >> + struct audit_tree_refs *p; >> >> + int n; >> >> + if (!tree) >> >> + return 0; >> >> + /* full ones */ >> >> + for (p = ctx->first_trees; p != ctx->trees; p = p->next) { >> >> + for (n = 0; n < 31; n++) >> >> + if (audit_tree_match(p->c[n], tree)) >> >> + return 1; >> >> + } >> >> + /* partial */ >> >> + if (p) { >> >> + for (n = ctx->tree_count; n < 31; n++) >> >> + if (audit_tree_match(p->c[n], tree)) >> >> + return 1; >> >> + } >> >> +#endif >> >> + return 0; >> >> +} >> >> + >> >> extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len); >> >> >> >> extern pid_t audit_sig_pid; >> >> extern kuid_t audit_sig_uid; >> >> extern u32 audit_sig_sid; >> >> >> >> -extern int audit_filter(int msgtype, unsigned int listtype); >> >> +extern int audit_filter(int msgtype, unsigned int listtype, >> >> + struct audit_context *ctx); >> >> >> >> #ifdef CONFIG_AUDITSYSCALL >> >> extern int audit_signal_info(int sig, struct task_struct *t); >> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c >> >> index 67e6956c0b61..d4d36389c3d7 100644 >> >> --- a/kernel/audit_tree.c >> >> +++ b/kernel/audit_tree.c >> >> @@ -675,9 +675,11 @@ void audit_trim_trees(void) >> >> >> >> int audit_make_tree(struct audit_krule *rule, char *pathname, u32 op) >> >> { >> >> + if (krule->listnr != AUDIT_FILTER_EXIT && >> >> + krule->listnr != AUDIT_FILTER_TYPE) >> >> + return -EINVAL; >> >> >> >> if (pathname[0] != '/' || >> >> - rule->listnr != AUDIT_FILTER_EXIT || >> >> op != Audit_equal || >> >> rule->inode_f || rule->watch || rule->tree) >> >> return -EINVAL; >> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c >> >> index 6db9847ca031..e1d70cb77ea3 100644 >> >> --- a/kernel/auditfilter.c >> >> +++ b/kernel/auditfilter.c >> >> @@ -1309,7 +1309,7 @@ int audit_compare_dname_path(const char *dname, const char *path, int parentlen) >> >> return strncmp(p, dname, dlen); >> >> } >> >> >> >> -int audit_filter(int msgtype, unsigned int listtype) >> >> +int audit_filter(int msgtype, unsigned int listtype, struct audit_context *ctx) >> >> { >> >> struct audit_entry *e; >> >> int ret = 1; /* Audit by default */ >> >> @@ -1363,6 +1363,10 @@ int audit_filter(int msgtype, unsigned int listtype) >> >> if (f->op == Audit_not_equal) >> >> result = !result; >> >> break; >> >> + case AUDIT_DIR: >> >> + if (ctx) >> >> + result = match_tree_refs(ctx, e->rule.tree); >> > >> > I don't see why you need to send a context to audit_filter, since the >> > rest of the function assumes the current task you can just use >> > audit_context() to hand to match_tree_refs(). You could even check that >> > ctx isn't NULL in match_tree_refs() to hide that code from >> > audit_filter(). >> >> I wasn't sure if I could do that, since audit_filter didn't need the >> context until now and it is called from two places: >> >> audit_log_start -- which accepts context as an argument (doesn't get >> it from task, and it can be called with ctx == NULL). > > Alright, if current has no context, then match_tree_refs() could test > for a NULL context and return since there are no trees to check. Having > said that, there are cases where audit_log_start() is deliberately > handed a NULL context which should be honoured. It appears we don't > have a choice but to pass in the context. OK, so I'll leave that part as it is. > >> audit_receive_msg -- this function doesn't work with context at all, >> so I wasn't sure if audit_filter should consider it being NULL or if >> it should get it from the current task. My hunch is the second option >> is the right one, but the first one is safer (AUDIT_DIR will simply >> never be checked here). I don't have such insight into the logic of >> audit_context's lifetime, so I need someone to tell me what makes more >> sense here. > > That is starting to work with context. The recent FEATURE_CHANGE patch > to connect records of the same event uses current->audit_context (now > audit_context()) from audit_log_feature_change() called from > audit_set_feature() called from audit_receive_msg(). > > There is also a work in progress to convert all the CONFIG_CHANGE > records over. I'm just trying to track down all the instances. OK, so does that mean that I should just pass audit_context() instead of NULL in audit_receive_msg()?

The part that calls audit_filter() only deals with user messages (AUDIT_USER/AUDIT_FIRST..LAST_USER_MESSAGE) -- does it make sense to use the task's context in AUDIT_FILTER_USER (right now just with AUDIT_DIR)? It already filters based on the task's credentials, so perhaps it does, but I don't know...

>> >> + break; >> >> default: >> >> goto unlock_and_return; >> >> } >> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> >> index ceb1c4596c51..0d00b9354886 100644 >> >> --- a/kernel/auditsc.c >> >> +++ b/kernel/auditsc.c >> >> @@ -125,11 +125,6 @@ struct audit_aux_data_bprm_fcaps { >> >> struct audit_cap_data new_pcap; >> >> }; >> >> >> >> -struct audit_tree_refs { >> >> - struct audit_tree_refs *next; >> >> - struct audit_chunk *c[31]; >> >> -}; >> >> - >> >> static int audit_match_perm(struct audit_context *ctx, int mask) >> >> { >> >> unsigned n; >> >> @@ -286,29 +281,6 @@ static void free_tree_refs(struct audit_context *ctx) >> >> } >> >> } >> >> >> >> -static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) >> >> -{ >> >> -#ifdef CONFIG_AUDIT_TREE >> >> - struct audit_tree_refs *p; >> >> - int n; >> >> - if (!tree) >> >> - return 0; >> >> - /* full ones */ >> >> - for (p = ctx->first_trees; p != ctx->trees; p = p->next) { >> >> - for (n = 0; n < 31; n++) >> >> - if (audit_tree_match(p->c[n], tree)) >> >> - return 1; >> >> - } >> >> - /* partial */ >> >> - if (p) { >> >> - for (n = ctx->tree_count; n < 31; n++) >> >> - if (audit_tree_match(p->c[n], tree)) >> >> - return 1; >> >> - } >> >> -#endif >> >> - return 0; >> >> -} >> >> - >> > >> > Why did you move match_tree_refs() out of auditsc.c? >> >> Because now it can be called from both 'audit_filter_rules()' (defined >> in auditsc.c) and 'audit_filter()' (defined in auditfilter.c). > > So since kernel/audit.h is included in all kernel/audit*.c files, having > the prototype in there from any kernel/audit*.c file should make the > function available to all functions in kernel/audit*.c files. You're right, I could have just added a declaration and export the function from auditsc.c... I will need to add a fallback definition for when CONFIG_AUDITSYSCALL is not enabled, but in that case tasks never have audit_context set anyway, so it's fine. I will just export the function in the next version. > >> I'm actually playing with the idea of unifying the filtering logic in >> these two functions, where sharing this function wouldn't be >> necessary. However, that is quite a big change (a lot of LOC being >> moved around) so I'd prefer the simple & dirty approach now and keep >> the cleanup for a later patch. > > Some of the filtering has already been refactored and merged. More > wouldn't hurt. > >> Thanks for the comments! >> >> >> static int audit_compare_uid(kuid_t uid, >> >> struct audit_names *name, >> >> struct audit_field *f, >> >> -- >> >> 2.17.0 >> >> >> > >> > - RGB >> > >> > -- >> > Richard Guy Briggs <rgb(a)redhat.com&gt; >> > Sr. S/W Engineer, Kernel Security, Base Operating Systems >> > Remote, Ottawa, Red Hat Canada >> > IRC: rgb, SunRaycer >> > Voice: +1.647.777.2635, Internal: (81) 32635 >> >> -- >> Ondrej Mosnacek <omosnace at redhat dot com> >> Associate Software Engineer, Security Technologies >> Red Hat, Inc. > > - RGB > > -- > Richard Guy Briggs <rgb(a)redhat.com&gt; > Sr. S/W Engineer, Kernel Security, Base Operating Systems > Remote, Ottawa, Red Hat Canada > IRC: rgb, SunRaycer > Voice: +1.647.777.2635, Internal: (81) 32635 -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.