Re: [PATCH 1/1] audit: add missing fields to AUDIT_CONFIG_CHANGE event

[Steve Grubb <sgrubb@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 <sgrubb@redhat.com> 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 <sgrubb@redhat.com>
> > ---
> > 
> >  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
<no matches>

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