Re: [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&gt; 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&gt; wrote: > > > On Mon, 14 Jan 2019 17:58:58 -0500 Paul Moore <paul(a)paul-moore.com&gt; wrote: > > > > On Mon, Dec 10, 2018 at 5:18 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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