[RFC][PATCH] collect security labels on user processes generating audit messages
Timothy R. Chavez
tinytim at us.ibm.com
Wed Feb 15 15:49:38 UTC 2006
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
More information about the Linux-audit
mailing list