Re: [RFC][PATCH] collect security labels on user processes generating audit messages
On Wed, 2006-02-15 at 08:47 -0500, Stephen Smalley wrote:
On Tue, 2006-02-14 at 17:48 -0600, Timothy R. Chavez wrote:
> diff --git a/include/linux/netlink.h b/include/linux/netlink.h
> index 6a2ccf7..ccd5905 100644
> --- a/include/linux/netlink.h
> +++ b/include/linux/netlink.h
> @@ -143,6 +143,7 @@ struct netlink_skb_parms
> __u32 dst_group;
> kernel_cap_t eff_cap;
> __u32 loginuid; /* Login (audit) uid */
> + __u32 secid; /* SELinux security id */
> };
>
> #define NETLINK_CB(skb) (*(struct netlink_skb_parms*)&((skb)->cb))
As a minor nit, does anyone know why '__u32' is used here vs. 'u32'?
The definition is already wrapped with an #ifdef __KERNEL__. I see that
you are being consistent with the existing fields, but then we use just
'u32' in the audit code and in the SELinux interfaces and code. Seems
like we should be consistent throughout, and I don't see a real reason
to use __u32 vs. just u32 if it is all kernel code and not included in
userland (or protected by #ifdef __KERNEL__).
Yeah.. I was wondering 'bout this myself. I'll just change secid to u32
for the time being.
[..]
> diff --git a/include/linux/security.h
b/include/linux/security.h
> index b4fe8aa..c6fe5fe 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1169,6 +1174,7 @@ struct security_operations {
> unsigned long arg5);
> void (*task_reparent_to_init) (struct task_struct * p);
> void (*task_to_inode)(struct task_struct *p, struct inode *inode);
> + void (*task_getsecid)(struct task_struct *tsk, __u32 *sid);
Same issue for the security hook interfaces.
And s/__u32/u32 on all the relevant functions too.
[..]
> @@ -2457,6 +2468,9 @@ static inline void security_task_reparen
> static inline void security_task_to_inode(struct task_struct *p, struct inode
*inode)
> { }
>
> +static inline void security_task_getsecid(struct task_struct *tsk, __u32 *sid)
> +{ }
> +
Should we set *sid = 0 here as in the dummy function, just to make sure
it doesn't contain garbage?
Yep.
[..]
> diff --git a/kernel/audit.c b/kernel/audit.c
> index d95efd6..4ca77dd 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -458,14 +462,20 @@ static int audit_receive_msg(struct sk_b
> err = audit_filter_user(&NETLINK_CB(skb), msg_type);
> if (err == 1) {
> err = 0;
> + if (selinux_available()) {
> + err = selinux_id_to_ctx(sid, &ctx, &len);
> + if (err < 0)
> + return err;
> + }
It seems unfortunate to have to wrap each call to a public SELinux
interface with a selinux_available() check, and the !
defined(CONFIG_SECURITY_SELINUX) case would actually work without such a
check since it just sets (ctx, len) to (NULL, 0). So the
selinux_available() check is only necessary for the case where SELinux
is disabled at runtime (selinux=0 or /selinux/disable). Possibly it
should be moved within selinux_id_to_ctx() so that callers can just call
selinux_id_to_ctx() unconditionally?
This makes sense to me. I'll go ahead and make the change. I wouldn't
even technically need the function or function call in my patch since
selinux_available() simply returns ss_initialized.
[..]
> ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
> if (ab) {
> audit_log_format(ab,
> - "user pid=%d uid=%u auid=%u msg='%.1024s'",
> - pid, uid, loginuid, (char *)data);
> + "user pid=%d uid=%u auid=%u subj=%s msg='%.1024s'",
> + pid, uid, loginuid, ctx ? ctx : "null", (char *)data);
Do you want those "subj=null" items in the output when SELinux is
disabled, or should there be a different audit_log_format call in the !
ctx case that completely omits "subj="?
I remember a while back, Steve wanting to reduce / remove the
conditional tokens in records (I think the argument had to do with
performance impact and parsing). Did I remember that correctly Steve?
Assuming we do want to print the token if ctx == NULL, is there a
standard way of printing NULL in the record? I should probably make
sure kernel/auditsc.c:audit_log_task_context() is consistent with
whatever we decide.
Thanks.
-tim