* [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
@ 2017-08-23 11:03 Richard Guy Briggs
2017-08-23 11:03 ` [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
2017-09-20 16:52 ` [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
0 siblings, 2 replies; 21+ messages in thread
From: Richard Guy Briggs @ 2017-08-23 11:03 UTC (permalink / raw)
To: linux-kernel, linux-audit
Cc: Richard Guy Briggs, Steven Rostedt, Eric Paris, Paul Moore, Steve Grubb
Tracefs or debugfs were causing hundreds to thousands of null PATH
records to be associated with the init_module and finit_module SYSCALL
records on a few modules when the following rule was in place for
startup:
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
This happens because the parent inode is not found in the task's
audit_names list and hence treats it as anonymous. This gives us no
information other than a numerical device number that may no longer be
visible upon log inspeciton, and an inode number.
Fill in the filesystem type, filesystem magic number and full pathname
from the filesystem mount point on previously null PATH records from
entries that have an anonymous parent from the child dentry using
dentry_path_raw().
Make the dentry argument of __audit_inode_child() non-const so that we
can take a reference to it in the case of an anonymous parent with
dget() and dget_parent() to be able to later print a partial path from
the host filesystem rather than null.
Since all we are given is an inode of the parent and the dentry of the
child, finding the path from the mount point to the root of the
filesystem is more challenging that would involve searching all
vfsmounts from "/" until a matching dentry is found for that
filesystem's root dentry. Even if one is found, there may be more than
one mount point. At this point the gain seems marginal since
knowing the filesystem type and path are a significant help in tracking
down the source of the PATH records and being to address them.
Sample output:
type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
...
type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
See: https://github.com/linux-audit/audit-kernel/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
v3:
fix audit_buffer leak and dname error allocation leak audit_log_name
only put audit_name->dentry if it is being replaced
v2:
minor cosmetic changes and support fs filter patch
---
include/linux/audit.h | 8 ++++----
kernel/audit.c | 19 +++++++++++++++++++
kernel/audit.h | 1 +
kernel/auditsc.c | 8 +++++++-
4 files changed, 31 insertions(+), 5 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2150bdc..1ef4ec8 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -240,7 +240,7 @@ extern void __audit_inode(struct filename *name, const struct dentry *dentry,
unsigned int flags);
extern void __audit_file(const struct file *);
extern void __audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type);
extern void __audit_seccomp(unsigned long syscall, long signr, int code);
extern void __audit_ptrace(struct task_struct *t);
@@ -305,7 +305,7 @@ static inline void audit_inode_parent_hidden(struct filename *name,
AUDIT_INODE_PARENT | AUDIT_INODE_HIDDEN);
}
static inline void audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type) {
if (unlikely(!audit_dummy_context()))
__audit_inode_child(parent, dentry, type);
@@ -486,7 +486,7 @@ static inline void __audit_inode(struct filename *name,
unsigned int flags)
{ }
static inline void __audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type)
{ }
static inline void audit_inode(struct filename *name,
@@ -500,7 +500,7 @@ static inline void audit_inode_parent_hidden(struct filename *name,
const struct dentry *dentry)
{ }
static inline void audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type)
{ }
static inline void audit_core_dumps(long signr)
diff --git a/kernel/audit.c b/kernel/audit.c
index 59e60e0..d6e6e4e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -72,6 +72,7 @@
#include <linux/freezer.h>
#include <linux/pid_namespace.h>
#include <net/netns/generic.h>
+#include <linux/dcache.h>
#include "audit.h"
@@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
name->gid = inode->i_gid;
name->rdev = inode->i_rdev;
security_inode_getsecid(inode, &name->osid);
+ if (name->dentry) {
+ dput(name->dentry);
+ name->dentry = NULL;
+ }
audit_copy_fcaps(name, dentry);
}
@@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
audit_log_n_untrustedstring(ab, n->name->name,
n->name_len);
}
+ } else if (n->dentry) {
+ char *fullpath;
+ const char *fullpathp = NULL;
+
+ fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
+ if (fullpath)
+ fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
+ if (IS_ERR(fullpathp)) {
+ fullpathp = NULL;
+ kfree(fullpath);
+ }
+ audit_log_format(ab, " name=%s(0x%lx):%s",
+ n->dentry->d_sb->s_type->name ?: "?",
+ n->dentry->d_sb->s_magic, fullpathp ?: "?");
} else
audit_log_format(ab, " name=(null)");
diff --git a/kernel/audit.h b/kernel/audit.h
index b331d9b..c01defb 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -85,6 +85,7 @@ struct audit_names {
unsigned long ino;
dev_t dev;
+ struct dentry *dentry;
umode_t mode;
kuid_t uid;
kgid_t gid;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4a42db5..11848df 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -74,6 +74,7 @@
#include <linux/string.h>
#include <linux/uaccess.h>
#include <uapi/linux/limits.h>
+#include <linux/dcache.h>
#include "audit.h"
@@ -881,6 +882,8 @@ static inline void audit_free_names(struct audit_context *context)
list_del(&n->list);
if (n->name)
putname(n->name);
+ if (n->dentry)
+ dput(n->dentry);
if (n->should_free)
kfree(n);
}
@@ -1861,7 +1864,7 @@ void __audit_file(const struct file *file)
* unsuccessful attempts.
*/
void __audit_inode_child(struct inode *parent,
- const struct dentry *dentry,
+ struct dentry *dentry,
const unsigned char type)
{
struct audit_context *context = current->audit_context;
@@ -1917,6 +1920,7 @@ void __audit_inode_child(struct inode *parent,
if (!n)
return;
audit_copy_inode(n, NULL, parent);
+ n->dentry = dget_parent(dentry);
}
if (!found_child) {
@@ -1938,6 +1942,8 @@ void __audit_inode_child(struct inode *parent,
audit_copy_inode(found_child, dentry, inode);
else
found_child->ino = AUDIT_INO_UNSET;
+ if (!found_parent)
+ found_child->dentry = dget(dentry);
}
EXPORT_SYMBOL_GPL(__audit_inode_child);
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-08-23 11:03 [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Richard Guy Briggs
@ 2017-08-23 11:03 ` Richard Guy Briggs
2017-09-07 22:36 ` Paul Moore
2017-10-19 19:58 ` Paul Moore
2017-09-20 16:52 ` [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
1 sibling, 2 replies; 21+ messages in thread
From: Richard Guy Briggs @ 2017-08-23 11:03 UTC (permalink / raw)
To: linux-kernel, linux-audit
Cc: Richard Guy Briggs, Steven Rostedt, Eric Paris, Paul Moore, Steve Grubb
Tracefs or debugfs were causing hundreds to thousands of PATH records to
be associated with the init_module and finit_module SYSCALL records on a
few modules when the following rule was in place for startup:
-a always,exit -F arch=x86_64 -S init_module -F key=mod-load
Provide a method to ignore these large number of PATH records from
overwhelming the logs if they are not of interest. Introduce a new
filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
which keys off the filesystem 4-octet hexadecimal magic identifier to
filter specific filesystem PATH records.
An example rule would look like:
-a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
-a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
Arguably the better way to address this issue is to disable tracefs and
debugfs on boot from production systems.
See: https://github.com/linux-audit/audit-kernel/issues/16
See: https://github.com/linux-audit/audit-userspace/issues/8
Test case: https://github.com/linux-audit/audit-testsuite/issues/42
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
v3: rebase
v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
---
include/uapi/linux/audit.h | 8 ++++++--
kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
kernel/auditsc.c | 23 +++++++++++++++++++++++
3 files changed, 61 insertions(+), 9 deletions(-)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..be71134 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -155,8 +155,9 @@
#define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
#define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
#define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
+#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
-#define AUDIT_NR_FILTERS 6
+#define AUDIT_NR_FILTERS 7
#define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
@@ -256,6 +257,7 @@
#define AUDIT_OBJ_LEV_HIGH 23
#define AUDIT_LOGINUID_SET 24
#define AUDIT_SESSIONID 25 /* Session ID */
+#define AUDIT_FSTYPE 26 /* FileSystem Type */
/* These are ONLY useful when checking
* at syscall exit time (AUDIT_AT_EXIT). */
@@ -335,13 +337,15 @@ enum {
#define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
#define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
#define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
+#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
#define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
- AUDIT_FEATURE_BITMAP_LOST_RESET)
+ AUDIT_FEATURE_BITMAP_LOST_RESET | \
+ AUDIT_FEATURE_BITMAP_FILTER_FS)
/* deprecated: AUDIT_VERSION_* */
#define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 0b0aa58..4a1758a 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_filter_list[3]),
LIST_HEAD_INIT(audit_filter_list[4]),
LIST_HEAD_INIT(audit_filter_list[5]),
-#if AUDIT_NR_FILTERS != 6
+ LIST_HEAD_INIT(audit_filter_list[6]),
+#if AUDIT_NR_FILTERS != 7
#error Fix audit_filter_list initialiser
#endif
};
@@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
LIST_HEAD_INIT(audit_rules_list[3]),
LIST_HEAD_INIT(audit_rules_list[4]),
LIST_HEAD_INIT(audit_rules_list[5]),
+ LIST_HEAD_INIT(audit_rules_list[6]),
};
DEFINE_MUTEX(audit_filter_mutex);
@@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
#endif
case AUDIT_FILTER_USER:
case AUDIT_FILTER_TYPE:
+ case AUDIT_FILTER_FS:
;
}
if (unlikely(rule->action == AUDIT_POSSIBLE)) {
@@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
entry->rule.listnr != AUDIT_FILTER_USER)
return -EINVAL;
break;
+ case AUDIT_FSTYPE:
+ if (entry->rule.listnr != AUDIT_FILTER_FS)
+ return -EINVAL;
+ break;
+ }
+
+ switch(entry->rule.listnr) {
+ case AUDIT_FILTER_FS:
+ switch(f->type) {
+ case AUDIT_FSTYPE:
+ case AUDIT_FILTERKEY:
+ break;
+ default:
+ return -EINVAL;
+ }
}
switch(f->type) {
@@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
return -EINVAL;
/* FALL THROUGH */
case AUDIT_ARCH:
+ case AUDIT_FSTYPE:
if (f->op != Audit_not_equal && f->op != Audit_equal)
return -EINVAL;
break;
@@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
+ case AUDIT_FILTER_USER:
+ case AUDIT_FILTER_TYPE:
+ case AUDIT_FILTER_FS:
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
@@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
#ifdef CONFIG_AUDITSYSCALL
int dont_count = 0;
- /* If either of these, don't count towards total */
- if (entry->rule.listnr == AUDIT_FILTER_USER ||
- entry->rule.listnr == AUDIT_FILTER_TYPE)
+ /* If any of these, don't count towards total */
+ switch(entry->rule.listnr) {
+ case AUDIT_FILTER_USER:
+ case AUDIT_FILTER_TYPE:
+ case AUDIT_FILTER_FS:
dont_count = 1;
+ }
#endif
mutex_lock(&audit_filter_mutex);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 11848df..ce6cbda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
struct inode *inode = d_backing_inode(dentry);
const char *dname = dentry->d_name.name;
struct audit_names *n, *found_parent = NULL, *found_child = NULL;
+ struct audit_entry *e;
+ struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
+ int i;
if (!context->in_syscall)
return;
+ rcu_read_lock();
+ if (!list_empty(list)) {
+ list_for_each_entry_rcu(e, list, list) {
+ for (i = 0; i < e->rule.field_count; i++) {
+ struct audit_field *f = &e->rule.fields[i];
+
+ if (f->type == AUDIT_FSTYPE) {
+ if (audit_comparator(parent->i_sb->s_magic,
+ f->op, f->val)) {
+ if (e->rule.action == AUDIT_NEVER) {
+ rcu_read_unlock();
+ return;
+ }
+ }
+ }
+ }
+ }
+ }
+ rcu_read_unlock();
+
if (inode)
handle_one(inode);
--
1.7.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-08-23 11:03 ` [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
@ 2017-09-07 22:36 ` Paul Moore
2017-09-07 22:40 ` Steven Rostedt
2017-10-10 0:13 ` Steve Grubb
2017-10-19 19:58 ` Paul Moore
1 sibling, 2 replies; 21+ messages in thread
From: Paul Moore @ 2017-09-07 22:36 UTC (permalink / raw)
To: Richard Guy Briggs, sgrubb; +Cc: linux-kernel, linux-audit, Steven Rostedt
On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Tracefs or debugfs were causing hundreds to thousands of PATH records to
> be associated with the init_module and finit_module SYSCALL records on a
> few modules when the following rule was in place for startup:
> -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>
> Provide a method to ignore these large number of PATH records from
> overwhelming the logs if they are not of interest. Introduce a new
> filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
> which keys off the filesystem 4-octet hexadecimal magic identifier to
> filter specific filesystem PATH records.
>
> An example rule would look like:
> -a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
> -a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
>
> Arguably the better way to address this issue is to disable tracefs and
> debugfs on boot from production systems.
>
> See: https://github.com/linux-audit/audit-kernel/issues/16
> See: https://github.com/linux-audit/audit-userspace/issues/8
> Test case: https://github.com/linux-audit/audit-testsuite/issues/42
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> ---
> v3: rebase
> v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
> ---
> include/uapi/linux/audit.h | 8 ++++++--
> kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
> kernel/auditsc.c | 23 +++++++++++++++++++++++
> 3 files changed, 61 insertions(+), 9 deletions(-)
Both this patch and 1/2 look okay, but I'm not going to merge either
until after the merge window closes.
Steve, I assume you are still happy with the kernel/userspace
interface for this?
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 0714a66..be71134 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -155,8 +155,9 @@
> #define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
> #define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
> #define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
> +#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
>
> -#define AUDIT_NR_FILTERS 6
> +#define AUDIT_NR_FILTERS 7
>
> #define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
>
> @@ -256,6 +257,7 @@
> #define AUDIT_OBJ_LEV_HIGH 23
> #define AUDIT_LOGINUID_SET 24
> #define AUDIT_SESSIONID 25 /* Session ID */
> +#define AUDIT_FSTYPE 26 /* FileSystem Type */
>
> /* These are ONLY useful when checking
> * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -335,13 +337,15 @@ enum {
> #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> +#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
>
> #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
> AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> - AUDIT_FEATURE_BITMAP_LOST_RESET)
> + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> + AUDIT_FEATURE_BITMAP_FILTER_FS)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 0b0aa58..4a1758a 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> LIST_HEAD_INIT(audit_filter_list[3]),
> LIST_HEAD_INIT(audit_filter_list[4]),
> LIST_HEAD_INIT(audit_filter_list[5]),
> -#if AUDIT_NR_FILTERS != 6
> + LIST_HEAD_INIT(audit_filter_list[6]),
> +#if AUDIT_NR_FILTERS != 7
> #error Fix audit_filter_list initialiser
> #endif
> };
> @@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> LIST_HEAD_INIT(audit_rules_list[3]),
> LIST_HEAD_INIT(audit_rules_list[4]),
> LIST_HEAD_INIT(audit_rules_list[5]),
> + LIST_HEAD_INIT(audit_rules_list[6]),
> };
>
> DEFINE_MUTEX(audit_filter_mutex);
> @@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
> #endif
> case AUDIT_FILTER_USER:
> case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_FS:
> ;
> }
> if (unlikely(rule->action == AUDIT_POSSIBLE)) {
> @@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> entry->rule.listnr != AUDIT_FILTER_USER)
> return -EINVAL;
> break;
> + case AUDIT_FSTYPE:
> + if (entry->rule.listnr != AUDIT_FILTER_FS)
> + return -EINVAL;
> + break;
> + }
> +
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_FS:
> + switch(f->type) {
> + case AUDIT_FSTYPE:
> + case AUDIT_FILTERKEY:
> + break;
> + default:
> + return -EINVAL;
> + }
> }
>
> switch(f->type) {
> @@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> return -EINVAL;
> /* FALL THROUGH */
> case AUDIT_ARCH:
> + case AUDIT_FSTYPE:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
> #ifdef CONFIG_AUDITSYSCALL
> int dont_count = 0;
>
> - /* If either of these, don't count towards total */
> - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> - entry->rule.listnr == AUDIT_FILTER_TYPE)
> + /* If any of these, don't count towards total */
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_USER:
> + case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_FS:
> dont_count = 1;
> + }
> #endif
>
> mutex_lock(&audit_filter_mutex);
> @@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
> #ifdef CONFIG_AUDITSYSCALL
> int dont_count = 0;
>
> - /* If either of these, don't count towards total */
> - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> - entry->rule.listnr == AUDIT_FILTER_TYPE)
> + /* If any of these, don't count towards total */
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_USER:
> + case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_FS:
> dont_count = 1;
> + }
> #endif
>
> mutex_lock(&audit_filter_mutex);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 11848df..ce6cbda 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
> struct inode *inode = d_backing_inode(dentry);
> const char *dname = dentry->d_name.name;
> struct audit_names *n, *found_parent = NULL, *found_child = NULL;
> + struct audit_entry *e;
> + struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
> + int i;
>
> if (!context->in_syscall)
> return;
>
> + rcu_read_lock();
> + if (!list_empty(list)) {
> + list_for_each_entry_rcu(e, list, list) {
> + for (i = 0; i < e->rule.field_count; i++) {
> + struct audit_field *f = &e->rule.fields[i];
> +
> + if (f->type == AUDIT_FSTYPE) {
> + if (audit_comparator(parent->i_sb->s_magic,
> + f->op, f->val)) {
> + if (e->rule.action == AUDIT_NEVER) {
> + rcu_read_unlock();
> + return;
> + }
> + }
> + }
> + }
> + }
> + }
> + rcu_read_unlock();
> +
> if (inode)
> handle_one(inode);
>
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-09-07 22:36 ` Paul Moore
@ 2017-09-07 22:40 ` Steven Rostedt
2017-09-07 23:05 ` Paul Moore
2017-10-10 0:13 ` Steve Grubb
1 sibling, 1 reply; 21+ messages in thread
From: Steven Rostedt @ 2017-09-07 22:40 UTC (permalink / raw)
To: Paul Moore; +Cc: Richard Guy Briggs, sgrubb, linux-kernel, linux-audit
On Thu, 7 Sep 2017 18:36:32 -0400
Paul Moore <paul@paul-moore.com> wrote:
> Steve, I assume you are still happy with the kernel/userspace
> interface for this?
I've been working on a lot of different things lately, and this has
totally been flushed out of my memory cache. What was the change that
this is making with regard to kernel/userspace and tracefs?
Thanks!
-- Steve
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-09-07 22:40 ` Steven Rostedt
@ 2017-09-07 23:05 ` Paul Moore
2017-09-07 23:07 ` Steven Rostedt
0 siblings, 1 reply; 21+ messages in thread
From: Paul Moore @ 2017-09-07 23:05 UTC (permalink / raw)
To: Steven Rostedt, sgrubb; +Cc: Richard Guy Briggs, linux-kernel, linux-audit
On Thu, Sep 7, 2017 at 6:40 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 7 Sep 2017 18:36:32 -0400
> Paul Moore <paul@paul-moore.com> wrote:
>
>> Steve, I assume you are still happy with the kernel/userspace
>> interface for this?
>
> I've been working on a lot of different things lately, and this has
> totally been flushed out of my memory cache. What was the change that
> this is making with regard to kernel/userspace and tracefs?
>
> Thanks!
My question was aimed at Steve Grubb, not you. Sorry for the
confusion, wrong Steve :)
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-09-07 23:05 ` Paul Moore
@ 2017-09-07 23:07 ` Steven Rostedt
0 siblings, 0 replies; 21+ messages in thread
From: Steven Rostedt @ 2017-09-07 23:07 UTC (permalink / raw)
To: Paul Moore; +Cc: sgrubb, Richard Guy Briggs, linux-kernel, linux-audit
On Thu, 7 Sep 2017 19:05:37 -0400
Paul Moore <paul@paul-moore.com> wrote:
>
> My question was aimed at Steve Grubb, not you. Sorry for the
> confusion, wrong Steve :)
>
Great good to hear. I thought I was having a senior moment.
-- Steve
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-08-23 11:03 [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Richard Guy Briggs
2017-08-23 11:03 ` [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
@ 2017-09-20 16:52 ` Paul Moore
2017-09-21 14:57 ` Richard Guy Briggs
` (2 more replies)
1 sibling, 3 replies; 21+ messages in thread
From: Paul Moore @ 2017-09-20 16:52 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-kernel, linux-audit, Steven Rostedt
On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Tracefs or debugfs were causing hundreds to thousands of null PATH
> records to be associated with the init_module and finit_module SYSCALL
> records on a few modules when the following rule was in place for
> startup:
> -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>
> This happens because the parent inode is not found in the task's
> audit_names list and hence treats it as anonymous. This gives us no
> information other than a numerical device number that may no longer be
> visible upon log inspeciton, and an inode number.
>
> Fill in the filesystem type, filesystem magic number and full pathname
> from the filesystem mount point on previously null PATH records from
> entries that have an anonymous parent from the child dentry using
> dentry_path_raw().
>
> Make the dentry argument of __audit_inode_child() non-const so that we
> can take a reference to it in the case of an anonymous parent with
> dget() and dget_parent() to be able to later print a partial path from
> the host filesystem rather than null.
>
> Since all we are given is an inode of the parent and the dentry of the
> child, finding the path from the mount point to the root of the
> filesystem is more challenging that would involve searching all
> vfsmounts from "/" until a matching dentry is found for that
> filesystem's root dentry. Even if one is found, there may be more than
> one mount point. At this point the gain seems marginal since
> knowing the filesystem type and path are a significant help in tracking
> down the source of the PATH records and being to address them.
>
> Sample output:
> type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> ...
> type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
>
> See: https://github.com/linux-audit/audit-kernel/issues/8
> Test case: https://github.com/linux-audit/audit-testsuite/issues/42
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> ---
> v3:
> fix audit_buffer leak and dname error allocation leak audit_log_name
> only put audit_name->dentry if it is being replaced
>
> v2:
> minor cosmetic changes and support fs filter patch
> ---
> include/linux/audit.h | 8 ++++----
> kernel/audit.c | 19 +++++++++++++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 8 +++++++-
> 4 files changed, 31 insertions(+), 5 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 59e60e0..d6e6e4e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -72,6 +72,7 @@
> #include <linux/freezer.h>
> #include <linux/pid_namespace.h>
> #include <net/netns/generic.h>
> +#include <linux/dcache.h>
>
> #include "audit.h"
>
> @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> name->gid = inode->i_gid;
> name->rdev = inode->i_rdev;
> security_inode_getsecid(inode, &name->osid);
> + if (name->dentry) {
> + dput(name->dentry);
> + name->dentry = NULL;
> + }
> audit_copy_fcaps(name, dentry);
> }
>
> @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> audit_log_n_untrustedstring(ab, n->name->name,
> n->name_len);
> }
> + } else if (n->dentry) {
> + char *fullpath;
> + const char *fullpathp = NULL;
> +
> + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> + if (fullpath)
> + fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
> + if (IS_ERR(fullpathp)) {
> + fullpathp = NULL;
> + kfree(fullpath);
> + }
> + audit_log_format(ab, " name=%s(0x%lx):%s",
> + n->dentry->d_sb->s_type->name ?: "?",
> + n->dentry->d_sb->s_magic, fullpathp ?: "?");
> } else
> audit_log_format(ab, " name=(null)");
While looking this over one more time before merging I realized that
you are missing the curly braces in the "if (fullpath)" if-statement
above. This is an easy fix, and appears to be the Right Thing, so I'm
just going to fix up the patch while merging; take a look at the
result in the audit/next tree and if you have any objections let me
know so I can back it out.
I'm also only merging this patch right now, patch 2/2 needs to wait
until the corresponding userspace is ready so we can test/verify it.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-09-20 16:52 ` [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
@ 2017-09-21 14:57 ` Richard Guy Briggs
2017-10-12 1:36 ` Richard Guy Briggs
2017-11-08 23:29 ` Steve Grubb
2 siblings, 0 replies; 21+ messages in thread
From: Richard Guy Briggs @ 2017-09-21 14:57 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-kernel, linux-audit, Steven Rostedt
On 2017-09-20 12:52, Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous. This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> >
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().
> >
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> >
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry. Even if one is found, there may be more than
> > one mount point. At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> >
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> > type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > ...
> > type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > ---
> > v3:
> > fix audit_buffer leak and dname error allocation leak audit_log_name
> > only put audit_name->dentry if it is being replaced
> >
> > v2:
> > minor cosmetic changes and support fs filter patch
> > ---
> > include/linux/audit.h | 8 ++++----
> > kernel/audit.c | 19 +++++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 8 +++++++-
> > 4 files changed, 31 insertions(+), 5 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <linux/dcache.h>
> >
> > #include "audit.h"
> >
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > name->gid = inode->i_gid;
> > name->rdev = inode->i_rdev;
> > security_inode_getsecid(inode, &name->osid);
> > + if (name->dentry) {
> > + dput(name->dentry);
> > + name->dentry = NULL;
> > + }
> > audit_copy_fcaps(name, dentry);
> > }
> >
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> > audit_log_n_untrustedstring(ab, n->name->name,
> > n->name_len);
> > }
> > + } else if (n->dentry) {
> > + char *fullpath;
> > + const char *fullpathp = NULL;
> > +
> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > + if (fullpath)
> > + fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
> > + if (IS_ERR(fullpathp)) {
> > + fullpathp = NULL;
> > + kfree(fullpath);
> > + }
> > + audit_log_format(ab, " name=%s(0x%lx):%s",
> > + n->dentry->d_sb->s_type->name ?: "?",
> > + n->dentry->d_sb->s_magic, fullpathp ?: "?");
> > } else
> > audit_log_format(ab, " name=(null)");
>
> While looking this over one more time before merging I realized that
> you are missing the curly braces in the "if (fullpath)" if-statement
> above. This is an easy fix, and appears to be the Right Thing, so I'm
> just going to fix up the patch while merging; take a look at the
> result in the audit/next tree and if you have any objections let me
> know so I can back it out.
Thanks for catching that. That fix looks correct to me.
> I'm also only merging this patch right now, patch 2/2 needs to wait
> until the corresponding userspace is ready so we can test/verify it.
I've now got tests to test patch 1 and patch 2 each seperately.
They have to be two seperate manual tests because they each require a
reboot with different rules.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-09-07 22:36 ` Paul Moore
2017-09-07 22:40 ` Steven Rostedt
@ 2017-10-10 0:13 ` Steve Grubb
1 sibling, 0 replies; 21+ messages in thread
From: Steve Grubb @ 2017-10-10 0:13 UTC (permalink / raw)
To: Paul Moore; +Cc: Richard Guy Briggs, linux-kernel, linux-audit, Steven Rostedt
On Thursday, September 7, 2017 6:36:32 PM EDT Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Tracefs or debugfs were causing hundreds to thousands of PATH records to
> > be associated with the init_module and finit_module SYSCALL records on a
> >
> > few modules when the following rule was in place for startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > Provide a method to ignore these large number of PATH records from
> > overwhelming the logs if they are not of interest. Introduce a new
> > filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
> > which keys off the filesystem 4-octet hexadecimal magic identifier to
> > filter specific filesystem PATH records.
> >
> > An example rule would look like:
> > -a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
> > -a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
> >
> > Arguably the better way to address this issue is to disable tracefs and
> > debugfs on boot from production systems.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/16
> > See: https://github.com/linux-audit/audit-userspace/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > ---
> > v3: rebase
> > v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
> > ---
> >
> > include/uapi/linux/audit.h | 8 ++++++--
> > kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
> > kernel/auditsc.c | 23 +++++++++++++++++++++++
> > 3 files changed, 61 insertions(+), 9 deletions(-)
>
> Both this patch and 1/2 look okay, but I'm not going to merge either
> until after the merge window closes.
>
> Steve, I assume you are still happy with the kernel/userspace
> interface for this?
The user space portion was applied a week or two ago. So, I guess proceed with
merging this.
-Steve
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 0714a66..be71134 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -155,8 +155,9 @@
> >
> > #define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system
> > watches */ #define AUDIT_FILTER_EXIT 0x04 /* Apply rule at
> > syscall exit */ #define AUDIT_FILTER_TYPE 0x05 /* Apply rule at
> > audit_log_start */>
> > +#define AUDIT_FILTER_FS 0x06 /* Apply rule at
> > __audit_inode_child */
> >
> > -#define AUDIT_NR_FILTERS 6
> > +#define AUDIT_NR_FILTERS 7
> >
> > #define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
> >
> > @@ -256,6 +257,7 @@
> >
> > #define AUDIT_OBJ_LEV_HIGH 23
> > #define AUDIT_LOGINUID_SET 24
> > #define AUDIT_SESSIONID 25 /* Session ID */
> >
> > +#define AUDIT_FSTYPE 26 /* FileSystem Type */
> >
> > /* These are ONLY useful when checking
> >
> > * at syscall exit time (AUDIT_AT_EXIT).
> > */
> >
> > @@ -335,13 +337,15 @@ enum {
> >
> > #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
> > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> > #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> >
> > +#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> >
> > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> >
> > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME |
> > \
> > AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
> > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER |
> > \
> >
> > - AUDIT_FEATURE_BITMAP_LOST_RESET)
> > + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> > + AUDIT_FEATURE_BITMAP_FILTER_FS)
> >
> > /* deprecated: AUDIT_VERSION_* */
> > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> >
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 0b0aa58..4a1758a 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> >
> > LIST_HEAD_INIT(audit_filter_list[3]),
> > LIST_HEAD_INIT(audit_filter_list[4]),
> > LIST_HEAD_INIT(audit_filter_list[5]),
> >
> > -#if AUDIT_NR_FILTERS != 6
> > + LIST_HEAD_INIT(audit_filter_list[6]),
> > +#if AUDIT_NR_FILTERS != 7
> >
> > #error Fix audit_filter_list initialiser
> > #endif
> > };
> >
> > @@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> >
> > LIST_HEAD_INIT(audit_rules_list[3]),
> > LIST_HEAD_INIT(audit_rules_list[4]),
> > LIST_HEAD_INIT(audit_rules_list[5]),
> >
> > + LIST_HEAD_INIT(audit_rules_list[6]),
> >
> > };
> >
> > DEFINE_MUTEX(audit_filter_mutex);
> >
> > @@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry
> > *entry)>
> > #endif
> >
> > case AUDIT_FILTER_USER:
> >
> > case AUDIT_FILTER_TYPE:
> > + case AUDIT_FILTER_FS:
> > ;
> >
> > }
> > if (unlikely(rule->action == AUDIT_POSSIBLE)) {
> >
> > @@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry
> > *entry, struct audit_field *f)>
> > entry->rule.listnr != AUDIT_FILTER_USER)
> >
> > return -EINVAL;
> >
> > break;
> >
> > + case AUDIT_FSTYPE:
> > + if (entry->rule.listnr != AUDIT_FILTER_FS)
> > + return -EINVAL;
> > + break;
> > + }
> > +
> > + switch(entry->rule.listnr) {
> > + case AUDIT_FILTER_FS:
> > + switch(f->type) {
> > + case AUDIT_FSTYPE:
> > + case AUDIT_FILTERKEY:
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> >
> > }
> >
> > switch(f->type) {
> >
> > @@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry
> > *entry, struct audit_field *f)>
> > return -EINVAL;
> >
> > /* FALL THROUGH */
> >
> > case AUDIT_ARCH:
> > + case AUDIT_FSTYPE:
> > if (f->op != Audit_not_equal && f->op != Audit_equal)
> >
> > return -EINVAL;
> >
> > break;
> >
> > @@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry
> > *entry)>
> > #ifdef CONFIG_AUDITSYSCALL
> >
> > int dont_count = 0;
> >
> > - /* If either of these, don't count towards total */
> > - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> > - entry->rule.listnr == AUDIT_FILTER_TYPE)
> > + /* If any of these, don't count towards total */
> > + switch(entry->rule.listnr) {
> > + case AUDIT_FILTER_USER:
> > + case AUDIT_FILTER_TYPE:
> >
> > + case AUDIT_FILTER_FS:
> > dont_count = 1;
> >
> > + }
> >
> > #endif
> >
> > mutex_lock(&audit_filter_mutex);
> >
> > @@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
> >
> > #ifdef CONFIG_AUDITSYSCALL
> >
> > int dont_count = 0;
> >
> > - /* If either of these, don't count towards total */
> > - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> > - entry->rule.listnr == AUDIT_FILTER_TYPE)
> > + /* If any of these, don't count towards total */
> > + switch(entry->rule.listnr) {
> > + case AUDIT_FILTER_USER:
> > + case AUDIT_FILTER_TYPE:
> >
> > + case AUDIT_FILTER_FS:
> > dont_count = 1;
> >
> > + }
> >
> > #endif
> >
> > mutex_lock(&audit_filter_mutex);
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 11848df..ce6cbda 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
> >
> > struct inode *inode = d_backing_inode(dentry);
> > const char *dname = dentry->d_name.name;
> > struct audit_names *n, *found_parent = NULL, *found_child = NULL;
> >
> > + struct audit_entry *e;
> > + struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
> > + int i;
> >
> > if (!context->in_syscall)
> >
> > return;
> >
> > + rcu_read_lock();
> > + if (!list_empty(list)) {
> > + list_for_each_entry_rcu(e, list, list) {
> > + for (i = 0; i < e->rule.field_count; i++) {
> > + struct audit_field *f =
> > &e->rule.fields[i];
> > +
> > + if (f->type == AUDIT_FSTYPE) {
> > + if
> > (audit_comparator(parent->i_sb->s_magic, +
> > f->op, f->val)) {
> > + if (e->rule.action ==
> > AUDIT_NEVER) { +
> > rcu_read_unlock(); +
> > return;
> > + }
> > + }
> > + }
> > + }
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> >
> > if (inode)
> >
> > handle_one(inode);
> >
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-09-20 16:52 ` [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
2017-09-21 14:57 ` Richard Guy Briggs
@ 2017-10-12 1:36 ` Richard Guy Briggs
2017-11-08 23:29 ` Steve Grubb
2 siblings, 0 replies; 21+ messages in thread
From: Richard Guy Briggs @ 2017-10-12 1:36 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-kernel, linux-audit, Steven Rostedt
On 2017-09-20 16:52, Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous. This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> >
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().
> >
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> >
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry. Even if one is found, there may be more than
> > one mount point. At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> >
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634
> > type=PATH msg=audit(1488317694.446:143): item=797 name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969 dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=796 name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > ...
> > type=PATH msg=audit(1488317694.446:143): item=1 name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=CREATE
> > type=PATH msg=audit(1488317694.446:143): item=0 name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313 success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6 pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > ---
> > v3:
> > fix audit_buffer leak and dname error allocation leak audit_log_name
> > only put audit_name->dentry if it is being replaced
> >
> > v2:
> > minor cosmetic changes and support fs filter patch
> > ---
> > include/linux/audit.h | 8 ++++----
> > kernel/audit.c | 19 +++++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 8 +++++++-
> > 4 files changed, 31 insertions(+), 5 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> > +#include <linux/dcache.h>
> >
> > #include "audit.h"
> >
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name, const struct dentry *dentry,
> > name->gid = inode->i_gid;
> > name->rdev = inode->i_rdev;
> > security_inode_getsecid(inode, &name->osid);
> > + if (name->dentry) {
> > + dput(name->dentry);
> > + name->dentry = NULL;
> > + }
> > audit_copy_fcaps(name, dentry);
> > }
> >
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context, struct audit_names *n,
> > audit_log_n_untrustedstring(ab, n->name->name,
> > n->name_len);
> > }
> > + } else if (n->dentry) {
> > + char *fullpath;
> > + const char *fullpathp = NULL;
> > +
> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > + if (fullpath)
> > + fullpathp = dentry_path_raw(n->dentry, fullpath, PATH_MAX);
> > + if (IS_ERR(fullpathp)) {
> > + fullpathp = NULL;
> > + kfree(fullpath);
> > + }
> > + audit_log_format(ab, " name=%s(0x%lx):%s",
> > + n->dentry->d_sb->s_type->name ?: "?",
> > + n->dentry->d_sb->s_magic, fullpathp ?: "?");
> > } else
> > audit_log_format(ab, " name=(null)");
>
> While looking this over one more time before merging I realized that
> you are missing the curly braces in the "if (fullpath)" if-statement
> above. This is an easy fix, and appears to be the Right Thing, so I'm
> just going to fix up the patch while merging; take a look at the
> result in the audit/next tree and if you have any objections let me
> know so I can back it out.
>
> I'm also only merging this patch right now, patch 2/2 needs to wait
> until the corresponding userspace is ready so we can test/verify it.
The tests have been brought up to date. The first test patch
(syscall_module_path) tests kernel patch 1/2 that adds names for the
NULL paths. The second test patch (syscall_module_path_filter) tests
kernel patch 2/2 and the correpsonding userspace patch to set the
filesystem PATH filters.
See https://github.com/linux-audit/audit-testsuite/pull/42
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-08-23 11:03 ` [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
2017-09-07 22:36 ` Paul Moore
@ 2017-10-19 19:58 ` Paul Moore
2017-10-19 20:10 ` Richard Guy Briggs
1 sibling, 1 reply; 21+ messages in thread
From: Paul Moore @ 2017-10-19 19:58 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-kernel, linux-audit, Steven Rostedt
On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Tracefs or debugfs were causing hundreds to thousands of PATH records to
> be associated with the init_module and finit_module SYSCALL records on a
> few modules when the following rule was in place for startup:
> -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>
> Provide a method to ignore these large number of PATH records from
> overwhelming the logs if they are not of interest. Introduce a new
> filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
> which keys off the filesystem 4-octet hexadecimal magic identifier to
> filter specific filesystem PATH records.
>
> An example rule would look like:
> -a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
> -a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
>
> Arguably the better way to address this issue is to disable tracefs and
> debugfs on boot from production systems.
>
> See: https://github.com/linux-audit/audit-kernel/issues/16
> See: https://github.com/linux-audit/audit-userspace/issues/8
> Test case: https://github.com/linux-audit/audit-testsuite/issues/42
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> ---
> v3: rebase
> v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
> ---
> include/uapi/linux/audit.h | 8 ++++++--
> kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
> kernel/auditsc.c | 23 +++++++++++++++++++++++
> 3 files changed, 61 insertions(+), 9 deletions(-)
I'm in the process of applying this patch right now, and I'm seeing a
lot of space/tab whitespace damage in this patch. I'll fix it up, but
please be more careful in the future.
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 0714a66..be71134 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -155,8 +155,9 @@
> #define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
> #define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
> #define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
> +#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
>
> -#define AUDIT_NR_FILTERS 6
> +#define AUDIT_NR_FILTERS 7
>
> #define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
>
> @@ -256,6 +257,7 @@
> #define AUDIT_OBJ_LEV_HIGH 23
> #define AUDIT_LOGINUID_SET 24
> #define AUDIT_SESSIONID 25 /* Session ID */
> +#define AUDIT_FSTYPE 26 /* FileSystem Type */
>
> /* These are ONLY useful when checking
> * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -335,13 +337,15 @@ enum {
> #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> +#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
>
> #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
> AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> - AUDIT_FEATURE_BITMAP_LOST_RESET)
> + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> + AUDIT_FEATURE_BITMAP_FILTER_FS)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 0b0aa58..4a1758a 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> LIST_HEAD_INIT(audit_filter_list[3]),
> LIST_HEAD_INIT(audit_filter_list[4]),
> LIST_HEAD_INIT(audit_filter_list[5]),
> -#if AUDIT_NR_FILTERS != 6
> + LIST_HEAD_INIT(audit_filter_list[6]),
> +#if AUDIT_NR_FILTERS != 7
> #error Fix audit_filter_list initialiser
> #endif
> };
> @@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> LIST_HEAD_INIT(audit_rules_list[3]),
> LIST_HEAD_INIT(audit_rules_list[4]),
> LIST_HEAD_INIT(audit_rules_list[5]),
> + LIST_HEAD_INIT(audit_rules_list[6]),
> };
>
> DEFINE_MUTEX(audit_filter_mutex);
> @@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
> #endif
> case AUDIT_FILTER_USER:
> case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_FS:
> ;
> }
> if (unlikely(rule->action == AUDIT_POSSIBLE)) {
> @@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> entry->rule.listnr != AUDIT_FILTER_USER)
> return -EINVAL;
> break;
> + case AUDIT_FSTYPE:
> + if (entry->rule.listnr != AUDIT_FILTER_FS)
> + return -EINVAL;
> + break;
> + }
> +
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_FS:
> + switch(f->type) {
> + case AUDIT_FSTYPE:
> + case AUDIT_FILTERKEY:
> + break;
> + default:
> + return -EINVAL;
> + }
> }
>
> switch(f->type) {
> @@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> return -EINVAL;
> /* FALL THROUGH */
> case AUDIT_ARCH:
> + case AUDIT_FSTYPE:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
> #ifdef CONFIG_AUDITSYSCALL
> int dont_count = 0;
>
> - /* If either of these, don't count towards total */
> - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> - entry->rule.listnr == AUDIT_FILTER_TYPE)
> + /* If any of these, don't count towards total */
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_USER:
> + case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_FS:
> dont_count = 1;
> + }
> #endif
>
> mutex_lock(&audit_filter_mutex);
> @@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
> #ifdef CONFIG_AUDITSYSCALL
> int dont_count = 0;
>
> - /* If either of these, don't count towards total */
> - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> - entry->rule.listnr == AUDIT_FILTER_TYPE)
> + /* If any of these, don't count towards total */
> + switch(entry->rule.listnr) {
> + case AUDIT_FILTER_USER:
> + case AUDIT_FILTER_TYPE:
> + case AUDIT_FILTER_FS:
> dont_count = 1;
> + }
> #endif
>
> mutex_lock(&audit_filter_mutex);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 11848df..ce6cbda 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
> struct inode *inode = d_backing_inode(dentry);
> const char *dname = dentry->d_name.name;
> struct audit_names *n, *found_parent = NULL, *found_child = NULL;
> + struct audit_entry *e;
> + struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
> + int i;
>
> if (!context->in_syscall)
> return;
>
> + rcu_read_lock();
> + if (!list_empty(list)) {
> + list_for_each_entry_rcu(e, list, list) {
> + for (i = 0; i < e->rule.field_count; i++) {
> + struct audit_field *f = &e->rule.fields[i];
> +
> + if (f->type == AUDIT_FSTYPE) {
> + if (audit_comparator(parent->i_sb->s_magic,
> + f->op, f->val)) {
> + if (e->rule.action == AUDIT_NEVER) {
> + rcu_read_unlock();
> + return;
> + }
> + }
> + }
> + }
> + }
> + }
> + rcu_read_unlock();
> +
> if (inode)
> handle_one(inode);
>
> --
> 1.7.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic
2017-10-19 19:58 ` Paul Moore
@ 2017-10-19 20:10 ` Richard Guy Briggs
0 siblings, 0 replies; 21+ messages in thread
From: Richard Guy Briggs @ 2017-10-19 20:10 UTC (permalink / raw)
To: Paul Moore; +Cc: linux-kernel, linux-audit, Steven Rostedt
On 2017-10-19 19:58, Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Tracefs or debugfs were causing hundreds to thousands of PATH records to
> > be associated with the init_module and finit_module SYSCALL records on a
> > few modules when the following rule was in place for startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > Provide a method to ignore these large number of PATH records from
> > overwhelming the logs if they are not of interest. Introduce a new
> > filter list "AUDIT_FILTER_FS", with a new field type AUDIT_FSTYPE,
> > which keys off the filesystem 4-octet hexadecimal magic identifier to
> > filter specific filesystem PATH records.
> >
> > An example rule would look like:
> > -a never,filesystem -F fstype=0x74726163 -F key=ignore_tracefs
> > -a never,filesystem -F fstype=0x64626720 -F key=ignore_debugfs
> >
> > Arguably the better way to address this issue is to disable tracefs and
> > debugfs on boot from production systems.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/16
> > See: https://github.com/linux-audit/audit-userspace/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > ---
> > v3: rebase
> > v2: convert AUDIT_FILTER_PATH to AUDIT_FILTER_FS
> > ---
> > include/uapi/linux/audit.h | 8 ++++++--
> > kernel/auditfilter.c | 39 ++++++++++++++++++++++++++++++++-------
> > kernel/auditsc.c | 23 +++++++++++++++++++++++
> > 3 files changed, 61 insertions(+), 9 deletions(-)
>
> I'm in the process of applying this patch right now, and I'm seeing a
> lot of space/tab whitespace damage in this patch. I'll fix it up, but
> please be more careful in the future.
Ack, I see them now, must have been a mouse copy/paste rather than the
safer editor/file copy/paste and obviously missed the checkpatch step.
Sorry for that. Thanks for cleaning it up.
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 0714a66..be71134 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -155,8 +155,9 @@
> > #define AUDIT_FILTER_WATCH 0x03 /* Apply rule to file system watches */
> > #define AUDIT_FILTER_EXIT 0x04 /* Apply rule at syscall exit */
> > #define AUDIT_FILTER_TYPE 0x05 /* Apply rule at audit_log_start */
> > +#define AUDIT_FILTER_FS 0x06 /* Apply rule at __audit_inode_child */
> >
> > -#define AUDIT_NR_FILTERS 6
> > +#define AUDIT_NR_FILTERS 7
> >
> > #define AUDIT_FILTER_PREPEND 0x10 /* Prepend to front of list */
> >
> > @@ -256,6 +257,7 @@
> > #define AUDIT_OBJ_LEV_HIGH 23
> > #define AUDIT_LOGINUID_SET 24
> > #define AUDIT_SESSIONID 25 /* Session ID */
> > +#define AUDIT_FSTYPE 26 /* FileSystem Type */
> >
> > /* These are ONLY useful when checking
> > * at syscall exit time (AUDIT_AT_EXIT). */
> > @@ -335,13 +337,15 @@ enum {
> > #define AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND 0x00000008
> > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> > #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> > +#define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> >
> > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> > AUDIT_FEATURE_BITMAP_EXECUTABLE_PATH | \
> > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> > - AUDIT_FEATURE_BITMAP_LOST_RESET)
> > + AUDIT_FEATURE_BITMAP_LOST_RESET | \
> > + AUDIT_FEATURE_BITMAP_FILTER_FS)
> >
> > /* deprecated: AUDIT_VERSION_* */
> > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 0b0aa58..4a1758a 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -56,7 +56,8 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> > LIST_HEAD_INIT(audit_filter_list[3]),
> > LIST_HEAD_INIT(audit_filter_list[4]),
> > LIST_HEAD_INIT(audit_filter_list[5]),
> > -#if AUDIT_NR_FILTERS != 6
> > + LIST_HEAD_INIT(audit_filter_list[6]),
> > +#if AUDIT_NR_FILTERS != 7
> > #error Fix audit_filter_list initialiser
> > #endif
> > };
> > @@ -67,6 +68,7 @@ struct list_head audit_filter_list[AUDIT_NR_FILTERS] = {
> > LIST_HEAD_INIT(audit_rules_list[3]),
> > LIST_HEAD_INIT(audit_rules_list[4]),
> > LIST_HEAD_INIT(audit_rules_list[5]),
> > + LIST_HEAD_INIT(audit_rules_list[6]),
> > };
> >
> > DEFINE_MUTEX(audit_filter_mutex);
> > @@ -263,6 +265,7 @@ static int audit_match_signal(struct audit_entry *entry)
> > #endif
> > case AUDIT_FILTER_USER:
> > case AUDIT_FILTER_TYPE:
> > + case AUDIT_FILTER_FS:
> > ;
> > }
> > if (unlikely(rule->action == AUDIT_POSSIBLE)) {
> > @@ -338,6 +341,21 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> > entry->rule.listnr != AUDIT_FILTER_USER)
> > return -EINVAL;
> > break;
> > + case AUDIT_FSTYPE:
> > + if (entry->rule.listnr != AUDIT_FILTER_FS)
> > + return -EINVAL;
> > + break;
> > + }
> > +
> > + switch(entry->rule.listnr) {
> > + case AUDIT_FILTER_FS:
> > + switch(f->type) {
> > + case AUDIT_FSTYPE:
> > + case AUDIT_FILTERKEY:
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > }
> >
> > switch(f->type) {
> > @@ -391,6 +409,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> > return -EINVAL;
> > /* FALL THROUGH */
> > case AUDIT_ARCH:
> > + case AUDIT_FSTYPE:
> > if (f->op != Audit_not_equal && f->op != Audit_equal)
> > return -EINVAL;
> > break;
> > @@ -910,10 +929,13 @@ static inline int audit_add_rule(struct audit_entry *entry)
> > #ifdef CONFIG_AUDITSYSCALL
> > int dont_count = 0;
> >
> > - /* If either of these, don't count towards total */
> > - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> > - entry->rule.listnr == AUDIT_FILTER_TYPE)
> > + /* If any of these, don't count towards total */
> > + switch(entry->rule.listnr) {
> > + case AUDIT_FILTER_USER:
> > + case AUDIT_FILTER_TYPE:
> > + case AUDIT_FILTER_FS:
> > dont_count = 1;
> > + }
> > #endif
> >
> > mutex_lock(&audit_filter_mutex);
> > @@ -989,10 +1011,13 @@ int audit_del_rule(struct audit_entry *entry)
> > #ifdef CONFIG_AUDITSYSCALL
> > int dont_count = 0;
> >
> > - /* If either of these, don't count towards total */
> > - if (entry->rule.listnr == AUDIT_FILTER_USER ||
> > - entry->rule.listnr == AUDIT_FILTER_TYPE)
> > + /* If any of these, don't count towards total */
> > + switch(entry->rule.listnr) {
> > + case AUDIT_FILTER_USER:
> > + case AUDIT_FILTER_TYPE:
> > + case AUDIT_FILTER_FS:
> > dont_count = 1;
> > + }
> > #endif
> >
> > mutex_lock(&audit_filter_mutex);
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 11848df..ce6cbda 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1871,10 +1871,33 @@ void __audit_inode_child(struct inode *parent,
> > struct inode *inode = d_backing_inode(dentry);
> > const char *dname = dentry->d_name.name;
> > struct audit_names *n, *found_parent = NULL, *found_child = NULL;
> > + struct audit_entry *e;
> > + struct list_head *list = &audit_filter_list[AUDIT_FILTER_FS];
> > + int i;
> >
> > if (!context->in_syscall)
> > return;
> >
> > + rcu_read_lock();
> > + if (!list_empty(list)) {
> > + list_for_each_entry_rcu(e, list, list) {
> > + for (i = 0; i < e->rule.field_count; i++) {
> > + struct audit_field *f = &e->rule.fields[i];
> > +
> > + if (f->type == AUDIT_FSTYPE) {
> > + if (audit_comparator(parent->i_sb->s_magic,
> > + f->op, f->val)) {
> > + if (e->rule.action == AUDIT_NEVER) {
> > + rcu_read_unlock();
> > + return;
> > + }
> > + }
> > + }
> > + }
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > if (inode)
> > handle_one(inode);
> >
> > --
> > 1.7.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
>
>
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-09-20 16:52 ` [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
2017-09-21 14:57 ` Richard Guy Briggs
2017-10-12 1:36 ` Richard Guy Briggs
@ 2017-11-08 23:29 ` Steve Grubb
2017-11-09 15:18 ` Paul Moore
2 siblings, 1 reply; 21+ messages in thread
From: Steve Grubb @ 2017-11-08 23:29 UTC (permalink / raw)
To: linux-audit; +Cc: Paul Moore, Richard Guy Briggs, linux-kernel, Steven Rostedt
On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> > records to be associated with the init_module and finit_module SYSCALL
> > records on a few modules when the following rule was in place for
> >
> > startup:
> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >
> > This happens because the parent inode is not found in the task's
> > audit_names list and hence treats it as anonymous. This gives us no
> > information other than a numerical device number that may no longer be
> > visible upon log inspeciton, and an inode number.
> >
> > Fill in the filesystem type, filesystem magic number and full pathname
> > from the filesystem mount point on previously null PATH records from
> > entries that have an anonymous parent from the child dentry using
> > dentry_path_raw().
Late reply...but I just noticed that this changes the format of the "name"
field - which is undesirable. Please put the file system type in a field all
by itself called "fstype". You can just leave it as the hex magic number
prepended with 0x and user space can do the lookup from there,
It might be simplest to just apply a corrective patch over top of this one so
that you don't have to muck about with git branches and commit messages.
-Steve
> > Make the dentry argument of __audit_inode_child() non-const so that we
> > can take a reference to it in the case of an anonymous parent with
> > dget() and dget_parent() to be able to later print a partial path from
> > the host filesystem rather than null.
> >
> > Since all we are given is an inode of the parent and the dentry of the
> > child, finding the path from the mount point to the root of the
> > filesystem is more challenging that would involve searching all
> > vfsmounts from "/" until a matching dentry is found for that
> > filesystem's root dentry. Even if one is found, there may be more than
> > one mount point. At this point the gain seems marginal since
> > knowing the filesystem type and path are a significant help in tracking
> > down the source of the PATH records and being to address them.
> >
> > Sample output:
> > type=PROCTITLE msg=audit(1488317694.446:143):
> > proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D006E66737634 type=PATH
> > msg=audit(1488317694.446:143): item=797
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid/format inode=15969
> > dev=00:09 mode=0100444 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=CREATE type=PATH
> > msg=audit(1488317694.446:143): item=796
> > name=tracefs(74726163):/events/nfs4/nfs4_setclientid inode=15964
> > dev=00:09 mode=040755 ouid=0 ogid=0 rdev=00:00
> > obj=system_u:object_r:tracefs_t:s0 nametype=PARENT ...
> > type=PATH msg=audit(1488317694.446:143): item=1
> > name=tracefs(74726163):/events/nfs4 inode=15571 dev=00:09 mode=040755
> > ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0
> > nametype=CREATE type=PATH msg=audit(1488317694.446:143): item=0
> > name=tracefs(74726163):/events inode=119 dev=00:09 mode=040755 ouid=0
> > ogid=0 rdev=00:00 obj=system_u:object_r:tracefs_t:s0 nametype=PARENT
> > type=UNKNOWN[1330] msg=audit(1488317694.446:143): name="nfsv4"
> > type=SYSCALL msg=audit(1488317694.446:143): arch=c000003e syscall=313
> > success=yes exit=0 a0=1 a1=55d5a35ce106 a2=0 a3=1 items=798 ppid=6
> > pid=528 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> > fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod"
> > subj=system_u:system_r:insmod_t:s0 key="mod-load"
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/8
> > Test case: https://github.com/linux-audit/audit-testsuite/issues/42
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > ---
> >
> > v3:
> > fix audit_buffer leak and dname error allocation leak audit_log_name
> > only put audit_name->dentry if it is being replaced
> >
> > v2:
> > minor cosmetic changes and support fs filter patch
> >
> > ---
> >
> > include/linux/audit.h | 8 ++++----
> > kernel/audit.c | 19 +++++++++++++++++++
> > kernel/audit.h | 1 +
> > kernel/auditsc.c | 8 +++++++-
> > 4 files changed, 31 insertions(+), 5 deletions(-)
>
> ...
>
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 59e60e0..d6e6e4e 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -72,6 +72,7 @@
> >
> > #include <linux/freezer.h>
> > #include <linux/pid_namespace.h>
> > #include <net/netns/generic.h>
> >
> > +#include <linux/dcache.h>
> >
> > #include "audit.h"
> >
> > @@ -2047,6 +2048,10 @@ void audit_copy_inode(struct audit_names *name,
> > const struct dentry *dentry,>
> > name->gid = inode->i_gid;
> > name->rdev = inode->i_rdev;
> > security_inode_getsecid(inode, &name->osid);
> >
> > + if (name->dentry) {
> > + dput(name->dentry);
> > + name->dentry = NULL;
> > + }
> >
> > audit_copy_fcaps(name, dentry);
> >
> > }
> >
> > @@ -2088,6 +2093,20 @@ void audit_log_name(struct audit_context *context,
> > struct audit_names *n,>
> > audit_log_n_untrustedstring(ab, n->name->name,
> >
> > n->name_len);
> >
> > }
> >
> > + } else if (n->dentry) {
> > + char *fullpath;
> > + const char *fullpathp = NULL;
> > +
> > + fullpath = kmalloc(PATH_MAX, GFP_KERNEL);
> > + if (fullpath)
> > + fullpathp = dentry_path_raw(n->dentry, fullpath,
> > PATH_MAX); + if (IS_ERR(fullpathp)) {
> > + fullpathp = NULL;
> > + kfree(fullpath);
> > + }
> > + audit_log_format(ab, " name=%s(0x%lx):%s",
> > + n->dentry->d_sb->s_type->name ?: "?",
> > + n->dentry->d_sb->s_magic, fullpathp ?:
> > "?");>
> > } else
> >
> > audit_log_format(ab, " name=(null)");
>
> While looking this over one more time before merging I realized that
> you are missing the curly braces in the "if (fullpath)" if-statement
> above. This is an easy fix, and appears to be the Right Thing, so I'm
> just going to fix up the patch while merging; take a look at the
> result in the audit/next tree and if you have any objections let me
> know so I can back it out.
>
> I'm also only merging this patch right now, patch 2/2 needs to wait
> until the corresponding userspace is ready so we can test/verify it.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-08 23:29 ` Steve Grubb
@ 2017-11-09 15:18 ` Paul Moore
2017-11-09 15:31 ` Steve Grubb
0 siblings, 1 reply; 21+ messages in thread
From: Paul Moore @ 2017-11-09 15:18 UTC (permalink / raw)
To: Steve Grubb, Richard Guy Briggs; +Cc: linux-audit, linux-kernel, Steven Rostedt
On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
>> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Tracefs or debugfs were causing hundreds to thousands of null PATH
>> > records to be associated with the init_module and finit_module SYSCALL
>> > records on a few modules when the following rule was in place for
>> >
>> > startup:
>> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
>> >
>> > This happens because the parent inode is not found in the task's
>> > audit_names list and hence treats it as anonymous. This gives us no
>> > information other than a numerical device number that may no longer be
>> > visible upon log inspeciton, and an inode number.
>> >
>> > Fill in the filesystem type, filesystem magic number and full pathname
>> > from the filesystem mount point on previously null PATH records from
>> > entries that have an anonymous parent from the child dentry using
>> > dentry_path_raw().
>
> Late reply...but I just noticed that this changes the format of the "name"
> field - which is undesirable. Please put the file system type in a field all
> by itself called "fstype". You can just leave it as the hex magic number
> prepended with 0x and user space can do the lookup from there,
>
> It might be simplest to just apply a corrective patch over top of this one so
> that you don't have to muck about with git branches and commit messages.
A quick note on the "corrective patch": given we are just days away
from the merge window opening, it is *way* to late for something like
that, at this point the only options are to leave it as-is or
yank/revert and make another pass during the next development phase.
As for the objection itself: ungh. There is really no good reason why
you couldn't have seen this in the *several* *months* prior to this;
Richard wrote a nice patch description which *included* sample audit
events, and you were involved in discussions regarding this patchset.
To say I'm disappointed would be an understatement.
I need to look at the rest of audit/next to see what a mess things
would be if I yanked this patch. I don't expect it to be bad, but
taking a look will also give Richard a chance to voice his thoughts;
it is his patch after all, it would be nice to see an "OK" from him.
Whatever we do, it needs to happen by the of the day today (Thursday,
November 9th) as we need time to build and test the revised patches.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-09 15:18 ` Paul Moore
@ 2017-11-09 15:31 ` Steve Grubb
2017-11-09 15:59 ` Paul Moore
0 siblings, 1 reply; 21+ messages in thread
From: Steve Grubb @ 2017-11-09 15:31 UTC (permalink / raw)
To: Paul Moore; +Cc: Richard Guy Briggs, linux-audit, linux-kernel, Steven Rostedt
On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, September 20, 2017 12:52:32 PM EST Paul Moore wrote:
> >> On Wed, Aug 23, 2017 at 7:03 AM, Richard Guy Briggs <rgb@redhat.com>
wrote:
> >> > Tracefs or debugfs were causing hundreds to thousands of null PATH
> >> > records to be associated with the init_module and finit_module SYSCALL
> >> > records on a few modules when the following rule was in place for
> >> >
> >> > startup:
> >> > -a always,exit -F arch=x86_64 -S init_module -F key=mod-load
> >> >
> >> > This happens because the parent inode is not found in the task's
> >> > audit_names list and hence treats it as anonymous. This gives us no
> >> > information other than a numerical device number that may no longer be
> >> > visible upon log inspeciton, and an inode number.
> >> >
> >> > Fill in the filesystem type, filesystem magic number and full pathname
> >> > from the filesystem mount point on previously null PATH records from
> >> > entries that have an anonymous parent from the child dentry using
> >> > dentry_path_raw().
> >
> > Late reply...but I just noticed that this changes the format of the "name"
> > field - which is undesirable. Please put the file system type in a field
> > all by itself called "fstype". You can just leave it as the hex magic
> > number prepended with 0x and user space can do the lookup from there,
> >
> > It might be simplest to just apply a corrective patch over top of this one
> > so that you don't have to muck about with git branches and commit
> > messages.
>
> A quick note on the "corrective patch": given we are just days away
> from the merge window opening, it is *way* to late for something like
> that, at this point the only options are to leave it as-is or
> yank/revert and make another pass during the next development phase.
Then yank it. I think that is overreacting but given the options you presented
its the only one that avoids changing a critical field format.
> As for the objection itself: ungh. There is really no good reason why
> you couldn't have seen this in the *several* *months* prior to this;
> Richard wrote a nice patch description which *included* sample audit
> events, and you were involved in discussions regarding this patchset.
> To say I'm disappointed would be an understatement.
I am also disappointed to find that we are modifying a searchable field that
has been defined since 2005. The "name" field is very important. It's used in
quite a few reports, its used in the text format, it's searchable, and we have
a dictionary that defines exactly what it is. Fields that are searchable and
used in common reports cannot be changed without a whole lot of coordination.
I'm also disappointed to have to point out that new information should go in
its own field. I thought this was common knowledge. In any event, it was
caught and problems can be avoided.
-Steve
> I need to look at the rest of audit/next to see what a mess things
> would be if I yanked this patch. I don't expect it to be bad, but
> taking a look will also give Richard a chance to voice his thoughts;
> it is his patch after all, it would be nice to see an "OK" from him.
> Whatever we do, it needs to happen by the of the day today (Thursday,
> November 9th) as we need time to build and test the revised patches.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-09 15:31 ` Steve Grubb
@ 2017-11-09 15:59 ` Paul Moore
2017-11-09 20:52 ` Richard Guy Briggs
0 siblings, 1 reply; 21+ messages in thread
From: Paul Moore @ 2017-11-09 15:59 UTC (permalink / raw)
To: Steve Grubb; +Cc: Richard Guy Briggs, linux-audit, linux-kernel, Steven Rostedt
On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
>> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote:
...
>> > Late reply...but I just noticed that this changes the format of the "name"
>> > field - which is undesirable. Please put the file system type in a field
>> > all by itself called "fstype". You can just leave it as the hex magic
>> > number prepended with 0x and user space can do the lookup from there,
>> >
>> > It might be simplest to just apply a corrective patch over top of this one
>> > so that you don't have to muck about with git branches and commit
>> > messages.
>>
>> A quick note on the "corrective patch": given we are just days away
>> from the merge window opening, it is *way* to late for something like
>> that, at this point the only options are to leave it as-is or
>> yank/revert and make another pass during the next development phase.
>
> Then yank it. I think that is overreacting but given the options you presented
> its the only one that avoids changing a critical field format.
It's not overreacting Steve, there is simply no way we can test and
adequately soak new changes in the few days we have left. Event
yanks/reverts carry a risk at this stage, but I consider that the less
risky option for these patches. Neither is a great option, and that
is why I'm rather annoyed.
>> As for the objection itself: ungh. There is really no good reason why
>> you couldn't have seen this in the *several* *months* prior to this;
>> Richard wrote a nice patch description which *included* sample audit
>> events, and you were involved in discussions regarding this patchset.
>> To say I'm disappointed would be an understatement.
>
> I am also disappointed to find that we are modifying a searchable field that
> has been defined since 2005. The "name" field is very important. It's used in
> quite a few reports, its used in the text format, it's searchable, and we have
> a dictionary that defines exactly what it is. Fields that are searchable and
> used in common reports cannot be changed without a whole lot of coordination.
> I'm also disappointed to have to point out that new information should go in
> its own field. I thought this was common knowledge. In any event, it was
> caught and problems can be avoided.
There are plenty of things to say about the above comment, but in the
interest of brevity I'm just going to leave it at the assumptions and
inflexibility in your audit userspace continue to amaze me in all the
worst ways. Regardless, as you say, the problem can likely be avoided
this time.
>> I need to look at the rest of audit/next to see what a mess things
>> would be if I yanked this patch. I don't expect it to be bad, but
>> taking a look will also give Richard a chance to voice his thoughts;
>> it is his patch after all, it would be nice to see an "OK" from him.
>> Whatever we do, it needs to happen by the of the day today (Thursday,
>> November 9th) as we need time to build and test the revised patches.
FWIW, I just went through audit/next and it looks like yanking patch
1/2 isn't going to be too painful; I'm waiting on the build to finish
now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
in audit/next as that appears unrelated to the pathname objection,
applies cleanly, and still offers value.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-09 15:59 ` Paul Moore
@ 2017-11-09 20:52 ` Richard Guy Briggs
2017-11-09 21:47 ` Paul Moore
2017-11-13 18:30 ` Steve Grubb
0 siblings, 2 replies; 21+ messages in thread
From: Richard Guy Briggs @ 2017-11-09 20:52 UTC (permalink / raw)
To: Paul Moore; +Cc: Steve Grubb, linux-audit, linux-kernel, Steven Rostedt
On 2017-11-09 10:59, Paul Moore wrote:
> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>
> ...
>
> >> > Late reply...but I just noticed that this changes the format of the "name"
> >> > field - which is undesirable. Please put the file system type in a field
> >> > all by itself called "fstype". You can just leave it as the hex magic
> >> > number prepended with 0x and user space can do the lookup from there,
> >> >
> >> > It might be simplest to just apply a corrective patch over top of this one
> >> > so that you don't have to muck about with git branches and commit
> >> > messages.
> >>
> >> A quick note on the "corrective patch": given we are just days away
> >> from the merge window opening, it is *way* to late for something like
> >> that, at this point the only options are to leave it as-is or
> >> yank/revert and make another pass during the next development phase.
> >
> > Then yank it. I think that is overreacting but given the options you presented
> > its the only one that avoids changing a critical field format.
>
> It's not overreacting Steve, there is simply no way we can test and
> adequately soak new changes in the few days we have left. Event
> yanks/reverts carry a risk at this stage, but I consider that the less
> risky option for these patches. Neither is a great option, and that
> is why I'm rather annoyed.
I don't really see that this is my choice to include it or not. This is
the upstream maintainer's decision.
I can't say I'd be thrilled to have my name on something that stuffs up
the system though. It still isn't clear to me why an incomplete path
from some seemingly random place in the filesystem tree is preferable to
something that gives it an anchor point, at least to human interpreters.
Adding an fstype to the record is an interesting idea, but then creates
a void for all the rest of the properly formed records that don't need
it and will need more work to find it, wasting bandwidth with
"fstype=?". How are the analysis tools stymied by a text prefix to a
path that it can't find anyways?
Since we have a chance to fix it before it goes upstream, I think it
should either be yanked and respun, or add a corrective patch and submit
them together.
> >> As for the objection itself: ungh. There is really no good reason why
> >> you couldn't have seen this in the *several* *months* prior to this;
> >> Richard wrote a nice patch description which *included* sample audit
> >> events, and you were involved in discussions regarding this patchset.
> >> To say I'm disappointed would be an understatement.
> >
> > I am also disappointed to find that we are modifying a searchable field that
> > has been defined since 2005. The "name" field is very important. It's used in
> > quite a few reports, its used in the text format, it's searchable, and we have
> > a dictionary that defines exactly what it is. Fields that are searchable and
> > used in common reports cannot be changed without a whole lot of coordination.
> > I'm also disappointed to have to point out that new information should go in
> > its own field. I thought this was common knowledge. In any event, it was
> > caught and problems can be avoided.
So why does this make it unsearchable? I still don't understand any
explanations that have been made so far.
> There are plenty of things to say about the above comment, but in the
> interest of brevity I'm just going to leave it at the assumptions and
> inflexibility in your audit userspace continue to amaze me in all the
> worst ways. Regardless, as you say, the problem can likely be avoided
> this time.
>
> >> I need to look at the rest of audit/next to see what a mess things
> >> would be if I yanked this patch. I don't expect it to be bad, but
> >> taking a look will also give Richard a chance to voice his thoughts;
> >> it is his patch after all, it would be nice to see an "OK" from him.
> >> Whatever we do, it needs to happen by the of the day today (Thursday,
> >> November 9th) as we need time to build and test the revised patches.
>
> FWIW, I just went through audit/next and it looks like yanking patch
> 1/2 isn't going to be too painful; I'm waiting on the build to finish
> now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
> in audit/next as that appears unrelated to the pathname objection,
> applies cleanly, and still offers value.
The irony here stuns me. 2/2 was supposed to be the more controvertial
one.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-09 20:52 ` Richard Guy Briggs
@ 2017-11-09 21:47 ` Paul Moore
2017-11-09 21:56 ` Richard Guy Briggs
2017-11-13 18:30 ` Steve Grubb
1 sibling, 1 reply; 21+ messages in thread
From: Paul Moore @ 2017-11-09 21:47 UTC (permalink / raw)
To: Richard Guy Briggs, Steve Grubb; +Cc: linux-audit, linux-kernel, Steven Rostedt
On Thu, Nov 9, 2017 at 3:52 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-11-09 10:59, Paul Moore wrote:
>> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
>> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>>
>> ...
>>
>> >> > Late reply...but I just noticed that this changes the format of the "name"
>> >> > field - which is undesirable. Please put the file system type in a field
>> >> > all by itself called "fstype". You can just leave it as the hex magic
>> >> > number prepended with 0x and user space can do the lookup from there,
>> >> >
>> >> > It might be simplest to just apply a corrective patch over top of this one
>> >> > so that you don't have to muck about with git branches and commit
>> >> > messages.
>> >>
>> >> A quick note on the "corrective patch": given we are just days away
>> >> from the merge window opening, it is *way* to late for something like
>> >> that, at this point the only options are to leave it as-is or
>> >> yank/revert and make another pass during the next development phase.
>> >
>> > Then yank it. I think that is overreacting but given the options you presented
>> > its the only one that avoids changing a critical field format.
>>
>> It's not overreacting Steve, there is simply no way we can test and
>> adequately soak new changes in the few days we have left. Event
>> yanks/reverts carry a risk at this stage, but I consider that the less
>> risky option for these patches. Neither is a great option, and that
>> is why I'm rather annoyed.
>
> I don't really see that this is my choice to include it or not. This is
> the upstream maintainer's decision.
You are right, however, while ultimately it isn't your choice I still
wanted to hear your opinion on this as you have put a lot of effort
into this patchset.
> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though. It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.
That confuses me too. My current thinking is that a partial, or
relative, path is not something we want.
> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?".
Not to mention we still have the relative path problem in this case.
> How are the analysis tools stymied by a text prefix to a path that it can't find anyways?
I've been wondering the same. My gut feeling isn't a positive comment
so I'll refrain from sharing it here.
> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.
The odds of agreeing upon a corrective patch and getting it tested and
soaked before the merge window opens is z-e-r-o. As I said earlier,
at the very top of my first response, this isn't an option (I'm hoping
you just missed reading that).
I've been testing audit/next without patch 1/2 this afternoon and it
is still looking okay; unless I see something arguing against it
within the next hour or two that's what I'm going to send up to Linus.
>> >> As for the objection itself: ungh. There is really no good reason why
>> >> you couldn't have seen this in the *several* *months* prior to this;
>> >> Richard wrote a nice patch description which *included* sample audit
>> >> events, and you were involved in discussions regarding this patchset.
>> >> To say I'm disappointed would be an understatement.
>> >
>> > I am also disappointed to find that we are modifying a searchable field that
>> > has been defined since 2005. The "name" field is very important. It's used in
>> > quite a few reports, its used in the text format, it's searchable, and we have
>> > a dictionary that defines exactly what it is. Fields that are searchable and
>> > used in common reports cannot be changed without a whole lot of coordination.
>> > I'm also disappointed to have to point out that new information should go in
>> > its own field. I thought this was common knowledge. In any event, it was
>> > caught and problems can be avoided.
>
> So why does this make it unsearchable? I still don't understand any
> explanations that have been made so far.
Agree.
>> There are plenty of things to say about the above comment, but in the
>> interest of brevity I'm just going to leave it at the assumptions and
>> inflexibility in your audit userspace continue to amaze me in all the
>> worst ways. Regardless, as you say, the problem can likely be avoided
>> this time.
>>
>> >> I need to look at the rest of audit/next to see what a mess things
>> >> would be if I yanked this patch. I don't expect it to be bad, but
>> >> taking a look will also give Richard a chance to voice his thoughts;
>> >> it is his patch after all, it would be nice to see an "OK" from him.
>> >> Whatever we do, it needs to happen by the of the day today (Thursday,
>> >> November 9th) as we need time to build and test the revised patches.
>>
>> FWIW, I just went through audit/next and it looks like yanking patch
>> 1/2 isn't going to be too painful; I'm waiting on the build to finish
>> now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
>> in audit/next as that appears unrelated to the pathname objection,
>> applies cleanly, and still offers value.
>
> The irony here stuns me. 2/2 was supposed to be the more controvertial
> one.
Yes, me too. I never thought patch 1/2 would be the problematic one.
Oh well. Do you have any objection to 2/2 going up to Linus?
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-09 21:47 ` Paul Moore
@ 2017-11-09 21:56 ` Richard Guy Briggs
0 siblings, 0 replies; 21+ messages in thread
From: Richard Guy Briggs @ 2017-11-09 21:56 UTC (permalink / raw)
To: Paul Moore; +Cc: Steve Grubb, linux-audit, linux-kernel, Steven Rostedt
On 2017-11-09 16:47, Paul Moore wrote:
> On Thu, Nov 9, 2017 at 3:52 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2017-11-09 10:59, Paul Moore wrote:
> >> On Thu, Nov 9, 2017 at 10:31 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> >> > On Thursday, November 9, 2017 10:18:10 AM EST Paul Moore wrote:
> >> >> On Wed, Nov 8, 2017 at 6:29 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> >>
> >> ...
> >>
> >> >> > Late reply...but I just noticed that this changes the format of the "name"
> >> >> > field - which is undesirable. Please put the file system type in a field
> >> >> > all by itself called "fstype". You can just leave it as the hex magic
> >> >> > number prepended with 0x and user space can do the lookup from there,
> >> >> >
> >> >> > It might be simplest to just apply a corrective patch over top of this one
> >> >> > so that you don't have to muck about with git branches and commit
> >> >> > messages.
> >> >>
> >> >> A quick note on the "corrective patch": given we are just days away
> >> >> from the merge window opening, it is *way* to late for something like
> >> >> that, at this point the only options are to leave it as-is or
> >> >> yank/revert and make another pass during the next development phase.
> >> >
> >> > Then yank it. I think that is overreacting but given the options you presented
> >> > its the only one that avoids changing a critical field format.
> >>
> >> It's not overreacting Steve, there is simply no way we can test and
> >> adequately soak new changes in the few days we have left. Event
> >> yanks/reverts carry a risk at this stage, but I consider that the less
> >> risky option for these patches. Neither is a great option, and that
> >> is why I'm rather annoyed.
> >
> > I don't really see that this is my choice to include it or not. This is
> > the upstream maintainer's decision.
>
> You are right, however, while ultimately it isn't your choice I still
> wanted to hear your opinion on this as you have put a lot of effort
> into this patchset.
>
> > I can't say I'd be thrilled to have my name on something that stuffs up
> > the system though. It still isn't clear to me why an incomplete path
> > from some seemingly random place in the filesystem tree is preferable to
> > something that gives it an anchor point, at least to human interpreters.
>
> That confuses me too. My current thinking is that a partial, or
> relative, path is not something we want.
>
> > Adding an fstype to the record is an interesting idea, but then creates
> > a void for all the rest of the properly formed records that don't need
> > it and will need more work to find it, wasting bandwidth with
> > "fstype=?".
>
> Not to mention we still have the relative path problem in this case.
>
> > How are the analysis tools stymied by a text prefix to a path that it can't find anyways?
>
> I've been wondering the same. My gut feeling isn't a positive comment
> so I'll refrain from sharing it here.
>
> > Since we have a chance to fix it before it goes upstream, I think it
> > should either be yanked and respun, or add a corrective patch and submit
> > them together.
>
> The odds of agreeing upon a corrective patch and getting it tested and
> soaked before the merge window opens is z-e-r-o. As I said earlier,
> at the very top of my first response, this isn't an option (I'm hoping
> you just missed reading that).
Oh, I read that. That's what informed my position. That should help
you make your decision.
> I've been testing audit/next without patch 1/2 this afternoon and it
> is still looking okay; unless I see something arguing against it
> within the next hour or two that's what I'm going to send up to Linus.
>
> >> >> As for the objection itself: ungh. There is really no good reason why
> >> >> you couldn't have seen this in the *several* *months* prior to this;
> >> >> Richard wrote a nice patch description which *included* sample audit
> >> >> events, and you were involved in discussions regarding this patchset.
> >> >> To say I'm disappointed would be an understatement.
> >> >
> >> > I am also disappointed to find that we are modifying a searchable field that
> >> > has been defined since 2005. The "name" field is very important. It's used in
> >> > quite a few reports, its used in the text format, it's searchable, and we have
> >> > a dictionary that defines exactly what it is. Fields that are searchable and
> >> > used in common reports cannot be changed without a whole lot of coordination.
> >> > I'm also disappointed to have to point out that new information should go in
> >> > its own field. I thought this was common knowledge. In any event, it was
> >> > caught and problems can be avoided.
> >
> > So why does this make it unsearchable? I still don't understand any
> > explanations that have been made so far.
>
> Agree.
>
> >> There are plenty of things to say about the above comment, but in the
> >> interest of brevity I'm just going to leave it at the assumptions and
> >> inflexibility in your audit userspace continue to amaze me in all the
> >> worst ways. Regardless, as you say, the problem can likely be avoided
> >> this time.
> >>
> >> >> I need to look at the rest of audit/next to see what a mess things
> >> >> would be if I yanked this patch. I don't expect it to be bad, but
> >> >> taking a look will also give Richard a chance to voice his thoughts;
> >> >> it is his patch after all, it would be nice to see an "OK" from him.
> >> >> Whatever we do, it needs to happen by the of the day today (Thursday,
> >> >> November 9th) as we need time to build and test the revised patches.
> >>
> >> FWIW, I just went through audit/next and it looks like yanking patch
> >> 1/2 isn't going to be too painful; I'm waiting on the build to finish
> >> now. Also, as a FYI, Richard's 2/2 filtering patch is going to remain
> >> in audit/next as that appears unrelated to the pathname objection,
> >> applies cleanly, and still offers value.
> >
> > The irony here stuns me. 2/2 was supposed to be the more controvertial
> > one.
>
> Yes, me too. I never thought patch 1/2 would be the problematic one.
> Oh well. Do you have any objection to 2/2 going up to Linus?
They are two fairly different solutions to the same problem. It can
stand on its own.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-09 20:52 ` Richard Guy Briggs
2017-11-09 21:47 ` Paul Moore
@ 2017-11-13 18:30 ` Steve Grubb
2017-11-13 19:01 ` Paul Moore
1 sibling, 1 reply; 21+ messages in thread
From: Steve Grubb @ 2017-11-13 18:30 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: Paul Moore, linux-audit, linux-kernel, Steven Rostedt
On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote:
> > >> > It might be simplest to just apply a corrective patch over top of
> > >> > this one so that you don't have to muck about with git branches and
> > >> > commit messages.
> > >>
> > >> A quick note on the "corrective patch": given we are just days away
> > >> from the merge window opening, it is *way* to late for something like
> > >> that, at this point the only options are to leave it as-is or
> > >> yank/revert and make another pass during the next development phase.
> > >
> > > Then yank it. I think that is overreacting but given the options you
> > > presented its the only one that avoids changing a critical field
> > > format.
> >
> > It's not overreacting Steve, there is simply no way we can test and
> > adequately soak new changes in the few days we have left.
Its just moving the output of the information a few lines down further in the
code. 10 minutes of work, tops.
> > Event yanks/reverts carry a risk at this stage, but I consider that the
> > less risky option for these patches. Neither is a great option, and that
> > is why I'm rather annoyed.
>
> I don't really see that this is my choice to include it or not. This is
> the upstream maintainer's decision.
>
> I can't say I'd be thrilled to have my name on something that stuffs up
> the system though. It still isn't clear to me why an incomplete path
> from some seemingly random place in the filesystem tree is preferable to
> something that gives it an anchor point, at least to human interpreters.
The path should stay. Just the file system type needs decoupling and moving.
> Adding an fstype to the record is an interesting idea, but then creates
> a void for all the rest of the properly formed records that don't need
> it and will need more work to find it, wasting bandwidth with
> "fstype=?".
I would let it optionally swing in and out at the end of the record. This
should never show up on a normal system because the rules will suppress
generating this information by default. So, it really won't be all that
visible.
> How are the analysis tools stymied by a text prefix to a path that it can't
> find anyways?
Because they do not actually resolve anything in the file system. They take
the event as ground truth and use that. Also, the tools expect name=value.
This has been the way since the beginning. We do not lump multiple independent
values together. And then what if the path has a special character in it? The
whole value then has to be encoded. And I don't think the patch is using
untrusted string like it should.
> Since we have a chance to fix it before it goes upstream, I think it
> should either be yanked and respun, or add a corrective patch and submit
> them together.
>
> > >> As for the objection itself: ungh. There is really no good reason why
> > >> you couldn't have seen this in the *several* *months* prior to this;
> > >> Richard wrote a nice patch description which *included* sample audit
> > >> events, and you were involved in discussions regarding this patchset.
> > >> To say I'm disappointed would be an understatement.
> > >
> > > I am also disappointed to find that we are modifying a searchable field
> > > that has been defined since 2005. The "name" field is very important.
> > > It's used in quite a few reports, its used in the text format, it's
> > > searchable, and we have a dictionary that defines exactly what it is.
> > > Fields that are searchable and used in common reports cannot be changed
> > > without a whole lot of coordination. I'm also disappointed to have to
> > > point out that new information should go in its own field. I thought
> > > this was common knowledge. In any event, it was caught and problems can
> > > be avoided.
>
> So why does this make it unsearchable?
I didn't say it makes in unsearchable, but now that you mention it...it does
in one case. Searchable fields are more important. They typically are the
object or subject or some kind of special attribute that is commonly searched
on to group events. Searchable fields can either be partial or full word
match. By combining information in the same field, it will change this
behavior. The path name is the object of the event. By combining information
that is not the object with it, everyone will have to change and update their
software to handle this change in behavior.
> I still don't understand any explanations that have been made so far
Try ausearch --text on those events, or aureport --file.
-Steve
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents
2017-11-13 18:30 ` Steve Grubb
@ 2017-11-13 19:01 ` Paul Moore
0 siblings, 0 replies; 21+ messages in thread
From: Paul Moore @ 2017-11-13 19:01 UTC (permalink / raw)
To: Steve Grubb, Richard Guy Briggs; +Cc: linux-audit, linux-kernel, Steven Rostedt
On Mon, Nov 13, 2017 at 1:30 PM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, November 9, 2017 3:52:46 PM EST Richard Guy Briggs wrote:
>> > >> > It might be simplest to just apply a corrective patch over top of
>> > >> > this one so that you don't have to muck about with git branches and
>> > >> > commit messages.
>> > >>
>> > >> A quick note on the "corrective patch": given we are just days away
>> > >> from the merge window opening, it is *way* to late for something like
>> > >> that, at this point the only options are to leave it as-is or
>> > >> yank/revert and make another pass during the next development phase.
>> > >
>> > > Then yank it. I think that is overreacting but given the options you
>> > > presented its the only one that avoids changing a critical field
>> > > format.
>> >
>> > It's not overreacting Steve, there is simply no way we can test and
>> > adequately soak new changes in the few days we have left.
>
> Its just moving the output of the information a few lines down further in the
> code. 10 minutes of work, tops.
It's like you don't even bother reading why I write ... it's not about
the amount of time needed to make the change, it's the other stuff I
mentioned. Regardless, it's a moot point now, the patch is out and it
isn't going up in the currently open merge window.
>> > Event yanks/reverts carry a risk at this stage, but I consider that the
>> > less risky option for these patches. Neither is a great option, and that
>> > is why I'm rather annoyed.
>>
>> I don't really see that this is my choice to include it or not. This is
>> the upstream maintainer's decision.
>>
>> I can't say I'd be thrilled to have my name on something that stuffs up
>> the system though. It still isn't clear to me why an incomplete path
>> from some seemingly random place in the filesystem tree is preferable to
>> something that gives it an anchor point, at least to human interpreters.
>
> The path should stay. Just the file system type needs decoupling and moving.
See my previous comments about relative pathnames, specifially the
part about me not being a fan of them in the PATH records.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-11-13 19:01 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-23 11:03 [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Richard Guy Briggs
2017-08-23 11:03 ` [PATCH ALT4 V3 2/2] audit: filter PATH records keyed on filesystem magic Richard Guy Briggs
2017-09-07 22:36 ` Paul Moore
2017-09-07 22:40 ` Steven Rostedt
2017-09-07 23:05 ` Paul Moore
2017-09-07 23:07 ` Steven Rostedt
2017-10-10 0:13 ` Steve Grubb
2017-10-19 19:58 ` Paul Moore
2017-10-19 20:10 ` Richard Guy Briggs
2017-09-20 16:52 ` [PATCH ALT4 V3 1/2] audit: show fstype:pathname for entries with anonymous parents Paul Moore
2017-09-21 14:57 ` Richard Guy Briggs
2017-10-12 1:36 ` Richard Guy Briggs
2017-11-08 23:29 ` Steve Grubb
2017-11-09 15:18 ` Paul Moore
2017-11-09 15:31 ` Steve Grubb
2017-11-09 15:59 ` Paul Moore
2017-11-09 20:52 ` Richard Guy Briggs
2017-11-09 21:47 ` Paul Moore
2017-11-09 21:56 ` Richard Guy Briggs
2017-11-13 18:30 ` Steve Grubb
2017-11-13 19:01 ` Paul Moore
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).