Re: [PATCH V9 2/3] audit: implement audit by executable

From: Richard Guy Briggs
Date: Fri Aug 07 2015 - 02:25:44 EST

Next message: Sergey Senozhatsky: "Re: [PATCH 2/3] zswap: dynamic pool creation"
Previous message: Byungchul Park: "[PATCH v3] sched: separate operations only for SMP from account_entity_en(de)queue"
In reply to: Paul Moore: "Re: [PATCH V9 2/3] audit: implement audit by executable"
Next in thread: Paul Moore: "Re: [PATCH V9 2/3] audit: implement audit by executable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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 <peter@xxxxxxxx>
> > Cc: Eric Paris <eparis@xxxxxxxxxx>
> > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> > ---
> > 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 <rbriggs@xxxxxxxxxx>
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Sergey Senozhatsky: "Re: [PATCH 2/3] zswap: dynamic pool creation"
Previous message: Byungchul Park: "[PATCH v3] sched: separate operations only for SMP from account_entity_en(de)queue"
In reply to: Paul Moore: "Re: [PATCH V9 2/3] audit: implement audit by executable"
Next in thread: Paul Moore: "Re: [PATCH V9 2/3] audit: implement audit by executable"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]