[RFC PATCH] selinux: log invalid contexts in AVCs

[Stephen Smalley]

On 1/18/19 5:04 AM, Ondrej Mosnacek wrote:
> In case a file has an invalid context set, in an AVC record generated
> upon access to such file, the target context is always reported as
> unlabeled. This patch adds new optional fields to the AVC record (slcon
> and tlcon) that report the actual context string if it differs from the
> one reported in scontext/tcontext. This is useful for diagnosing SELinux
> denials.
> 
> To trigger an AVC that illustrates this situation:
> 
>      # setenforce 0
>      # touch /tmp/testfile
>      # setfattr -n security.selinux -v system_u:object_r:banana_t:s0 /tmp/testfile
>      # runcon system_u:system_r:sshd_t:s0 cat /tmp/testfile
> 
> AVC before:
> 
> type=AVC msg=audit(1547801083.248:11): avc:  denied  { open } for  pid=1149 comm="cat" path="/tmp/testfile" dev="tmpfs" ino=6608 scontext=system_u:system_r:sshd_t:s0 tcontext=system_u:object_r:unlabeled_t:s15:c0.c1023 tclass=file permissive=1
> 
> AVC after:
> 
> type=AVC msg=audit(1547801083.248:11): avc:  denied  { open } for  pid=1149 comm="cat" path="/tmp/testfile" dev="tmpfs" ino=6608 scontext=system_u:system_r:sshd_t:s0 tcontext=system_u:object_r:unlabeled_t:s15:c0.c1023 tlcon=system_u:object_r:banana_t:s0 tclass=file permissive=1
> 
> Cc: Daniel Walsh <dwalsh at redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
> Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
> ---
>   security/selinux/avc.c | 49 +++++++++++++++++++++++++-----------------
>   1 file changed, 29 insertions(+), 20 deletions(-)
> 
> I'm not entirely sure about the record format here, so I'm Cc'ing
> linux-audit and Steve for feedback. I went for optional fields to
> minimize the size of the record, but maybe a different format is
> preferred. If so, let me know and I'll do a respin.
> 
> Also, I accept suggestions for better field names than "slcon"/"tlcon"
> ("lcon" is meant as an acronym for "literal context", but I'm not sure
> if that's a good name...).

We've typically referred to them as "raw contexts", as in the comments 
in hooks.c:selinux_inode_getsecurity():
         /*
          * If the caller has CAP_MAC_ADMIN, then get the raw context
          * value even if it is not defined by current policy; otherwise,
          * use the in-core value under current policy.
          * Use the non-auditing forms of the permission checks since
          * getxattr may be called by unprivileged processes commonly
          * and lack of permission just means that we fall back to the
          * in-core context value, not a denial.
          */

On the other hand, we also use that term in the selinux userspace to 
refer to the kernel contexts before they are translated by mcstransd. 
Still, I think raw is better than literal here.

> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 9b63d8ee1687..4a181ed56e37 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -165,6 +165,32 @@ static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
>   	audit_log_format(ab, " }");
>   }
>   
> +static void avc_dump_sid(struct audit_buffer *ab, struct selinux_state *state,
> +			 u32 sid, char type)
> +{
> +	int rc;
> +	char *context, *lcontext;
> +	u32 context_len, lcontext_len;
> +
> +	rc = security_sid_to_context(state, sid, &context, &context_len);
> +	if (rc) {
> +		audit_log_format(ab, "%csid=%d ", type, sid);
> +		return;
> +	}
> +
> +	audit_log_format(ab, "%ccontext=%s ", type, context);
> +
> +	/* in case of invalid context report also the actual context string */
> +	rc = security_sid_to_context_force(state, sid, &lcontext,
> +					   &lcontext_len);
> +	if (!rc) {
> +		if (strcmp(context, lcontext))
> +			audit_log_format(ab, "%clcon=%s ", type, lcontext);
> +		kfree(lcontext);
> +	}
> +	kfree(context);

Optimally we'd have a nicer way of testing whether a SID corresponds to 
an invalid context rather than having to turn it into a context string 
and compare.  Maybe a new security server interface?

> +}
> +
>   /**
>    * avc_dump_query - Display a SID pair and a class in human-readable form.
>    * @ssid: source security identifier
> @@ -174,28 +200,11 @@ static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
>   static void avc_dump_query(struct audit_buffer *ab, struct selinux_state *state,
>   			   u32 ssid, u32 tsid, u16 tclass)
>   {
> -	int rc;
> -	char *scontext;
> -	u32 scontext_len;
> -
> -	rc = security_sid_to_context(state, ssid, &scontext, &scontext_len);
> -	if (rc)
> -		audit_log_format(ab, "ssid=%d", ssid);
> -	else {
> -		audit_log_format(ab, "scontext=%s", scontext);
> -		kfree(scontext);
> -	}
> -
> -	rc = security_sid_to_context(state, tsid, &scontext, &scontext_len);
> -	if (rc)
> -		audit_log_format(ab, " tsid=%d", tsid);
> -	else {
> -		audit_log_format(ab, " tcontext=%s", scontext);
> -		kfree(scontext);
> -	}
> +	avc_dump_sid(ab, state, ssid, 's');
> +	avc_dump_sid(ab, state, tsid, 't');
>   
>   	BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -	audit_log_format(ab, " tclass=%s", secclass_map[tclass-1].name);
> +	audit_log_format(ab, "tclass=%s", secclass_map[tclass-1].name);
>   }
>   
>   /**
>