4:17 p.m.
Make a number of changes to normalize CONFIG_CHANGE records by adding
missing op= fields, providing more information in existing op fields
(optional last patch) and connecting all records to existing audit
events. The user record needs special-casing since its content isn't
directly related to the call that logs it.
Since tree purge records are processed after the EOE record is produced,
the order of operation of the EOE record and the purge will have to be
reversed so that the purge records can be included in the event.
The last patch is included for completeness understanding it may be more
information than necessary.
For reference, here are the calling methods and function tree for all
CONFIG_CHANGE events with fields:
- audit_log_config_change()
- add "op=set" to fields: "[op] <param-name> old auid ses subj
res"
- AUDIT_SET:AUDIT_STATUS_PID
- AUDIT_SET:AUDIT_STATUS_LOST
- audit_do_config_change()
- AUDIT_SET:AUDIT_STATUS_FAILURE
- AUDIT_SET:AUDIT_STATUS_ENABLED
- AUDIT_SET:AUDIT_STATUS_RATE_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_WAIT_TIME
- audit_log_rule_change()
- fields: "auid ses subj op key list res"
- AUDIT_ADD_RULE -F dir=...
- AUDIT_DEL_RULE -F dir=...
- audit_log_common_recv_msg()
- fields: "pid uid auid ses subj ..."
- AUDIT_*USER* events (not CONFIG_CHANGE like all the rest)
- AUDIT_LOCKED add "op={add,remove}_rule" to "[op] audit_enabled
res"
- AUDIT_TRIM "op=trim res"
- AUDIT_MAKE_EQUIV: "op=make_equiv old new res"
- AUDIT_TTY_SET: "op=tty_set old-enabled new-enabled old-log_passwd
new-log_passwd res"
- audit_mark_log_rule_change()
- add ":mark" to op in fields: "uid ses op=autoremove_rule[] path key list
res"
- audit_autoremove_mark_rule()
- audit_mark_handle_event()
- audit_mark_fsnotify_ops.handle_event
- audit_tree_log_remove_rule() called from kill_rules()
- add to op ":tree:%s" to fields: "op=remove_rule[] dir key list
res"
- from trim_marked()
- AUDIT_TRIM: audit_trim_trees() "trim"
- audit_add_tree_rule() iterate_mounts err "add"
- audit_add_rule()
- audit_rule_change()
- AUDIT_ADD_RULE -F dir=...
- AUDIT_MAKE_EQUIV: audit_tag_tree() iterate_mounts err "equiv"
- from audit_kill_trees()
- __audit_free() "free"
- do_exit()
- copy_process() err
- __audit_syscall_exit() "exit"
- from evict_chunk() "evict"
- audit_tree_freeing_mark()
- audit_tree_ops.freeing_mark
- audit_watch_log_rule_change()
add to op ":watch:%s" to fields "auid ses op={updated,remove}_rule[] path
key list res"
- audit_update_watch() "updated_rules:watch:inval" :
"updated_rules:watch:set"
- audit_watch_handle_event() FS_CREATE|FS_MOVED_TO,
FS_DELETE|FS_MOVED_FROM
- audit_watch_fsnotify_ops.handle_event
- audit_remove_parent_watches() "remove_rule:watch:parent"
- audit_watch_handle_event() FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF
- audit_watch_fsnotify_ops.handle_event
- audit_seccomp_actions_logged()
- fields: "op actions old-actions res"
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Sources of AUDIT_CONFIG_CHANGE records and their current and proposed
fields are listed here
https://github.com/linux-audit/audit-kernel/issues/59#issuecomment-445055154
Changelog:
v3:
- un-clever %s_rule to not break up op values
- create audit_log_user_recv_msg() and squash into record connection
- squash kill_trees context handling with kill-trees before EOE
- rebase on audit/next (v4.20-rc1) with 2a1fe215e730 ("audit: use current whenever
possible")
- remove parens in extended format
v2:
- re-order audit_log_exit() and audit_kill_trees()
- drop EOE reordering patch
- rebase on 4.18-rc1 (audit/next)
Richard Guy Briggs (4):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: hand taken context to audit_kill_trees for syscall logging
audit: extend config_change mark/watch/tree rule changes
kernel/audit.c | 33 +++++++++++++++++++++++----------
kernel/audit.h | 4 ++--
kernel/audit_fsnotify.c | 4 ++--
kernel/audit_tree.c | 28 +++++++++++++++-------------
kernel/audit_watch.c | 8 +++++---
kernel/auditfilter.c | 2 +-
kernel/auditsc.c | 12 ++++++------
7 files changed, 54 insertions(+), 37 deletions(-)
--
1.8.3.1
4:17 p.m.
New subject: [PATCH ghak59 V3 1/4] audit: give a clue what CONFIG_CHANGE op was involved
The failure to add an audit rule due to audit locked gives no clue
what CONFIG_CHANGE operation failed.
Similarly the set operation is the only other operation that doesn't
give the "op=" field to indicate the action.
All other CONFIG_CHANGE records include an op= field to give a clue as
to what sort of configuration change is being executed.
Since these are the only CONFIG_CHANGE records that that do not have an
op= field, add them to bring them in line with the rest.
Old records:
type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=...
audit_enabled=2 res=0
type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1
auid=unset ses=unset subj=... res=yes
New records:
type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=...
op=add_rule audit_enabled=2 res=0
type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1
auid=unset ses=unset subj=... res=yes
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 779671883349..0e8026423fbd 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -400,7 +400,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32
old,
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return rc;
- audit_log_format(ab, "%s=%u old=%u ", function_name, new, old);
+ audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
audit_log_session_info(ab);
rc = audit_log_task_context(ab);
if (rc)
@@ -1363,7 +1363,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
return -EINVAL;
if (audit_enabled == AUDIT_LOCKED) {
audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
- audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled);
+ audit_log_format(ab, " op=%s audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ? "add_rule" : "remove_rule",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1
4:10 p.m.
New subject: [PATCH ghak59 V3 1/4] audit: give a clue what CONFIG_CHANGE op was involved
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
The failure to add an audit rule due to audit locked gives no clue
what CONFIG_CHANGE operation failed.
Similarly the set operation is the only other operation that doesn't
give the "op=" field to indicate the action.
All other CONFIG_CHANGE records include an op= field to give a clue as
to what sort of configuration change is being executed.
Since these are the only CONFIG_CHANGE records that that do not have an
op= field, add them to bring them in line with the rest.
Old records:
type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=...
audit_enabled=2 res=0
type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1
auid=unset ses=unset subj=... res=yes
New records:
type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=...
op=add_rule audit_enabled=2 res=0
type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1
auid=unset ses=unset subj=... res=yes
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Merged, but I had to fixup a line length issue as reported by
checkpatch.pl. While I don't think we need to always follow
checkpatch.pl 100%, please make every effort to ensure that it likes
you line lengths.
diff --git a/kernel/audit.c b/kernel/audit.c
index 779671883349..0e8026423fbd 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -400,7 +400,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32
old,
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return rc;
- audit_log_format(ab, "%s=%u old=%u ", function_name, new, old);
+ audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
audit_log_session_info(ab);
rc = audit_log_task_context(ab);
if (rc)
@@ -1363,7 +1363,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
return -EINVAL;
if (audit_enabled == AUDIT_LOCKED) {
audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
- audit_log_format(ab, " audit_enabled=%d res=0",
audit_enabled);
+ audit_log_format(ab, " op=%s audit_enabled=%d res=0",
+ msg_type == AUDIT_ADD_RULE ?
"add_rule" : "remove_rule",
+ audit_enabled);
audit_log_end(ab);
return -EPERM;
}
--
1.8.3.1
--
paul moore
www.paul-moore.com
4:17 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
Tie syscall information to all CONFIG_CHANGE calls since they are all a
result of user actions.
Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of
AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
and since the AUDIT_CONFIG_CHANGE message type has been converted to a
syscall accompanied record type, special-case the AUDIT_USER_* range of
messages so they remain standalone records.
See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 27 +++++++++++++++++++--------
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 2 +-
kernel/audit_watch.c | 2 +-
kernel/auditfilter.c | 2 +-
5 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 0e8026423fbd..a321fea94cc6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -397,7 +397,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32
old,
struct audit_buffer *ab;
int rc = 0;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return rc;
audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
@@ -1054,7 +1054,8 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type)
return err;
}
-static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+static void audit_log_common_recv_msg(struct audit_context *context,
+ struct audit_buffer **ab, u16 msg_type)
{
uid_t uid = from_kuid(&init_user_ns, current_uid());
pid_t pid = task_tgid_nr(current);
@@ -1064,7 +1065,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16
msg_type)
return;
}
- *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+ *ab = audit_log_start(context, GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u ", pid, uid);
@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16
msg_type)
audit_log_task_context(*ab);
}
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ audit_log_common_recv_msg(NULL, ab, msg_type);
+}
+
+static inline void audit_log_config_change_alt(struct audit_buffer **ab)
+{
+ audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1339,7 +1350,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
if (err)
break;
}
- audit_log_common_recv_msg(&ab, msg_type);
+ audit_log_user_recv_msg(&ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
@@ -1362,7 +1373,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
if (nlmsg_len(nlh) < sizeof(struct audit_rule_data))
return -EINVAL;
if (audit_enabled == AUDIT_LOCKED) {
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_config_change_alt(&ab);
audit_log_format(ab, " op=%s audit_enabled=%d res=0",
msg_type == AUDIT_ADD_RULE ? "add_rule" : "remove_rule",
audit_enabled);
@@ -1376,7 +1387,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
break;
case AUDIT_TRIM:
audit_trim_trees();
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_config_change_alt(&ab);
audit_log_format(ab, " op=trim res=1");
audit_log_end(ab);
break;
@@ -1406,7 +1417,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
/* OK, here comes... */
err = audit_tag_tree(old, new);
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_config_change_alt(&ab);
audit_log_format(ab, " op=make_equiv old=");
audit_log_untrustedstring(ab, old);
@@ -1474,7 +1485,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr
*nlh)
old.enabled = t & AUDIT_TTY_ENABLE;
old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_config_change_alt(&ab);
audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
" old-log_passwd=%d new-log_passwd=%d res=%d",
old.enabled, s.enabled, old.log_passwd,
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index cf4512a33675..37ae95cfb7f4 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark
*audit_mark, c
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
audit_log_session_info(ab);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index d4af4d97f847..b0bd59ef4271 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -530,7 +530,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
audit_log_format(ab, "op=remove_rule dir=");
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 20ef9ba134b0..e8d1adeb2223 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct
audit_watc
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (!ab)
return;
audit_log_session_info(ab);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index bf309f2592c4..26a80a9d43a9 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1091,7 +1091,7 @@ static void audit_log_rule_change(char *action, struct audit_krule
*rule, int re
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (!ab)
return;
audit_log_session_info(ab);
--
1.8.3.1
4:58 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
Tie syscall information to all CONFIG_CHANGE calls since they are all a
result of user actions.
Exclude user records from syscall context:
Since the function audit_log_common_recv_msg() is shared by a number of
AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
and since the AUDIT_CONFIG_CHANGE message type has been converted to a
syscall accompanied record type, special-case the AUDIT_USER_* range of
messages so they remain standalone records.
See: https://github.com/linux-audit/audit-kernel/issues/59
See: https://github.com/linux-audit/audit-kernel/issues/50
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.c | 27 +++++++++++++++++++--------
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 2 +-
kernel/audit_watch.c | 2 +-
kernel/auditfilter.c | 2 +-
5 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 0e8026423fbd..a321fea94cc6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab,
u16 msg_type)
audit_log_task_context(*ab);
}
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16 msg_type)
+{
+ audit_log_common_recv_msg(NULL, ab, msg_type);
+}
This makes sense because this is used by "user" records ...
+static inline void audit_log_config_change_alt(struct audit_buffer
**ab)
+{
+ audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
+}
... and I don't believe this makes sense because there is no real
logical grouping with the callers like there is for
audit_log_user_recv_msg().
--
paul moore
www.paul-moore.com
10:21 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-14 17:58, Paul Moore wrote:
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> Tie syscall information to all CONFIG_CHANGE calls since they are all a
> result of user actions.
>
> Exclude user records from syscall context:
> Since the function audit_log_common_recv_msg() is shared by a number of
> AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> syscall accompanied record type, special-case the AUDIT_USER_* range of
> messages so they remain standalone records.
>
> See: https://github.com/linux-audit/audit-kernel/issues/59
> See: https://github.com/linux-audit/audit-kernel/issues/50
> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> kernel/audit.c | 27 +++++++++++++++++++--------
> kernel/audit_fsnotify.c | 2 +-
> kernel/audit_tree.c | 2 +-
> kernel/audit_watch.c | 2 +-
> kernel/auditfilter.c | 2 +-
> 5 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 0e8026423fbd..a321fea94cc6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct audit_buffer
**ab, u16 msg_type)
> audit_log_task_context(*ab);
> }
>
> +static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16 msg_type)
> +{
> + audit_log_common_recv_msg(NULL, ab, msg_type);
> +}
This makes sense because this is used by "user" records ...
> +static inline void audit_log_config_change_alt(struct audit_buffer **ab)
> +{
> + audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
> +}
... and I don't believe this makes sense because there is no real
logical grouping with the callers like there is for
audit_log_user_recv_msg().
I don't follow "logical grouping". They are all CONFIG_CHANGE record
prefixes with the current context.
Can you suggest an alternate name or another way of sharing
audit_log_common_recv_msg() since the only differences between the two
are a NULL context vs current task's context and the message type. I
wasn't particularly happy with this name either. I'd really like to
refactor this with all the rest of the CONFIG_CHANGE records, but there
is too much of a format difference to make it work without reordering or
deleting useless fields.
I know you had suggested making two different functions, but I think
they are more similar than different and merit the common factored code.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)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
6:23 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Tue, Jan 15, 2019 at 11:21 AM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2019-01-14 17:58, Paul Moore wrote:
> On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > Tie syscall information to all CONFIG_CHANGE calls since they are all a
> > result of user actions.
> >
> > Exclude user records from syscall context:
> > Since the function audit_log_common_recv_msg() is shared by a number of
> > AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_* record types,
> > and since the AUDIT_CONFIG_CHANGE message type has been converted to a
> > syscall accompanied record type, special-case the AUDIT_USER_* range of
> > messages so they remain standalone records.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > ---
> > kernel/audit.c | 27 +++++++++++++++++++--------
> > kernel/audit_fsnotify.c | 2 +-
> > kernel/audit_tree.c | 2 +-
> > kernel/audit_watch.c | 2 +-
> > kernel/auditfilter.c | 2 +-
> > 5 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 0e8026423fbd..a321fea94cc6 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
audit_buffer **ab, u16 msg_type)
> > audit_log_task_context(*ab);
> > }
> >
> > +static inline void audit_log_user_recv_msg(struct audit_buffer **ab, u16
msg_type)
> > +{
> > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > +}
>
> This makes sense because this is used by "user" records ...
>
> > +static inline void audit_log_config_change_alt(struct audit_buffer **ab)
> > +{
> > + audit_log_common_recv_msg(audit_context(), ab, AUDIT_CONFIG_CHANGE);
> > +}
>
> ... and I don't believe this makes sense because there is no real
> logical grouping with the callers like there is for
> audit_log_user_recv_msg().
I don't follow "logical grouping". They are all CONFIG_CHANGE record
prefixes with the current context.
The audit_log_user_recv_msg() callers have a logical grouping because
they are all user generated records which we've decided shouldn't be
associated with any audit records tied to the current task.
The audit_log_config_change_alt() callers seem only to be grouped by
the fact that they are share some common audit_log_config_change_alt()
parameters; they don't appear to have anything else in common. Yes,
sometimes we do create functions like audit_log_config_change_alt()
for reasons such as these, but I don't believe it is necessary, or
desirable, in this particular patch(set).
My comments on your v2 of this patchset suggested the creation of
audit_log_user_recv_msg() instead of what you did with
__audit_log_common_recv_msg(). You made that suggested change for v3
- good - but with v3 you also introduced audit_log_config_change_alt -
not good.
Get rid of audit_log_config_change_alt() (respin, retest, etc.) and
post this revised single patch (the others in the patchset that are ok
are already in audit/next) and we can get it into audit/next.
Can you suggest an alternate name or another way of sharing
audit_log_common_recv_msg() since the only differences between the two
are a NULL context vs current task's context and the message type. I
wasn't particularly happy with this name either. I'd really like to
refactor this with all the rest of the CONFIG_CHANGE records, but there
is too much of a format difference to make it work without reordering or
deleting useless fields.
I know you had suggested making two different functions, but I think
they are more similar than different and merit the common factored code.
> paul moore
- RGB
--
Richard Guy Briggs <rgb(a)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
--
paul moore
www.paul-moore.com
3:32 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Mon, 14 Jan 2019 17:58:58 -0500
Paul Moore <paul(a)paul-moore.com> wrote:
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com>
wrote:
>
> Tie syscall information to all CONFIG_CHANGE calls since they are
> all a result of user actions.
Please don't tie syscall information to this. The syscall will be
sendto. We don't need that information, its implicit. Also, doing this
will possibly wreck things in libauparse. Please test the events with
ausearch --format csv and --format text. IFF the event looks better or
the same should we do this. If stuff disappears, the patch is
breaking things
-Steve
> Exclude user records from syscall context:
> Since the function audit_log_common_recv_msg() is shared by a
> number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_*
> record types, and since the AUDIT_CONFIG_CHANGE message type has
> been converted to a syscall accompanied record type, special-case
> the AUDIT_USER_* range of messages so they remain standalone
> records.
>
> See: https://github.com/linux-audit/audit-kernel/issues/59
> See: https://github.com/linux-audit/audit-kernel/issues/50
> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> ---
> kernel/audit.c | 27 +++++++++++++++++++--------
> kernel/audit_fsnotify.c | 2 +-
> kernel/audit_tree.c | 2 +-
> kernel/audit_watch.c | 2 +-
> kernel/auditfilter.c | 2 +-
> 5 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 0e8026423fbd..a321fea94cc6 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
> }
>
> +static inline void audit_log_user_recv_msg(struct audit_buffer
> **ab, u16 msg_type) +{
> + audit_log_common_recv_msg(NULL, ab, msg_type);
> +}
This makes sense because this is used by "user" records ...
> +static inline void audit_log_config_change_alt(struct audit_buffer
> **ab) +{
> + audit_log_common_recv_msg(audit_context(), ab,
> AUDIT_CONFIG_CHANGE); +}
... and I don't believe this makes sense because there is no real
logical grouping with the callers like there is for
audit_log_user_recv_msg().
7:21 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com> wrote:
On Mon, 14 Jan 2019 17:58:58 -0500
Paul Moore <paul(a)paul-moore.com> wrote:
> On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com>
> wrote:
> >
> > Tie syscall information to all CONFIG_CHANGE calls since they are
> > all a result of user actions.
Please don't tie syscall information to this. The syscall will be
sendto. We don't need that information, its implicit. Also, doing this
will possibly wreck things in libauparse. Please test the events with
ausearch --format csv and --format text. IFF the event looks better or
the same should we do this. If stuff disappears, the patch is
breaking things
We've discussed this quite a bit already; connecting associated
records into a single event is something that should happen, needs to
happen, and will happen. Conceptually it makes no sense to record the
syscall (and any other associated records) which triggers the audit
configuration change, and the configuration change record itself as
two distinct events - they are the same event. We've also heard from
a prominent user that associating records in this way is desirable.
If the ausearch csv and text audit log transformations can't handle
this particular change, I would consider that a shortcoming of that
code. We have multi-record events now, and this is only going to
increase in the future.
Richard, if you can't make the requested changes to this patch and
resubmit by ... let's say the middle of next week? that should be
enough time, yes? ... please let me know and I'll make the changes and
get this merged.
--
paul moore
www.paul-moore.com
9:34 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 08:21, Paul Moore wrote:
On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com>
wrote:
> On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <paul(a)paul-moore.com> wrote:
> > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > Tie syscall information to all CONFIG_CHANGE calls since they are
> > > all a result of user actions.
>
> Please don't tie syscall information to this. The syscall will be
> sendto. We don't need that information, its implicit. Also, doing this
> will possibly wreck things in libauparse. Please test the events with
> ausearch --format csv and --format text. IFF the event looks better or
> the same should we do this. If stuff disappears, the patch is
> breaking things
We've discussed this quite a bit already; connecting associated
records into a single event is something that should happen, needs to
happen, and will happen. Conceptually it makes no sense to record the
syscall (and any other associated records) which triggers the audit
configuration change, and the configuration change record itself as
two distinct events - they are the same event. We've also heard from
a prominent user that associating records in this way is desirable.
If the ausearch csv and text audit log transformations can't handle
this particular change, I would consider that a shortcoming of that
code. We have multi-record events now, and this is only going to
increase in the future.
Richard, if you can't make the requested changes to this patch and
resubmit by ... let's say the middle of next week? that should be
enough time, yes? ... please let me know and I'll make the changes and
get this merged.
I would do the change, which should be very trivial, but I'm dense
enough that I still don't know what you want. In the last 6 months I've
asked a number of direct questions that have not been directly
addressed. Perhaps I should be able to figure it out from the more
general or fundamental principles replies I've gotten (which have been
helpful, but perhaps incomplete), but I'm still having some trouble.
Perhaps I'm exposing my limitations.
I am understanding mixed messages.
On the one hand, you have said you dislike single-user functions, which
is what the audit_log_user_recv_msg() function is (We had an ongoing
argument about it for audit_set_contid()). Meanwhile, the
audit_log_config_change_alt() function is used in four places and
simplifies the call by hiding the audit_context() call and the
AUDIT_CONFIG_CHANGE message type. It would revert to its original form
of audit_log_common_recv_msg(&ab, type) but prefixing audit_context()
from what I am understanding from your comments and I don't really
understand how that is better. If I go with the latter, I might as well
just call audit_log_common_recv_msg() directly with a NULL context
parameter for AUDIT_*USER* records too. What am I missing?
If I might attempt another go at humour to convey my perplexion (is that
even a word?) I feel like I'm playing Calvin-ball, but I'm not Calvin.
I don't think I'm being a troll, and hope I'm more technically competent
than I currently feel.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)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
11:58 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 10:34 AM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2019-01-17 08:21, Paul Moore wrote:
> On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com> wrote:
> > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <paul(a)paul-moore.com>
wrote:
> > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > > Tie syscall information to all CONFIG_CHANGE calls since they are
> > > > all a result of user actions.
> >
> > Please don't tie syscall information to this. The syscall will be
> > sendto. We don't need that information, its implicit. Also, doing this
> > will possibly wreck things in libauparse. Please test the events with
> > ausearch --format csv and --format text. IFF the event looks better or
> > the same should we do this. If stuff disappears, the patch is
> > breaking things
>
> We've discussed this quite a bit already; connecting associated
> records into a single event is something that should happen, needs to
> happen, and will happen. Conceptually it makes no sense to record the
> syscall (and any other associated records) which triggers the audit
> configuration change, and the configuration change record itself as
> two distinct events - they are the same event. We've also heard from
> a prominent user that associating records in this way is desirable.
>
> If the ausearch csv and text audit log transformations can't handle
> this particular change, I would consider that a shortcoming of that
> code. We have multi-record events now, and this is only going to
> increase in the future.
>
> Richard, if you can't make the requested changes to this patch and
> resubmit by ... let's say the middle of next week? that should be
> enough time, yes? ... please let me know and I'll make the changes and
> get this merged.
I would do the change, which should be very trivial, but I'm dense
enough that I still don't know what you want. In the last 6 months I've
asked a number of direct questions that have not been directly
addressed. Perhaps I should be able to figure it out from the more
general or fundamental principles replies I've gotten (which have been
helpful, but perhaps incomplete), but I'm still having some trouble.
Perhaps I'm exposing my limitations.
Since code is unambiguous, let me just cut and paste what I was
thinking (be warned, this is a cut-n-paste, so the whitespace is
probably mangled):
diff --git a/kernel/audit.c b/kernel/audit.c
index d412fb4ae6d5..d2caef6ef09e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -396,7 +396,7 @@ static int audit_log_config_change(char
*function_name, u32 new, u32 old,
struct audit_buffer *ab;
int rc = 0;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return rc;
audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
@@ -1053,7 +1053,8 @@ static int audit_netlink_ok(struct sk_buff *skb,
u16 msg_type)
return err;
}
-static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+static void audit_log_common_recv_msg(struct audit_context *context,
+ struct audit_buffer **ab, u16 msg_type)
{
uid_t uid = from_kuid(&init_user_ns, current_uid());
pid_t pid = task_tgid_nr(current);
@@ -1063,7 +1064,7 @@ static void audit_log_common_recv_msg(struct
audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+ *ab = audit_log_start(context, GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u ", pid, uid);
@@ -1071,6 +1072,11 @@ static void audit_log_common_recv_msg(struct
audit_buffer **ab, u16 msg_type)
audit_log_task_context(*ab);
}
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
u16 msg_type)
+{
+ audit_log_common_recv_msg(NULL, ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1338,7 +1344,7 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
if (err)
break;
}
- audit_log_common_recv_msg(&ab, msg_type);
+ audit_log_user_recv_msg(&ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
@@ -1361,7 +1367,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
if (nlmsg_len(nlh) < sizeof(struct audit_rule_data))
return -EINVAL;
if (audit_enabled == AUDIT_LOCKED) {
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=%s audit_enabled=%d res=0",
msg_type == AUDIT_ADD_RULE ?
"add_rule" :
"remove_rule",
@@ -1376,7 +1383,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
break;
case AUDIT_TRIM:
audit_trim_trees();
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=trim res=1");
audit_log_end(ab);
break;
@@ -1406,7 +1414,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
/* OK, here comes... */
err = audit_tag_tree(old, new);
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=make_equiv old=");
audit_log_untrustedstring(ab, old);
@@ -1474,7 +1483,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
old.enabled = t & AUDIT_TTY_ENABLE;
old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=tty_set old-enabled=%d new-enabled=%d"
" old-log_passwd=%d new-log_passwd=%d res=%d",
old.enabled, s.enabled, old.log_passwd,
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index cf4512a33675..37ae95cfb7f4 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct
audit_fsnotify_mark *audit_mark, c
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
audit_log_session_info(ab);
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 20ef9ba134b0..e8d1adeb2223 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct
audit_krule *r, struct audit_watc
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (!ab)
return;
audit_log_session_info(ab);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index bf309f2592c4..26a80a9d43a9 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1091,7 +1091,7 @@ static void audit_log_rule_change(char *action,
struct audit_krule *rule, int re
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (!ab)
return;
audit_log_session_info(ab);
--
paul moore
www.paul-moore.com
5:18 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 12:58, Paul Moore wrote:
On Thu, Jan 17, 2019 at 10:34 AM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
>
> On 2019-01-17 08:21, Paul Moore wrote:
> > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com> wrote:
> > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <paul(a)paul-moore.com>
wrote:
> > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > Tie syscall information to all CONFIG_CHANGE calls since they
are
> > > > > all a result of user actions.
> > >
> > > Please don't tie syscall information to this. The syscall will be
> > > sendto. We don't need that information, its implicit. Also, doing
this
> > > will possibly wreck things in libauparse. Please test the events with
> > > ausearch --format csv and --format text. IFF the event looks better or
> > > the same should we do this. If stuff disappears, the patch is
> > > breaking things
> >
> > We've discussed this quite a bit already; connecting associated
> > records into a single event is something that should happen, needs to
> > happen, and will happen. Conceptually it makes no sense to record the
> > syscall (and any other associated records) which triggers the audit
> > configuration change, and the configuration change record itself as
> > two distinct events - they are the same event. We've also heard from
> > a prominent user that associating records in this way is desirable.
> >
> > If the ausearch csv and text audit log transformations can't handle
> > this particular change, I would consider that a shortcoming of that
> > code. We have multi-record events now, and this is only going to
> > increase in the future.
> >
> > Richard, if you can't make the requested changes to this patch and
> > resubmit by ... let's say the middle of next week? that should be
> > enough time, yes? ... please let me know and I'll make the changes and
> > get this merged.
>
> I would do the change, which should be very trivial, but I'm dense
> enough that I still don't know what you want. In the last 6 months I've
> asked a number of direct questions that have not been directly
> addressed. Perhaps I should be able to figure it out from the more
> general or fundamental principles replies I've gotten (which have been
> helpful, but perhaps incomplete), but I'm still having some trouble.
> Perhaps I'm exposing my limitations.
Since code is unambiguous, let me just cut and paste what I was
thinking (be warned, this is a cut-n-paste, so the whitespace is
probably mangled):
Thank you. Very clear. I don't see the point of
audit_log_user_recv_msg() for reasons already stated but that's ok.
Since we're looking at these, here are the various field formats of the
AUDIT_CONFIG_CHANGE records.
Steve and Paul, are they worth attempting to normalize? Some can't
because there are two parameters changed in the same record.
I'm pretty sure some of the suggestions will break Steve's parsers, but I'm
also certain that some are already broken in their current state or were never
coded in. I've tried triggering all of these, but failed on a couple and have
pinged Al Viro a couple of times for some clues how to do so and had no reply.
If it isn't worth it, I'll leave them as they are.
audit_log_config_change op %s old auid ses subj res
op %s old res
audit_log_rule_change auid ses subj op key list res
op key list res
audit_log_common_recv_msg ADD/DEL_RULE pid uid auid ses subj op audit_enabled res
op audit_enabled res
audit_log_common_recv_msg TRIM pid uid auid ses subj op res
op res
audit_log_common_recv_msg MAKE_EQUIV pid uid auid ses subj op old new res
op dir old res (order/label change)
audit_log_common_recv_msg TTY_SET pid uid auid ses subj op old-enabled new-enabled
old-log_passwd new-log_passwd res
op enabled old-enabled log_passwd old-log_passwd
res (order/label change)
audit_mark_log_rule_change auid ses op path key list res
op path key list res
audit_tree_log_remove_rule op dir key list res
op dir key list res
audit_watch_log_rule_change auid ses op path key list res
op path key list res
audit_seccomp_actions_logged op actions old-actions res
op actions old res (label change)
diff --git a/kernel/audit.c b/kernel/audit.c
index d412fb4ae6d5..d2caef6ef09e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -396,7 +396,7 @@ static int audit_log_config_change(char
*function_name, u32 new, u32 old,
struct audit_buffer *ab;
int rc = 0;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return rc;
audit_log_format(ab, "op=set %s=%u old=%u ", function_name, new, old);
@@ -1053,7 +1053,8 @@ static int audit_netlink_ok(struct sk_buff *skb,
u16 msg_type)
return err;
}
-static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
+static void audit_log_common_recv_msg(struct audit_context *context,
+ struct audit_buffer **ab, u16 msg_type)
{
uid_t uid = from_kuid(&init_user_ns, current_uid());
pid_t pid = task_tgid_nr(current);
@@ -1063,7 +1064,7 @@ static void audit_log_common_recv_msg(struct
audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+ *ab = audit_log_start(context, GFP_KERNEL, msg_type);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u ", pid, uid);
@@ -1071,6 +1072,11 @@ static void audit_log_common_recv_msg(struct
audit_buffer **ab, u16 msg_type)
audit_log_task_context(*ab);
}
+static inline void audit_log_user_recv_msg(struct audit_buffer **ab,
u16 msg_type)
+{
+ audit_log_common_recv_msg(NULL, ab, msg_type);
+}
+
int is_audit_feature_set(int i)
{
return af.features & AUDIT_FEATURE_TO_MASK(i);
@@ -1338,7 +1344,7 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
if (err)
break;
}
- audit_log_common_recv_msg(&ab, msg_type);
+ audit_log_user_recv_msg(&ab, msg_type);
if (msg_type != AUDIT_USER_TTY)
audit_log_format(ab, " msg='%.*s'",
AUDIT_MESSAGE_TEXT_MAX,
@@ -1361,7 +1367,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
if (nlmsg_len(nlh) < sizeof(struct audit_rule_data))
return -EINVAL;
if (audit_enabled == AUDIT_LOCKED) {
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=%s audit_enabled=%d res=0",
msg_type == AUDIT_ADD_RULE ?
"add_rule" :
"remove_rule",
@@ -1376,7 +1383,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
break;
case AUDIT_TRIM:
audit_trim_trees();
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=trim res=1");
audit_log_end(ab);
break;
@@ -1406,7 +1414,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
/* OK, here comes... */
err = audit_tag_tree(old, new);
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=make_equiv old=");
audit_log_untrustedstring(ab, old);
@@ -1474,7 +1483,8 @@ static int audit_receive_msg(struct sk_buff
*skb, struct nlmsghdr *nlh)
old.enabled = t & AUDIT_TTY_ENABLE;
old.log_passwd = !!(t & AUDIT_TTY_LOG_PASSWD);
- audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE);
+ audit_log_common_recv_msg(audit_context(), &ab,
+ AUDIT_CONFIG_CHANGE);
audit_log_format(ab, " op=tty_set old-enabled=%d
new-enabled=%d"
" old-log_passwd=%d new-log_passwd=%d res=%d",
old.enabled, s.enabled, old.log_passwd,
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index cf4512a33675..37ae95cfb7f4 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -127,7 +127,7 @@ static void audit_mark_log_rule_change(struct
audit_fsnotify_mark *audit_mark, c
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
audit_log_session_info(ab);
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 20ef9ba134b0..e8d1adeb2223 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -242,7 +242,7 @@ static void audit_watch_log_rule_change(struct
audit_krule *r, struct audit_watc
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_NOFS, AUDIT_CONFIG_CHANGE);
if (!ab)
return;
audit_log_session_info(ab);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index bf309f2592c4..26a80a9d43a9 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1091,7 +1091,7 @@ static void audit_log_rule_change(char *action,
struct audit_krule *rule, int re
if (!audit_enabled)
return;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (!ab)
return;
audit_log_session_info(ab);
--
paul moore
www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb(a)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
9:26 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 6:19 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2019-01-17 12:58, Paul Moore wrote:
> On Thu, Jan 17, 2019 at 10:34 AM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> >
> > On 2019-01-17 08:21, Paul Moore wrote:
> > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com>
wrote:
> > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore
<paul(a)paul-moore.com> wrote:
> > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > > Tie syscall information to all CONFIG_CHANGE calls since
they are
> > > > > > all a result of user actions.
> > > >
> > > > Please don't tie syscall information to this. The syscall will
be
> > > > sendto. We don't need that information, its implicit. Also, doing
this
> > > > will possibly wreck things in libauparse. Please test the events
with
> > > > ausearch --format csv and --format text. IFF the event looks better
or
> > > > the same should we do this. If stuff disappears, the patch is
> > > > breaking things
> > >
> > > We've discussed this quite a bit already; connecting associated
> > > records into a single event is something that should happen, needs to
> > > happen, and will happen. Conceptually it makes no sense to record the
> > > syscall (and any other associated records) which triggers the audit
> > > configuration change, and the configuration change record itself as
> > > two distinct events - they are the same event. We've also heard from
> > > a prominent user that associating records in this way is desirable.
> > >
> > > If the ausearch csv and text audit log transformations can't handle
> > > this particular change, I would consider that a shortcoming of that
> > > code. We have multi-record events now, and this is only going to
> > > increase in the future.
> > >
> > > Richard, if you can't make the requested changes to this patch and
> > > resubmit by ... let's say the middle of next week? that should be
> > > enough time, yes? ... please let me know and I'll make the changes
and
> > > get this merged.
> >
> > I would do the change, which should be very trivial, but I'm dense
> > enough that I still don't know what you want. In the last 6 months
I've
> > asked a number of direct questions that have not been directly
> > addressed. Perhaps I should be able to figure it out from the more
> > general or fundamental principles replies I've gotten (which have been
> > helpful, but perhaps incomplete), but I'm still having some trouble.
> > Perhaps I'm exposing my limitations.
>
> Since code is unambiguous, let me just cut and paste what I was
> thinking (be warned, this is a cut-n-paste, so the whitespace is
> probably mangled):
Thank you. Very clear. I don't see the point of
audit_log_user_recv_msg() for reasons already stated but that's ok.
That's a fair comment. I think there is some value in having the
function dedicated for the user messages since they are fairly unique
in that we don't ever want to associate them with the current task,
but it does result in a single use function (which I generally
dislike). Who knows, maybe at some future point in time we'll get rid
of audit_log_user_recv_msg(), but let's stick with it for now.
Since we're looking at these, here are the various field formats
of the
AUDIT_CONFIG_CHANGE records.
Steve and Paul, are they worth attempting to normalize?
Right now, my vote is "no". Although I'm voting that way pretty much
just because I want to get this patch in during this development cycle
and I'm fairly certain that trying to reach any consensus around
normalization is going to drag this out. I would strongly encourage
you to just tweak this patch as we've just talked about and leave it
at that for the time being.
--
paul moore
www.paul-moore.com
6:35 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 22:26, Paul Moore wrote:
On Thu, Jan 17, 2019 at 6:19 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2019-01-17 12:58, Paul Moore wrote:
> > On Thu, Jan 17, 2019 at 10:34 AM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > >
> > > On 2019-01-17 08:21, Paul Moore wrote:
> > > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com>
wrote:
> > > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore
<paul(a)paul-moore.com> wrote:
> > > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > > > Tie syscall information to all CONFIG_CHANGE calls
since they are
> > > > > > > all a result of user actions.
> > > > >
> > > > > Please don't tie syscall information to this. The syscall
will be
> > > > > sendto. We don't need that information, its implicit. Also,
doing this
> > > > > will possibly wreck things in libauparse. Please test the events
with
> > > > > ausearch --format csv and --format text. IFF the event looks
better or
> > > > > the same should we do this. If stuff disappears, the patch is
> > > > > breaking things
> > > >
> > > > We've discussed this quite a bit already; connecting associated
> > > > records into a single event is something that should happen, needs
to
> > > > happen, and will happen. Conceptually it makes no sense to record
the
> > > > syscall (and any other associated records) which triggers the audit
> > > > configuration change, and the configuration change record itself as
> > > > two distinct events - they are the same event. We've also heard
from
> > > > a prominent user that associating records in this way is desirable.
> > > >
> > > > If the ausearch csv and text audit log transformations can't
handle
> > > > this particular change, I would consider that a shortcoming of that
> > > > code. We have multi-record events now, and this is only going to
> > > > increase in the future.
> > > >
> > > > Richard, if you can't make the requested changes to this patch
and
> > > > resubmit by ... let's say the middle of next week? that should
be
> > > > enough time, yes? ... please let me know and I'll make the
changes and
> > > > get this merged.
> > >
> > > I would do the change, which should be very trivial, but I'm dense
> > > enough that I still don't know what you want. In the last 6 months
I've
> > > asked a number of direct questions that have not been directly
> > > addressed. Perhaps I should be able to figure it out from the more
> > > general or fundamental principles replies I've gotten (which have
been
> > > helpful, but perhaps incomplete), but I'm still having some trouble.
> > > Perhaps I'm exposing my limitations.
> >
> > Since code is unambiguous, let me just cut and paste what I was
> > thinking (be warned, this is a cut-n-paste, so the whitespace is
> > probably mangled):
>
> Thank you. Very clear. I don't see the point of
> audit_log_user_recv_msg() for reasons already stated but that's ok.
That's a fair comment. I think there is some value in having the
function dedicated for the user messages since they are fairly unique
in that we don't ever want to associate them with the current task,
but it does result in a single use function (which I generally
dislike). Who knows, maybe at some future point in time we'll get rid
of audit_log_user_recv_msg(), but let's stick with it for now.
> Since we're looking at these, here are the various field formats of the
> AUDIT_CONFIG_CHANGE records.
>
> Steve and Paul, are they worth attempting to normalize?
Right now, my vote is "no". Although I'm voting that way pretty much
just because I want to get this patch in during this development cycle
and I'm fairly certain that trying to reach any consensus around
normalization is going to drag this out. I would strongly encourage
you to just tweak this patch as we've just talked about and leave it
at that for the time being.
Agreed. They are already inconsistent, so this patch isn't going to make
that worse.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)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
10:08 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, 17 Jan 2019 08:21:40 -0500
Paul Moore <paul(a)paul-moore.com> wrote:
On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com>
wrote:
> On Mon, 14 Jan 2019 17:58:58 -0500
> Paul Moore <paul(a)paul-moore.com> wrote:
>
> > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
> > <rgb(a)redhat.com> wrote:
> > >
> > > Tie syscall information to all CONFIG_CHANGE calls since they
> > > are all a result of user actions.
>
> Please don't tie syscall information to this. The syscall will be
> sendto. We don't need that information, its implicit. Also, doing
> this will possibly wreck things in libauparse. Please test the
> events with ausearch --format csv and --format text. IFF the event
> looks better or the same should we do this. If stuff disappears,
> the patch is breaking things
We've discussed this quite a bit already;
Yes, and you still don't seem be listening. You have to cooperate on
modifying events. We as a community need to respect each other's needs
and work together to solve problems. What this is saying sounds a lot
like I don't care if it breaks things, I'm gonna do it my way. Tough
luck.
You do not have to make sense of any of these events. So, what does it
really matter to you how the event is formed? What I'm asking for is
have these changes been vetted to ensure that they are not breaking
things?
connecting associated records into a single event is something that
should happen, needs to happen, and will happen. Conceptually it
makes no sense to record the syscall (and any other associated
records) which triggers the audit configuration change, and the
configuration change record itself as two distinct events - they are
the same event.
Except that they are not. The design of the audit system is to save
disk space as much as possible by emitting single record events on
certain event types that are simple. To add a syscall to it adds useless
information (such as a socket address record), slows down processing,
and wastes disk space. If you get a SYSCALL record, that indicates that
you have triggered an event that the system admin has placed explicit
rules on. That is different than the common criteria required
configuration change event.
We've also heard from a prominent user that
associating records in this way is desirable.
If the ausearch csv and text audit log transformations can't handle
this particular change, I would consider that a shortcoming of that
code. We have multi-record events now, and this is only going to
increase in the future.
Isn't there some kind a guideline about not breaking user space?
-Steve
Richard, if you can't make the requested changes to this patch
and
resubmit by ... let's say the middle of next week? that should be
enough time, yes? ... please let me know and I'll make the changes and
get this merged.
11:36 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 11:08 AM Steve Grubb <sgrubb(a)redhat.com> wrote:
On Thu, 17 Jan 2019 08:21:40 -0500
Paul Moore <paul(a)paul-moore.com> wrote:
> On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com> wrote:
> > On Mon, 14 Jan 2019 17:58:58 -0500
> > Paul Moore <paul(a)paul-moore.com> wrote:
> >
> > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
> > > <rgb(a)redhat.com> wrote:
> > > >
> > > > Tie syscall information to all CONFIG_CHANGE calls since they
> > > > are all a result of user actions.
> >
> > Please don't tie syscall information to this. The syscall will be
> > sendto. We don't need that information, its implicit. Also, doing
> > this will possibly wreck things in libauparse. Please test the
> > events with ausearch --format csv and --format text. IFF the event
> > looks better or the same should we do this. If stuff disappears,
> > the patch is breaking things
>
> We've discussed this quite a bit already;
Yes, and you still don't seem be listening. You have to cooperate on
modifying events. We as a community need to respect each other's needs
and work together to solve problems. What this is saying sounds a lot
like I don't care if it breaks things, I'm gonna do it my way. Tough
luck.
I see you added LKML to the To/CC line. Fun. Unless they've been
following the audit list for the past several years I'm not sure they
have all the context to fully understand some of the things in this
thread, but sure, why not.
I understand you are frustrated Steve, I get it. I can also
understand how you feel that I'm not listening to you because I'm not
agreeing with you on this, I get this too. However, we've talked
about this quite a bit and keeping individual audit records split
across multiple events when they are all part of the same event just
doesn't make sense to me. The users who have commented on this also
agree that associated records should be combined into one event. I
definitely don't want to put words in Richard's mouth, but I believe
he has also said that he believes that associating all related records
into a single event makes sense.
I'm listening to you Steve - look at all the times I've asked for your
perspective when it comes to certification requirements? - I just
don't always agree with you.
You do not have to make sense of any of these events. So, what does
it
really matter to you how the event is formed? What I'm asking for is
have these changes been vetted to ensure that they are not breaking
things?
We are not changing the record formats, all we are doing is changing
the timestamp/event counter so that related records are grouped
together into the same event. For example, in this particular case
(describing this for the LKML crowd that may be reading this) we are
combining the syscall audit record that triggers the audit
configuration change with the audit configuration change record into a
single event. The code currently treats the syscall record and the
audit config change record as separate events - which is just silly as
they belong to the same event, triggered by the same user action -
this patch should fix this by associating the two records so they are
part of the same audit event. From my perspective this is a bug fix.
> connecting associated records into a single event is something
that
> should happen, needs to happen, and will happen. Conceptually it
> makes no sense to record the syscall (and any other associated
> records) which triggers the audit configuration change, and the
> configuration change record itself as two distinct events - they are
> the same event.
Except that they are not. The design of the audit system is to save
disk space as much as possible by emitting single record events on
certain event types that are simple. To add a syscall to it adds useless
information (such as a socket address record), slows down processing,
and wastes disk space. If you get a SYSCALL record, that indicates that
you have triggered an event that the system admin has placed explicit
rules on. That is different than the common criteria required
configuration change event.
I've said this many times in the past, I believe Richard has said the
same too (maybe it was in a GH issue?), but I'll say it again ... if
the system's audit config is such that syscall records are not being
generated, then this patch will not cause them to be generated; all
this patch does is ensure that *if* a syscall record is being
generated *and* an audit config change record is being generated, then
the two records will share the same event counter and treated as a
single event. This change does not cause any change to the amount of
information emitted by the kernel, all it does is group the related
records together.
> We've also heard from a prominent user that
> associating records in this way is desirable.
>
> If the ausearch csv and text audit log transformations can't handle
> this particular change, I would consider that a shortcoming of that
> code. We have multi-record events now, and this is only going to
> increase in the future.
Isn't there some kind a guideline about not breaking user space?
Yes, when that bit of userspace code predates the change. You knew
this was coming when you wrote that tool, writing userspace
applications that make poor assumptions and using that as a way to
block kernel development is not something I look upon favorably.
--
paul moore
www.paul-moore.com
1:26 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 12:36, Paul Moore wrote:
On Thu, Jan 17, 2019 at 11:08 AM Steve Grubb
<sgrubb(a)redhat.com> wrote:
> On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore <paul(a)paul-moore.com> wrote:
> > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com> wrote:
> > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <paul(a)paul-moore.com>
wrote:
> > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > Tie syscall information to all CONFIG_CHANGE calls since they
> > > > > are all a result of user actions.
I see you added LKML to the To/CC line. Fun. Unless they've
been
following the audit list for the past several years I'm not sure they
have all the context to fully understand some of the things in this
thread, but sure, why not.
I added LKML in the original patchset posting, not Steve.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)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
1:32 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On Thu, Jan 17, 2019 at 2:26 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2019-01-17 12:36, Paul Moore wrote:
> On Thu, Jan 17, 2019 at 11:08 AM Steve Grubb <sgrubb(a)redhat.com> wrote:
> > On Thu, 17 Jan 2019 08:21:40 -0500 Paul Moore <paul(a)paul-moore.com>
wrote:
> > > On Thu, Jan 17, 2019 at 4:33 AM Steve Grubb <sgrubb(a)redhat.com>
wrote:
> > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore
<paul(a)paul-moore.com> wrote:
> > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > > Tie syscall information to all CONFIG_CHANGE calls since
they
> > > > > > are all a result of user actions.
> I see you added LKML to the To/CC line. Fun. Unless they've been
> following the audit list for the past several years I'm not sure they
> have all the context to fully understand some of the things in this
> thread, but sure, why not.
I added LKML in the original patchset posting, not Steve.
That you did, my apologies. Why did my mail client highlight that as
new? Oh well, my comments stand regardless; I'm just sorry about the
LKML noise I added to the top.
--
paul moore
www.paul-moore.com
9:05 a.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 10:32, Steve Grubb wrote:
On Mon, 14 Jan 2019 17:58:58 -0500
Paul Moore <paul(a)paul-moore.com> wrote:
> On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com>
> wrote:
> >
> > Tie syscall information to all CONFIG_CHANGE calls since they are
> > all a result of user actions.
Please don't tie syscall information to this. The syscall will be
sendto. We don't need that information, its implicit. Also, doing this
will possibly wreck things in libauparse. Please test the events with
ausearch --format csv and --format text. IFF the event looks better or
the same should we do this. If stuff disappears, the patch is
breaking things
Steve, I hope you aren't talking about the AUDIT_*USER* records, because
this patch intentionally leaves them unassociated with the syscall
record.
The config change records are related.
-Steve
> > Exclude user records from syscall context:
> > Since the function audit_log_common_recv_msg() is shared by a
> > number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_*
> > record types, and since the AUDIT_CONFIG_CHANGE message type has
> > been converted to a syscall accompanied record type, special-case
> > the AUDIT_USER_* range of messages so they remain standalone
> > records.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/59
> > See: https://github.com/linux-audit/audit-kernel/issues/50
> > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > ---
> > kernel/audit.c | 27 +++++++++++++++++++--------
> > kernel/audit_fsnotify.c | 2 +-
> > kernel/audit_tree.c | 2 +-
> > kernel/audit_watch.c | 2 +-
> > kernel/auditfilter.c | 2 +-
> > 5 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 0e8026423fbd..a321fea94cc6 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> > audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
> > }
> >
> > +static inline void audit_log_user_recv_msg(struct audit_buffer
> > **ab, u16 msg_type) +{
> > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > +}
>
> This makes sense because this is used by "user" records ...
>
> > +static inline void audit_log_config_change_alt(struct audit_buffer
> > **ab) +{
> > + audit_log_common_recv_msg(audit_context(), ab,
> > AUDIT_CONFIG_CHANGE); +}
>
> ... and I don't believe this makes sense because there is no real
> logical grouping with the callers like there is for
> audit_log_user_recv_msg().
- RGB
--
Richard Guy Briggs <rgb(a)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
1:21 p.m.
New subject: [PATCH ghak59 V3 2/4] audit: add syscall information to CONFIG_CHANGE records
On 2019-01-17 10:05, Richard Guy Briggs wrote:
On 2019-01-17 10:32, Steve Grubb wrote:
> On Mon, 14 Jan 2019 17:58:58 -0500
> Paul Moore <paul(a)paul-moore.com> wrote:
>
> > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com>
> > wrote:
> > >
> > > Tie syscall information to all CONFIG_CHANGE calls since they are
> > > all a result of user actions.
>
> Please don't tie syscall information to this. The syscall will be
> sendto. We don't need that information, its implicit. Also, doing this
> will possibly wreck things in libauparse. Please test the events with
> ausearch --format csv and --format text. IFF the event looks better or
> the same should we do this. If stuff disappears, the patch is
> breaking things
Steve, I hope you aren't talking about the AUDIT_*USER* records, because
this patch intentionally leaves them unassociated with the syscall
record.
The config change records are related.
The association of all AUDIT_CONFIG_CHANGE records with syscall records
provides subject attributes that are missing from several of the
different AUDIT_CONFIG_CHANGE records.
audit_mark_log_rule_change missing: subj
audit_tree_log_remove_rule missing: auid, ses, subj
audit_watch_log_rule_change missing: subj
audit_seccomp_actions_logged missing: auid, ses, subj
> -Steve
>
> > > Exclude user records from syscall context:
> > > Since the function audit_log_common_recv_msg() is shared by a
> > > number of AUDIT_CONFIG_CHANGE and the entire range of AUDIT_USER_*
> > > record types, and since the AUDIT_CONFIG_CHANGE message type has
> > > been converted to a syscall accompanied record type, special-case
> > > the AUDIT_USER_* range of messages so they remain standalone
> > > records.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/59
> > > See: https://github.com/linux-audit/audit-kernel/issues/50
> > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
> > > ---
> > > kernel/audit.c | 27 +++++++++++++++++++--------
> > > kernel/audit_fsnotify.c | 2 +-
> > > kernel/audit_tree.c | 2 +-
> > > kernel/audit_watch.c | 2 +-
> > > kernel/auditfilter.c | 2 +-
> > > 5 files changed, 23 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > index 0e8026423fbd..a321fea94cc6 100644
> > > --- a/kernel/audit.c
> > > +++ b/kernel/audit.c
> > > @@ -1072,6 +1073,16 @@ static void audit_log_common_recv_msg(struct
> > > audit_buffer **ab, u16 msg_type) audit_log_task_context(*ab);
> > > }
> > >
> > > +static inline void audit_log_user_recv_msg(struct audit_buffer
> > > **ab, u16 msg_type) +{
> > > + audit_log_common_recv_msg(NULL, ab, msg_type);
> > > +}
> >
> > This makes sense because this is used by "user" records ...
> >
> > > +static inline void audit_log_config_change_alt(struct audit_buffer
> > > **ab) +{
> > > + audit_log_common_recv_msg(audit_context(), ab,
> > > AUDIT_CONFIG_CHANGE); +}
> >
> > ... and I don't believe this makes sense because there is no real
> > logical grouping with the callers like there is for
> > audit_log_user_recv_msg().
- RGB
- RGB
--
Richard Guy Briggs <rgb(a)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
4:17 p.m.
New subject: [PATCH ghak59 V3 3/4] audit: hand taken context to audit_kill_trees for syscall logging
Since the context is derived from the task parameter handed to
__audit_free(), hand the context to audit_kill_trees() so it can be used
to associate with a syscall record. This requires adding the context
parameter to kill_rules() rather than using the current audit_context.
The callers of trim_marked() and evict_chunk() still have their context.
The EOE record was being issued prior to the pruning of the killed_tree
list.
Move the kill_trees call before the audit_log_exit call in
__audit_free() and __audit_syscall_exit() so that any pruned trees
CONFIG_CHANGE records are included with the associated syscall event by
the user library due to the EOE record flagging the end of the event.
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.h | 4 ++--
kernel/audit_tree.c | 18 ++++++++++--------
kernel/auditsc.c | 12 ++++++------
3 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index 91421679a168..6ffb70575082 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -314,7 +314,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
extern int audit_tag_tree(char *old, char *new);
extern const char *audit_tree_path(struct audit_tree *tree);
extern void audit_put_tree(struct audit_tree *tree);
-extern void audit_kill_trees(struct list_head *list);
+extern void audit_kill_trees(struct audit_context *context);
#else
#define audit_remove_tree_rule(rule) BUG()
#define audit_add_tree_rule(rule) -EINVAL
@@ -323,7 +323,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
#define audit_put_tree(tree) (void)0
#define audit_tag_tree(old, new) -EINVAL
#define audit_tree_path(rule) "" /* never called */
-#define audit_kill_trees(list) BUG()
+#define audit_kill_trees(context) BUG()
#endif
extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index b0bd59ef4271..bf77d265e68e 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -524,13 +524,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
return 0;
}
-static void audit_tree_log_remove_rule(struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule
*rule)
{
struct audit_buffer *ab;
if (!audit_enabled)
return;
- ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
audit_log_format(ab, "op=remove_rule dir=");
@@ -540,7 +540,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
audit_log_end(ab);
}
-static void kill_rules(struct audit_tree *tree)
+static void kill_rules(struct audit_context *context, struct audit_tree *tree)
{
struct audit_krule *rule, *next;
struct audit_entry *entry;
@@ -551,7 +551,7 @@ static void kill_rules(struct audit_tree *tree)
list_del_init(&rule->rlist);
if (rule->tree) {
/* not a half-baked one */
- audit_tree_log_remove_rule(rule);
+ audit_tree_log_remove_rule(context, rule);
if (entry->rule.exe)
audit_remove_mark(entry->rule.exe);
rule->tree = NULL;
@@ -633,7 +633,7 @@ static void trim_marked(struct audit_tree *tree)
tree->goner = 1;
spin_unlock(&hash_lock);
mutex_lock(&audit_filter_mutex);
- kill_rules(tree);
+ kill_rules(audit_context(), tree);
list_del_init(&tree->list);
mutex_unlock(&audit_filter_mutex);
prune_one(tree);
@@ -973,8 +973,10 @@ static void audit_schedule_prune(void)
* ... and that one is done if evict_chunk() decides to delay until the end
* of syscall. Runs synchronously.
*/
-void audit_kill_trees(struct list_head *list)
+void audit_kill_trees(struct audit_context *context)
{
+ struct list_head *list = &context->killed_trees;
+
audit_ctl_lock();
mutex_lock(&audit_filter_mutex);
@@ -982,7 +984,7 @@ void audit_kill_trees(struct list_head *list)
struct audit_tree *victim;
victim = list_entry(list->next, struct audit_tree, list);
- kill_rules(victim);
+ kill_rules(context, victim);
list_del_init(&victim->list);
mutex_unlock(&audit_filter_mutex);
@@ -1017,7 +1019,7 @@ static void evict_chunk(struct audit_chunk *chunk)
list_del_init(&owner->same_root);
spin_unlock(&hash_lock);
if (!postponed) {
- kill_rules(owner);
+ kill_rules(audit_context(), owner);
list_move(&owner->list, &prune_list);
need_prune = 1;
} else {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6593a5207fb0..b585ceb2f7a2 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1444,6 +1444,9 @@ void __audit_free(struct task_struct *tsk)
if (!context)
return;
+ if (!list_empty(&context->killed_trees))
+ audit_kill_trees(context);
+
/* We are called either by do_exit() or the fork() error handling code;
* in the former case tsk == current and in the latter tsk is a
* random task_struct that doesn't doesn't have any meaningful data we
@@ -1460,9 +1463,6 @@ void __audit_free(struct task_struct *tsk)
audit_log_exit();
}
- if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
-
audit_set_context(tsk, NULL);
audit_free_context(context);
}
@@ -1537,6 +1537,9 @@ void __audit_syscall_exit(int success, long return_code)
if (!context)
return;
+ if (!list_empty(&context->killed_trees))
+ audit_kill_trees(context);
+
if (!context->dummy && context->in_syscall) {
if (success)
context->return_valid = AUDITSC_SUCCESS;
@@ -1571,9 +1574,6 @@ void __audit_syscall_exit(int success, long return_code)
context->in_syscall = 0;
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
- if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
-
audit_free_names(context);
unroll_tree_refs(context, NULL, 0);
audit_free_aux(context);
--
1.8.3.1
5:06 p.m.
New subject: [PATCH ghak59 V3 3/4] audit: hand taken context to audit_kill_trees for syscall logging
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
Since the context is derived from the task parameter handed to
__audit_free(), hand the context to audit_kill_trees() so it can be used
to associate with a syscall record. This requires adding the context
parameter to kill_rules() rather than using the current audit_context.
The callers of trim_marked() and evict_chunk() still have their context.
The EOE record was being issued prior to the pruning of the killed_tree
list.
Move the kill_trees call before the audit_log_exit call in
__audit_free() and __audit_syscall_exit() so that any pruned trees
CONFIG_CHANGE records are included with the associated syscall event by
the user library due to the EOE record flagging the end of the event.
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.h | 4 ++--
kernel/audit_tree.c | 18 ++++++++++--------
kernel/auditsc.c | 12 ++++++------
3 files changed, 18 insertions(+), 16 deletions(-)
Merged.
diff --git a/kernel/audit.h b/kernel/audit.h
index 91421679a168..6ffb70575082 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -314,7 +314,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
extern int audit_tag_tree(char *old, char *new);
extern const char *audit_tree_path(struct audit_tree *tree);
extern void audit_put_tree(struct audit_tree *tree);
-extern void audit_kill_trees(struct list_head *list);
+extern void audit_kill_trees(struct audit_context *context);
#else
#define audit_remove_tree_rule(rule) BUG()
#define audit_add_tree_rule(rule) -EINVAL
@@ -323,7 +323,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
#define audit_put_tree(tree) (void)0
#define audit_tag_tree(old, new) -EINVAL
#define audit_tree_path(rule) "" /* never called */
-#define audit_kill_trees(list) BUG()
+#define audit_kill_trees(context) BUG()
#endif
extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index b0bd59ef4271..bf77d265e68e 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -524,13 +524,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
return 0;
}
-static void audit_tree_log_remove_rule(struct audit_krule *rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule
*rule)
{
struct audit_buffer *ab;
if (!audit_enabled)
return;
- ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
audit_log_format(ab, "op=remove_rule dir=");
@@ -540,7 +540,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule)
audit_log_end(ab);
}
-static void kill_rules(struct audit_tree *tree)
+static void kill_rules(struct audit_context *context, struct audit_tree *tree)
{
struct audit_krule *rule, *next;
struct audit_entry *entry;
@@ -551,7 +551,7 @@ static void kill_rules(struct audit_tree *tree)
list_del_init(&rule->rlist);
if (rule->tree) {
/* not a half-baked one */
- audit_tree_log_remove_rule(rule);
+ audit_tree_log_remove_rule(context, rule);
if (entry->rule.exe)
audit_remove_mark(entry->rule.exe);
rule->tree = NULL;
@@ -633,7 +633,7 @@ static void trim_marked(struct audit_tree *tree)
tree->goner = 1;
spin_unlock(&hash_lock);
mutex_lock(&audit_filter_mutex);
- kill_rules(tree);
+ kill_rules(audit_context(), tree);
list_del_init(&tree->list);
mutex_unlock(&audit_filter_mutex);
prune_one(tree);
@@ -973,8 +973,10 @@ static void audit_schedule_prune(void)
* ... and that one is done if evict_chunk() decides to delay until the end
* of syscall. Runs synchronously.
*/
-void audit_kill_trees(struct list_head *list)
+void audit_kill_trees(struct audit_context *context)
{
+ struct list_head *list = &context->killed_trees;
+
audit_ctl_lock();
mutex_lock(&audit_filter_mutex);
@@ -982,7 +984,7 @@ void audit_kill_trees(struct list_head *list)
struct audit_tree *victim;
victim = list_entry(list->next, struct audit_tree, list);
- kill_rules(victim);
+ kill_rules(context, victim);
list_del_init(&victim->list);
mutex_unlock(&audit_filter_mutex);
@@ -1017,7 +1019,7 @@ static void evict_chunk(struct audit_chunk *chunk)
list_del_init(&owner->same_root);
spin_unlock(&hash_lock);
if (!postponed) {
- kill_rules(owner);
+ kill_rules(audit_context(), owner);
list_move(&owner->list, &prune_list);
need_prune = 1;
} else {
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6593a5207fb0..b585ceb2f7a2 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1444,6 +1444,9 @@ void __audit_free(struct task_struct *tsk)
if (!context)
return;
+ if (!list_empty(&context->killed_trees))
+ audit_kill_trees(context);
+
/* We are called either by do_exit() or the fork() error handling code;
* in the former case tsk == current and in the latter tsk is a
* random task_struct that doesn't doesn't have any meaningful data we
@@ -1460,9 +1463,6 @@ void __audit_free(struct task_struct *tsk)
audit_log_exit();
}
- if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
-
audit_set_context(tsk, NULL);
audit_free_context(context);
}
@@ -1537,6 +1537,9 @@ void __audit_syscall_exit(int success, long return_code)
if (!context)
return;
+ if (!list_empty(&context->killed_trees))
+ audit_kill_trees(context);
+
if (!context->dummy && context->in_syscall) {
if (success)
context->return_valid = AUDITSC_SUCCESS;
@@ -1571,9 +1574,6 @@ void __audit_syscall_exit(int success, long return_code)
context->in_syscall = 0;
context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0;
- if (!list_empty(&context->killed_trees))
- audit_kill_trees(&context->killed_trees);
-
audit_free_names(context);
unroll_tree_refs(context, NULL, 0);
audit_free_aux(context);
--
1.8.3.1
--
paul moore
www.paul-moore.com
4:17 p.m.
New subject: [PATCH ghak59 V3 4/4] audit: extend config_change mark/watch/tree rule changes
Give a clue as to the source of mark, watch and tree rule changes.
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.h | 4 ++--
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 24 ++++++++++++------------
kernel/audit_watch.c | 6 ++++--
kernel/auditsc.c | 4 ++--
5 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/kernel/audit.h b/kernel/audit.h
index 6ffb70575082..545dd8fcb036 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -314,7 +314,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
extern int audit_tag_tree(char *old, char *new);
extern const char *audit_tree_path(struct audit_tree *tree);
extern void audit_put_tree(struct audit_tree *tree);
-extern void audit_kill_trees(struct audit_context *context);
+extern void audit_kill_trees(struct audit_context *context, char *trig);
#else
#define audit_remove_tree_rule(rule) BUG()
#define audit_add_tree_rule(rule) -EINVAL
@@ -323,7 +323,7 @@ extern void audit_log_d_path_exe(struct audit_buffer *ab,
#define audit_put_tree(tree) (void)0
#define audit_tag_tree(old, new) -EINVAL
#define audit_tree_path(rule) "" /* never called */
-#define audit_kill_trees(context) BUG()
+#define audit_kill_trees(context, trig) BUG()
#endif
extern char *audit_unpack_string(void **bufp, size_t *remain, size_t len);
diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
index 37ae95cfb7f4..d25cd3760b5d 100644
--- a/kernel/audit_fsnotify.c
+++ b/kernel/audit_fsnotify.c
@@ -156,7 +156,7 @@ static void audit_autoremove_mark_rule(struct audit_fsnotify_mark
*audit_mark)
struct audit_krule *rule = audit_mark->rule;
struct audit_entry *entry = container_of(rule, struct audit_entry, rule);
- audit_mark_log_rule_change(audit_mark, "autoremove_rule");
+ audit_mark_log_rule_change(audit_mark, "autoremove_rule:mark");
audit_del_rule(entry);
}
diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
index bf77d265e68e..160169fa45a8 100644
--- a/kernel/audit_tree.c
+++ b/kernel/audit_tree.c
@@ -524,7 +524,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree)
return 0;
}
-static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule
*rule)
+static void audit_tree_log_remove_rule(struct audit_context *context, struct audit_krule
*rule, char *trig)
{
struct audit_buffer *ab;
@@ -533,14 +533,14 @@ static void audit_tree_log_remove_rule(struct audit_context
*context, struct aud
ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
if (unlikely(!ab))
return;
- audit_log_format(ab, "op=remove_rule dir=");
+ audit_log_format(ab, "op=remove_rule:tree:%s dir=", trig);
audit_log_untrustedstring(ab, rule->tree->pathname);
audit_log_key(ab, rule->filterkey);
audit_log_format(ab, " list=%d res=1", rule->listnr);
audit_log_end(ab);
}
-static void kill_rules(struct audit_context *context, struct audit_tree *tree)
+static void kill_rules(struct audit_context *context, struct audit_tree *tree, char
*trig)
{
struct audit_krule *rule, *next;
struct audit_entry *entry;
@@ -551,7 +551,7 @@ static void kill_rules(struct audit_context *context, struct
audit_tree *tree)
list_del_init(&rule->rlist);
if (rule->tree) {
/* not a half-baked one */
- audit_tree_log_remove_rule(context, rule);
+ audit_tree_log_remove_rule(context, rule, trig);
if (entry->rule.exe)
audit_remove_mark(entry->rule.exe);
rule->tree = NULL;
@@ -607,7 +607,7 @@ static void prune_one(struct audit_tree *victim)
/* trim the uncommitted chunks from tree */
-static void trim_marked(struct audit_tree *tree)
+static void trim_marked(struct audit_tree *tree, char *trig)
{
struct list_head *p, *q;
spin_lock(&hash_lock);
@@ -633,7 +633,7 @@ static void trim_marked(struct audit_tree *tree)
tree->goner = 1;
spin_unlock(&hash_lock);
mutex_lock(&audit_filter_mutex);
- kill_rules(audit_context(), tree);
+ kill_rules(audit_context(), tree, trig);
list_del_init(&tree->list);
mutex_unlock(&audit_filter_mutex);
prune_one(tree);
@@ -714,7 +714,7 @@ void audit_trim_trees(void)
node->index &= ~(1U<<31);
}
spin_unlock(&hash_lock);
- trim_marked(tree);
+ trim_marked(tree, "trim");
drop_collected_mounts(root_mnt);
skip_it:
put_tree(tree);
@@ -847,7 +847,7 @@ int audit_add_tree_rule(struct audit_krule *rule)
node->index &= ~(1U<<31);
spin_unlock(&hash_lock);
} else {
- trim_marked(tree);
+ trim_marked(tree, "add");
goto Err;
}
@@ -949,7 +949,7 @@ int audit_tag_tree(char *old, char *new)
node->index &= ~(1U<<31);
spin_unlock(&hash_lock);
} else {
- trim_marked(tree);
+ trim_marked(tree, "equiv");
}
put_tree(tree);
@@ -973,7 +973,7 @@ static void audit_schedule_prune(void)
* ... and that one is done if evict_chunk() decides to delay until the end
* of syscall. Runs synchronously.
*/
-void audit_kill_trees(struct audit_context *context)
+void audit_kill_trees(struct audit_context *context, char *trig)
{
struct list_head *list = &context->killed_trees;
@@ -984,7 +984,7 @@ void audit_kill_trees(struct audit_context *context)
struct audit_tree *victim;
victim = list_entry(list->next, struct audit_tree, list);
- kill_rules(context, victim);
+ kill_rules(context, victim, trig);
list_del_init(&victim->list);
mutex_unlock(&audit_filter_mutex);
@@ -1019,7 +1019,7 @@ static void evict_chunk(struct audit_chunk *chunk)
list_del_init(&owner->same_root);
spin_unlock(&hash_lock);
if (!postponed) {
- kill_rules(audit_context(), owner);
+ kill_rules(audit_context(), owner, "evict");
list_move(&owner->list, &prune_list);
need_prune = 1;
} else {
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index e8d1adeb2223..f8a5770c6c8c 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -315,7 +315,9 @@ static void audit_update_watch(struct audit_parent *parent,
if (oentry->rule.exe)
audit_remove_mark(oentry->rule.exe);
- audit_watch_log_rule_change(r, owatch, "updated_rules");
+ audit_watch_log_rule_change(r, owatch, invalidating ?
+ "updated_rules:watch:inval" :
+ "updated_rules:watch:set");
call_rcu(&oentry->rcu, audit_free_rule_rcu);
}
@@ -343,7 +345,7 @@ static void audit_remove_parent_watches(struct audit_parent *parent)
list_for_each_entry_safe(w, nextw, &parent->watches, wlist) {
list_for_each_entry_safe(r, nextr, &w->rules, rlist) {
e = container_of(r, struct audit_entry, rule);
- audit_watch_log_rule_change(r, w, "remove_rule");
+ audit_watch_log_rule_change(r, w, "remove_rule:watch:parent");
if (e->rule.exe)
audit_remove_mark(e->rule.exe);
list_del(&r->rlist);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b585ceb2f7a2..8592448f1d8f 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1445,7 +1445,7 @@ void __audit_free(struct task_struct *tsk)
return;
if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
+ audit_kill_trees(context, "free");
/* We are called either by do_exit() or the fork() error handling code;
* in the former case tsk == current and in the latter tsk is a
@@ -1538,7 +1538,7 @@ void __audit_syscall_exit(int success, long return_code)
return;
if (!list_empty(&context->killed_trees))
- audit_kill_trees(context);
+ audit_kill_trees(context, "exit");
if (!context->dummy && context->in_syscall) {
if (success)
--
1.8.3.1
5:16 p.m.
New subject: [PATCH ghak59 V3 4/4] audit: extend config_change mark/watch/tree rule changes
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
Give a clue as to the source of mark, watch and tree rule changes.
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
---
kernel/audit.h | 4 ++--
kernel/audit_fsnotify.c | 2 +-
kernel/audit_tree.c | 24 ++++++++++++------------
kernel/audit_watch.c | 6 ++++--
kernel/auditsc.c | 4 ++--
5 files changed, 21 insertions(+), 19 deletions(-)
In the previous revision of this patchset I mentioned not wanting to
change right now, I stand by that in this patchset.
--
paul moore
www.paul-moore.com
4:31 p.m.
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
Make a number of changes to normalize CONFIG_CHANGE records by
adding
missing op= fields, providing more information in existing op fields
(optional last patch) and connecting all records to existing audit
events. The user record needs special-casing since its content isn't
directly related to the call that logs it.
Since tree purge records are processed after the EOE record is produced,
the order of operation of the EOE record and the purge will have to be
reversed so that the purge records can be included in the event.
The last patch is included for completeness understanding it may be more
information than necessary.
For reference, here are the calling methods and function tree for all
CONFIG_CHANGE events with fields:
- audit_log_config_change()
- add "op=set" to fields: "[op] <param-name> old auid ses
subj res"
- AUDIT_SET:AUDIT_STATUS_PID
- AUDIT_SET:AUDIT_STATUS_LOST
- audit_do_config_change()
- AUDIT_SET:AUDIT_STATUS_FAILURE
- AUDIT_SET:AUDIT_STATUS_ENABLED
- AUDIT_SET:AUDIT_STATUS_RATE_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_LIMIT
- AUDIT_SET:AUDIT_STATUS_BACKLOG_WAIT_TIME
- audit_log_rule_change()
- fields: "auid ses subj op key list res"
- AUDIT_ADD_RULE -F dir=...
- AUDIT_DEL_RULE -F dir=...
- audit_log_common_recv_msg()
- fields: "pid uid auid ses subj ..."
- AUDIT_*USER* events (not CONFIG_CHANGE like all the rest)
- AUDIT_LOCKED add "op={add,remove}_rule" to "[op] audit_enabled
res"
- AUDIT_TRIM "op=trim res"
- AUDIT_MAKE_EQUIV: "op=make_equiv old new res"
- AUDIT_TTY_SET: "op=tty_set old-enabled new-enabled old-log_passwd
new-log_passwd res"
- audit_mark_log_rule_change()
- add ":mark" to op in fields: "uid ses op=autoremove_rule[] path
key list res"
- audit_autoremove_mark_rule()
- audit_mark_handle_event()
- audit_mark_fsnotify_ops.handle_event
- audit_tree_log_remove_rule() called from kill_rules()
- add to op ":tree:%s" to fields: "op=remove_rule[] dir key list
res"
- from trim_marked()
- AUDIT_TRIM: audit_trim_trees() "trim"
- audit_add_tree_rule() iterate_mounts err "add"
- audit_add_rule()
- audit_rule_change()
- AUDIT_ADD_RULE -F dir=...
- AUDIT_MAKE_EQUIV: audit_tag_tree() iterate_mounts err
"equiv"
- from audit_kill_trees()
- __audit_free() "free"
- do_exit()
- copy_process() err
- __audit_syscall_exit() "exit"
- from evict_chunk() "evict"
- audit_tree_freeing_mark()
- audit_tree_ops.freeing_mark
- audit_watch_log_rule_change()
add to op ":watch:%s" to fields "auid ses
op={updated,remove}_rule[] path key list res"
- audit_update_watch() "updated_rules:watch:inval" :
"updated_rules:watch:set"
- audit_watch_handle_event() FS_CREATE|FS_MOVED_TO,
FS_DELETE|FS_MOVED_FROM
- audit_watch_fsnotify_ops.handle_event
- audit_remove_parent_watches() "remove_rule:watch:parent"
- audit_watch_handle_event() FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF
- audit_watch_fsnotify_ops.handle_event
- audit_seccomp_actions_logged()
- fields: "op actions old-actions res"
See: https://github.com/linux-audit/audit-kernel/issues/50
See: https://github.com/linux-audit/audit-kernel/issues/59
Sources of AUDIT_CONFIG_CHANGE records and their current and proposed
fields are listed here
https://github.com/linux-audit/audit-kernel/issues/59#issuecomment-445055154
Changelog:
v3:
- un-clever %s_rule to not break up op values
- create audit_log_user_recv_msg() and squash into record connection
- squash kill_trees context handling with kill-trees before EOE
- rebase on audit/next (v4.20-rc1) with 2a1fe215e730 ("audit: use current whenever
possible")
- remove parens in extended format
v2:
- re-order audit_log_exit() and audit_kill_trees()
- drop EOE reordering patch
- rebase on 4.18-rc1 (audit/next)
Richard Guy Briggs (4):
audit: give a clue what CONFIG_CHANGE op was involved
audit: add syscall information to CONFIG_CHANGE records
audit: hand taken context to audit_kill_trees for syscall logging
audit: extend config_change mark/watch/tree rule changes
kernel/audit.c | 33 +++++++++++++++++++++++----------
kernel/audit.h | 4 ++--
kernel/audit_fsnotify.c | 4 ++--
kernel/audit_tree.c | 28 +++++++++++++++-------------
kernel/audit_watch.c | 8 +++++---
kernel/auditfilter.c | 2 +-
kernel/auditsc.c | 12 ++++++------
7 files changed, 54 insertions(+), 37 deletions(-)
In order to make sure expectations are set appropriately, as we are at
-rc6 right now this is not something that would go into audit/next now
(assuming everything looks okay on review), it would go into
audit/next *after* the upcoming merge window.
--
paul moore
www.paul-moore.com
4:41 p.m.
On 2018-12-11 17:31, Paul Moore wrote:
On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> Make a number of changes to normalize CONFIG_CHANGE records by adding
> missing op= fields, providing more information in existing op fields
> (optional last patch) and connecting all records to existing audit
> events. The user record needs special-casing since its content isn't
> directly related to the call that logs it.
>
> Since tree purge records are processed after the EOE record is produced,
> the order of operation of the EOE record and the purge will have to be
> reversed so that the purge records can be included in the event.
>
> The last patch is included for completeness understanding it may be more
> information than necessary.
>
> For reference, here are the calling methods and function tree for all
> CONFIG_CHANGE events with fields:
> - audit_log_config_change()
> - add "op=set" to fields: "[op] <param-name> old auid
ses subj res"
> - AUDIT_SET:AUDIT_STATUS_PID
> - AUDIT_SET:AUDIT_STATUS_LOST
> - audit_do_config_change()
> - AUDIT_SET:AUDIT_STATUS_FAILURE
> - AUDIT_SET:AUDIT_STATUS_ENABLED
> - AUDIT_SET:AUDIT_STATUS_RATE_LIMIT
> - AUDIT_SET:AUDIT_STATUS_BACKLOG_LIMIT
> - AUDIT_SET:AUDIT_STATUS_BACKLOG_WAIT_TIME
> - audit_log_rule_change()
> - fields: "auid ses subj op key list res"
> - AUDIT_ADD_RULE -F dir=...
> - AUDIT_DEL_RULE -F dir=...
> - audit_log_common_recv_msg()
> - fields: "pid uid auid ses subj ..."
> - AUDIT_*USER* events (not CONFIG_CHANGE like all the rest)
> - AUDIT_LOCKED add "op={add,remove}_rule" to "[op]
audit_enabled res"
> - AUDIT_TRIM "op=trim res"
> - AUDIT_MAKE_EQUIV: "op=make_equiv old new res"
> - AUDIT_TTY_SET: "op=tty_set old-enabled new-enabled old-log_passwd
new-log_passwd res"
> - audit_mark_log_rule_change()
> - add ":mark" to op in fields: "uid ses op=autoremove_rule[]
path key list res"
> - audit_autoremove_mark_rule()
> - audit_mark_handle_event()
> - audit_mark_fsnotify_ops.handle_event
> - audit_tree_log_remove_rule() called from kill_rules()
> - add to op ":tree:%s" to fields: "op=remove_rule[] dir key
list res"
> - from trim_marked()
> - AUDIT_TRIM: audit_trim_trees() "trim"
> - audit_add_tree_rule() iterate_mounts err "add"
> - audit_add_rule()
> - audit_rule_change()
> - AUDIT_ADD_RULE -F dir=...
> - AUDIT_MAKE_EQUIV: audit_tag_tree() iterate_mounts err
"equiv"
> - from audit_kill_trees()
> - __audit_free() "free"
> - do_exit()
> - copy_process() err
> - __audit_syscall_exit() "exit"
> - from evict_chunk() "evict"
> - audit_tree_freeing_mark()
> - audit_tree_ops.freeing_mark
> - audit_watch_log_rule_change()
> add to op ":watch:%s" to fields "auid ses
op={updated,remove}_rule[] path key list res"
> - audit_update_watch() "updated_rules:watch:inval" :
"updated_rules:watch:set"
> - audit_watch_handle_event() FS_CREATE|FS_MOVED_TO,
FS_DELETE|FS_MOVED_FROM
> - audit_watch_fsnotify_ops.handle_event
> - audit_remove_parent_watches() "remove_rule:watch:parent"
> - audit_watch_handle_event() FS_DELETE_SELF|FS_UNMOUNT|FS_MOVE_SELF
> - audit_watch_fsnotify_ops.handle_event
> - audit_seccomp_actions_logged()
> - fields: "op actions old-actions res"
>
>
> See: https://github.com/linux-audit/audit-kernel/issues/50
> See: https://github.com/linux-audit/audit-kernel/issues/59
>
> Sources of AUDIT_CONFIG_CHANGE records and their current and proposed
> fields are listed here
>
https://github.com/linux-audit/audit-kernel/issues/59#issuecomment-445055154
>
> Changelog:
> v3:
> - un-clever %s_rule to not break up op values
> - create audit_log_user_recv_msg() and squash into record connection
> - squash kill_trees context handling with kill-trees before EOE
> - rebase on audit/next (v4.20-rc1) with 2a1fe215e730 ("audit: use current
whenever possible")
> - remove parens in extended format
>
> v2:
> - re-order audit_log_exit() and audit_kill_trees()
> - drop EOE reordering patch
> - rebase on 4.18-rc1 (audit/next)
>
> Richard Guy Briggs (4):
> audit: give a clue what CONFIG_CHANGE op was involved
> audit: add syscall information to CONFIG_CHANGE records
> audit: hand taken context to audit_kill_trees for syscall logging
> audit: extend config_change mark/watch/tree rule changes
>
> kernel/audit.c | 33 +++++++++++++++++++++++----------
> kernel/audit.h | 4 ++--
> kernel/audit_fsnotify.c | 4 ++--
> kernel/audit_tree.c | 28 +++++++++++++++-------------
> kernel/audit_watch.c | 8 +++++---
> kernel/auditfilter.c | 2 +-
> kernel/auditsc.c | 12 ++++++------
> 7 files changed, 54 insertions(+), 37 deletions(-)
In order to make sure expectations are set appropriately, as we are at
-rc6 right now this is not something that would go into audit/next now
(assuming everything looks okay on review), it would go into
audit/next *after* the upcoming merge window.
I agree it is a bit late for this. I wasn't expecting it to go in this
one. I'm filling the queue since I'm blocked on other review for
ghak81(5.5wks), ghak90(5.5wks), ghak100(3.5wks). ghak90 missed another
merge window.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)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
5:26 p.m.
On Tue, Dec 11, 2018 at 5:41 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2018-12-11 17:31, Paul Moore wrote:
> On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
...
> > Richard Guy Briggs (4):
> > audit: give a clue what CONFIG_CHANGE op was involved
> > audit: add syscall information to CONFIG_CHANGE records
> > audit: hand taken context to audit_kill_trees for syscall logging
> > audit: extend config_change mark/watch/tree rule changes
> >
> > kernel/audit.c | 33 +++++++++++++++++++++++----------
> > kernel/audit.h | 4 ++--
> > kernel/audit_fsnotify.c | 4 ++--
> > kernel/audit_tree.c | 28 +++++++++++++++-------------
> > kernel/audit_watch.c | 8 +++++---
> > kernel/auditfilter.c | 2 +-
> > kernel/auditsc.c | 12 ++++++------
> > 7 files changed, 54 insertions(+), 37 deletions(-)
>
> In order to make sure expectations are set appropriately, as we are at
> -rc6 right now this is not something that would go into audit/next now
> (assuming everything looks okay on review), it would go into
> audit/next *after* the upcoming merge window.
I agree it is a bit late for this. I wasn't expecting it to go in this
one. I'm filling the queue since I'm blocked on other review for
ghak81(5.5wks), ghak90(5.5wks), ghak100(3.5wks). ghak90 missed another
merge window.
As discussed previously, GHAK81
(https://github.com/linux-audit/audit-kernel/issues/81) is something
that I consider part of the audit container ID work (GHAK90). I
believe it's time to stop treating it as a separate issue.
The audit container ID work, GHAK90
(https://github.com/linux-audit/audit-kernel/issues/90), is where all
the dragon's lie. That one takes a good deal of time to review, and
quite frankly I'm really the only one who seems to be looking at it
anymore, so it takes a bit longer.
Beside the fact that GHAK100
(https://github.com/linux-audit/audit-kernel/issues/100) was marked as
a RFC, I've been waiting to hear back from the VFS folks if they are
comfortable with it. Miklos Szeredi in particular had some concerns
and it isn't clear to me from that thread that his concerns have been
resolved.
--
paul moore
www.paul-moore.com
8:45 p.m.
On 2018-12-11 18:26, Paul Moore wrote:
On Tue, Dec 11, 2018 at 5:41 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2018-12-11 17:31, Paul Moore wrote:
> > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
...
> > > Richard Guy Briggs (4):
> > > audit: give a clue what CONFIG_CHANGE op was involved
> > > audit: add syscall information to CONFIG_CHANGE records
> > > audit: hand taken context to audit_kill_trees for syscall logging
> > > audit: extend config_change mark/watch/tree rule changes
> > >
> > > kernel/audit.c | 33 +++++++++++++++++++++++----------
> > > kernel/audit.h | 4 ++--
> > > kernel/audit_fsnotify.c | 4 ++--
> > > kernel/audit_tree.c | 28 +++++++++++++++-------------
> > > kernel/audit_watch.c | 8 +++++---
> > > kernel/auditfilter.c | 2 +-
> > > kernel/auditsc.c | 12 ++++++------
> > > 7 files changed, 54 insertions(+), 37 deletions(-)
> >
> > In order to make sure expectations are set appropriately, as we are at
> > -rc6 right now this is not something that would go into audit/next now
> > (assuming everything looks okay on review), it would go into
> > audit/next *after* the upcoming merge window.
>
> I agree it is a bit late for this. I wasn't expecting it to go in this
> one. I'm filling the queue since I'm blocked on other review for
> ghak81(5.5wks), ghak90(5.5wks), ghak100(3.5wks). ghak90 missed another
> merge window.
As discussed previously, GHAK81
(https://github.com/linux-audit/audit-kernel/issues/81) is something
that I consider part of the audit container ID work (GHAK90). I
believe it's time to stop treating it as a separate issue.
Fine by me. It was included in the ghak90 patchset this time and still
is in v5, waiting to get the questions replied to that arose out of the
review of v4 around Hallowe'en.
The audit container ID work, GHAK90
(https://github.com/linux-audit/audit-kernel/issues/90), is where all
the dragon's lie. That one takes a good deal of time to review, and
quite frankly I'm really the only one who seems to be looking at it
anymore, so it takes a bit longer.
We're working on finding other reviewers.
Beside the fact that GHAK100
(https://github.com/linux-audit/audit-kernel/issues/100) was marked as
a RFC, I've been waiting to hear back from the VFS folks if they are
comfortable with it. Miklos Szeredi in particular had some concerns
and it isn't clear to me from that thread that his concerns have been
resolved.
I'm fine with Miklos' concerns and have ideas to address them. I'd be
quite interested in your quick review to see if it is headed in the
right direction and I'm also hoping for opinions from you and the vfs
guys on Steve's question.
paul moore
- RGB
--
Richard Guy Briggs <rgb(a)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
6:57 a.m.
On Tue, Dec 11, 2018 at 9:45 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2018-12-11 18:26, Paul Moore wrote:
> On Tue, Dec 11, 2018 at 5:41 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2018-12-11 17:31, Paul Moore wrote:
> > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
>
> ...
>
> > > > Richard Guy Briggs (4):
> > > > audit: give a clue what CONFIG_CHANGE op was involved
> > > > audit: add syscall information to CONFIG_CHANGE records
> > > > audit: hand taken context to audit_kill_trees for syscall logging
> > > > audit: extend config_change mark/watch/tree rule changes
> > > >
> > > > kernel/audit.c | 33 +++++++++++++++++++++++----------
> > > > kernel/audit.h | 4 ++--
> > > > kernel/audit_fsnotify.c | 4 ++--
> > > > kernel/audit_tree.c | 28 +++++++++++++++-------------
> > > > kernel/audit_watch.c | 8 +++++---
> > > > kernel/auditfilter.c | 2 +-
> > > > kernel/auditsc.c | 12 ++++++------
> > > > 7 files changed, 54 insertions(+), 37 deletions(-)
> > >
> > > In order to make sure expectations are set appropriately, as we are at
> > > -rc6 right now this is not something that would go into audit/next now
> > > (assuming everything looks okay on review), it would go into
> > > audit/next *after* the upcoming merge window.
> >
> > I agree it is a bit late for this. I wasn't expecting it to go in this
> > one. I'm filling the queue since I'm blocked on other review for
> > ghak81(5.5wks), ghak90(5.5wks), ghak100(3.5wks). ghak90 missed another
> > merge window.
>
> As discussed previously, GHAK81
> (https://github.com/linux-audit/audit-kernel/issues/81) is something
> that I consider part of the audit container ID work (GHAK90). I
> believe it's time to stop treating it as a separate issue.
Fine by me. It was included in the ghak90 patchset this time and still
is in v5, waiting to get the questions replied to that arose out of the
review of v4 around Hallowe'en.
If you knew ([1]) I didn't want GHAK81 treated as a separate issue,
but instead included as part of GHAK90, why did you bother separating
it out in your latest nag emails?
[1]I didn't feel like digging through my sent mail to find out when we
discussed this last so I could include a passive aggressive date, that
exercise is left to the reader. I'm sure you'll understand.
--
paul moore
www.paul-moore.com
2173
days inactive
2212
days old
linux-audit@lists.linux-audit.osci.io
28 comments
3 participants
participants (3)
-
Paul Moore -
Richard Guy Briggs -
Steve Grubb