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

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

[Steve Grubb]

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(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index c2e7e3a..2620847 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -484,6 +484,8 @@ static inline void	    audit_log_secctx(struct audit_buffer *ab, u32 secid)
 extern int audit_log_task_context(struct audit_buffer *ab);
 extern void audit_log_task_info(struct audit_buffer *ab,
 				struct task_struct *tsk);
+extern void audit_log_task_simple(struct audit_buffer *ab,
+				struct task_struct *tsk);
 
 extern int		    audit_update_lsm_rules(void);
 
@@ -540,6 +542,9 @@ static inline int audit_log_task_context(struct audit_buffer *ab)
 static inline void audit_log_task_info(struct audit_buffer *ab,
 				       struct task_struct *tsk)
 { }
+static inline void audit_log_task_simple(struct audit_buffer *ab,
+				       struct task_struct *tsk)
+{ }
 #define audit_enabled 0
 #endif /* CONFIG_AUDIT */
 static inline void audit_log_string(struct audit_buffer *ab, const char *buf)
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)
+{
+	const struct cred *cred;
+	char comm[sizeof(tsk->comm)];
+	char *tty;
+
+	if (!ab)
+		return;
+
+	/* tsk == current */
+	cred = current_cred();
+
+	spin_lock_irq(&tsk->sighand->siglock);
+	if (tsk->signal && tsk->signal->tty && tsk->signal->tty->name)
+		tty = tsk->signal->tty->name;
+	else
+		tty = "(none)";
+	spin_unlock_irq(&tsk->sighand->siglock);
+
+	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));
+
+	audit_log_task_context(ab); /* subj= */
+	audit_log_format(ab, " comm=");
+	audit_log_untrustedstring(ab, get_task_comm(comm, tsk));
+
+	audit_log_d_path_exe(ab, tsk->mm); /* exe= */
+}
+EXPORT_SYMBOL(audit_log_task_simple);
+
 /* Run custom bind function on netlink socket group connect or bind requests. */
 static int audit_bind(struct net *net, int group)
 {
-- 
2.4.3

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

[Paul Moore]

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() ... 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.

> 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.

-- 
paul moore
security @ redhat

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

[Steve Grubb]

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

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

[Paul Moore]

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@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