[PATCH 1/1] audit: log binding and unbinding to netlink multicast

[Steve Grubb]

On Monday, October 16, 2017 5:35:55 PM EDT Paul Moore wrote:
> On Fri, Oct 13, 2017 at 3:58 PM, Steve Grubb <sgrubb at redhat.com> wrote:
> > Log information about programs connecting and disconnecting to the audit
> > netlink multicast socket. This is needed so that during investigations a
> > security officer can tell who or what had access to the audit trail. This
> > helps to meet the FAU_SAR.2 requirement for Common Criteria. Sample
> > event:
> > 
> > type=UNKNOWN[1332] msg=audit(1507924331.540:3): pid=1 uid=0
> > auid=4294967295 tty=(none) ses=4294967295 subj=kernel comm="systemd"
> > exe="/usr/lib/systemd/systemd" nlnk-grp=1 op=connect res=1
> > 
> > Signed-off-by: sgrubb <sgrubb at redhat.com>
> > ---
> > 
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/audit.c             | 48
> >  ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 45
> >  insertions(+), 4 deletions(-)
> 
> Since I think this is going to involve a respin, I just want to
> mention again "sgrubb" vs "Steve Grubb".  More comments inline ...
> 
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 0714a66f0e0c..892e63d9f2c1 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -112,6 +112,7 @@
> > 
> >  #define AUDIT_FEATURE_CHANGE   1328    /* audit log listing feature
> >  changes */ #define AUDIT_REPLACE          1329    /* Replace auditd if
> >  this packet unanswerd */ #define AUDIT_KERN_MODULE      1330    /*
> >  Kernel Module events */
> > 
> > +#define AUDIT_EVENT_LISTENER   1332    /* Task joined multicast read
> > socket */
>
> What Richard said.  Basically AUDIT_EVENT_LISTENER should be 1331 or
> have a *really* good explanation as to why it needs to be 1332.

Because 1331 is already assigned and in https://git.kernel.org/pub/scm/linux/
kernel/git/jack/linux-fs.git/log/?h=for_next as commit 
de8cd83e91bc3ee212b3e6ec6e4283af9e4ab269.

If you want me to assign 1331 which is already assigned to AUDIT_FANOTIFY in 
the user space piece, then it will make your testing...not look right. So, how 
do you want it?


> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 1baabc9539b4..3d2461be83a8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1476,22 +1476,61 @@ static void audit_receive(struct sk_buff  *skb)
> > 
> >         mutex_unlock(&audit_cmd_mutex);
> >  
> >  }
> > 
> > +/* Log information about who is connecting to the audit multicast socket
> > */ +static void audit_log_multicast_bind(int group, const char *op, int
> > err) +{
> > +       const struct cred *cred;
> > +       struct tty_struct *tty;
> > +       char comm[sizeof(current->comm)];
> > +       struct audit_buffer *ab;
> > +
> > +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_EVENT_LISTENER);
> 
> It really seems like this should be associated with the current task,
> e.g. "audit_log_start(current->audit_context, ...)".  After all, the
> whole point of this record is to capture information about the subject
> who is binding to the multicast socket.

OK, easy enough.


> > +       if (!ab)
> > +               return;
> > +
> > +       cred = current_cred();
> > +       tty = audit_get_tty(current);
> > +
> > +       audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > +                       task_pid_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_put_tty(tty);
> > +       audit_log_task_context(ab); /* subj= */
> > +       audit_log_format(ab, " comm=");
> > +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > +       audit_log_d_path_exe(ab, current->mm); /* exe= */
> 
> We could drop a lot of the above if this was an associated record.

Really do not want that to be the way we do things. We never have relied on 
syscall records for simple events.


> > +       audit_log_format(ab, " nlnk-grp=%d op=%s res=%d", group, op,
> > !err);
> > +       audit_log_end(ab);
> > +}
> > +
> > 
> >  /* Run custom bind function on netlink socket group connect or bind
> >  requests. */> 
> > -static int audit_bind(struct net *net, int group)
> > +static int audit_multicast_bind(struct net *net, int group)
> > 
> >  {
> > 
> > +       int err = 0;
> > +
> > 
> >         if (!capable(CAP_AUDIT_READ))
> > 
> > -               return -EPERM;
> > +               err = -EPERM;
> > +       audit_log_multicast_bind(group, "connect", err);
> > 
> > -       return 0;
> > +       return err;
> > +}
> > +
> > +static void audit_multicast_unbind(struct net *net, int group)
> > +{
> > +       audit_log_multicast_bind(group, "disconnect", 0);
> > 
> >  }
> >  
> >  static int __net_init audit_net_init(struct net *net)
> >  {
> >  
> >         struct netlink_kernel_cfg cfg = {
> >         
> >                 .input  = audit_receive,
> > 
> > -               .bind   = audit_bind,
> > +               .bind   = audit_multicast_bind,
> > 
> >                 .flags  = NL_CFG_F_NONROOT_RECV,
> >                 .groups = AUDIT_NLGRP_MAX,
> > 
> > +               .unbind = audit_multicast_unbind,
> 
> Please group the unbind with the bind, it is much easier to read that way.

No problem.

-Steve

> >         };
> >         
> >         struct audit_net *aunet = net_generic(net, audit_net_id);