[PATCH] LSPP audit enablement: storing selinux ocontext and scontext
Dustin Kirkland
dustin.kirkland at us.ibm.com
Fri Oct 7 18:24:13 UTC 2005
I'm addressing Amy's concerns and attaching an updated patch with the
editions discussed inline.
On Fri, 2005-09-02 at 23:19 -0600, Amy Griffis wrote:
> Some comments inline.
>
> > diff -uprN linux-2.6.13-rc6-mm2/ipc/msg.c linux-2.6.13-rc6-mm2-lspp_audit/ipc/msg.c
> > --- linux-2.6.13-rc6-mm2/ipc/msg.c 2005-08-29 11:32:16.000000000 -0500
> > +++ linux-2.6.13-rc6-mm2-lspp_audit/ipc/msg.c 2005-08-29 10:37:51.000000000 -0500
> > @@ -443,6 +443,13 @@ asmlinkage long sys_msgctl (int msqid, i
> > if (msq == NULL)
> > goto out_up;
> >
> > + ipcp = &msq->q_perm;
>
> Please double check that ipcp isn't being set twice, as it appears.
I think you're right. I rearranged these 4 lines to make it a little
clearer.
> > +
> > + if (cmd == IPC_SET) {
> > + if ((err = audit_ipc_security_context(ipcp)))
>
> I think it's desirable to limit the proliferation of audit hooks where
> possible. In this case, grabbing q_perm could be consolidated with
> /the current audit_ipc_perms hook, although this would mean processing
> under the spinlock more of the time.
I suppose audit_ipc_security_context() and audit_ipc_perms() could be
collapsed into a single function. But then this ficticious audit_ipc()
would need to wrapped with the spin lock. I don't think this tradeoff
would be desirable. I would think that we want only functionality that
needs to be protected bound to a spin lock.
In any case, does the addition of this code really constitute egregious
proliferation of audit hooks?
// existing spinlock just above here...
ipcp = &msq->q_perm;
if (cmd == IPC_SET) {
if ((err = audit_ipc_security_context(ipcp)))
goto out_unlock_up;
}
> > diff -uprN linux-2.6.13-rc6-mm2/ipc/util.c linux-2.6.13-rc6-mm2-lspp_audit/ipc/util.c
> > --- linux-2.6.13-rc6-mm2/ipc/util.c 2005-08-29 11:32:16.000000000 -0500
> > +++ linux-2.6.13-rc6-mm2-lspp_audit/ipc/util.c 2005-08-29 10:39:17.000000000 -0500
> > @@ -466,6 +467,8 @@ int ipcperms (struct kern_ipc_perm *ipcp
> > { /* flag will most probably be 0 or S_...UGO from <linux/stat.h> */
> > int requested_mode, granted_mode;
> >
> > + audit_ipc_security_context(ipcp);
>
> Why no error check?
Because audit_ipc_security_context() handles its errors internally by
calling audit_panic(), which can fail silently, printk a message, or
under the most strict configuration panic the kernel. I think this
gives an administrator the flexibility needed to silently ignore such
context auditing failures, receive debug messages, or in super-strict
mode, panic the system.
It seems there are disagreements with this methodology. Please respond
with a working patch that handles this behavior better if you still
disagree.
Some have suggested propagating errno's up to the calling syscalls.
This rippled into other unforeseen places in my efforts to accomplish
this and I never found a clean implementation.
> Does hooking ipcperms cover all the ipc operations where we need the
> security context?
I poked through all of ipc/*.c looking for EPERM and EACCES, which
identify security-relevant error points. According to our current
interpretation of the LSPP spec, we only need to be concerned with
IPC_SET operations. And in this case, I think we have the coverage that
we need.
> > diff -uprN linux-2.6.13-rc6-mm2/kernel/auditsc.c linux-2.6.13-rc6-mm2-lspp_audit/kernel/auditsc.c
> > --- linux-2.6.13-rc6-mm2/kernel/auditsc.c 2005-08-29 11:32:16.000000000 -0500
> > +++ linux-2.6.13-rc6-mm2-lspp_audit/kernel/auditsc.c 2005-08-29 11:09:43.000000000 -0500
> > +struct audit_aux_data_security_context {
> > + struct audit_aux_data d;
> > + char *security_context;
> > + size_t security_context_len;
> > +};
>
> I'd prefer not to add another aux struct just to hold ipc security
> context. I don't see any reason why this couldn't be added to
> audit_aux_data_ipcctl below.
Well, audit_aux_data_security_context is a little more generic than
audit_aux_data_ipcctl (which is ipc-specific). I tend to disagree with
this suggestion, as the security_context information should be across
objects besides just ipc.
> Also, the security_context_len field is unused.
That's true. It's easily enough removed. But as is, it's simply
mimmicing the definition of audit_aux_sockaddr. I'm ambivalent about
this change. I'm leaving as is, since it mirrors previous struct
definitions. If the consensus is to eliminate it, I'm not opposed...
Following the standard set forth by the rest of this file, it seems that
the preferred manner in which to create an extension to an audit record
is to create a structure as above to hold the new data.
> > @@ -868,6 +912,11 @@ static void audit_log_exit(struct audit_
> > struct audit_aux_data_path *axi = (void *)aux;
> > audit_log_d_path(ab, "path=", axi->dentry, axi->mnt);
> > break; }
> > + case AUDIT_SECURITY_CONTEXT: {
> > + struct audit_aux_data_security_context *axi = (void *)aux;
> > + audit_log_format(ab, " ocontext=%s", axi->security_context);
> > + kfree(axi->security_context);
>
> Freeing the security context in audit_free_aux() would be more
> consistent with the current code.
Agreed. Changed in code. Please review.
> > @@ -1118,6 +1171,37 @@ void audit_putname(const char *name)
> > #endif
> > }
> >
> > +void audit_inode_security_context(int idx, const struct inode *inode)
> > +{
> > + struct audit_context *context = current->audit_context;
> > + int len = 0;
> > +
> > + if (!audit_macxattr)
> > + return;
> > +
> > + len = security_inode_getsecurity((struct inode *)inode, audit_macxattr, NULL, 0);
> > + if (len < 0) {
> > + if (len != -EOPNOTSUPP)
> > + audit_panic("security_inode_getsecurity error in audit_inode_security_context");
>
> I don't think audit_panic is needed anywhere in this function, as the
> syscall operation hasn't occured yet.
Hmm. Okay, well I'm open to ideas on how to go about fixing this. Note
that audit_inode_security_context() is a void function, as is
audit_inode(), and it would be very difficult to propagate this error up
to the calling syscall, as has been suggested.
How then do we guarantee that we won't lose audit records in the case
where some part of audit_inode_security_context() fails? There isn't a
return mechanism for propagating that information upward to the
path_lookup() function that needs to audit the inode.
> > @@ -1292,3 +1378,58 @@ void audit_signal_info(int sig, struct t
> > }
> > }
> >
> > +int audit_ipc_security_context(struct kern_ipc_perm *ipcp)
> > +{
> > + struct audit_aux_data_security_context *ax;
> > + struct audit_context *context = current->audit_context;
> > + int len = 0;
> > +
> > + if (likely(!context))
> > + return 0;
> > +
> > + ax = kmalloc(sizeof(*ax), GFP_ATOMIC);
>
> Might add a comment as to why you're using GFP_ATOMIC here.
Actually, this should probably be GFP_KERNEL. We should be able to
sleep at this point without a problem.
Searching the rest of the file, though I see a similar kmalloc() in
audit_avc_path() that is GFP_ATOMIC... Anyone know why? I think I was
just mirroring the behavior there... I took the liberty to changing
that one to GFP_KERNEL as well. Correct me if this is not correct.
> > + if (!ax) {
> > + audit_panic("memory allocation error in audit_ipc_security_context");
>
> Again, don't think audit_panic is needed.
Same comment just above... Open to suggestions...
> > diff -uprN linux-2.6.13-rc6-mm2/security/selinux/hooks.c linux-2.6.13-rc6-mm2-lspp_audit/security/selinux/hooks.c
> > --- linux-2.6.13-rc6-mm2/security/selinux/hooks.c 2005-08-29 11:32:17.000000000 -0500
> > +++ linux-2.6.13-rc6-mm2-lspp_audit/security/selinux/hooks.c 2005-08-29 10:37:51.000000000 -0500
> > @@ -116,6 +116,35 @@ static struct security_operations *secon
> > static LIST_HEAD(superblock_security_head);
> > static DEFINE_SPINLOCK(sb_security_lock);
> >
> > +/* Return security context for a given sid or just the context
> > + length if the buffer is null or length is 0 */
> > +static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
> > +{
> > + char *context;
> > + unsigned len;
> > + int rc;
> > +
> > + if (buffer && !size)
> > + return -ERANGE;
> > +
> > + rc = security_sid_to_context(sid, &context, &len);
> > + if (rc)
> > + return rc;
> > +
> > + if (!buffer || !size)
>
> You don't need to check !size here. Based on your other conditionals,
> it can only be possible if you have !buffer, which you've already
> checked.
Good catch. I agree. Fixed.
> > + goto getsecurity_exit;
> > +
> > + if (size < len) {
> > + kfree(context);
> > + return -ERANGE;
>
> Use getsecurity_exit here.
Ok. getsecurity_exit label will return len, so I'll need to set len =
-ERANGE. Fixed in new patch.
> > @@ -3362,6 +3374,13 @@ out:
> > return err;
> > }
> >
> > +static int selinux_socket_getsecurity(struct socket *sock, void *buffer, size_t size)
> > +{
> > + struct inode_security_struct *isec = SOCK_INODE(sock)->i_security;
> > +
> > + return(selinux_getsecurity(isec->sid, buffer, size));
> > +}
>
> This patch appears to be unfinished (?)
> I don't see any code using this function.
That's true. This was a stub for more work that I think will need to be
done. I've removed this code from this patch.
--
A few other minor changes...
In include/linux/audit.h, 1500-1599 has been set aside for LSPP use.
The first two entries are AUDIT_SUBJECT_CONTEXT=1500 and
AUDIT_OBJECT_CONTEXT=1501. (This replaced the temporarily-stubbed-in
definition of AUDIT_SECURITY_CONTEXT=1350).
The audit record now uses "subj" instead of "scontext" and "obj" instead
of "ocontext". Saved a few bytes space.
Patch is attached. David: If you want me diff-ed against something else
(your git tree?), let me know and I'll port forward (yet again).
:-Dustin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 2.6.13-rc6-mm2-context_labels.patch
Type: text/x-patch
Size: 20373 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/linux-audit/attachments/20051007/9308f674/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://listman.redhat.com/archives/linux-audit/attachments/20051007/9308f674/attachment.sig>
More information about the Linux-audit
mailing list