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

[Steve Grubb <sgrubb@redhat.com>]

On Friday, July 24, 2015 06:54:27 PM Paul Moore wrote:
> On Thursday, July 23, 2015 04:45:10 PM Steve Grubb wrote:
> > The audit subsystem could use a function that logs the commonly needed
> > fields for a typical audit event. This logs less that audit_log_task_info
> > and reduces the need to hand code individual fields.
> > 
> > Signed-off-by: Steve Grubb <sgrubb@redhat.com>
> > ---
> > 
> >  include/linux/audit.h |  5 +++++
> >  kernel/audit.c        | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 40 insertions(+)
> 
> Additional comments below, but I'd like to see this patch change
> audit_log_task_info() to call audit_log_task_simple() 

They really can't without messing up parsers. The order is different for a 
reason. The audit_log_task_info records all kinds of stuff that is really not 
needed. It does pids, current credentials, extended uid, extended gid, and 
then tty and session, comm, exe, and then context. This wastes disk space.

The new function is what should be used for most cases because it sticks to 
what is necessary for "hardwired" events - those that are not dictated by 
syscall or file watches. It provides pid, uid, auid, tty, session, context, 
comm, exe. Because it jettisons all the stuff that doesn't matter, one cannot 
call the other.


> ... or, why not just call audit_log_task_info() if the audit bind/unbind is
> going to be the only one to benefit from audit_log_task_simple()?  Yes, I
> know that audit_log_task_info() records more than you need, but this
> duplication of code because of the record format mess makes me very grumpy.

I'd rather see us move some other things to audit_log_task_simple over the 
long term than hand code things. This is really for "hardwired" events such as 
config changes, anomalies, and access decisions by frameworks. Group ids are 
only applicable to file access, so it belongs in the syscall event and nowhere 
else. Ppid is useless without a mapping back to that process's name. Extended 
credentials are used on file access, so we get them in syscall records. We 
don't need to waste disk space.


> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 1c13e42..29fb38b 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1100,6 +1100,41 @@ static void audit_receive(struct sk_buff  *skb)
> > 
> >  	mutex_unlock(&audit_cmd_mutex);
> >  
> >  }
> > 
> > +/* This function logs the essential information needed to understand
> > + * what or who is causing the event */
> > +void audit_log_task_simple(struct audit_buffer *ab, struct task_struct
> > *tsk)
> 
> ...
> 
> > +	audit_log_format(ab, "pid=%u uid=%u auid=%u tty=%s ses=%u",
> > +			 task_pid_nr(tsk),
> > +			 from_kuid(&init_user_ns, cred->uid),
> > +			 from_kuid(&init_user_ns, audit_get_loginuid(tsk)),
> > +			 tty, audit_get_sessionid(tsk));
> 
> You should check the format string against audit_log_task_info(); they don't
> match.

That is correct. It mostly matches the order of just about everything else. 
For example, user space originating events get this:

pid=16430 uid=0 auid=4325 ses=3  subj=unconfined

anom_abend:  auid=4325 uid=4325 gid=4325 ses=1 subj=unconfined pid=1680

config_change: auid=4325 ses=1

integrity: pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel

The new order is:

pid=10068 uid=0 auid=4325  tty=pts0 ses=1 subj=unconfined
comm= exe=

This is an easy switch to make in searching and reporting because things are 
mostly  in the same order but with extra fields or start with a different field 
making it easy to decide between old and new order.

-Steve