[redhat-lspp] security context in audit records (audit.39 kernel)

[Stephen Smalley]

On Thu, 2005-05-19 at 14:58 -0500, Daniel H. Jones wrote:
> Return error from audit_set_mac_security_xattr_name if name already set.

Ok, that's getting too verbose even for me ;)  audit_set_mac,
audit_set_macxattr, something like that.  Chapter 4 of
Documentation/CodingStyle.

> To do list:
> Check other places in IPC code that maybe should call 
> audit_ipc_security_context. (not caught by ipcperms).

Easiest thing to do is look at where we inserted security hooks into the
IPC code, and then omit the cases where it is already covered by the
ipcperms call.  In your case, you likely want to insert your audit hooks
before the DAC ownership tests whereas the security hooks follow them.

> +extern int audit_set_mac_security_xattr_name(char *name);

audit_set_mac, const char *

> +static inline int audit_set_mac_security_xattr_name(char *name)

ditto

> +static inline void audit_panic(const char *message)
> +{
> +	return 0;
> +}

Void functions don't return a value.

> +static const char *mac_security_xattr_name = NULL;

NULL is implicit.

> +static void audit_log_task_security_context(struct audit_buffer *ab);
> +void audit_inode_security_context(int idx, const struct inode *inode);
> +

Could likely avoid these prototypes just by moving the functions prior
to first call.

> +	len = security_getprocattr(current, "current", security_context, len);
> +	if (len < 0 ) {
> +		audit_panic("security_getprocattr error in audit_log_task_security_context");
> +		kfree(security_context);
> +		return;
> +	}

Try to use a centralized exit path for functions, see Chapter 6 of
Documentation/CodingStyle.  goto out and have the out path handle the
kfree and return.

> +static int selinux_getsecurity(u32 sid, void *buffer, size_t size);
> +

Likewise, why not put the function first rather than requiring a
prototype, and then you can make it static inline as well.

> @@ -4086,6 +4123,11 @@
>  	error = security_sid_to_context(sid, &context, &len);
>  	if (error)
>  		return error;
> +
> +	if (!value || !size) {
> +		kfree(context);
> +		return len;
> +	}
>  	if (len > size) {
>  		kfree(context);
>  		return -ERANGE;

Any reason you can't use your selinux_getsecurity() helper for
selinux_getprocattr() now that you are having it also return len for a
NULL value?  However, a note of caution:  while proc_pid_attr_read will
never pass a NULL value (and thus you can use that as your check for
returning the actual length), it could possibly pass a 0 size if
userspace passed a 0 size to the read(2) call, in which case it would
_not_ expect getprocattr to be returning a non-zero length.  So you
might need getprocattr to still reject a zero size with -ERANGE
(restoring the original check).  This would still allow you to use
selinux_getsecurity(), as long as you check for the 0 size first so it
never reaches it in that case.

> @@ -4321,6 +4364,10 @@
>  	if (register_security (&selinux_ops))
>  		panic("SELinux: Unable to register with kernel.\n");
>  
> +	if (audit_set_mac_security_xattr_name(XATTR_SELINUX_SUFFIX)) {
> +		printk(KERN_WARNING "SELinux: Unable to register xattr name with audit.\n");	
> +	}
> +

Not sure whether you want it to just warn in this case or panic.

-- 
Stephen Smalley
National Security Agency