[PATCH] LSPP audit enablement: storing selinux ocontext and scontext

[Amy Griffis]

Hi Dustin,

On Fri, Oct 07, 2005 at 01:24:13PM -0500, Dustin Kirkland wrote:
> I'm addressing Amy's concerns and attaching an updated patch with the
> editions discussed inline.

Thanks.

> On Fri, 2005-09-02 at 23:19 -0600, Amy Griffis wrote: 
> > > 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.  

If you look at audit_ipc_perms(), it fails the syscall for any audit
failures.  I agree with you that we need a design for audit error
handling.  But until we have that, we should stay consistent with the
current behavior, which is to fail the syscall when possible.

I see that the ipcperms() callers are throwing away the return code
and setting -EACCES for all failures.  Even so, we should return
-ENOMEM, and hopefully they will be updated at some point to propagate
other errors.  Of course we've also discussed hooking somewhere other
than ipcperms, so this might be moot.

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

Okay, I can take a look at improving the error handling when I get a
chance.

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

I can't think of a reason to make it generic other than to save
space.  Since the other aux data structs are already allocated
dynamically, this approach is actually taking more space as well as
introducing the overhead of having to walk more elements in the aux
list.

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

I don't see why we would want to add an unused field just to look
like another structure.

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

You added a security_context field to audit_names.  So adding a
comparable field to audit_aux_data_ipcctl makes sense.

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

Saw it, thanks.

> > > +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, 

Right, but it doesn't have to be void.

> as is audit_inode(), 

audit_inode is void because up to this point, there haven't been any
errors to report.  The memory has been pre-allocated.

> and it would be very difficult to propagate this error up to the
> calling syscall, as has been suggested.

I don't see why it would be difficult.  path_lookup returns an int, so
we could make audit_inode return an int if we need to.

By the way, have you run into any issues with allocating memory during
path_lookup?

> How then do we guarantee that we won't lose audit records in the
> case where some part of audit_inode_security_context() fails?

We wouldn't lose records.  Theoretically, the syscall would fail and
audit would log any information it had collected.  But with -ENOMEM,
the system probably has other issues.

> > > +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);
<snip comments>
> > > +	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...

audit_ipc_security_context returns an int and the calling functions
are propagating errors, so there should be no issue.

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

Thanks.  Just make sure to clarify that this patch is for collecting
security label context for inodes and ipc only, and I'll be happy. :-)

Amy