From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Grubb Subject: Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event Date: Thu, 12 Oct 2017 18:13:28 -0400 Message-ID: <2420682.maDfsdr0Ad@x2> References: <13849202.1oBXyD8xfy@x2> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Paul Moore Cc: Linux Audit List-Id: linux-audit@redhat.com On Thursday, October 12, 2017 5:04:41 PM EDT Paul Moore wrote: > On Thu, Oct 12, 2017 at 3:57 PM, Steve Grubb wrote: > > There are very important fields necessary to understand who is adding > > audit rules and a little more context about the environment in which > > its happening. This adds pid, uid, tty, subj, comm, and exe > > information to the event. These are required fields. > > > > Signed-off-by: sgrubb > > --- > > > > kernel/audit_watch.c | 23 +++++++++++++++++++---- > > kernel/auditfilter.c | 18 +++++++++++++++--- > > 2 files changed, 34 insertions(+), 7 deletions(-) > > > > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c > > index 9eb8b3511636..63abc2ba1372 100644 > > --- a/kernel/audit_watch.c > > +++ b/kernel/audit_watch.c > > @@ -239,14 +239,29 @@ static struct audit_watch *audit_dupe_watch(struct > > audit_watch *old)> > > static void audit_watch_log_rule_change(struct audit_krule *r, struct > > audit_watch *w, char *op) { > > > > if (audit_enabled) { > > > > + struct tty_struct *tty; > > + const struct cred *cred; > > > > struct audit_buffer *ab; > > > > + char comm[sizeof(current->comm)]; > > + > > > > ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE); > > if (unlikely(!ab)) > > > > return; > > > > - audit_log_format(ab, "auid=%u ses=%u op=%s", > > - from_kuid(&init_user_ns, > > audit_get_loginuid(current)), - > > audit_get_sessionid(current), op); - audit_log_format(ab, " > > path="); > > + > > + tty = audit_get_tty(current); > > + cred = current_cred(); > > + audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s > > ses=%u", > > + task_tgid_nr(current), > > + from_kuid(&init_user_ns, cred->uid), > > + from_kuid(&init_user_ns, > > + audit_get_loginuid(current)), > > + tty ? tty_name(tty) : "(none)", > > + audit_get_sessionid(current)); > > Another reminder that in general I'm not going to accept patches that > shuffle the fields or insert fields in the middle of a record; if you > want to add new fields to a record, add them at the end. I see no > reason to break with the rule for this patch. This has never been a rule. There are times I've suggested adding things at the end because I looked at the parsers and saw that was the best solution. But that is an informed decision based on looking at the code. Besides, there are 9 places where AUDIT_CONFIG_CHANGE is logged and we'll have to parse it 9 different ways if we simply add things at the end. That said, I did some testing. Here's a sample event: type=CONFIG_CHANGE msg=audit(1507836246.134:98): pid=576 uid=0 auid=4294967295 tty=(none) ses=4294967295 subj=system_u:system_r:unconfined_service_t:s0 comm="auditctl" exe="/usr/sbin/auditctl" op=add_rule key="modules" list=4 res=1 and a current event: type=CONFIG_CHANGE msg=audit(1507827262.547:6): audit_enabled=1 old=1 auid=4294967295 ses=4294967295 subj=system_u:system_r:unconfined_service_t:s0 op=add_rule key="sched" list=4 res=1 So, the old event has auid, session, and subj. Testing ausearch with those fields on the new event yields this: [root@f26-audit ~]# ausearch --start boot -m config_change --loginuid 4294967295 >/dev/null [root@f26-audit ~]# echo $? 0 [root@f26-audit ~]# ausearch --start boot -m config_change --session 4294967295 >/dev/null [root@f26-audit ~]# echo $? 0 [root@f26-audit ~]# ausearch --start boot -m config_change --subject service_t >/dev/null [root@f26-audit ~]# echo $? 0 So, ausearch still finds all the fields its supposed to. Does it find anything it doesn't know about? [root@f26-audit ~]# ausearch --start boot -m config_change -ui 0 >/dev/null So, a current or older ausearch is not harmed by any of these changes. It maintains the exact same behavior. The only time we have a problem is when there are changes introduced that are not coordinated or tested. This has been tested. This patch closes the last big hole that the auparse_normalizer sees on boot. -Steve > > + audit_log_task_context(ab); > > + audit_log_format(ab, " comm="); > > + audit_log_untrustedstring(ab, get_task_comm(comm, > > current)); + audit_log_d_path_exe(ab, current->mm); > > + audit_log_format(ab, "op=%s path=", op); > > > > audit_log_untrustedstring(ab, w->path); > > audit_log_key(ab, r->filterkey); > > audit_log_format(ab, " list=%d res=1", r->listnr); > > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > index 0b0aa5854dac..5e2a953da29a 100644 > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c > > @@ -1065,17 +1065,29 @@ static void audit_list_rules(int seq, struct > > sk_buff_head *q)> > > static void audit_log_rule_change(char *action, struct audit_krule *rule, > > int res) { > > > > struct audit_buffer *ab; > > > > - uid_t loginuid = from_kuid(&init_user_ns, > > audit_get_loginuid(current)); - unsigned int sessionid = > > audit_get_sessionid(current); > > + struct tty_struct *tty; > > + const struct cred *cred; > > + char comm[sizeof(current->comm)]; > > > > if (!audit_enabled) > > > > return; > > > > + tty = audit_get_tty(current); > > + cred = current_cred(); > > + > > > > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); > > if (!ab) > > > > return; > > > > - audit_log_format(ab, "auid=%u ses=%u" ,loginuid, sessionid); > > + audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u", > > + task_tgid_nr(current), > > + from_kuid(&init_user_ns, cred->uid), > > + from_kuid(&init_user_ns, > > audit_get_loginuid(current)), + tty ? tty_name(tty) > > : "(none)", > > + audit_get_sessionid(current)); > > > > audit_log_task_context(ab); > > > > + audit_log_format(ab, " comm="); > > + audit_log_untrustedstring(ab, get_task_comm(comm, current)); > > + audit_log_d_path_exe(ab, current->mm); > > > > audit_log_format(ab, " op=%s", action); > > audit_log_key(ab, rule->filterkey); > > audit_log_format(ab, " list=%d res=%d", rule->listnr, res); > > > > -- > > 2.13.6 > > > > > > -- > > Linux-audit mailing list > > Linux-audit@redhat.com > > https://www.redhat.com/mailman/listinfo/linux-audit