[RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR
Ondrej Mosnacek
omosnace at redhat.com
Mon Jun 4 11:32:15 UTC 2018
2018-06-01 22:05 GMT+02:00 Richard Guy Briggs <rgb at redhat.com>:
> On 2018-06-01 10:12, Ondrej Mosnacek wrote:
>> 2018-05-31 22:52 GMT+02:00 Richard Guy Briggs <rgb at redhat.com>:
>> > 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 at redhat.com>
>> >> ---
>> >> 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 at redhat.com>
>> > 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 at redhat.com>
> 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.
More information about the Linux-audit
mailing list