Re: [PATCH 1/2] audit: log binding and unbinding to netlink multicast socket
On Tuesday, July 28, 2015 10:31:54 AM Steve Grubb wrote:
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(a)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.
If we can't use _task_simple() inside of _task_info() then just use
audit_log_task_info(). Yes, it probably wastes a few extra bytes each time
these records are generated, but these records aren't likely to be frequent.
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.
Where can we use _task_simple() beyond these new records? Show me this has
some reuse in the existing code base and I'll reconsider keeping
_task_simple(), but right now it just looks like code duplication to me.
> ... 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.
See above; we're not going to hand code things, just use _task_info().
Long term we are going to be ditching this awful fixed string format.
> > 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:
I was talking about some of the scalar format specifiers, e.g. "%u" vs
"%d",
but it doesn't matter so much anymore as it looks like we'll need to use
_task_info().
--
paul moore
security @ redhat