[PATCH] change lspp inode auditing

[Stephen Smalley]

On Wed, 2006-03-29 at 13:28 -0500, Steve Grubb wrote:
> Hi,
> 
> This is a first draft patch to change the auditing of inodes for lspp.
> Previously, we were gathering the context instead of the sid. Now in this patch, 
> we gather just the sid and convert to context only if an audit event is being 
> output. This patch makes no effort to account for policy_load. It also inserts
> some functions that are likely going upstream via Se Linux kernel people. So,
> that will need to be resolved before this patch is final. In any event its
> good enough to test with. This patch brings the performance hit from
> 146% down to 11%. We need a similar patch for IPC syscall auditing.

Not that I disagree with this change in approach, but I think that when
it has come up in the past, there has been concern expressed about the
fact that we could end up not being able to generate the context from
the SID when the audit record is being emitted (due to OOM condition),
and the operation has already occurred at that point.  Of course, there
are also other potential failure cases at the point, so I'm not sure it
is crucial, as long as audit_panic is called as appropriate.  Just
wanted to make sure that this point was understood by everyone.  But I
agree that pre-allocating the contexts is insane.

> diff -urp linux-2.6.16.x86_64.orig/include/linux/selinux.h linux-2.6.16.x86_64/include/linux/selinux.h
> --- linux-2.6.16.x86_64.orig/include/linux/selinux.h	2006-03-29 10:40:42.000000000 -0500
> +++ linux-2.6.16.x86_64/include/linux/selinux.h	2006-03-29 10:27:06.000000000 -0500
> @@ -13,6 +13,8 @@
>  #ifndef _LINUX_SELINUX_H
>  #define _LINUX_SELINUX_H
>  
> +#include <linux/fs.h>

Just put an empty decl for struct inode here, to avoid header
inter-dependencies:
	struct inode;

> @@ -76,6 +78,26 @@ void selinux_audit_set_callback(int (*ca
>   */
>  void selinux_task_ctxid(struct task_struct *tsk, u32 *ctxid);
>  
> +/**
> + *     selinux_ctxid_to_string - map a security context ID to a string
> + *     @ctxid: security context ID to be converted.
> + *     @ctx: address of context string to be returned
> + *     @ctxlen: length of returned context string.
> + *
> + *     Returns 0 if successful, -errno if not.  On success, the context
> + *     string will be allocated internally, and the caller must call
> + *     kfree() on it after use.
> + */
> +int selinux_ctxid_to_string(u32 ctxid, char **ctx, u32 *ctxlen);

Didn't Tim's patch for saving and auditing the netlink sender
SID/context have a similar interface, based on James' proposed API for
iptables?  Just need to make sure that we settle on a single interface
used by them all.  From later comments below, I think we'll want a
gfp_mask provided and I'm not sure we need the *ctxlen at all, as
SELinux handles the allocation internally.

> +/**
> + *     selinux_get_inode_sid - get the inode's security context ID
> + *     @inode: inode structure to get the sid from.
> + *
> + *     Returns the sid if successful and 0 if unset
> + */
> +u32 selinux_get_inode_sid(const struct inode *inode);

I'd favor returning an int (0 == success, -errno for failure) and
providing the SID via pointer arg like other interfaces.  Or if there
are no possible error cases, make it void, but still supply the SID via
argument.

> diff -urp linux-2.6.16.x86_64.orig/kernel/auditsc.c linux-2.6.16.x86_64/kernel/auditsc.c
> --- linux-2.6.16.x86_64.orig/kernel/auditsc.c	2006-03-29 10:40:48.000000000 -0500
> +++ linux-2.6.16.x86_64/kernel/auditsc.c	2006-03-29 10:26:45.000000000 -0500
> @@ -729,9 +726,24 @@ static void audit_log_exit(struct audit_
>  					 context->names[i].gid, 
>  					 MAJOR(context->names[i].rdev), 
>  					 MINOR(context->names[i].rdev));
> -		if (context->names[i].ctx) {
> -			audit_log_format(ab, " obj=%s",
> -					context->names[i].ctx);
> +		if (context->names[i].osid != 0) {
> +			char *ctx = NULL;
> +			int len = 0;
> +			if (selinux_ctxid_to_string(
> +				context->names[i].osid, &ctx, &len) == 0) {
> +				ctx = kmalloc(len, gfp_mask);
> +				if (ctx) {
> +					selinux_ctxid_to_string(
> +		                                context->names[i].osid,
> +						&ctx, &len);
> +				}
> +			}

Unless I'm confused (quite possible ;), the above sequence shouldn't be
necessary and will actually leak the allocated buffer because SELinux
will overwrite the pointer with its own.  The SELinux internal functions
(e.g. security_sid_to_context) handle the allocation of a context buffer
to the right size and set *ctx to it, so the caller never needs to play
this game.   Some of the hook interfaces unfortunately require the
caller to guess and provide a buffer that they allocate, but I don't
think we want to continue that trend.  SELinux should just set *ctx to
the context buffer it allocates and you are done.  You should likely
pass the gfp_mask down into the SELinux interface and propagate it down
to the internal code so that we can conform with whatever the caller
needs.

> diff -urp linux-2.6.16.x86_64.orig/security/selinux/exports.c linux-2.6.16.x86_64/security/selinux/exports.c
> --- linux-2.6.16.x86_64.orig/security/selinux/exports.c	2006-03-29 10:40:51.000000000 -0500
> +++ linux-2.6.16.x86_64/security/selinux/exports.c	2006-03-29 10:26:45.000000000 -0500
> +u32 selinux_get_inode_sid(const struct inode *inode)
> +{
> +	struct inode_security_struct *isec = inode->i_security;
> +	return isec->sid;
> +}

I think you need to check for selinux_enabled here, c.f. Darrel's
patches for audit-by-context.  Keep in mind that SELinux can be runtime
disabled by init (if SELINUX=disabled in /etc/selinux/config).

-- 
Stephen Smalley
National Security Agency