[RFC PATCH 2/2] [WIP] audit: allow other filter list types for AUDIT_DIR
Ondrej Mosnacek
omosnace at redhat.com
Fri Jun 1 08:12:37 UTC 2018
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).
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.
>
>> + 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).
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.
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.
More information about the Linux-audit
mailing list