* [PATCH] audit: fix uninitialized variable in audit_add_rule()
From: Paul Moore @ 2015-08-05 15:22 UTC (permalink / raw)
To: linux-audit; +Cc: rgb
As reported by the 0-Day testing service:
kernel/auditfilter.c: In function 'audit_rule_change':
>> kernel/auditfilter.c:864:6: warning: 'err' may be used uninit...
int err;
Cc: Richard Guy Briggs <rgb@redhat.com>
Signed-off-by: Paul Moore <pmoore@redhat.com>
---
kernel/auditfilter.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 4cb9b44..83f6d29 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -861,7 +861,7 @@ static inline int audit_add_rule(struct audit_entry *entry)
struct audit_watch *watch = entry->rule.watch;
struct audit_tree *tree = entry->rule.tree;
struct list_head *list;
- int err;
+ int err = 0;
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
^ permalink raw reply related
* Re: [PATCH V5] audit: save signal match info in case entry passed in is the one deleted
From: Paul Moore @ 2015-08-05 15:12 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <d82d0b6211720eedb017399115ad7bfc83d10116.1438764814.git.rgb@redhat.com>
On Wednesday, August 05, 2015 05:23:10 AM Richard Guy Briggs wrote:
> 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_remove_mark_rule().
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> Revision history:
> v4 -> v5:
> Move mutex_unlock after out label.
> Move list_del group after test for signal to remove temp variable.
>
> ---
> This patch was split out from the audit by executable path patch set due to
> the potential to use it elsewhere.
>
> In particular, some questions came up while assessing the potential for code
> reuse:
>
> Why does audit_remove_parent_watches() not call audit_del_rule() for
> each entry found?
> Is audit_signals not properly decremented?
> Is audit_n_rules not properly decremented?
>
> Why does kill_rules() not call audit_del_rule() for each entry
> found? Is audit_signals not properly decremented?
> Is audit_n_rules not properly decremented?
>
> kernel/auditfilter.c | 13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 4cb9b44..1b110fb 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -953,7 +953,6 @@ static inline int audit_del_rule(struct audit_entry
> *entry) mutex_lock(&audit_filter_mutex);
> e = audit_find_rule(entry, &list);
> if (!e) {
> - mutex_unlock(&audit_filter_mutex);
> ret = -ENOENT;
> goto out;
> }
> @@ -964,9 +963,8 @@ static inline int audit_del_rule(struct audit_entry
> *entry) if (e->rule.tree)
> audit_remove_tree_rule(&e->rule);
>
> - list_del_rcu(&e->list);
> - list_del(&e->rule.list);
> - call_rcu(&e->rcu, audit_free_rule_rcu);
> + if (e->rule.exe)
> + audit_remove_mark_rule(&e->rule);
What?
I think you munged a cut n' paste somehow. This code doesn't even compile.
> #ifdef CONFIG_AUDITSYSCALL
> if (!dont_count)
> @@ -975,9 +973,14 @@ static inline int audit_del_rule(struct audit_entry
> *entry) if (!audit_match_signal(entry))
> audit_signals--;
> #endif
> - mutex_unlock(&audit_filter_mutex);
> +
> + list_del_rcu(&e->list);
> + list_del(&e->rule.list);
> + call_rcu(&e->rcu, audit_free_rule_rcu);
>
> out:
> + mutex_unlock(&audit_filter_mutex);
> +
> if (tree)
> audit_put_tree(tree); /* that's the temporary one */
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V4 (was V6)] audit: save signal match info in case entry passed in is the one deleted
From: Richard Guy Briggs @ 2015-08-05 9:25 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel
In-Reply-To: <2901073.DtnZNkzh6Q@sifl>
On 15/08/04, Paul Moore wrote:
> On Saturday, August 01, 2015 03:44:01 PM Richard Guy Briggs wrote:
> > 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_remove_mark_rule().
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > kernel/auditfilter.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 4cb9b44..afb63b3 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -943,6 +943,7 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) int ret = 0;
> > #ifdef CONFIG_AUDITSYSCALL
> > int dont_count = 0;
> > + int match_signal = !audit_match_signal(entry);
> >
> > /* If either of these, don't count towards total */
> > if (entry->rule.listnr == AUDIT_FILTER_USER ||
> > @@ -972,7 +973,7 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) if (!dont_count)
> > audit_n_rules--;
> >
> > - if (!audit_match_signal(entry))
> > + if (match_signal)
> > audit_signals--;
> > #endif
> > mutex_unlock(&audit_filter_mutex);
>
> Why not simply move this second CONFIG_AUDITSYSCALL above the list_del()
> calls? Am I missing something?
Good point. That did occur to me at one point when I wasn't in front of
the code and promptly forgot once I was. That will neatly remove the
temporary variable.
> Also, while we're fixing up audit_del_rule(), why not also move the
> mutex_unlock() call to after the "out" jump target and then drop the
> mutex_unlock() call in the audit_find_rule() error case? Not your fault, but
> the code seems silly as-is.
Yes, agreed. Another nice catch.
These changes will affect "audit by executable" so I'll re-spin that
patch set to make it easier to apply.
> paul moore
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
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
^ permalink raw reply
* [PATCH V8 3/3] audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-05 9:25 UTC (permalink / raw)
To: linux-audit, linux-kernel
Cc: Richard Guy Briggs, sgrubb, pmoore, eparis, peter
In-Reply-To: <cover.1438764903.git.rgb@redhat.com>
This adds the ability to audit the actions of children of a not-yet-running
process.
This is a split-out of a heavily modified version of a patch originally
submitted by Eric Paris with some ideas from Peter Moody.
Cc: Peter Moody <peter@hda3.com>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/uapi/linux/audit.h | 1 +
kernel/auditfilter.c | 5 +++++
kernel/auditsc.c | 11 +++++++++++
3 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index e2ca600..55a8dec 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -267,6 +267,7 @@
#define AUDIT_OBJ_GID 110
#define AUDIT_FIELD_COMPARE 111
#define AUDIT_EXE 112
+#define AUDIT_EXE_CHILDREN 113
#define AUDIT_ARG0 200
#define AUDIT_ARG1 (AUDIT_ARG0+1)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index c662638..802f0cc 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -406,6 +406,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
if (f->op != Audit_equal)
return -EINVAL;
if (entry->rule.listnr != AUDIT_FILTER_EXIT)
@@ -547,6 +548,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
entry->rule.filterkey = str;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
if (entry->rule.exe || f->val > PATH_MAX)
goto exit_free;
str = audit_unpack_string(&bufp, &remain, f->val);
@@ -643,6 +645,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
audit_pack_string(&bufp, krule->filterkey);
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
data->buflen += data->values[i] =
audit_pack_string(&bufp, audit_mark_path(krule->exe));
break;
@@ -710,6 +713,7 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
return 1;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
/* both paths exist based on above type compare */
if (strcmp(audit_mark_path(a->exe),
audit_mark_path(b->exe)))
@@ -838,6 +842,7 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
new->filterkey = fk;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
err = audit_dupe_exe(new, old);
break;
}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e9bac2b..4f2b515 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -469,6 +469,17 @@ static int audit_filter_rules(struct task_struct *tsk,
case AUDIT_EXE:
result = audit_exe_compare(tsk, rule->exe);
break;
+ case AUDIT_EXE_CHILDREN:
+ {
+ struct task_struct *ptsk;
+ for (ptsk = tsk; ptsk->parent->pid > 0; ptsk = find_task_by_vpid(ptsk->parent->pid)) {
+ if (audit_exe_compare(ptsk, rule->exe)) {
+ ++result;
+ break;
+ }
+ }
+ }
+ break;
case AUDIT_UID:
result = audit_uid_comparator(cred->uid, f->op, f->uid);
break;
--
1.7.1
^ permalink raw reply related
* [PATCH V8 2/3] audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-05 9:25 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1438764903.git.rgb@redhat.com>
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@hda3.com>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
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 | 50 +++++++++++++++++++++++++++++++++++++++++++-
kernel/auditsc.c | 3 ++
7 files changed, 94 insertions(+), 2 deletions(-)
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 401bc1a..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);
}
@@ -1070,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;
}
@@ -1363,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;
--
1.7.1
^ permalink raw reply related
* [PATCH V8 1/3] audit: clean simple fsnotify implementation
From: Richard Guy Briggs @ 2015-08-05 9:25 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1438764903.git.rgb@redhat.com>
This is to be used to audit by executable path 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.
Move the access to the entry for audit_match_signal() to the beginning of
the audit_del_rule() function in case the entry found is the same one passed
in. This will enable it to be used by audit_autoremove_mark_rule(),
kill_rules() and audit_remove_parent_watches().
This is a heavily modified and merged version of two patches originally
submitted by Eric Paris.
Cc: Peter Moody <peter@hda3.com>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/Makefile | 2 +-
kernel/audit.h | 14 +++
kernel/audit_fsnotify.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++
kernel/auditfilter.c | 2 +-
4 files changed, 231 insertions(+), 2 deletions(-)
create mode 100644 kernel/audit_fsnotify.c
diff --git a/kernel/Makefile b/kernel/Makefile
index 60c302c..58b5b52 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
+obj-$(CONFIG_AUDIT_WATCH) += audit_watch.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 d641f9b..46d10dd 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_tree;
struct audit_chunk;
@@ -252,6 +253,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[];
@@ -269,6 +271,13 @@ extern int audit_add_watch(struct audit_krule *krule, struct list_head **list);
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);
+
#else
#define audit_put_watch(w) {}
#define audit_get_watch(w) {}
@@ -278,6 +287,11 @@ extern int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev
#define audit_watch_path(w) ""
#define audit_watch_compare(w, i, d) 0
+#define audit_alloc_mark(k, p, l) (ERR_PTR(-EINVAL))
+#define audit_mark_path(m) ""
+#define audit_remove_mark(m)
+#define audit_remove_mark_rule(k)
+#define audit_mark_compare(m, i, d) 0
#endif /* CONFIG_AUDIT_WATCH */
#ifdef CONFIG_AUDIT_TREE
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
new file mode 100644
index 0000000..654c6c7
--- /dev/null
+++ b/kernel/audit_fsnotify.c
@@ -0,0 +1,215 @@
+/* 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_fsnotify_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_fsnotify_mark_free(audit_mark);
+}
+
+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 == AUDIT_INO_UNSET)
+ return 0;
+ return (mark->ino == ino) && (mark->dev == dev);
+}
+
+static void audit_update_mark(struct audit_fsnotify_mark *audit_mark,
+ struct inode *inode)
+{
+ audit_mark->dev = inode ? inode->i_sb->s_dev : AUDIT_DEV_UNSET;
+ audit_mark->ino = inode ? inode->i_ino : AUDIT_INO_UNSET;
+}
+
+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;
+ 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);
+
+ audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
+ if (unlikely(!audit_mark)) {
+ 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 = pathname;
+ audit_update_mark(audit_mark, dentry->d_inode);
+ audit_mark->rule = krule;
+
+ ret = fsnotify_add_mark(&audit_mark->mark, audit_fsnotify_group, inode, NULL, true);
+ if (ret < 0) {
+ audit_fsnotify_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);
+}
+
+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 1b110fb..401bc1a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -935,7 +935,7 @@ static inline int audit_add_rule(struct audit_entry *entry)
}
/* 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;
--
1.7.1
^ permalink raw reply related
* [PATCH V8 0/3] audit by executable name
From: Richard Guy Briggs @ 2015-08-05 9:25 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs
Please see the accompanying userspace patchset:
https://www.redhat.com/archives/linux-audit/2015-July/thread.html
[[PATCH V2] 0/2] Log on the future execution of a path
The userspace interface is not expected to change appreciably unless something
important has been overlooked. Setting and deleting rules works as expected.
If the path does not exist at rule creation time, it will be re-evaluated every
time there is a change to the parent directory at which point the change in
device and inode will be noted.
Here's a sample run:
Test for addition, trigger and deletion of tree executable rule:
# auditctl -a always,exit -S all -F dir=/tmp -F exe=/usr/bin/touch -F key=exetest_tree
----
time->Sat Jul 11 10:41:50 2015
type=CONFIG_CHANGE msg=audit(1436629310.720:44711): auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="add_rule" key="exetest_tree" list=4 res=1
----
# /usr/bin/touch /tmp/test
----
time->Sat Jul 11 10:41:50 2015
type=PROCTITLE msg=audit(1436629310.757:44712): proctitle=2F7573722F62696E2F746F756368002F746D702F74657374
type=PATH msg=audit(1436629310.757:44712): item=1 name="/tmp/test" inode=166932 dev=00:24 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE
type=PATH msg=audit(1436629310.757:44712): item=0 name="/tmp/" inode=11525 dev=00:24 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT
type=CWD msg=audit(1436629310.757:44712): cwd="/root"
type=SYSCALL msg=audit(1436629310.757:44712): arch=c000003e syscall=2 success=yes exit=3 a0=7ffdee2f9e27 a1=941 a2=1b6 a3=691 items=2 ppid=17655 pid=17762 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=ttyS0 ses=1 comm="touch" exe="/usr/bin/touch" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="exetest_tree"
----
# auditctl -d always,exit -S all -F dir=/tmp -F exe=/usr/bin/touch -F key=exetest_tree
----
time->Sat Jul 11 10:41:50 2015
type=CONFIG_CHANGE msg=audit(1436629310.839:44713): auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 op="remove_rule" key="exetest_tree" list=4 res=1
----
Revision history:
v8: Re-spin due to mods to:
"audit: save signal match info in case entry passed in is the one deleted"
v7: Add Audit Feature Bitmap macro AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH to
AUDIT_FEATURE_BITMAP_ALL.
Split out new patch to use macros for unset inode and device values.
Remove BUG() usage in stubs.
Rename audit_mark_free() to audit_fsnotify_mark_free().
Remove unused audit_get_mark() and audit_put_mark() functions.
Rework ino and dev comparisons and assignments, using macros and existing
funcs and eliminating temp variables.
Move audit_update_mark() above its first usage.
Move contents of kernel/audit_exe.c to kernel/audit_watch.c.
Merge patch 3 with 1, merge patch 4 with 1 and 3 and rewrite the patch descriptions.
Split out patch to audit by executable children.
v6: Explicitly declare prototypes as external.
Rename audit_dup_exe() to audit_dupe_exe() consistent with rule, watch, lsm_field.
Rebased on v4.1.
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.
Add audit_remove_mark_rule() to provide similar interface as
audit_remove_{watch,tree}_rule().
Simplify stubs to defines.
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().
Return -ENOMEM rather than null in case of memory allocation failure for
audit_mark in audit_alloc_mark().
Rename audit_free_mark() to audit_mark_free() to avoid association with
{i,d,fa}notify_free_mark() and audit_watch_free_mark().
Clean up exe with similar interface as watch and tree.
Clean up audit exe mark just before audit_free_rule() rather than in it to
avoid mutex in software interrupt context.
Fixed bug in audit_dupe_exe() that returned error rather than valid pointer.
v5: Revert patch "Let audit_free_rule() take care of calling
audit_remove_mark()." since it caused a group mark deadlock.
https://www.redhat.com/archives/linux-audit/2014-October/msg00024.html
v4: Re-order and squash down fixups
Fix audit_dup_exe() to copy pathname string before calling audit_alloc_mark().
https://www.redhat.com/archives/linux-audit/2014-August/msg00065.html
v3: Rationalize and rename some function names and clean up get/put and free code.
Rename several "watch" references to "mark".
Rename audit_remove_rule() to audit_remove_mark_rule().
Let audit_free_rule() take care of calling audit_remove_mark().
Put audit_alloc_mark() arguments in same order as watch, tree and inode.
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_remove_mark_rule().
https://www.redhat.com/archives/linux-audit/2014-July/msg00000.html
v2: Misguided attempt to add in audit_exe similar to watches
https://www.redhat.com/archives/linux-audit/2014-June/msg00066.html
v1.5: eparis' switch to fsnotify
https://www.redhat.com/archives/linux-audit/2014-May/msg00046.html
https://www.redhat.com/archives/linux-audit/2014-May/msg00066.html
v1: Change to path interface instead of inode
https://www.redhat.com/archives/linux-audit/2014-May/msg00017.html
v0: Peter Moodie's original patches
https://www.redhat.com/archives/linux-audit/2012-August/msg00033.html
Future step:
Get full-path notify working.
Richard Guy Briggs (3):
audit: clean simple fsnotify implementation
audit: implement audit by executable
audit: add audit by children of executable path
include/linux/audit.h | 1 +
include/uapi/linux/audit.h | 6 +-
kernel/Makefile | 2 +-
kernel/audit.h | 18 ++++
kernel/audit_fsnotify.c | 215 ++++++++++++++++++++++++++++++++++++++++++++
kernel/audit_tree.c | 2 +
kernel/audit_watch.c | 31 +++++++
kernel/auditfilter.c | 57 +++++++++++-
kernel/auditsc.c | 14 +++
9 files changed, 342 insertions(+), 4 deletions(-)
create mode 100644 kernel/audit_fsnotify.c
^ permalink raw reply
* [PATCH V5] audit: save signal match info in case entry passed in is the one deleted
From: Richard Guy Briggs @ 2015-08-05 9:23 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs, sgrubb, pmoore, eparis
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_remove_mark_rule().
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
Revision history:
v4 -> v5:
Move mutex_unlock after out label.
Move list_del group after test for signal to remove temp variable.
---
This patch was split out from the audit by executable path patch set due to the
potential to use it elsewhere.
In particular, some questions came up while assessing the potential for code
reuse:
Why does audit_remove_parent_watches() not call audit_del_rule() for
each entry found?
Is audit_signals not properly decremented?
Is audit_n_rules not properly decremented?
Why does kill_rules() not call audit_del_rule() for each entry found?
Is audit_signals not properly decremented?
Is audit_n_rules not properly decremented?
kernel/auditfilter.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 4cb9b44..1b110fb 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -953,7 +953,6 @@ static inline int audit_del_rule(struct audit_entry *entry)
mutex_lock(&audit_filter_mutex);
e = audit_find_rule(entry, &list);
if (!e) {
- mutex_unlock(&audit_filter_mutex);
ret = -ENOENT;
goto out;
}
@@ -964,9 +963,8 @@ static inline int audit_del_rule(struct audit_entry *entry)
if (e->rule.tree)
audit_remove_tree_rule(&e->rule);
- list_del_rcu(&e->list);
- list_del(&e->rule.list);
- call_rcu(&e->rcu, audit_free_rule_rcu);
+ if (e->rule.exe)
+ audit_remove_mark_rule(&e->rule);
#ifdef CONFIG_AUDITSYSCALL
if (!dont_count)
@@ -975,9 +973,14 @@ static inline int audit_del_rule(struct audit_entry *entry)
if (!audit_match_signal(entry))
audit_signals--;
#endif
- mutex_unlock(&audit_filter_mutex);
+
+ list_del_rcu(&e->list);
+ list_del(&e->rule.list);
+ call_rcu(&e->rcu, audit_free_rule_rcu);
out:
+ mutex_unlock(&audit_filter_mutex);
+
if (tree)
audit_put_tree(tree); /* that's the temporary one */
--
1.7.1
^ permalink raw reply related
* Re: [PATCH V4 (was V6)] audit: macros to replace unset inode and device values
From: Richard Guy Briggs @ 2015-08-05 6:32 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <7058833.MqXKtuIFx4@sifl>
On 15/08/04, Paul Moore wrote:
> On Saturday, August 01, 2015 03:42:22 PM Richard Guy Briggs wrote:
> > This is a patch to clean up a number of places were casted magic numbers are
> > used to represent unset inode and device numbers inpreparation for the
> > audit by executable path patch set.
> >
> > Richard Guy Briggs (1):
> > audit: use macros for unset inode and device values
> >
> > include/uapi/linux/audit.h | 2 ++
> > kernel/audit.c | 2 +-
> > kernel/audit_watch.c | 8 ++++----
> > kernel/auditsc.c | 6 +++---
> > 4 files changed, 10 insertions(+), 8 deletions(-)
>
> Also, when there is only one patch in the patchset, no need to send a cover
> email, e.g. patch 0/1, just put the text in the patch description itself.
Or drop it into a comment after the "---" demarcation... Which would be
better for questions that turn out to need no further followup.
> paul moore
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
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
^ permalink raw reply
* Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values
From: Richard Guy Briggs @ 2015-08-05 6:30 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <2436529.cXDcHDUN5i@sifl>
On 15/08/04, Paul Moore wrote:
> On Saturday, August 01, 2015 03:42:23 PM Richard Guy Briggs wrote:
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > include/uapi/linux/audit.h | 2 ++
> > kernel/audit.c | 2 +-
> > kernel/audit_watch.c | 8 ++++----
> > kernel/auditsc.c | 6 +++---
> > 4 files changed, 10 insertions(+), 8 deletions(-)
>
> Yipee, less magic numbers!
>
> However, one question for you ... are we ever going to see a device or inode
> set to -1 in the userspace facing API? In other words, should the new
> #defines go in the uapi headers or simply in kernel/audit.h? Unless it is
> part of the API, let's leave it out of uapi as we have to be very careful
> about that stuff and I'd prefer to keep it minimal.
This is a good point. I did briefly thing about this at one point.
Perhaps Steve can answer this. It would be trivial to move it back to
uapi if needed. Would you be ok with it in include/linux/audit.h for
now?
> Also, if we can put the #defines in kernel/audit.h we can use the proper type
> for AUDIT_DEV_UNSET which would make me happy.
>
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index d3475e1..971df22 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -440,6 +440,8 @@ struct audit_tty_status {
> > };
> >
> > #define AUDIT_UID_UNSET (unsigned int)-1
> > +#define AUDIT_INO_UNSET (unsigned long)-1
> > +#define AUDIT_DEV_UNSET (unsigned)-1
> >
> > /* audit_rule_data supports filter rules with both integer and string
> > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 1c13e42..d546003 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1761,7 +1761,7 @@ void audit_log_name(struct audit_context *context,
> > struct audit_names *n, } else
> > audit_log_format(ab, " name=(null)");
> >
> > - if (n->ino != (unsigned long)-1)
> > + if (n->ino != AUDIT_INO_UNSET)
> > audit_log_format(ab, " inode=%lu"
> > " dev=%02x:%02x mode=%#ho"
> > " ouid=%u ogid=%u rdev=%02x:%02x",
> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> > index 8f123d7..c668bfc 100644
> > --- a/kernel/audit_watch.c
> > +++ b/kernel/audit_watch.c
> > @@ -138,7 +138,7 @@ char *audit_watch_path(struct audit_watch *watch)
> >
> > int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t
> > dev) {
> > - return (watch->ino != (unsigned long)-1) &&
> > + return (watch->ino != AUDIT_INO_UNSET) &&
> > (watch->ino == ino) &&
> > (watch->dev == dev);
> > }
> > @@ -179,8 +179,8 @@ static struct audit_watch *audit_init_watch(char *path)
> > INIT_LIST_HEAD(&watch->rules);
> > atomic_set(&watch->count, 1);
> > watch->path = path;
> > - watch->dev = (dev_t)-1;
> > - watch->ino = (unsigned long)-1;
> > + watch->dev = AUDIT_DEV_UNSET;
> > + watch->ino = AUDIT_INO_UNSET;
> >
> > return watch;
> > }
> > @@ -493,7 +493,7 @@ static int audit_watch_handle_event(struct
> > fsnotify_group *group, if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
> > audit_update_watch(parent, dname, inode->i_sb->s_dev, inode->i_ino, 0);
> > else if (mask & (FS_DELETE|FS_MOVED_FROM))
> > - audit_update_watch(parent, dname, (dev_t)-1, (unsigned long)-1, 1);
> > + audit_update_watch(parent, dname, AUDIT_DEV_UNSET, AUDIT_INO_UNSET, 1);
> > else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
> > audit_remove_parent_watches(parent);
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 9fb9d1c..701ea5c 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -180,7 +180,7 @@ static int audit_match_filetype(struct audit_context
> > *ctx, int val) return 0;
> >
> > list_for_each_entry(n, &ctx->names_list, list) {
> > - if ((n->ino != -1) &&
> > + if ((n->ino != AUDIT_INO_UNSET) &&
> > ((n->mode & S_IFMT) == mode))
> > return 1;
> > }
> > @@ -1683,7 +1683,7 @@ static struct audit_names *audit_alloc_name(struct
> > audit_context *context, aname->should_free = true;
> > }
> >
> > - aname->ino = (unsigned long)-1;
> > + aname->ino = AUDIT_INO_UNSET;
> > aname->type = type;
> > list_add_tail(&aname->list, &context->names_list);
> >
> > @@ -1925,7 +1925,7 @@ void __audit_inode_child(const struct inode *parent,
> > if (inode)
> > audit_copy_inode(found_child, dentry, inode);
> > else
> > - found_child->ino = (unsigned long)-1;
> > + found_child->ino = AUDIT_INO_UNSET;
> > }
> > EXPORT_SYMBOL_GPL(__audit_inode_child);
>
> --
> paul moore
> security @ redhat
>
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
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
^ permalink raw reply
* Re: [PATCH V4 (was V6)] audit: save signal match info in case entry passed in is the one deleted
From: Paul Moore @ 2015-08-04 23:04 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <79ae63edcc644461001cd37b8ff1f9b1458f45f7.1438446636.git.rgb@redhat.com>
On Saturday, August 01, 2015 03:44:01 PM Richard Guy Briggs wrote:
> 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_remove_mark_rule().
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/auditfilter.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 4cb9b44..afb63b3 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -943,6 +943,7 @@ static inline int audit_del_rule(struct audit_entry
> *entry) int ret = 0;
> #ifdef CONFIG_AUDITSYSCALL
> int dont_count = 0;
> + int match_signal = !audit_match_signal(entry);
>
> /* If either of these, don't count towards total */
> if (entry->rule.listnr == AUDIT_FILTER_USER ||
> @@ -972,7 +973,7 @@ static inline int audit_del_rule(struct audit_entry
> *entry) if (!dont_count)
> audit_n_rules--;
>
> - if (!audit_match_signal(entry))
> + if (match_signal)
> audit_signals--;
> #endif
> mutex_unlock(&audit_filter_mutex);
Why not simply move this second CONFIG_AUDITSYSCALL above the list_del()
calls? Am I missing something?
Also, while we're fixing up audit_del_rule(), why not also move the
mutex_unlock() call to after the "out" jump target and then drop the
mutex_unlock() call in the audit_find_rule() error case? Not your fault, but
the code seems silly as-is.
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V4 (was V6)] audit: macros to replace unset inode and device values
From: Paul Moore @ 2015-08-04 22:37 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <cover.1438446565.git.rgb@redhat.com>
On Saturday, August 01, 2015 03:42:22 PM Richard Guy Briggs wrote:
> This is a patch to clean up a number of places were casted magic numbers are
> used to represent unset inode and device numbers inpreparation for the
> audit by executable path patch set.
>
> Richard Guy Briggs (1):
> audit: use macros for unset inode and device values
>
> include/uapi/linux/audit.h | 2 ++
> kernel/audit.c | 2 +-
> kernel/audit_watch.c | 8 ++++----
> kernel/auditsc.c | 6 +++---
> 4 files changed, 10 insertions(+), 8 deletions(-)
Also, when there is only one patch in the patchset, no need to send a cover
email, e.g. patch 0/1, just put the text in the patch description itself.
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V4 (was V6)] audit: use macros for unset inode and device values
From: Paul Moore @ 2015-08-04 22:34 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel
In-Reply-To: <ef5a7624121e3bf41cd016ff18bec77d12d947d9.1438446565.git.rgb@redhat.com>
On Saturday, August 01, 2015 03:42:23 PM Richard Guy Briggs wrote:
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/uapi/linux/audit.h | 2 ++
> kernel/audit.c | 2 +-
> kernel/audit_watch.c | 8 ++++----
> kernel/auditsc.c | 6 +++---
> 4 files changed, 10 insertions(+), 8 deletions(-)
Yipee, less magic numbers!
However, one question for you ... are we ever going to see a device or inode
set to -1 in the userspace facing API? In other words, should the new
#defines go in the uapi headers or simply in kernel/audit.h? Unless it is
part of the API, let's leave it out of uapi as we have to be very careful
about that stuff and I'd prefer to keep it minimal.
Also, if we can put the #defines in kernel/audit.h we can use the proper type
for AUDIT_DEV_UNSET which would make me happy.
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index d3475e1..971df22 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -440,6 +440,8 @@ struct audit_tty_status {
> };
>
> #define AUDIT_UID_UNSET (unsigned int)-1
> +#define AUDIT_INO_UNSET (unsigned long)-1
> +#define AUDIT_DEV_UNSET (unsigned)-1
>
> /* audit_rule_data supports filter rules with both integer and string
> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 1c13e42..d546003 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1761,7 +1761,7 @@ void audit_log_name(struct audit_context *context,
> struct audit_names *n, } else
> audit_log_format(ab, " name=(null)");
>
> - if (n->ino != (unsigned long)-1)
> + if (n->ino != AUDIT_INO_UNSET)
> audit_log_format(ab, " inode=%lu"
> " dev=%02x:%02x mode=%#ho"
> " ouid=%u ogid=%u rdev=%02x:%02x",
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 8f123d7..c668bfc 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -138,7 +138,7 @@ char *audit_watch_path(struct audit_watch *watch)
>
> int audit_watch_compare(struct audit_watch *watch, unsigned long ino, dev_t
> dev) {
> - return (watch->ino != (unsigned long)-1) &&
> + return (watch->ino != AUDIT_INO_UNSET) &&
> (watch->ino == ino) &&
> (watch->dev == dev);
> }
> @@ -179,8 +179,8 @@ static struct audit_watch *audit_init_watch(char *path)
> INIT_LIST_HEAD(&watch->rules);
> atomic_set(&watch->count, 1);
> watch->path = path;
> - watch->dev = (dev_t)-1;
> - watch->ino = (unsigned long)-1;
> + watch->dev = AUDIT_DEV_UNSET;
> + watch->ino = AUDIT_INO_UNSET;
>
> return watch;
> }
> @@ -493,7 +493,7 @@ static int audit_watch_handle_event(struct
> fsnotify_group *group, if (mask & (FS_CREATE|FS_MOVED_TO) && inode)
> audit_update_watch(parent, dname, inode->i_sb->s_dev, inode->i_ino, 0);
> else if (mask & (FS_DELETE|FS_MOVED_FROM))
> - audit_update_watch(parent, dname, (dev_t)-1, (unsigned long)-1, 1);
> + audit_update_watch(parent, dname, AUDIT_DEV_UNSET, AUDIT_INO_UNSET, 1);
> else if (mask & (FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF))
> audit_remove_parent_watches(parent);
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 9fb9d1c..701ea5c 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -180,7 +180,7 @@ static int audit_match_filetype(struct audit_context
> *ctx, int val) return 0;
>
> list_for_each_entry(n, &ctx->names_list, list) {
> - if ((n->ino != -1) &&
> + if ((n->ino != AUDIT_INO_UNSET) &&
> ((n->mode & S_IFMT) == mode))
> return 1;
> }
> @@ -1683,7 +1683,7 @@ static struct audit_names *audit_alloc_name(struct
> audit_context *context, aname->should_free = true;
> }
>
> - aname->ino = (unsigned long)-1;
> + aname->ino = AUDIT_INO_UNSET;
> aname->type = type;
> list_add_tail(&aname->list, &context->names_list);
>
> @@ -1925,7 +1925,7 @@ void __audit_inode_child(const struct inode *parent,
> if (inode)
> audit_copy_inode(found_child, dentry, inode);
> else
> - found_child->ino = (unsigned long)-1;
> + found_child->ino = AUDIT_INO_UNSET;
> }
> EXPORT_SYMBOL_GPL(__audit_inode_child);
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V4 (was V6) 2/2] audit: eliminate unnecessary extra layer of watch parent references
From: Paul Moore @ 2015-08-04 22:28 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <4d84814704ad61bc547ac74395882a6092f5be09.1438446498.git.rgb@redhat.com>
On Saturday, August 01, 2015 03:41:13 PM Richard Guy Briggs wrote:
> The audit watch parent count was imbalanced, adding an unnecessary layer of
> watch parent references. Decrement the additional parent reference when a
> watch is reused, already having a reference to the parent.
>
> audit_find_parent() gets a reference to the parent, if the parent is
> already known. This additional parental reference is not needed if the
> watch is subsequently found by audit_add_to_parent(), and consumed if
> the watch does not already exist, so we need to put the parent if the
> watch is found, and do nothing if this new watch is added to the parent.
>
> If the parent wasn't already known, it is created with a refcount of 1
> and added to the audit_watch_group, then incremented by one to be
> subsequently consumed by the newly created watch in
> audit_add_to_parent().
>
> The rule points to the watch, not to the parent, so the rule's refcount
> gets bumped, not the parent's.
>
> See LKML, 2015-07-16
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit_watch.c | 6 ++----
> 1 files changed, 2 insertions(+), 4 deletions(-)
Merged.
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index f33f54c..8f123d7 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -391,11 +391,12 @@ static void audit_add_to_parent(struct audit_krule
> *krule,
>
> audit_get_watch(w);
> krule->watch = watch = w;
> +
> + audit_put_parent(parent);
> break;
> }
>
> if (!watch_found) {
> - audit_get_parent(parent);
> watch->parent = parent;
>
> audit_get_watch(watch);
> @@ -436,9 +437,6 @@ int audit_add_watch(struct audit_krule *krule, struct
> list_head **list)
>
> audit_add_to_parent(krule, parent);
>
> - /* match get in audit_find_parent or audit_init_parent */
> - audit_put_parent(parent);
> -
> h = audit_hash_ino((u32)watch->ino);
> *list = &audit_inode_hash[h];
> error:
--
paul moore
security @ redhat
^ permalink raw reply
* Re: [PATCH V4 (was V6) 1/2] audit: eliminate unnecessary extra layer of watch references
From: Paul Moore @ 2015-08-04 22:27 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <728d3132b9294ce23be7e95b22d82a9291d9c5fc.1438446498.git.rgb@redhat.com>
On Saturday, August 01, 2015 03:41:12 PM Richard Guy Briggs wrote:
> The audit watch count was imbalanced, adding an unnecessary layer of watch
> references. Only add the second reference when it is added to a parent.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit_watch.c | 5 ++---
> kernel/auditfilter.c | 16 +++-------------
> 2 files changed, 5 insertions(+), 16 deletions(-)
Merged. I'll push it out as soon as I finish up the reviews tonight.
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 6e30024..f33f54c 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -203,7 +203,6 @@ int audit_to_watch(struct audit_krule *krule, char
> *path, int len, u32 op) if (IS_ERR(watch))
> return PTR_ERR(watch);
>
> - audit_get_watch(watch);
> krule->watch = watch;
>
> return 0;
> @@ -387,8 +386,7 @@ static void audit_add_to_parent(struct audit_krule
> *krule,
>
> watch_found = 1;
>
> - /* put krule's and initial refs to temporary watch */
> - audit_put_watch(watch);
> + /* put krule's ref to temporary watch */
> audit_put_watch(watch);
>
> audit_get_watch(w);
> @@ -400,6 +398,7 @@ static void audit_add_to_parent(struct audit_krule
> *krule, audit_get_parent(parent);
> watch->parent = parent;
>
> + audit_get_watch(watch);
> list_add(&watch->wlist, &parent->watches);
> }
> list_add(&krule->rlist, &watch->rules);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 72e1660..4cb9b44 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -549,8 +549,6 @@ exit_nofree:
> return entry;
>
> exit_free:
> - if (entry->rule.watch)
> - audit_put_watch(entry->rule.watch); /* matches initial get */
> if (entry->rule.tree)
> audit_put_tree(entry->rule.tree); /* that's the temporary one */
> audit_free_rule(entry);
> @@ -881,7 +879,7 @@ static inline int audit_add_rule(struct audit_entry
> *entry) /* normally audit_add_tree_rule() will free it on failure */
> if (tree)
> audit_put_tree(tree);
> - goto error;
> + return err;
> }
>
> if (watch) {
> @@ -895,14 +893,14 @@ static inline int audit_add_rule(struct audit_entry
> *entry) */
> if (tree)
> audit_put_tree(tree);
> - goto error;
> + return err;
> }
> }
> if (tree) {
> err = audit_add_tree_rule(&entry->rule);
> if (err) {
> mutex_unlock(&audit_filter_mutex);
> - goto error;
> + return err;
> }
> }
>
> @@ -933,11 +931,6 @@ static inline int audit_add_rule(struct audit_entry
> *entry) #endif
> mutex_unlock(&audit_filter_mutex);
>
> - return 0;
> -
> -error:
> - if (watch)
> - audit_put_watch(watch); /* tmp watch, matches initial get */
> return err;
> }
>
> @@ -945,7 +938,6 @@ error:
> static inline int audit_del_rule(struct audit_entry *entry)
> {
> struct audit_entry *e;
> - struct audit_watch *watch = entry->rule.watch;
> struct audit_tree *tree = entry->rule.tree;
> struct list_head *list;
> int ret = 0;
> @@ -986,8 +978,6 @@ static inline int audit_del_rule(struct audit_entry
> *entry) mutex_unlock(&audit_filter_mutex);
>
> out:
> - if (watch)
> - audit_put_watch(watch); /* match initial get */
> if (tree)
> audit_put_tree(tree); /* that's the temporary one */
--
paul moore
security @ redhat
^ permalink raw reply
* Re: Watching over non-existent folder to maintain a generic audit.rules file
From: Burn Alting @ 2015-08-04 22:26 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
In-Reply-To: <17553516.FC0zugFrpV@x2>
On Tue, 2015-08-04 at 15:55 -0400, Steve Grubb wrote:
> On Tuesday, August 04, 2015 03:57:17 PM Florian Crouzat wrote:
> > On 07/29/2015 08:24 AM, Florian Crouzat wrote:
> > > On 07/29/2015 12:39 AM, Burn Alting wrote:
> > >> On Tue, 2015-07-28 at 15:23 -0400, Steve Grubb wrote:
> > >>> On Tuesday, July 28, 2015 05:26:18 PM Florian Crouzat wrote:
> > >>>> Unfortunately, I do not only watch over system-related files and
> > >>>> folders
> > >>>> but also applicative ones (eg custom path where some private keys are
> > >>>> stored, etc) ..
> > >>>> My problem is that these folders do not exists on all hosts thus making
> > >>>> it impossible to write a generic audit.rules files.
> > >>>
> > >>> What kernel are you using? And user space package?
> > >>>
> > >>>> As I said, I have thousands of hosts and I can't imagine deploying
> > >>>> different files on every hosts depending on the profile of the host.
> > >>>> I know puppet could help me for this kind of stuff but I don't have it
> > >>>> yet and even though, it would be difficult to configure.
> > >>>
> > >>> As of the 2.3 user space release, there is a utility, augenrules which
> > >>> takes files in /etc/audit/rules.d/ and compiles them into an
> > >>> audit.rules file. So, it would be possible for you to package up some
> > >>> rules for bind and install them when you install bind and have your
> > >>> package install a
> > >>> /etc/audit/rules.d/bind.rules file. You can have a base config, and then
> > >>> one for each kind of daemon or role that the machine serves.
> > >>>
> > >>>> How do you guys usually workaround this issue ? I'm pretty sure I'm not
> > >>>> the first one wanting to deploy a generic hardening across many hosts
> > >>>> (but maybe I'm the only one using auditd to watch over something else
> > >>>> than pure system-related stuff?
> > >>>
> > >>> Others can chime in here.
> > >>
> > >> As Steve suggests, you should base you efforts around augenrules ...
> > >> that why it was written. If you have a project that delivers a new
> > >> capability, then part of the security element of it's transition to
> > >> operations would be to have the project identify the sensitive files and
> > >> have the system administrators deploy project specific .rules files
> > >> in /etc/audit/rules.d to monitor them.
> > >>
> > >> If you have pre 2.3 audit user space deployments, then it is not
> > >> difficult to deploy your own augenrules setup, but don't deploy it in
> > >> the 'production' locations ... it's a bitch to remove when a 2.3 audit
> > >> user space upgrade comes ... lots of rpm clashes.
> > >>
> > >> A word to the wise on file watches. If you have a capability who's
> > >> 'service/process' continually accesses configuration files you will
> > >> typically mask this out by having the service start under the init.d
> > >> regime at boot time and configure auditd to not monitor the 'unset'
> > >> auid. The problem comes when a sustainment staff member, elevates
> > >> privilege, and restarts the service. At this, all file accesses by the
> > >> service/process will be attributed to the sustainment staff members uid,
> > >> not the 'unset' user. This appears to have been addressed by systemd, so
> > >> if your Linux release supports systemd, and you configure your
> > >> application to use it appropriately, it will not have the problem.
> > >> There are workarounds for the init.d based service regime, but that will
> > >> have to be a separate post if ane are interested.
> > >
> > > Hey,
> > >
> > > Thanks for the answers.
> > >
> > > I have both up-to-date EL6 and EL7 hosts with latests kernel available
> > > in base channel so I think it means I have >=2.3 auditd package.
> > > I'll definitely have a look into augenrules and how to use it.
> > > I'll probably come back at you with mores questions after that.
> > >
> > > Florian
> >
> > Okay so if I get this straight, I should probably have a default
> > *generic* audit file for OS/kernel-related stuff and then deploy on
> > specific hosts a bunch of specific rule files that would be compiled
> > every now and then by an augenrules cronjob ?
>
> You do not need a cron job. When audit starts, there is a need to load audit
> rules into the kernel. The audit software was changed to use augenrules to do
> that. It checks to see if they have changes since last compilation and if so
> rebuilds them. Otherwise it just loads them through auditctl.
>
>
> > What would be the best practice to be alerted in case the compilation
> > fails (augenrules) ?
>
> I think it returns a failed code of some kind.
Given a standard working system the exit code is that from the auditctl
utility invoked as per
auditctl -R ${DestinationFile}
where ${DestinationFile} is the newly created rules file.
Typically, in an enterprise environment, you should note if your rules
file fails to load via an absence of standard audit sent to your
centralised audit repository. Or you could take the direct approach as
part of your system deployment test the execution of /sbin/augenrules
--load.
>
> -Steve
>
> > That sounds ... reasonnable. But still, I'll have to maintain these
> > specific files and find a way to deliver them to these specific hosts.
Yes, that is always going to be the case in an enterprise environment.
There are a number of ways of doing this, all centered around either a
directly centralised configuration management system (puppet and
friends) or a manual indirect one.
And to answer your early question. Yes, on all systems deploy a
standard /etc/audit/rules.d/base.rules. Then, for those that require
customisation, add say the file /etc/audit/rules.d/local.rules which has
the localised additional rules. The tough case is where your
localisation needs to change the base.rules files in things other than
the -D, -b, -f and -e directives). Solution ... have a very robust
centralised configuration management system.
Burn
> >
> > Florian
^ permalink raw reply
* Re: Watching over non-existent folder to maintain a generic audit.rules file
From: Steve Grubb @ 2015-08-04 19:55 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <55C0C4BD.9070408@floriancrouzat.net>
On Tuesday, August 04, 2015 03:57:17 PM Florian Crouzat wrote:
> On 07/29/2015 08:24 AM, Florian Crouzat wrote:
> > On 07/29/2015 12:39 AM, Burn Alting wrote:
> >> On Tue, 2015-07-28 at 15:23 -0400, Steve Grubb wrote:
> >>> On Tuesday, July 28, 2015 05:26:18 PM Florian Crouzat wrote:
> >>>> Unfortunately, I do not only watch over system-related files and
> >>>> folders
> >>>> but also applicative ones (eg custom path where some private keys are
> >>>> stored, etc) ..
> >>>> My problem is that these folders do not exists on all hosts thus making
> >>>> it impossible to write a generic audit.rules files.
> >>>
> >>> What kernel are you using? And user space package?
> >>>
> >>>> As I said, I have thousands of hosts and I can't imagine deploying
> >>>> different files on every hosts depending on the profile of the host.
> >>>> I know puppet could help me for this kind of stuff but I don't have it
> >>>> yet and even though, it would be difficult to configure.
> >>>
> >>> As of the 2.3 user space release, there is a utility, augenrules which
> >>> takes files in /etc/audit/rules.d/ and compiles them into an
> >>> audit.rules file. So, it would be possible for you to package up some
> >>> rules for bind and install them when you install bind and have your
> >>> package install a
> >>> /etc/audit/rules.d/bind.rules file. You can have a base config, and then
> >>> one for each kind of daemon or role that the machine serves.
> >>>
> >>>> How do you guys usually workaround this issue ? I'm pretty sure I'm not
> >>>> the first one wanting to deploy a generic hardening across many hosts
> >>>> (but maybe I'm the only one using auditd to watch over something else
> >>>> than pure system-related stuff?
> >>>
> >>> Others can chime in here.
> >>
> >> As Steve suggests, you should base you efforts around augenrules ...
> >> that why it was written. If you have a project that delivers a new
> >> capability, then part of the security element of it's transition to
> >> operations would be to have the project identify the sensitive files and
> >> have the system administrators deploy project specific .rules files
> >> in /etc/audit/rules.d to monitor them.
> >>
> >> If you have pre 2.3 audit user space deployments, then it is not
> >> difficult to deploy your own augenrules setup, but don't deploy it in
> >> the 'production' locations ... it's a bitch to remove when a 2.3 audit
> >> user space upgrade comes ... lots of rpm clashes.
> >>
> >> A word to the wise on file watches. If you have a capability who's
> >> 'service/process' continually accesses configuration files you will
> >> typically mask this out by having the service start under the init.d
> >> regime at boot time and configure auditd to not monitor the 'unset'
> >> auid. The problem comes when a sustainment staff member, elevates
> >> privilege, and restarts the service. At this, all file accesses by the
> >> service/process will be attributed to the sustainment staff members uid,
> >> not the 'unset' user. This appears to have been addressed by systemd, so
> >> if your Linux release supports systemd, and you configure your
> >> application to use it appropriately, it will not have the problem.
> >> There are workarounds for the init.d based service regime, but that will
> >> have to be a separate post if ane are interested.
> >
> > Hey,
> >
> > Thanks for the answers.
> >
> > I have both up-to-date EL6 and EL7 hosts with latests kernel available
> > in base channel so I think it means I have >=2.3 auditd package.
> > I'll definitely have a look into augenrules and how to use it.
> > I'll probably come back at you with mores questions after that.
> >
> > Florian
>
> Okay so if I get this straight, I should probably have a default
> *generic* audit file for OS/kernel-related stuff and then deploy on
> specific hosts a bunch of specific rule files that would be compiled
> every now and then by an augenrules cronjob ?
You do not need a cron job. When audit starts, there is a need to load audit
rules into the kernel. The audit software was changed to use augenrules to do
that. It checks to see if they have changes since last compilation and if so
rebuilds them. Otherwise it just loads them through auditctl.
> What would be the best practice to be alerted in case the compilation
> fails (augenrules) ?
I think it returns a failed code of some kind.
-Steve
> That sounds ... reasonnable. But still, I'll have to maintain these
> specific files and find a way to deliver them to these specific hosts.
>
> Florian
>
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply
* Re: Watching over non-existent folder to maintain a generic audit.rules file
From: Florian Crouzat @ 2015-08-04 13:57 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <55B87199.9070803@floriancrouzat.net>
On 07/29/2015 08:24 AM, Florian Crouzat wrote:
> On 07/29/2015 12:39 AM, Burn Alting wrote:
>> On Tue, 2015-07-28 at 15:23 -0400, Steve Grubb wrote:
>>> On Tuesday, July 28, 2015 05:26:18 PM Florian Crouzat wrote:
>>>> Unfortunately, I do not only watch over system-related files and folders
>>>> but also applicative ones (eg custom path where some private keys are
>>>> stored, etc) ..
>>>> My problem is that these folders do not exists on all hosts thus making
>>>> it impossible to write a generic audit.rules files.
>>>
>>> What kernel are you using? And user space package?
>>>
>>>
>>>> As I said, I have thousands of hosts and I can't imagine deploying
>>>> different files on every hosts depending on the profile of the host.
>>>> I know puppet could help me for this kind of stuff but I don't have it
>>>> yet and even though, it would be difficult to configure.
>>>
>>> As of the 2.3 user space release, there is a utility, augenrules which takes
>>> files in /etc/audit/rules.d/ and compiles them into an audit.rules file. So, it
>>> would be possible for you to package up some rules for bind and install them
>>> when you install bind and have your package install a
>>> /etc/audit/rules.d/bind.rules file. You can have a base config, and then one for
>>> each kind of daemon or role that the machine serves.
>>>
>>>
>>>> How do you guys usually workaround this issue ? I'm pretty sure I'm not
>>>> the first one wanting to deploy a generic hardening across many hosts
>>>> (but maybe I'm the only one using auditd to watch over something else
>>>> than pure system-related stuff?
>>>
>>> Others can chime in here.
>>
>> As Steve suggests, you should base you efforts around augenrules ...
>> that why it was written. If you have a project that delivers a new
>> capability, then part of the security element of it's transition to
>> operations would be to have the project identify the sensitive files and
>> have the system administrators deploy project specific .rules files
>> in /etc/audit/rules.d to monitor them.
>>
>> If you have pre 2.3 audit user space deployments, then it is not
>> difficult to deploy your own augenrules setup, but don't deploy it in
>> the 'production' locations ... it's a bitch to remove when a 2.3 audit
>> user space upgrade comes ... lots of rpm clashes.
>>
>> A word to the wise on file watches. If you have a capability who's
>> 'service/process' continually accesses configuration files you will
>> typically mask this out by having the service start under the init.d
>> regime at boot time and configure auditd to not monitor the 'unset'
>> auid. The problem comes when a sustainment staff member, elevates
>> privilege, and restarts the service. At this, all file accesses by the
>> service/process will be attributed to the sustainment staff members uid,
>> not the 'unset' user. This appears to have been addressed by systemd, so
>> if your Linux release supports systemd, and you configure your
>> application to use it appropriately, it will not have the problem.
>> There are workarounds for the init.d based service regime, but that will
>> have to be a separate post if ane are interested.
>>
>
> Hey,
>
> Thanks for the answers.
>
> I have both up-to-date EL6 and EL7 hosts with latests kernel available
> in base channel so I think it means I have >=2.3 auditd package.
> I'll definitely have a look into augenrules and how to use it.
> I'll probably come back at you with mores questions after that.
>
> Florian
Okay so if I get this straight, I should probably have a default
*generic* audit file for OS/kernel-related stuff and then deploy on
specific hosts a bunch of specific rule files that would be compiled
every now and then by an augenrules cronjob ?
What would be the best practice to be alerted in case the compilation
fails (augenrules) ?
That sounds ... reasonnable. But still, I'll have to maintain these
specific files and find a way to deliver them to these specific hosts.
Florian
^ permalink raw reply
* Re: auid=0
From: Steve Grubb @ 2015-08-03 19:06 UTC (permalink / raw)
To: rshaw1; +Cc: linux-audit
In-Reply-To: <5078d5cdd4a92b5757767c0f248ef91b.squirrel@webmail.umbc.edu>
On Monday, August 03, 2015 02:53:52 PM rshaw1@umbc.edu wrote:
> > On Monday, August 03, 2015 02:11:31 PM rshaw1@umbc.edu wrote:
> >> Comparing the "official" STIG content with the scap-security-guide
> >> content, the former seems to have added corresponding rules for "-F
> >> auid=0" that aren't present in scap-security guide. i.e. where
> >> scap-security-guide will just have one rule:
> >>
> >> -a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid>=500 -F
> >> auid!=4294967295 -k delete
> >>
> >> the official content will have the above plus:
> >>
> >> -a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid=0 -k delete
> >>
> >> Is the addition necessary?
> >
> > Does the official STIG allow root logins? If so, I think that is a big
> > mistake
> > and should be fixed. If it does not allow root logins, then the only way
> > I can
> > think of having auid to be 0 is for root cron jobs.
>
> It doesn't, but that doesn't mean they don't love to layer the hell out of
> things.
I wouldn't expect root cron jobs to be a threat. But if they are worried about
insider threats, then there should be a bunch of other new rules that use the
inter-object comparator.
> >> It doesn't seem to be, as the rules caught root usage of, for example,
> >> chmod
> >> just fine without it (I had used su; not sure if there's a difference
> >> between
> >> that and other ways of being root.) I would like to make sure I'm right
> >> before asking one group or the other to delete or add it, respectively.
> >
> > Perhaps they consider root cronjobs to be an attack vector?
>
> Perhaps. What about exploiting a daemon that runs as root?
Daemons have auid set to -1. To get auid to be anything other than -1, the
loginuid has to be set. That is typically done by pam_loginuid.so.
> Would cron be the only one to show up as auid=0?
Yes, because cron calls pam_loginuid to setup the user session. Atd would also
do this, but I think the code is now based off the same executable.
> I know there is some sort of thing that happens where the daemon will
> inherit the <somthing>uid of anyone who restarts it, but I'm not sure what
> it would start as at boot.
>
> I'm hoping to not have to nearly double the amount of rules, but I'm
> guessing it doesn't work to have auid >=500, !=4294967295, and =0 in the
> same rule (when I tried it, it seemed to stop catching chmod altogether).
Everything in a rule forms an "and" expression. So, auid>=500 && auid=0 is
logically false.
-Steve
^ permalink raw reply
* Re: auid=0
From: rshaw1 @ 2015-08-03 18:53 UTC (permalink / raw)
To: Steve Grubb; +Cc: linux-audit
In-Reply-To: <8232042.ns10VY4MF9@x2>
> On Monday, August 03, 2015 02:11:31 PM rshaw1@umbc.edu wrote:
>> Comparing the "official" STIG content with the scap-security-guide
>> content, the former seems to have added corresponding rules for "-F
>> auid=0" that aren't present in scap-security guide. i.e. where
>> scap-security-guide will just have one rule:
>>
>> -a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid>=500 -F
>> auid!=4294967295 -k delete
>>
>> the official content will have the above plus:
>>
>> -a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid=0 -k delete
>>
>> Is the addition necessary?
>
> Does the official STIG allow root logins? If so, I think that is a big
> mistake
> and should be fixed. If it does not allow root logins, then the only way
> I can
> think of having auid to be 0 is for root cron jobs.
It doesn't, but that doesn't mean they don't love to layer the hell out of
things.
>> It doesn't seem to be, as the rules caught root usage of, for example,
>> chmod
>> just fine without it (I had used su; not sure if there's a difference
>> between
>> that and other ways of being root.) I would like to make sure I'm right
>> before asking one group or the other to delete or add it, respectively.
>
> Perhaps they consider root cronjobs to be an attack vector?
Perhaps. What about exploiting a daemon that runs as root? Would cron be
the only one to show up as auid=0? I know there is some sort of thing
that happens where the daemon will inherit the <somthing>uid of anyone who
restarts it, but I'm not sure what it would start as at boot.
I'm hoping to not have to nearly double the amount of rules, but I'm
guessing it doesn't work to have auid >=500, !=4294967295, and =0 in the
same rule (when I tried it, it seemed to stop catching chmod altogether).
--Ray
^ permalink raw reply
* Re: auid=0
From: Steve Grubb @ 2015-08-03 18:21 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <3db3c7197b826469a01470b399b61d28.squirrel@webmail.umbc.edu>
On Monday, August 03, 2015 02:11:31 PM rshaw1@umbc.edu wrote:
> Comparing the "official" STIG content with the scap-security-guide
> content, the former seems to have added corresponding rules for "-F
> auid=0" that aren't present in scap-security guide. i.e. where
> scap-security-guide will just have one rule:
>
> -a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid>=500 -F
> auid!=4294967295 -k delete
>
> the official content will have the above plus:
>
> -a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid=0 -k delete
>
> Is the addition necessary?
Does the official STIG allow root logins? If so, I think that is a big mistake
and should be fixed. If it does not allow root logins, then the only way I can
think of having auid to be 0 is for root cron jobs.
> It doesn't seem to be, as the rules caught root usage of, for example, chmod
> just fine without it (I had used su; not sure if there's a difference between
> that and other ways of being root.) I would like to make sure I'm right
> before asking one group or the other to delete or add it, respectively.
Perhaps they consider root cronjobs to be an attack vector?
-Steve
^ permalink raw reply
* auid=0
From: rshaw1 @ 2015-08-03 18:11 UTC (permalink / raw)
To: linux-audit
Comparing the "official" STIG content with the scap-security-guide
content, the former seems to have added corresponding rules for "-F
auid=0" that aren't present in scap-security guide. i.e. where
scap-security-guide will just have one rule:
-a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid>=500 -F
auid!=4294967295 -k delete
the official content will have the above plus:
-a always,exit -F arch=ARCH -S <a bunch of stuff> -F auid=0 -k delete
Is the addition necessary? It doesn't seem to be, as the rules caught
root usage of, for example, chmod just fine without it (I had used su; not
sure if there's a difference between that and other ways of being root.)
I would like to make sure I'm right before asking one group or the other
to delete or add it, respectively.
--Ray
^ permalink raw reply
* Re: [PATCH V6 2/4] audit: clean simple fsnotify implementation
From: Richard Guy Briggs @ 2015-08-01 20:03 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-audit, linux-kernel, sgrubb, eparis
In-Reply-To: <2244755.2LgxqKMseS@sifl>
On 15/07/16, Paul Moore wrote:
> On Tuesday, July 14, 2015 11:50:24 AM Richard Guy Briggs wrote:
> > 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@redhat.com>
> >
> > 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().
>
> Definitely enough changes here to call this your own; credit Eric in the
> description and just stick with your sign off.
Done.
> > Based-on-code-by: Eric Paris <eparis@redhat.com>
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> Based on the patch description, should this be patch 1/4 instead of 2/4?
Or 1/2 as you have suggested.
> > 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
> > @@ -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;
> > +}
>
> No BUG(), we've got enough of those things already.
Cleaned them out...
> > 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.
> > + */
>
> ...
>
> > +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);
> > +}
>
> It seems like audit_fsnotify_mark_free() might be more consistent with the
> rest of the naming, yes?
Uh, yes, ok.
> > +#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
>
> If this code is '#if 0' let's dump it. We need it or we don't, keeping it
> around as dead weight is dangerous.
Agreed. Missed that.
> > +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);
> > +}
>
> It seems like there should be a #define for -1 inodes, did you check? I
> generally hate magic numbers like this because I'm pretty stupid and tend to
> forget their meaning over time ....
I found nothing obvious, but it does surprise me a bit too...
> Also, at some point we should make (or find?) some generic inode comparison
> function/macro. I'm amazed at home many times I see (i_foo == ino && i_dev ==
> dev).
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...
> > +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;
> > + }
>
> My comments on the ino and dev variables from the other patch apply here.
>
> > + audit_mark = ERR_PTR(-ENOMEM);
> > + local_pathname = kstrdup(pathname, GFP_KERNEL);
> > + if (!local_pathname)
> > + goto out;
>
> Why not just kstrdup() into audit_mark->path directly? I don't get the
> fascination with local variables. Also, why bother with the strdup if the
> audit_mark malloc is going to fail?
Yeah, I didn't like it either, that's why 4/4 exists...
> > + 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)
> > +{
>
> That is a lot of letters ... who about audit_mark_log_change()?
I used audit_watch_log_rule_change() and audit_tree_log_remove_rule() as
references... I think it is fine.
> > +/* 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);
>
> What happens when BUG_ON() is compiled out? Let's back this up with some
> normal error checking if you think this is a real concern. If it isn't a real
> concern, why do we have a BUG_ON()? This doesn't strike me as something that
> is going to be a real problem.
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.
> > + 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;
> > + };
>
> We can do better than BUG() in the default catch-all above. Maybe a
> prink(KERN_ERR ...)?
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.
> > 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);
>
> Is the bit above worthy of it's own bugfix patch independent of this fsnotify
> implementation, or is it only an issue with this new fsnotify code?
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...
> paul moore
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
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
^ permalink raw reply
* [PATCH V7 3/3] audit: add audit by children of executable path
From: Richard Guy Briggs @ 2015-08-01 19:48 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1438456536.git.rgb@redhat.com>
This adds the ability to audit the actions of children of a not-yet-running
process.
This is a split-out of a heavily modified version of a patch originally
submitted by Eric Paris with some ideas from Peter Moody.
Cc: Peter Moody <peter@hda3.com>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/uapi/linux/audit.h | 1 +
kernel/auditfilter.c | 5 +++++
kernel/auditsc.c | 11 +++++++++++
3 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index e2ca600..55a8dec 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -267,6 +267,7 @@
#define AUDIT_OBJ_GID 110
#define AUDIT_FIELD_COMPARE 111
#define AUDIT_EXE 112
+#define AUDIT_EXE_CHILDREN 113
#define AUDIT_ARG0 200
#define AUDIT_ARG1 (AUDIT_ARG0+1)
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index fa3672b..e8bb735 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -406,6 +406,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
if (f->op != Audit_equal)
return -EINVAL;
if (entry->rule.listnr != AUDIT_FILTER_EXIT)
@@ -547,6 +548,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
entry->rule.filterkey = str;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
if (entry->rule.exe || f->val > PATH_MAX)
goto exit_free;
str = audit_unpack_string(&bufp, &remain, f->val);
@@ -643,6 +645,7 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
audit_pack_string(&bufp, krule->filterkey);
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
data->buflen += data->values[i] =
audit_pack_string(&bufp, audit_mark_path(krule->exe));
break;
@@ -710,6 +713,7 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
return 1;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
/* both paths exist based on above type compare */
if (strcmp(audit_mark_path(a->exe),
audit_mark_path(b->exe)))
@@ -838,6 +842,7 @@ struct audit_entry *audit_dupe_rule(struct audit_krule *old)
new->filterkey = fk;
break;
case AUDIT_EXE:
+ case AUDIT_EXE_CHILDREN:
err = audit_dupe_exe(new, old);
break;
}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index e9bac2b..4f2b515 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -469,6 +469,17 @@ static int audit_filter_rules(struct task_struct *tsk,
case AUDIT_EXE:
result = audit_exe_compare(tsk, rule->exe);
break;
+ case AUDIT_EXE_CHILDREN:
+ {
+ struct task_struct *ptsk;
+ for (ptsk = tsk; ptsk->parent->pid > 0; ptsk = find_task_by_vpid(ptsk->parent->pid)) {
+ if (audit_exe_compare(ptsk, rule->exe)) {
+ ++result;
+ break;
+ }
+ }
+ }
+ break;
case AUDIT_UID:
result = audit_uid_comparator(cred->uid, f->op, f->uid);
break;
--
1.7.1
^ permalink raw reply related
* [PATCH V7 2/3] audit: implement audit by executable
From: Richard Guy Briggs @ 2015-08-01 19:48 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1438456536.git.rgb@redhat.com>
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@hda3.com>
Cc: Eric Paris <eparis@redhat.com>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
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(-)
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 dcfcac2..fa3672b 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);
}
@@ -965,6 +1008,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);
+
list_del_rcu(&e->list);
list_del(&e->rule.list);
call_rcu(&e->rcu, audit_free_rule_rcu);
@@ -1068,8 +1114,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;
}
@@ -1361,6 +1410,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;
--
1.7.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox