[redhat-lspp] Re: [PATCH] change lspp inode auditing

[Stephen Smalley]

On Thu, 2006-03-30 at 08:57 -0500, Steve Grubb wrote:
> There is one last loose end to this patch. I am adding a function,
> selinux_ctxid_to_string. Is there an official SE Linux kernel API that
> provides this? I'll fix Tim's patch to use the same API.

Nothing official; James' experimental patches for the iptables
integration used:
int selinux_id_to_ctx(u32 ctxid, char **ctx, u32 *ctxlen);
But see comments below on your proposed interface.

> 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 18:08:04.000000000 -0500
> +++ linux-2.6.16.x86_64/include/linux/selinux.h	2006-03-29 20:29:32.000000000 -0500
> @@ -76,6 +77,28 @@ 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.
> + *     @gfp_mask: memory type mask
> + *
> + *     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, gfp_t gfp_mask);

On second look, I think you can drop the gfp_mask because we have to use
GFP_ATOMIC in context_struct_to_string regardless of what the caller is
using, due to taking the policy rdlock (and looking later in the patch,
you don't pass it down any further, so it serves no purpose).  Also,
since you explicitly identify this as a _to_string interface, I think
you can drop the ctxlen argument; the returned string is NUL-terminated
anyway - unless the caller needs the length for some purpose.

>  #else
>  
>  static inline int selinux_audit_rule_init(u32 field, u32 op,
> @@ -107,6 +130,18 @@ static inline void selinux_task_ctxid(st
>  	*ctxid = 0;
>  }
>  
> +static inline void selinux_ctxid_to_string(u32 ctxid, char **ctx, u32 *ctxlen, gfp_t gfp_mask)

Mismatch in the return type with the #if clause, should be int here.
Make sure you compile with and without SELinux enabled, and also test
with SELinux runtime-disabled via SELINUX=disabled
in /etc/selinux/config and by booting with selinux=0 (sorry, too many
options).

> +			if (selinux_ctxid_to_string(
> +				context->names[i].osid, &ctx, &len, gfp_mask)){ 
> +				audit_log_format(ab, " obj=%u",
> +						context->names[i].osid);
> +			} else 
> +				audit_log_format(ab, " obj=%s", ctx);
> +			kfree(ctx);

Not much value in displaying the SID, although we do it elsewhere as
well (e.g. in the AVC) as a fallback - mapping it will then require a
dump of kernel memory at that time.  Likely have to call audit_panic in
this scenario to meet the criteria, but the admin can always set
audit_panic to not actually panic the machine.

BTW, you kfree(ctx) unconditionally above, so you better initialize it
to NULL prior to calling selinux_ctxid_to_string().  len has the wrong
type too (int vs. u32), but I think you can drop it altogether.

> +	if (security_inode_xattr_getsuffix())
> +		selinux_get_inode_sid(inode, &context->names[idx].osid);
> +	else
> +		context->names[idx].osid = 0;

Why not just unconditionally call the interface, which will set the SID
to zero if selinux isn't enabled?  Then you can also remove
security_inode_xattr_getsuffix() altogether IIUC.

> 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 18:08:08.000000000 -0500
> +++ linux-2.6.16.x86_64/security/selinux/exports.c	2006-03-29 20:24:55.000000000 -0500
> @@ -26,3 +27,27 @@ void selinux_task_ctxid(struct task_stru
>  	else
>  		*ctxid = 0;
>  }
> +
> +extern int ss_initialized;
> +
> +int selinux_ctxid_to_string(u32 ctxid, char **ctx, u32 *ctxlen, gfp_t gfp_mask)
> +{
> +       if (ss_initialized)
> +               return security_sid_to_context(ctxid, ctx, ctxlen);

security_sid_to_context actually has an internal handler for !
ss_initialized that should handle all initial SIDs (and you shouldn't
have any other SIDs until policy is loaded), so I suspect that the test
here should be for selinux_enabled instead.

-- 
Stephen Smalley
National Security Agency