audit: log binding and unbinding to netlink multicast socket
PATCH 1/2 audit: log binding and unbinding to netlink multicast socket
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.
--- include/linux/audit.h | 5 +++++ kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
include/linux/audit.h +5 -0
@@ -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)
kernel/audit.c +35 -0
@@ -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) {
Signature · 1 line
2.4.3
Quoted 9 lines · 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_infoand reduces the need to hand code individual fields.Signed-off-by Steve Grubb <[email protected]>--- 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.
Quoted 12 lines
kernel/audit.c +3
-0@@ -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)
...
kernel/audit.c static void audit_receive(struct sk_buff *skb) · line 1128
Quoted 5 lines
kernel/audit.c +5
-0+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.
Signature · 2 lines
paul moore security @ redhat
Quoted 8 lines · Paul Moore
Quoted 7 lines · 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_infoand reduces the need to hand code individual fields.Signed-off-by Steve Grubb <[email protected]>---
Quoted 6 lines
Quoted 3 lines
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 callaudit_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.
Quoted 4 lines
... or, why not just call
audit_log_task_info()if the audit bind/unbind is going to be the only one to benefit fromaudit_log_task_simple()? Yes, I know thataudit_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.
Quoted 17 lines
Quoted 14 lines
kernel/audit.c +3
-0@@ -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)...
Quoted 8 lines
kernel/audit.c static void audit_receive(struct sk_buff *skb) · line 1128
Quoted 5 lines
kernel/audit.c +5
-0+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
Quoted 14 lines · Steve Grubb
Quoted 13 lines · Paul Moore
Quoted 11 lines · 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 <[email protected]>---
include/linux/audit.h | 5 +++++ kernel/audit.c | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+)
Quoted 8 lines
Quoted 2 lines
Additional comments below, but I'd like to see this patch change
audit_log_task_info()to callaudit_log_task_simple()They really can't without messing up parsers. The order is different for a reason. The
audit_log_task_inforecords 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.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 5 lines
... or, why not just call
audit_log_task_info()if the audit bind/unbind is going to be the only one to benefit fromaudit_log_task_simple()? Yes, I know thataudit_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_simpleover 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.
Quoted 26 lines
Quoted 17 lines
Quoted 14 lines
kernel/audit.c +3
-0@@ -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)...
Quoted 8 lines
kernel/audit.c static void audit_receive(struct sk_buff *skb) · line 1128
Quoted 5 lines
kernel/audit.c +5
-0+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.
Quoted 2 lines
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().
Signature · 2 lines
paul moore security @ redhat