[PATCH v3 0/4] Report raw context in AVCs + refactoring

[PATCH v3 0/4] Report raw context in AVCs + refactoring

[Ondrej Mosnacek]

Changes in v3:
- do some minor refactoring while there
- move new fields to the end of the record
- introduce a new security_sid_to_context_inval() function to get the raw
  context instead of (ab)using strcmp() to check if the raw context is
  different from the effective one

v2: https://lore.kernel.org/selinux/20190121153605.26847-1-omosnace@redhat.com/T/
Changes in v2:
- rename new fields to *rawcon

v1: https://lore.kernel.org/selinux/20190118100429.11703-1-omosnace@redhat.com/T/

Ondrej Mosnacek (4):
  selinux: inline some AVC functions used only once
  selinux: replace some BUG_ON()s with a WARN_ON()
  selinux: remove some useless BUG_ONs
  selinux: log invalid contexts in AVCs

 security/selinux/avc.c              | 159 +++++++++++++---------------
 security/selinux/include/security.h |   3 +
 security/selinux/ss/services.c      |  37 ++++++-
 3 files changed, 109 insertions(+), 90 deletions(-)

-- 
2.20.1

[PATCH v3 0/4] Report raw context in AVCs + refactoring

[Ondrej Mosnacek]

Changes in v3:
- do some minor refactoring while there
- move new fields to the end of the record
- introduce a new security_sid_to_context_inval() function to get the raw
  context instead of (ab)using strcmp() to check if the raw context is
  different from the effective one

v2: https://lore.kernel.org/selinux/20190121153605.26847-1-omosnace@redhat.com/T/
Changes in v2:
- rename new fields to *rawcon

v1: https://lore.kernel.org/selinux/20190118100429.11703-1-omosnace@redhat.com/T/

Ondrej Mosnacek (4):
  selinux: inline some AVC functions used only once
  selinux: replace some BUG_ON()s with a WARN_ON()
  selinux: remove some useless BUG_ONs
  selinux: log invalid contexts in AVCs

 security/selinux/avc.c              | 159 +++++++++++++---------------
 security/selinux/include/security.h |   3 +
 security/selinux/ss/services.c      |  37 ++++++-
 3 files changed, 109 insertions(+), 90 deletions(-)

-- 
2.20.1

[PATCH v3 1/4] selinux: inline some AVC functions used only once

[Ondrej Mosnacek]

avc_dump_av() and avc_dump_query() are each used only in one place. Get
rid of them and open code their contents in the call sites.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c | 140 +++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 82 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 9b63d8ee1687..502162eeb3a0 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -129,75 +129,6 @@ static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
 	return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
 }
 
-/**
- * avc_dump_av - Display an access vector in human-readable form.
- * @tclass: target security class
- * @av: access vector
- */
-static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
-{
-	const char **perms;
-	int i, perm;
-
-	if (av == 0) {
-		audit_log_format(ab, " null");
-		return;
-	}
-
-	BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
-	perms = secclass_map[tclass-1].perms;
-
-	audit_log_format(ab, " {");
-	i = 0;
-	perm = 1;
-	while (i < (sizeof(av) * 8)) {
-		if ((perm & av) && perms[i]) {
-			audit_log_format(ab, " %s", perms[i]);
-			av &= ~perm;
-		}
-		i++;
-		perm <<= 1;
-	}
-
-	if (av)
-		audit_log_format(ab, " 0x%x", av);
-
-	audit_log_format(ab, " }");
-}
-
-/**
- * avc_dump_query - Display a SID pair and a class in human-readable form.
- * @ssid: source security identifier
- * @tsid: target security identifier
- * @tclass: target security class
- */
-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);
-	}
-
-	BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
-	audit_log_format(ab, " tclass=%s", secclass_map[tclass-1].name);
-}
-
 /**
  * avc_init - Initialize the AVC.
  *
@@ -735,11 +666,37 @@ out:
 static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 {
 	struct common_audit_data *ad = a;
-	audit_log_format(ab, "avc:  %s ",
-			 ad->selinux_audit_data->denied ? "denied" : "granted");
-	avc_dump_av(ab, ad->selinux_audit_data->tclass,
-			ad->selinux_audit_data->audited);
-	audit_log_format(ab, " for ");
+	struct selinux_audit_data *sad = ad->selinux_audit_data;
+	u32 av = sad->audited;
+	const char **perms;
+	int i, perm;
+
+	audit_log_format(ab, "avc:  %s ", sad->denied ? "denied" : "granted");
+
+	if (av == 0) {
+		audit_log_string(ab, " null");
+		return;
+	}
+
+	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
+	perms = secclass_map[sad->tclass-1].perms;
+
+	audit_log_string(ab, " {");
+	i = 0;
+	perm = 1;
+	while (i < (sizeof(av) * 8)) {
+		if ((perm & av) && perms[i]) {
+			audit_log_format(ab, " %s", perms[i]);
+			av &= ~perm;
+		}
+		i++;
+		perm <<= 1;
+	}
+
+	if (av)
+		audit_log_format(ab, " 0x%x", av);
+
+	audit_log_string(ab, " } for ");
 }
 
 /**
@@ -751,15 +708,34 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 {
 	struct common_audit_data *ad = a;
-	audit_log_format(ab, " ");
-	avc_dump_query(ab, ad->selinux_audit_data->state,
-		       ad->selinux_audit_data->ssid,
-		       ad->selinux_audit_data->tsid,
-		       ad->selinux_audit_data->tclass);
-	if (ad->selinux_audit_data->denied) {
-		audit_log_format(ab, " permissive=%u",
-				 ad->selinux_audit_data->result ? 0 : 1);
+	struct selinux_audit_data *sad = ad->selinux_audit_data;
+	char *scontext;
+	u32 scontext_len;
+	int rc;
+
+	rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
+				     &scontext_len);
+	if (rc)
+		audit_log_format(ab, " ssid=%d", sad->ssid);
+	else {
+		audit_log_format(ab, " scontext=%s", scontext);
+		kfree(scontext);
 	}
+
+	rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
+				     &scontext_len);
+	if (rc)
+		audit_log_format(ab, " tsid=%d", sad->tsid);
+	else {
+		audit_log_format(ab, " tcontext=%s", scontext);
+		kfree(scontext);
+	}
+
+	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
+	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
+
+	if (sad->denied)
+		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
 }
 
 /* This is the slow part of avc audit with big stack footprint */
-- 
2.20.1

[PATCH v3 1/4] selinux: inline some AVC functions used only once

[Ondrej Mosnacek]

avc_dump_av() and avc_dump_query() are each used only in one place. Get
rid of them and open code their contents in the call sites.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c | 140 +++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 82 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 9b63d8ee1687..502162eeb3a0 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -129,75 +129,6 @@ static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
 	return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
 }
 
-/**
- * avc_dump_av - Display an access vector in human-readable form.
- * @tclass: target security class
- * @av: access vector
- */
-static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
-{
-	const char **perms;
-	int i, perm;
-
-	if (av == 0) {
-		audit_log_format(ab, " null");
-		return;
-	}
-
-	BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
-	perms = secclass_map[tclass-1].perms;
-
-	audit_log_format(ab, " {");
-	i = 0;
-	perm = 1;
-	while (i < (sizeof(av) * 8)) {
-		if ((perm & av) && perms[i]) {
-			audit_log_format(ab, " %s", perms[i]);
-			av &= ~perm;
-		}
-		i++;
-		perm <<= 1;
-	}
-
-	if (av)
-		audit_log_format(ab, " 0x%x", av);
-
-	audit_log_format(ab, " }");
-}
-
-/**
- * avc_dump_query - Display a SID pair and a class in human-readable form.
- * @ssid: source security identifier
- * @tsid: target security identifier
- * @tclass: target security class
- */
-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);
-	}
-
-	BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
-	audit_log_format(ab, " tclass=%s", secclass_map[tclass-1].name);
-}
-
 /**
  * avc_init - Initialize the AVC.
  *
@@ -735,11 +666,37 @@ out:
 static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 {
 	struct common_audit_data *ad = a;
-	audit_log_format(ab, "avc:  %s ",
-			 ad->selinux_audit_data->denied ? "denied" : "granted");
-	avc_dump_av(ab, ad->selinux_audit_data->tclass,
-			ad->selinux_audit_data->audited);
-	audit_log_format(ab, " for ");
+	struct selinux_audit_data *sad = ad->selinux_audit_data;
+	u32 av = sad->audited;
+	const char **perms;
+	int i, perm;
+
+	audit_log_format(ab, "avc:  %s ", sad->denied ? "denied" : "granted");
+
+	if (av == 0) {
+		audit_log_string(ab, " null");
+		return;
+	}
+
+	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
+	perms = secclass_map[sad->tclass-1].perms;
+
+	audit_log_string(ab, " {");
+	i = 0;
+	perm = 1;
+	while (i < (sizeof(av) * 8)) {
+		if ((perm & av) && perms[i]) {
+			audit_log_format(ab, " %s", perms[i]);
+			av &= ~perm;
+		}
+		i++;
+		perm <<= 1;
+	}
+
+	if (av)
+		audit_log_format(ab, " 0x%x", av);
+
+	audit_log_string(ab, " } for ");
 }
 
 /**
@@ -751,15 +708,34 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 {
 	struct common_audit_data *ad = a;
-	audit_log_format(ab, " ");
-	avc_dump_query(ab, ad->selinux_audit_data->state,
-		       ad->selinux_audit_data->ssid,
-		       ad->selinux_audit_data->tsid,
-		       ad->selinux_audit_data->tclass);
-	if (ad->selinux_audit_data->denied) {
-		audit_log_format(ab, " permissive=%u",
-				 ad->selinux_audit_data->result ? 0 : 1);
+	struct selinux_audit_data *sad = ad->selinux_audit_data;
+	char *scontext;
+	u32 scontext_len;
+	int rc;
+
+	rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
+				     &scontext_len);
+	if (rc)
+		audit_log_format(ab, " ssid=%d", sad->ssid);
+	else {
+		audit_log_format(ab, " scontext=%s", scontext);
+		kfree(scontext);
 	}
+
+	rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
+				     &scontext_len);
+	if (rc)
+		audit_log_format(ab, " tsid=%d", sad->tsid);
+	else {
+		audit_log_format(ab, " tcontext=%s", scontext);
+		kfree(scontext);
+	}
+
+	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
+	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
+
+	if (sad->denied)
+		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
 }
 
 /* This is the slow part of avc audit with big stack footprint */
-- 
2.20.1

Re: [PATCH v3 1/4] selinux: inline some AVC functions used only once

[Stephen Smalley]

On 1/25/19 5:06 AM, Ondrej Mosnacek wrote:
> avc_dump_av() and avc_dump_query() are each used only in one place. Get
> rid of them and open code their contents in the call sites.
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/avc.c | 140 +++++++++++++++++------------------------
>   1 file changed, 58 insertions(+), 82 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 9b63d8ee1687..502162eeb3a0 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -129,75 +129,6 @@ static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
>   	return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
>   }
>   
> -/**
> - * avc_dump_av - Display an access vector in human-readable form.
> - * @tclass: target security class
> - * @av: access vector
> - */
> -static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
> -{
> -	const char **perms;
> -	int i, perm;
> -
> -	if (av == 0) {
> -		audit_log_format(ab, " null");
> -		return;
> -	}
> -
> -	BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -	perms = secclass_map[tclass-1].perms;
> -
> -	audit_log_format(ab, " {");
> -	i = 0;
> -	perm = 1;
> -	while (i < (sizeof(av) * 8)) {
> -		if ((perm & av) && perms[i]) {
> -			audit_log_format(ab, " %s", perms[i]);
> -			av &= ~perm;
> -		}
> -		i++;
> -		perm <<= 1;
> -	}
> -
> -	if (av)
> -		audit_log_format(ab, " 0x%x", av);
> -
> -	audit_log_format(ab, " }");
> -}
> -
> -/**
> - * avc_dump_query - Display a SID pair and a class in human-readable form.
> - * @ssid: source security identifier
> - * @tsid: target security identifier
> - * @tclass: target security class
> - */
> -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);
> -	}
> -
> -	BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -	audit_log_format(ab, " tclass=%s", secclass_map[tclass-1].name);
> -}
> -
>   /**
>    * avc_init - Initialize the AVC.
>    *
> @@ -735,11 +666,37 @@ out:
>   static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>   {
>   	struct common_audit_data *ad = a;
> -	audit_log_format(ab, "avc:  %s ",
> -			 ad->selinux_audit_data->denied ? "denied" : "granted");
> -	avc_dump_av(ab, ad->selinux_audit_data->tclass,
> -			ad->selinux_audit_data->audited);
> -	audit_log_format(ab, " for ");
> +	struct selinux_audit_data *sad = ad->selinux_audit_data;
> +	u32 av = sad->audited;
> +	const char **perms;
> +	int i, perm;
> +
> +	audit_log_format(ab, "avc:  %s ", sad->denied ? "denied" : "granted");
> +
> +	if (av == 0) {
> +		audit_log_string(ab, " null");
> +		return;
> +	}
> +
> +	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> +	perms = secclass_map[sad->tclass-1].perms;
> +
> +	audit_log_string(ab, " {");
> +	i = 0;
> +	perm = 1;
> +	while (i < (sizeof(av) * 8)) {
> +		if ((perm & av) && perms[i]) {
> +			audit_log_format(ab, " %s", perms[i]);
> +			av &= ~perm;
> +		}
> +		i++;
> +		perm <<= 1;
> +	}
> +
> +	if (av)
> +		audit_log_format(ab, " 0x%x", av);
> +
> +	audit_log_string(ab, " } for ");
>   }
>   
>   /**
> @@ -751,15 +708,34 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>   static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>   {
>   	struct common_audit_data *ad = a;
> -	audit_log_format(ab, " ");
> -	avc_dump_query(ab, ad->selinux_audit_data->state,
> -		       ad->selinux_audit_data->ssid,
> -		       ad->selinux_audit_data->tsid,
> -		       ad->selinux_audit_data->tclass);
> -	if (ad->selinux_audit_data->denied) {
> -		audit_log_format(ab, " permissive=%u",
> -				 ad->selinux_audit_data->result ? 0 : 1);
> +	struct selinux_audit_data *sad = ad->selinux_audit_data;
> +	char *scontext;
> +	u32 scontext_len;
> +	int rc;
> +
> +	rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
> +				     &scontext_len);
> +	if (rc)
> +		audit_log_format(ab, " ssid=%d", sad->ssid);
> +	else {
> +		audit_log_format(ab, " scontext=%s", scontext);
> +		kfree(scontext);
>   	}
> +
> +	rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
> +				     &scontext_len);
> +	if (rc)
> +		audit_log_format(ab, " tsid=%d", sad->tsid);
> +	else {
> +		audit_log_format(ab, " tcontext=%s", scontext);
> +		kfree(scontext);
> +	}
> +
> +	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> +	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
> +
> +	if (sad->denied)
> +		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
>   }
>   
>   /* This is the slow part of avc audit with big stack footprint */
> 

Re: [PATCH v3 1/4] selinux: inline some AVC functions used only once

[Paul Moore]

On Fri, Jan 25, 2019 at 5:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> avc_dump_av() and avc_dump_query() are each used only in one place. Get
> rid of them and open code their contents in the call sites.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/avc.c | 140 +++++++++++++++++------------------------
>  1 file changed, 58 insertions(+), 82 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 9b63d8ee1687..502162eeb3a0 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -129,75 +129,6 @@ static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
>         return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
>  }
>
> -/**
> - * avc_dump_av - Display an access vector in human-readable form.
> - * @tclass: target security class
> - * @av: access vector
> - */
> -static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
> -{
> -       const char **perms;
> -       int i, perm;
> -
> -       if (av == 0) {
> -               audit_log_format(ab, " null");
> -               return;
> -       }
> -
> -       BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -       perms = secclass_map[tclass-1].perms;
> -
> -       audit_log_format(ab, " {");
> -       i = 0;
> -       perm = 1;
> -       while (i < (sizeof(av) * 8)) {
> -               if ((perm & av) && perms[i]) {
> -                       audit_log_format(ab, " %s", perms[i]);
> -                       av &= ~perm;
> -               }
> -               i++;
> -               perm <<= 1;
> -       }
> -
> -       if (av)
> -               audit_log_format(ab, " 0x%x", av);
> -
> -       audit_log_format(ab, " }");
> -}
> -
> -/**
> - * avc_dump_query - Display a SID pair and a class in human-readable form.
> - * @ssid: source security identifier
> - * @tsid: target security identifier
> - * @tclass: target security class
> - */
> -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);
> -       }
> -
> -       BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -       audit_log_format(ab, " tclass=%s", secclass_map[tclass-1].name);
> -}
> -
>  /**
>   * avc_init - Initialize the AVC.
>   *
> @@ -735,11 +666,37 @@ out:
>  static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>  {
>         struct common_audit_data *ad = a;
> -       audit_log_format(ab, "avc:  %s ",
> -                        ad->selinux_audit_data->denied ? "denied" : "granted");
> -       avc_dump_av(ab, ad->selinux_audit_data->tclass,
> -                       ad->selinux_audit_data->audited);
> -       audit_log_format(ab, " for ");
> +       struct selinux_audit_data *sad = ad->selinux_audit_data;
> +       u32 av = sad->audited;
> +       const char **perms;
> +       int i, perm;
> +
> +       audit_log_format(ab, "avc:  %s ", sad->denied ? "denied" : "granted");
> +
> +       if (av == 0) {
> +               audit_log_string(ab, " null");
> +               return;
> +       }
> +
> +       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> +       perms = secclass_map[sad->tclass-1].perms;
> +
> +       audit_log_string(ab, " {");
> +       i = 0;
> +       perm = 1;
> +       while (i < (sizeof(av) * 8)) {
> +               if ((perm & av) && perms[i]) {
> +                       audit_log_format(ab, " %s", perms[i]);
> +                       av &= ~perm;
> +               }
> +               i++;
> +               perm <<= 1;
> +       }
> +
> +       if (av)
> +               audit_log_format(ab, " 0x%x", av);
> +
> +       audit_log_string(ab, " } for ");
>  }
>
>  /**
> @@ -751,15 +708,34 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>  static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>  {
>         struct common_audit_data *ad = a;
> -       audit_log_format(ab, " ");
> -       avc_dump_query(ab, ad->selinux_audit_data->state,
> -                      ad->selinux_audit_data->ssid,
> -                      ad->selinux_audit_data->tsid,
> -                      ad->selinux_audit_data->tclass);
> -       if (ad->selinux_audit_data->denied) {
> -               audit_log_format(ab, " permissive=%u",
> -                                ad->selinux_audit_data->result ? 0 : 1);
> +       struct selinux_audit_data *sad = ad->selinux_audit_data;
> +       char *scontext;
> +       u32 scontext_len;
> +       int rc;
> +
> +       rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
> +                                    &scontext_len);
> +       if (rc)
> +               audit_log_format(ab, " ssid=%d", sad->ssid);
> +       else {
> +               audit_log_format(ab, " scontext=%s", scontext);
> +               kfree(scontext);
>         }
> +
> +       rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
> +                                    &scontext_len);
> +       if (rc)
> +               audit_log_format(ab, " tsid=%d", sad->tsid);
> +       else {
> +               audit_log_format(ab, " tcontext=%s", scontext);
> +               kfree(scontext);
> +       }
> +
> +       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> +       audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
> +
> +       if (sad->denied)
> +               audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
>  }
>
>  /* This is the slow part of avc audit with big stack footprint */
> --
> 2.20.1
>


-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 1/4] selinux: inline some AVC functions used only once

[Paul Moore]

On Fri, Jan 25, 2019 at 5:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> avc_dump_av() and avc_dump_query() are each used only in one place. Get
> rid of them and open code their contents in the call sites.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/avc.c | 140 +++++++++++++++++------------------------
>  1 file changed, 58 insertions(+), 82 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 9b63d8ee1687..502162eeb3a0 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -129,75 +129,6 @@ static inline int avc_hash(u32 ssid, u32 tsid, u16 tclass)
>         return (ssid ^ (tsid<<2) ^ (tclass<<4)) & (AVC_CACHE_SLOTS - 1);
>  }
>
> -/**
> - * avc_dump_av - Display an access vector in human-readable form.
> - * @tclass: target security class
> - * @av: access vector
> - */
> -static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
> -{
> -       const char **perms;
> -       int i, perm;
> -
> -       if (av == 0) {
> -               audit_log_format(ab, " null");
> -               return;
> -       }
> -
> -       BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -       perms = secclass_map[tclass-1].perms;
> -
> -       audit_log_format(ab, " {");
> -       i = 0;
> -       perm = 1;
> -       while (i < (sizeof(av) * 8)) {
> -               if ((perm & av) && perms[i]) {
> -                       audit_log_format(ab, " %s", perms[i]);
> -                       av &= ~perm;
> -               }
> -               i++;
> -               perm <<= 1;
> -       }
> -
> -       if (av)
> -               audit_log_format(ab, " 0x%x", av);
> -
> -       audit_log_format(ab, " }");
> -}
> -
> -/**
> - * avc_dump_query - Display a SID pair and a class in human-readable form.
> - * @ssid: source security identifier
> - * @tsid: target security identifier
> - * @tclass: target security class
> - */
> -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);
> -       }
> -
> -       BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -       audit_log_format(ab, " tclass=%s", secclass_map[tclass-1].name);
> -}
> -
>  /**
>   * avc_init - Initialize the AVC.
>   *
> @@ -735,11 +666,37 @@ out:
>  static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>  {
>         struct common_audit_data *ad = a;
> -       audit_log_format(ab, "avc:  %s ",
> -                        ad->selinux_audit_data->denied ? "denied" : "granted");
> -       avc_dump_av(ab, ad->selinux_audit_data->tclass,
> -                       ad->selinux_audit_data->audited);
> -       audit_log_format(ab, " for ");
> +       struct selinux_audit_data *sad = ad->selinux_audit_data;
> +       u32 av = sad->audited;
> +       const char **perms;
> +       int i, perm;
> +
> +       audit_log_format(ab, "avc:  %s ", sad->denied ? "denied" : "granted");
> +
> +       if (av == 0) {
> +               audit_log_string(ab, " null");
> +               return;
> +       }
> +
> +       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> +       perms = secclass_map[sad->tclass-1].perms;
> +
> +       audit_log_string(ab, " {");
> +       i = 0;
> +       perm = 1;
> +       while (i < (sizeof(av) * 8)) {
> +               if ((perm & av) && perms[i]) {
> +                       audit_log_format(ab, " %s", perms[i]);
> +                       av &= ~perm;
> +               }
> +               i++;
> +               perm <<= 1;
> +       }
> +
> +       if (av)
> +               audit_log_format(ab, " 0x%x", av);
> +
> +       audit_log_string(ab, " } for ");
>  }
>
>  /**
> @@ -751,15 +708,34 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>  static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>  {
>         struct common_audit_data *ad = a;
> -       audit_log_format(ab, " ");
> -       avc_dump_query(ab, ad->selinux_audit_data->state,
> -                      ad->selinux_audit_data->ssid,
> -                      ad->selinux_audit_data->tsid,
> -                      ad->selinux_audit_data->tclass);
> -       if (ad->selinux_audit_data->denied) {
> -               audit_log_format(ab, " permissive=%u",
> -                                ad->selinux_audit_data->result ? 0 : 1);
> +       struct selinux_audit_data *sad = ad->selinux_audit_data;
> +       char *scontext;
> +       u32 scontext_len;
> +       int rc;
> +
> +       rc = security_sid_to_context(sad->state, sad->ssid, &scontext,
> +                                    &scontext_len);
> +       if (rc)
> +               audit_log_format(ab, " ssid=%d", sad->ssid);
> +       else {
> +               audit_log_format(ab, " scontext=%s", scontext);
> +               kfree(scontext);
>         }
> +
> +       rc = security_sid_to_context(sad->state, sad->tsid, &scontext,
> +                                    &scontext_len);
> +       if (rc)
> +               audit_log_format(ab, " tsid=%d", sad->tsid);
> +       else {
> +               audit_log_format(ab, " tcontext=%s", scontext);
> +               kfree(scontext);
> +       }
> +
> +       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
> +       audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
> +
> +       if (sad->denied)
> +               audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
>  }
>
>  /* This is the slow part of avc audit with big stack footprint */
> --
> 2.20.1
>


-- 
paul moore
www.paul-moore.com

[PATCH v3 2/4] selinux: replace some BUG_ON()s with a WARN_ON()

[Ondrej Mosnacek]

We don't need to crash the machine in these cases. Let's just detect the
buggy state early and error out with a warning.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 502162eeb3a0..5ebad47391c9 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -678,7 +678,6 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 		return;
 	}
 
-	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
 	perms = secclass_map[sad->tclass-1].perms;
 
 	audit_log_string(ab, " {");
@@ -731,7 +730,6 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 		kfree(scontext);
 	}
 
-	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
 	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
 
 	if (sad->denied)
@@ -748,6 +746,9 @@ noinline int slow_avc_audit(struct selinux_state *state,
 	struct common_audit_data stack_data;
 	struct selinux_audit_data sad;
 
+	if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map)))
+		return -EINVAL;
+
 	if (!a) {
 		a = &stack_data;
 		a->type = LSM_AUDIT_DATA_NONE;
-- 
2.20.1

[PATCH v3 2/4] selinux: replace some BUG_ON()s with a WARN_ON()

[Ondrej Mosnacek]

We don't need to crash the machine in these cases. Let's just detect the
buggy state early and error out with a warning.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 502162eeb3a0..5ebad47391c9 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -678,7 +678,6 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
 		return;
 	}
 
-	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
 	perms = secclass_map[sad->tclass-1].perms;
 
 	audit_log_string(ab, " {");
@@ -731,7 +730,6 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 		kfree(scontext);
 	}
 
-	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
 	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
 
 	if (sad->denied)
@@ -748,6 +746,9 @@ noinline int slow_avc_audit(struct selinux_state *state,
 	struct common_audit_data stack_data;
 	struct selinux_audit_data sad;
 
+	if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map)))
+		return -EINVAL;
+
 	if (!a) {
 		a = &stack_data;
 		a->type = LSM_AUDIT_DATA_NONE;
-- 
2.20.1

Re: [PATCH v3 2/4] selinux: replace some BUG_ON()s with a WARN_ON()

[Stephen Smalley]

On 1/25/19 5:06 AM, Ondrej Mosnacek wrote:
> We don't need to crash the machine in these cases. Let's just detect the
> buggy state early and error out with a warning.
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/avc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 502162eeb3a0..5ebad47391c9 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -678,7 +678,6 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>   		return;
>   	}
>   
> -	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
>   	perms = secclass_map[sad->tclass-1].perms;
>   
>   	audit_log_string(ab, " {");
> @@ -731,7 +730,6 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>   		kfree(scontext);
>   	}
>   
> -	BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
>   	audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
>   
>   	if (sad->denied)
> @@ -748,6 +746,9 @@ noinline int slow_avc_audit(struct selinux_state *state,
>   	struct common_audit_data stack_data;
>   	struct selinux_audit_data sad;
>   
> +	if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map)))
> +		return -EINVAL;
> +
>   	if (!a) {
>   		a = &stack_data;
>   		a->type = LSM_AUDIT_DATA_NONE;
> 

Re: [PATCH v3 2/4] selinux: replace some BUG_ON()s with a WARN_ON()

[Paul Moore]

On Fri, Jan 25, 2019 at 5:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> We don't need to crash the machine in these cases. Let's just detect the
> buggy state early and error out with a warning.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/avc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

It's always good to remove BUG_ON()s.  Merged, thanks.

> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 502162eeb3a0..5ebad47391c9 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -678,7 +678,6 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>                 return;
>         }
>
> -       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
>         perms = secclass_map[sad->tclass-1].perms;
>
>         audit_log_string(ab, " {");
> @@ -731,7 +730,6 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>                 kfree(scontext);
>         }
>
> -       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
>         audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
>
>         if (sad->denied)
> @@ -748,6 +746,9 @@ noinline int slow_avc_audit(struct selinux_state *state,
>         struct common_audit_data stack_data;
>         struct selinux_audit_data sad;
>
> +       if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map)))
> +               return -EINVAL;
> +
>         if (!a) {
>                 a = &stack_data;
>                 a->type = LSM_AUDIT_DATA_NONE;
> --
> 2.20.1
>


-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 2/4] selinux: replace some BUG_ON()s with a WARN_ON()

[Paul Moore]

On Fri, Jan 25, 2019 at 5:07 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> We don't need to crash the machine in these cases. Let's just detect the
> buggy state early and error out with a warning.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/avc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

It's always good to remove BUG_ON()s.  Merged, thanks.

> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 502162eeb3a0..5ebad47391c9 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -678,7 +678,6 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a)
>                 return;
>         }
>
> -       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
>         perms = secclass_map[sad->tclass-1].perms;
>
>         audit_log_string(ab, " {");
> @@ -731,7 +730,6 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>                 kfree(scontext);
>         }
>
> -       BUG_ON(!sad->tclass || sad->tclass >= ARRAY_SIZE(secclass_map));
>         audit_log_format(ab, " tclass=%s", secclass_map[sad->tclass-1].name);
>
>         if (sad->denied)
> @@ -748,6 +746,9 @@ noinline int slow_avc_audit(struct selinux_state *state,
>         struct common_audit_data stack_data;
>         struct selinux_audit_data sad;
>
> +       if (WARN_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map)))
> +               return -EINVAL;
> +
>         if (!a) {
>                 a = &stack_data;
>                 a->type = LSM_AUDIT_DATA_NONE;
> --
> 2.20.1
>


-- 
paul moore
www.paul-moore.com

[PATCH v3 3/4] selinux: remove some useless BUG_ONs

[Ondrej Mosnacek]

These BUG_ONs do not really protect from any catastrophic situation so
there is no need to have them there.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 5ebad47391c9..478fa4213c25 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state,
 	int rc = 0, rc2;
 
 	xp_node = &local_xp_node;
-	BUG_ON(!requested);
 
 	rcu_read_lock();
 
@@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
 	int rc = 0;
 	u32 denied;
 
-	BUG_ON(!requested);
-
 	rcu_read_lock();
 
 	node = avc_lookup(state->avc, ssid, tsid, tclass);
-- 
2.20.1

[PATCH v3 3/4] selinux: remove some useless BUG_ONs

[Ondrej Mosnacek]

These BUG_ONs do not really protect from any catastrophic situation so
there is no need to have them there.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 5ebad47391c9..478fa4213c25 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state,
 	int rc = 0, rc2;
 
 	xp_node = &local_xp_node;
-	BUG_ON(!requested);
 
 	rcu_read_lock();
 
@@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
 	int rc = 0;
 	u32 denied;
 
-	BUG_ON(!requested);
-
 	rcu_read_lock();
 
 	node = avc_lookup(state->avc, ssid, tsid, tclass);
-- 
2.20.1

Re: [PATCH v3 3/4] selinux: remove some useless BUG_ONs

[Stephen Smalley]

On 1/25/19 5:06 AM, Ondrej Mosnacek wrote:
> These BUG_ONs do not really protect from any catastrophic situation so
> there is no need to have them there.

They are to catch bugs in callers that pass requested==0.  That is 
always indicative of a bug in the caller (e.g. failed to correctly 
compute the permissions).  Otherwise, we will silently allow such calls 
and not notice them.

At the least, they should be WARN_ONs.

> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>   security/selinux/avc.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 5ebad47391c9..478fa4213c25 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state,
>   	int rc = 0, rc2;
>   
>   	xp_node = &local_xp_node;
> -	BUG_ON(!requested);
>   
>   	rcu_read_lock();
>   
> @@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
>   	int rc = 0;
>   	u32 denied;
>   
> -	BUG_ON(!requested);
> -
>   	rcu_read_lock();
>   
>   	node = avc_lookup(state->avc, ssid, tsid, tclass);
> 

Re: [PATCH v3 3/4] selinux: remove some useless BUG_ONs

[Ondrej Mosnacek]

On Fri, Jan 25, 2019 at 2:49 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 1/25/19 5:06 AM, Ondrej Mosnacek wrote:
> > These BUG_ONs do not really protect from any catastrophic situation so
> > there is no need to have them there.
>
> They are to catch bugs in callers that pass requested==0.  That is
> always indicative of a bug in the caller (e.g. failed to correctly
> compute the permissions).  Otherwise, we will silently allow such calls
> and not notice them.
>
> At the least, they should be WARN_ONs.

OK, seems that switching to WARN_ON() will be a better choice.

Paul, you can apply the series without this patch and I will post a
corrected patch separately (if that's OK with you).

>
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >   security/selinux/avc.c | 3 ---
> >   1 file changed, 3 deletions(-)
> >
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index 5ebad47391c9..478fa4213c25 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state,
> >       int rc = 0, rc2;
> >
> >       xp_node = &local_xp_node;
> > -     BUG_ON(!requested);
> >
> >       rcu_read_lock();
> >
> > @@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
> >       int rc = 0;
> >       u32 denied;
> >
> > -     BUG_ON(!requested);
> > -
> >       rcu_read_lock();
> >
> >       node = avc_lookup(state->avc, ssid, tsid, tclass);
> >
>

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v3 3/4] selinux: remove some useless BUG_ONs

[Ondrej Mosnacek]

On Fri, Jan 25, 2019 at 2:49 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 1/25/19 5:06 AM, Ondrej Mosnacek wrote:
> > These BUG_ONs do not really protect from any catastrophic situation so
> > there is no need to have them there.
>
> They are to catch bugs in callers that pass requested==0.  That is
> always indicative of a bug in the caller (e.g. failed to correctly
> compute the permissions).  Otherwise, we will silently allow such calls
> and not notice them.
>
> At the least, they should be WARN_ONs.

OK, seems that switching to WARN_ON() will be a better choice.

Paul, you can apply the series without this patch and I will post a
corrected patch separately (if that's OK with you).

>
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >   security/selinux/avc.c | 3 ---
> >   1 file changed, 3 deletions(-)
> >
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index 5ebad47391c9..478fa4213c25 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -1044,7 +1044,6 @@ int avc_has_extended_perms(struct selinux_state *state,
> >       int rc = 0, rc2;
> >
> >       xp_node = &local_xp_node;
> > -     BUG_ON(!requested);
> >
> >       rcu_read_lock();
> >
> > @@ -1134,8 +1133,6 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
> >       int rc = 0;
> >       u32 denied;
> >
> > -     BUG_ON(!requested);
> > -
> >       rcu_read_lock();
> >
> >       node = avc_lookup(state->avc, ssid, tsid, tclass);
> >
>

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [PATCH v3 3/4] selinux: remove some useless BUG_ONs

[Paul Moore]

On Fri, Jan 25, 2019 at 11:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, Jan 25, 2019 at 2:49 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 1/25/19 5:06 AM, Ondrej Mosnacek wrote:
> > > These BUG_ONs do not really protect from any catastrophic situation so
> > > there is no need to have them there.
> >
> > They are to catch bugs in callers that pass requested==0.  That is
> > always indicative of a bug in the caller (e.g. failed to correctly
> > compute the permissions).  Otherwise, we will silently allow such calls
> > and not notice them.
> >
> > At the least, they should be WARN_ONs.
>
> OK, seems that switching to WARN_ON() will be a better choice.
>
> Paul, you can apply the series without this patch and I will post a
> corrected patch separately (if that's OK with you).

Yep.  Patches 1, 2, and 4 should now be in selinux/next.

-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 3/4] selinux: remove some useless BUG_ONs

[Paul Moore]

On Fri, Jan 25, 2019 at 11:15 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, Jan 25, 2019 at 2:49 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > On 1/25/19 5:06 AM, Ondrej Mosnacek wrote:
> > > These BUG_ONs do not really protect from any catastrophic situation so
> > > there is no need to have them there.
> >
> > They are to catch bugs in callers that pass requested==0.  That is
> > always indicative of a bug in the caller (e.g. failed to correctly
> > compute the permissions).  Otherwise, we will silently allow such calls
> > and not notice them.
> >
> > At the least, they should be WARN_ONs.
>
> OK, seems that switching to WARN_ON() will be a better choice.
>
> Paul, you can apply the series without this patch and I will post a
> corrected patch separately (if that's OK with you).

Yep.  Patches 1, 2, and 4 should now be in selinux/next.

-- 
paul moore
www.paul-moore.com

[PATCH v3 4/4] selinux: log invalid contexts in AVCs

[Ondrej Mosnacek]

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
(srawcon and trawcon) that report the actual context string if it
differs from the one reported in scontext/tcontext. This is useful for
diagnosing SELinux denials involving invalid contexts.

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 tclass=file permissive=1 trawcon=system_u:object_r:banana_t:s0

Note that it is also possible to encounter this situation with the
'scontext' field - e.g. when a new policy is loaded while a process is
running, whose context is not valid in the new policy.

Cc: Daniel Walsh <dwalsh@redhat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c              | 15 ++++++++++++
 security/selinux/include/security.h |  3 +++
 security/selinux/ss/services.c      | 37 +++++++++++++++++++++++++----
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 478fa4213c25..047de65589bd 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -734,6 +734,21 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 
 	if (sad->denied)
 		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
+
+	/* in case of invalid context report also the actual context string */
+	rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
+					   &scontext_len);
+	if (!rc && scontext) {
+		audit_log_format(ab, " srawcon=%s", scontext);
+		kfree(scontext);
+	}
+
+	rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext,
+					   &scontext_len);
+	if (!rc && scontext) {
+		audit_log_format(ab, " trawcon=%s", scontext);
+		kfree(scontext);
+	}
 }
 
 /* This is the slow part of avc audit with big stack footprint */
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index ba8eedf42b90..f68fb25b5702 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -255,6 +255,9 @@ int security_sid_to_context(struct selinux_state *state, u32 sid,
 int security_sid_to_context_force(struct selinux_state *state,
 				  u32 sid, char **scontext, u32 *scontext_len);
 
+int security_sid_to_context_inval(struct selinux_state *state,
+				  u32 sid, char **scontext, u32 *scontext_len);
+
 int security_context_to_sid(struct selinux_state *state,
 			    const char *scontext, u32 scontext_len,
 			    u32 *out_sid, gfp_t gfp);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index dd44126c8d14..9be05c3e99dc 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1281,7 +1281,8 @@ const char *security_get_initial_sid_context(u32 sid)
 
 static int security_sid_to_context_core(struct selinux_state *state,
 					u32 sid, char **scontext,
-					u32 *scontext_len, int force)
+					u32 *scontext_len, int force,
+					int only_invalid)
 {
 	struct policydb *policydb;
 	struct sidtab *sidtab;
@@ -1326,8 +1327,14 @@ static int security_sid_to_context_core(struct selinux_state *state,
 		rc = -EINVAL;
 		goto out_unlock;
 	}
-	rc = context_struct_to_string(policydb, context, scontext,
-				      scontext_len);
+	if (only_invalid && !context->len) {
+		scontext = NULL;
+		scontext_len = 0;
+		rc = 0;
+	} else {
+		rc = context_struct_to_string(policydb, context, scontext,
+					      scontext_len);
+	}
 out_unlock:
 	read_unlock(&state->ss->policy_rwlock);
 out:
@@ -1349,14 +1356,34 @@ int security_sid_to_context(struct selinux_state *state,
 			    u32 sid, char **scontext, u32 *scontext_len)
 {
 	return security_sid_to_context_core(state, sid, scontext,
-					    scontext_len, 0);
+					    scontext_len, 0, 0);
 }
 
 int security_sid_to_context_force(struct selinux_state *state, u32 sid,
 				  char **scontext, u32 *scontext_len)
 {
 	return security_sid_to_context_core(state, sid, scontext,
-					    scontext_len, 1);
+					    scontext_len, 1, 0);
+}
+
+/**
+ * security_sid_to_context_inval - Obtain a context for a given SID if it
+ *                                 is invalid.
+ * @sid: security identifier, SID
+ * @scontext: security context
+ * @scontext_len: length in bytes
+ *
+ * Write the string representation of the context associated with @sid
+ * into a dynamically allocated string of the correct size, but only if the
+ * context is invalid in the current policy.  Set @scontext to point to
+ * this string (or NULL if the context is valid) and set @scontext_len to
+ * the length of the string (or 0 if the context is valid).
+ */
+int security_sid_to_context_inval(struct selinux_state *state, u32 sid,
+				  char **scontext, u32 *scontext_len)
+{
+	return security_sid_to_context_core(state, sid, scontext,
+					    scontext_len, 1, 1);
 }
 
 /*
-- 
2.20.1

[PATCH v3 4/4] selinux: log invalid contexts in AVCs

[Ondrej Mosnacek]

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
(srawcon and trawcon) that report the actual context string if it
differs from the one reported in scontext/tcontext. This is useful for
diagnosing SELinux denials involving invalid contexts.

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 tclass=file permissive=1 trawcon=system_u:object_r:banana_t:s0

Note that it is also possible to encounter this situation with the
'scontext' field - e.g. when a new policy is loaded while a process is
running, whose context is not valid in the new policy.

Cc: Daniel Walsh <dwalsh@redhat.com>
Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/avc.c              | 15 ++++++++++++
 security/selinux/include/security.h |  3 +++
 security/selinux/ss/services.c      | 37 +++++++++++++++++++++++++----
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 478fa4213c25..047de65589bd 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -734,6 +734,21 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
 
 	if (sad->denied)
 		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
+
+	/* in case of invalid context report also the actual context string */
+	rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
+					   &scontext_len);
+	if (!rc && scontext) {
+		audit_log_format(ab, " srawcon=%s", scontext);
+		kfree(scontext);
+	}
+
+	rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext,
+					   &scontext_len);
+	if (!rc && scontext) {
+		audit_log_format(ab, " trawcon=%s", scontext);
+		kfree(scontext);
+	}
 }
 
 /* This is the slow part of avc audit with big stack footprint */
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index ba8eedf42b90..f68fb25b5702 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -255,6 +255,9 @@ int security_sid_to_context(struct selinux_state *state, u32 sid,
 int security_sid_to_context_force(struct selinux_state *state,
 				  u32 sid, char **scontext, u32 *scontext_len);
 
+int security_sid_to_context_inval(struct selinux_state *state,
+				  u32 sid, char **scontext, u32 *scontext_len);
+
 int security_context_to_sid(struct selinux_state *state,
 			    const char *scontext, u32 scontext_len,
 			    u32 *out_sid, gfp_t gfp);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index dd44126c8d14..9be05c3e99dc 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1281,7 +1281,8 @@ const char *security_get_initial_sid_context(u32 sid)
 
 static int security_sid_to_context_core(struct selinux_state *state,
 					u32 sid, char **scontext,
-					u32 *scontext_len, int force)
+					u32 *scontext_len, int force,
+					int only_invalid)
 {
 	struct policydb *policydb;
 	struct sidtab *sidtab;
@@ -1326,8 +1327,14 @@ static int security_sid_to_context_core(struct selinux_state *state,
 		rc = -EINVAL;
 		goto out_unlock;
 	}
-	rc = context_struct_to_string(policydb, context, scontext,
-				      scontext_len);
+	if (only_invalid && !context->len) {
+		scontext = NULL;
+		scontext_len = 0;
+		rc = 0;
+	} else {
+		rc = context_struct_to_string(policydb, context, scontext,
+					      scontext_len);
+	}
 out_unlock:
 	read_unlock(&state->ss->policy_rwlock);
 out:
@@ -1349,14 +1356,34 @@ int security_sid_to_context(struct selinux_state *state,
 			    u32 sid, char **scontext, u32 *scontext_len)
 {
 	return security_sid_to_context_core(state, sid, scontext,
-					    scontext_len, 0);
+					    scontext_len, 0, 0);
 }
 
 int security_sid_to_context_force(struct selinux_state *state, u32 sid,
 				  char **scontext, u32 *scontext_len)
 {
 	return security_sid_to_context_core(state, sid, scontext,
-					    scontext_len, 1);
+					    scontext_len, 1, 0);
+}
+
+/**
+ * security_sid_to_context_inval - Obtain a context for a given SID if it
+ *                                 is invalid.
+ * @sid: security identifier, SID
+ * @scontext: security context
+ * @scontext_len: length in bytes
+ *
+ * Write the string representation of the context associated with @sid
+ * into a dynamically allocated string of the correct size, but only if the
+ * context is invalid in the current policy.  Set @scontext to point to
+ * this string (or NULL if the context is valid) and set @scontext_len to
+ * the length of the string (or 0 if the context is valid).
+ */
+int security_sid_to_context_inval(struct selinux_state *state, u32 sid,
+				  char **scontext, u32 *scontext_len)
+{
+	return security_sid_to_context_core(state, sid, scontext,
+					    scontext_len, 1, 1);
 }
 
 /*
-- 
2.20.1

Re: [PATCH v3 4/4] selinux: log invalid contexts in AVCs

[Stephen Smalley]

On 1/25/19 5:06 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
> (srawcon and trawcon) that report the actual context string if it
> differs from the one reported in scontext/tcontext. This is useful for
> diagnosing SELinux denials involving invalid contexts.
> 
> 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 tclass=file permissive=1 trawcon=system_u:object_r:banana_t:s0
> 
> Note that it is also possible to encounter this situation with the
> 'scontext' field - e.g. when a new policy is loaded while a process is
> running, whose context is not valid in the new policy.
> 
> Cc: Daniel Walsh <dwalsh@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/avc.c              | 15 ++++++++++++
>   security/selinux/include/security.h |  3 +++
>   security/selinux/ss/services.c      | 37 +++++++++++++++++++++++++----
>   3 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 478fa4213c25..047de65589bd 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -734,6 +734,21 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>   
>   	if (sad->denied)
>   		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
> +
> +	/* in case of invalid context report also the actual context string */
> +	rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
> +					   &scontext_len);
> +	if (!rc && scontext) {
> +		audit_log_format(ab, " srawcon=%s", scontext);
> +		kfree(scontext);
> +	}
> +
> +	rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext,
> +					   &scontext_len);
> +	if (!rc && scontext) {
> +		audit_log_format(ab, " trawcon=%s", scontext);
> +		kfree(scontext);
> +	}
>   }
>   
>   /* This is the slow part of avc audit with big stack footprint */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ba8eedf42b90..f68fb25b5702 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -255,6 +255,9 @@ int security_sid_to_context(struct selinux_state *state, u32 sid,
>   int security_sid_to_context_force(struct selinux_state *state,
>   				  u32 sid, char **scontext, u32 *scontext_len);
>   
> +int security_sid_to_context_inval(struct selinux_state *state,
> +				  u32 sid, char **scontext, u32 *scontext_len);
> +
>   int security_context_to_sid(struct selinux_state *state,
>   			    const char *scontext, u32 scontext_len,
>   			    u32 *out_sid, gfp_t gfp);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index dd44126c8d14..9be05c3e99dc 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1281,7 +1281,8 @@ const char *security_get_initial_sid_context(u32 sid)
>   
>   static int security_sid_to_context_core(struct selinux_state *state,
>   					u32 sid, char **scontext,
> -					u32 *scontext_len, int force)
> +					u32 *scontext_len, int force,
> +					int only_invalid)
>   {
>   	struct policydb *policydb;
>   	struct sidtab *sidtab;
> @@ -1326,8 +1327,14 @@ static int security_sid_to_context_core(struct selinux_state *state,
>   		rc = -EINVAL;
>   		goto out_unlock;
>   	}
> -	rc = context_struct_to_string(policydb, context, scontext,
> -				      scontext_len);
> +	if (only_invalid && !context->len) {
> +		scontext = NULL;
> +		scontext_len = 0;
> +		rc = 0;
> +	} else {
> +		rc = context_struct_to_string(policydb, context, scontext,
> +					      scontext_len);
> +	}
>   out_unlock:
>   	read_unlock(&state->ss->policy_rwlock);
>   out:
> @@ -1349,14 +1356,34 @@ int security_sid_to_context(struct selinux_state *state,
>   			    u32 sid, char **scontext, u32 *scontext_len)
>   {
>   	return security_sid_to_context_core(state, sid, scontext,
> -					    scontext_len, 0);
> +					    scontext_len, 0, 0);
>   }
>   
>   int security_sid_to_context_force(struct selinux_state *state, u32 sid,
>   				  char **scontext, u32 *scontext_len)
>   {
>   	return security_sid_to_context_core(state, sid, scontext,
> -					    scontext_len, 1);
> +					    scontext_len, 1, 0);
> +}
> +
> +/**
> + * security_sid_to_context_inval - Obtain a context for a given SID if it
> + *                                 is invalid.
> + * @sid: security identifier, SID
> + * @scontext: security context
> + * @scontext_len: length in bytes
> + *
> + * Write the string representation of the context associated with @sid
> + * into a dynamically allocated string of the correct size, but only if the
> + * context is invalid in the current policy.  Set @scontext to point to
> + * this string (or NULL if the context is valid) and set @scontext_len to
> + * the length of the string (or 0 if the context is valid).
> + */
> +int security_sid_to_context_inval(struct selinux_state *state, u32 sid,
> +				  char **scontext, u32 *scontext_len)
> +{
> +	return security_sid_to_context_core(state, sid, scontext,
> +					    scontext_len, 1, 1);
>   }
>   
>   /*
> 

Re: [PATCH v3 4/4] selinux: log invalid contexts in AVCs

[Stephen Smalley]

On 1/25/19 5:06 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
> (srawcon and trawcon) that report the actual context string if it
> differs from the one reported in scontext/tcontext. This is useful for
> diagnosing SELinux denials involving invalid contexts.
> 
> 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 tclass=file permissive=1 trawcon=system_u:object_r:banana_t:s0
> 
> Note that it is also possible to encounter this situation with the
> 'scontext' field - e.g. when a new policy is loaded while a process is
> running, whose context is not valid in the new policy.
> 
> Cc: Daniel Walsh <dwalsh@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/avc.c              | 15 ++++++++++++
>   security/selinux/include/security.h |  3 +++
>   security/selinux/ss/services.c      | 37 +++++++++++++++++++++++++----
>   3 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 478fa4213c25..047de65589bd 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -734,6 +734,21 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>   
>   	if (sad->denied)
>   		audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
> +
> +	/* in case of invalid context report also the actual context string */
> +	rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
> +					   &scontext_len);
> +	if (!rc && scontext) {
> +		audit_log_format(ab, " srawcon=%s", scontext);
> +		kfree(scontext);
> +	}
> +
> +	rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext,
> +					   &scontext_len);
> +	if (!rc && scontext) {
> +		audit_log_format(ab, " trawcon=%s", scontext);
> +		kfree(scontext);
> +	}
>   }
>   
>   /* This is the slow part of avc audit with big stack footprint */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ba8eedf42b90..f68fb25b5702 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -255,6 +255,9 @@ int security_sid_to_context(struct selinux_state *state, u32 sid,
>   int security_sid_to_context_force(struct selinux_state *state,
>   				  u32 sid, char **scontext, u32 *scontext_len);
>   
> +int security_sid_to_context_inval(struct selinux_state *state,
> +				  u32 sid, char **scontext, u32 *scontext_len);
> +
>   int security_context_to_sid(struct selinux_state *state,
>   			    const char *scontext, u32 scontext_len,
>   			    u32 *out_sid, gfp_t gfp);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index dd44126c8d14..9be05c3e99dc 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1281,7 +1281,8 @@ const char *security_get_initial_sid_context(u32 sid)
>   
>   static int security_sid_to_context_core(struct selinux_state *state,
>   					u32 sid, char **scontext,
> -					u32 *scontext_len, int force)
> +					u32 *scontext_len, int force,
> +					int only_invalid)
>   {
>   	struct policydb *policydb;
>   	struct sidtab *sidtab;
> @@ -1326,8 +1327,14 @@ static int security_sid_to_context_core(struct selinux_state *state,
>   		rc = -EINVAL;
>   		goto out_unlock;
>   	}
> -	rc = context_struct_to_string(policydb, context, scontext,
> -				      scontext_len);
> +	if (only_invalid && !context->len) {
> +		scontext = NULL;
> +		scontext_len = 0;
> +		rc = 0;
> +	} else {
> +		rc = context_struct_to_string(policydb, context, scontext,
> +					      scontext_len);
> +	}
>   out_unlock:
>   	read_unlock(&state->ss->policy_rwlock);
>   out:
> @@ -1349,14 +1356,34 @@ int security_sid_to_context(struct selinux_state *state,
>   			    u32 sid, char **scontext, u32 *scontext_len)
>   {
>   	return security_sid_to_context_core(state, sid, scontext,
> -					    scontext_len, 0);
> +					    scontext_len, 0, 0);
>   }
>   
>   int security_sid_to_context_force(struct selinux_state *state, u32 sid,
>   				  char **scontext, u32 *scontext_len)
>   {
>   	return security_sid_to_context_core(state, sid, scontext,
> -					    scontext_len, 1);
> +					    scontext_len, 1, 0);
> +}
> +
> +/**
> + * security_sid_to_context_inval - Obtain a context for a given SID if it
> + *                                 is invalid.
> + * @sid: security identifier, SID
> + * @scontext: security context
> + * @scontext_len: length in bytes
> + *
> + * Write the string representation of the context associated with @sid
> + * into a dynamically allocated string of the correct size, but only if the
> + * context is invalid in the current policy.  Set @scontext to point to
> + * this string (or NULL if the context is valid) and set @scontext_len to
> + * the length of the string (or 0 if the context is valid).
> + */
> +int security_sid_to_context_inval(struct selinux_state *state, u32 sid,
> +				  char **scontext, u32 *scontext_len)
> +{
> +	return security_sid_to_context_core(state, sid, scontext,
> +					    scontext_len, 1, 1);
>   }
>   
>   /*
> 

Re: [PATCH v3 4/4] selinux: log invalid contexts in AVCs

[Paul Moore]

On Fri, Jan 25, 2019 at 5:07 AM Ondrej Mosnacek <omosnace@redhat.com> 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
> (srawcon and trawcon) that report the actual context string if it
> differs from the one reported in scontext/tcontext. This is useful for
> diagnosing SELinux denials involving invalid contexts.
>
> 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 tclass=file permissive=1 trawcon=system_u:object_r:banana_t:s0
>
> Note that it is also possible to encounter this situation with the
> 'scontext' field - e.g. when a new policy is loaded while a process is
> running, whose context is not valid in the new policy.
>
> Cc: Daniel Walsh <dwalsh@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/avc.c              | 15 ++++++++++++
>  security/selinux/include/security.h |  3 +++
>  security/selinux/ss/services.c      | 37 +++++++++++++++++++++++++----
>  3 files changed, 50 insertions(+), 5 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 478fa4213c25..047de65589bd 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -734,6 +734,21 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>
>         if (sad->denied)
>                 audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
> +
> +       /* in case of invalid context report also the actual context string */
> +       rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
> +                                          &scontext_len);
> +       if (!rc && scontext) {
> +               audit_log_format(ab, " srawcon=%s", scontext);
> +               kfree(scontext);
> +       }
> +
> +       rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext,
> +                                          &scontext_len);
> +       if (!rc && scontext) {
> +               audit_log_format(ab, " trawcon=%s", scontext);
> +               kfree(scontext);
> +       }
>  }
>
>  /* This is the slow part of avc audit with big stack footprint */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ba8eedf42b90..f68fb25b5702 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -255,6 +255,9 @@ int security_sid_to_context(struct selinux_state *state, u32 sid,
>  int security_sid_to_context_force(struct selinux_state *state,
>                                   u32 sid, char **scontext, u32 *scontext_len);
>
> +int security_sid_to_context_inval(struct selinux_state *state,
> +                                 u32 sid, char **scontext, u32 *scontext_len);
> +
>  int security_context_to_sid(struct selinux_state *state,
>                             const char *scontext, u32 scontext_len,
>                             u32 *out_sid, gfp_t gfp);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index dd44126c8d14..9be05c3e99dc 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1281,7 +1281,8 @@ const char *security_get_initial_sid_context(u32 sid)
>
>  static int security_sid_to_context_core(struct selinux_state *state,
>                                         u32 sid, char **scontext,
> -                                       u32 *scontext_len, int force)
> +                                       u32 *scontext_len, int force,
> +                                       int only_invalid)
>  {
>         struct policydb *policydb;
>         struct sidtab *sidtab;
> @@ -1326,8 +1327,14 @@ static int security_sid_to_context_core(struct selinux_state *state,
>                 rc = -EINVAL;
>                 goto out_unlock;
>         }
> -       rc = context_struct_to_string(policydb, context, scontext,
> -                                     scontext_len);
> +       if (only_invalid && !context->len) {
> +               scontext = NULL;
> +               scontext_len = 0;
> +               rc = 0;
> +       } else {
> +               rc = context_struct_to_string(policydb, context, scontext,
> +                                             scontext_len);
> +       }
>  out_unlock:
>         read_unlock(&state->ss->policy_rwlock);
>  out:
> @@ -1349,14 +1356,34 @@ int security_sid_to_context(struct selinux_state *state,
>                             u32 sid, char **scontext, u32 *scontext_len)
>  {
>         return security_sid_to_context_core(state, sid, scontext,
> -                                           scontext_len, 0);
> +                                           scontext_len, 0, 0);
>  }
>
>  int security_sid_to_context_force(struct selinux_state *state, u32 sid,
>                                   char **scontext, u32 *scontext_len)
>  {
>         return security_sid_to_context_core(state, sid, scontext,
> -                                           scontext_len, 1);
> +                                           scontext_len, 1, 0);
> +}
> +
> +/**
> + * security_sid_to_context_inval - Obtain a context for a given SID if it
> + *                                 is invalid.
> + * @sid: security identifier, SID
> + * @scontext: security context
> + * @scontext_len: length in bytes
> + *
> + * Write the string representation of the context associated with @sid
> + * into a dynamically allocated string of the correct size, but only if the
> + * context is invalid in the current policy.  Set @scontext to point to
> + * this string (or NULL if the context is valid) and set @scontext_len to
> + * the length of the string (or 0 if the context is valid).
> + */
> +int security_sid_to_context_inval(struct selinux_state *state, u32 sid,
> +                                 char **scontext, u32 *scontext_len)
> +{
> +       return security_sid_to_context_core(state, sid, scontext,
> +                                           scontext_len, 1, 1);
>  }
>
>  /*
> --
> 2.20.1
>


-- 
paul moore
www.paul-moore.com

Re: [PATCH v3 4/4] selinux: log invalid contexts in AVCs

[Paul Moore]

On Fri, Jan 25, 2019 at 5:07 AM Ondrej Mosnacek <omosnace@redhat.com> 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
> (srawcon and trawcon) that report the actual context string if it
> differs from the one reported in scontext/tcontext. This is useful for
> diagnosing SELinux denials involving invalid contexts.
>
> 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 tclass=file permissive=1 trawcon=system_u:object_r:banana_t:s0
>
> Note that it is also possible to encounter this situation with the
> 'scontext' field - e.g. when a new policy is loaded while a process is
> running, whose context is not valid in the new policy.
>
> Cc: Daniel Walsh <dwalsh@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/avc.c              | 15 ++++++++++++
>  security/selinux/include/security.h |  3 +++
>  security/selinux/ss/services.c      | 37 +++++++++++++++++++++++++----
>  3 files changed, 50 insertions(+), 5 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 478fa4213c25..047de65589bd 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -734,6 +734,21 @@ static void avc_audit_post_callback(struct audit_buffer *ab, void *a)
>
>         if (sad->denied)
>                 audit_log_format(ab, " permissive=%u", sad->result ? 0 : 1);
> +
> +       /* in case of invalid context report also the actual context string */
> +       rc = security_sid_to_context_inval(sad->state, sad->ssid, &scontext,
> +                                          &scontext_len);
> +       if (!rc && scontext) {
> +               audit_log_format(ab, " srawcon=%s", scontext);
> +               kfree(scontext);
> +       }
> +
> +       rc = security_sid_to_context_inval(sad->state, sad->tsid, &scontext,
> +                                          &scontext_len);
> +       if (!rc && scontext) {
> +               audit_log_format(ab, " trawcon=%s", scontext);
> +               kfree(scontext);
> +       }
>  }
>
>  /* This is the slow part of avc audit with big stack footprint */
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ba8eedf42b90..f68fb25b5702 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -255,6 +255,9 @@ int security_sid_to_context(struct selinux_state *state, u32 sid,
>  int security_sid_to_context_force(struct selinux_state *state,
>                                   u32 sid, char **scontext, u32 *scontext_len);
>
> +int security_sid_to_context_inval(struct selinux_state *state,
> +                                 u32 sid, char **scontext, u32 *scontext_len);
> +
>  int security_context_to_sid(struct selinux_state *state,
>                             const char *scontext, u32 scontext_len,
>                             u32 *out_sid, gfp_t gfp);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index dd44126c8d14..9be05c3e99dc 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1281,7 +1281,8 @@ const char *security_get_initial_sid_context(u32 sid)
>
>  static int security_sid_to_context_core(struct selinux_state *state,
>                                         u32 sid, char **scontext,
> -                                       u32 *scontext_len, int force)
> +                                       u32 *scontext_len, int force,
> +                                       int only_invalid)
>  {
>         struct policydb *policydb;
>         struct sidtab *sidtab;
> @@ -1326,8 +1327,14 @@ static int security_sid_to_context_core(struct selinux_state *state,
>                 rc = -EINVAL;
>                 goto out_unlock;
>         }
> -       rc = context_struct_to_string(policydb, context, scontext,
> -                                     scontext_len);
> +       if (only_invalid && !context->len) {
> +               scontext = NULL;
> +               scontext_len = 0;
> +               rc = 0;
> +       } else {
> +               rc = context_struct_to_string(policydb, context, scontext,
> +                                             scontext_len);
> +       }
>  out_unlock:
>         read_unlock(&state->ss->policy_rwlock);
>  out:
> @@ -1349,14 +1356,34 @@ int security_sid_to_context(struct selinux_state *state,
>                             u32 sid, char **scontext, u32 *scontext_len)
>  {
>         return security_sid_to_context_core(state, sid, scontext,
> -                                           scontext_len, 0);
> +                                           scontext_len, 0, 0);
>  }
>
>  int security_sid_to_context_force(struct selinux_state *state, u32 sid,
>                                   char **scontext, u32 *scontext_len)
>  {
>         return security_sid_to_context_core(state, sid, scontext,
> -                                           scontext_len, 1);
> +                                           scontext_len, 1, 0);
> +}
> +
> +/**
> + * security_sid_to_context_inval - Obtain a context for a given SID if it
> + *                                 is invalid.
> + * @sid: security identifier, SID
> + * @scontext: security context
> + * @scontext_len: length in bytes
> + *
> + * Write the string representation of the context associated with @sid
> + * into a dynamically allocated string of the correct size, but only if the
> + * context is invalid in the current policy.  Set @scontext to point to
> + * this string (or NULL if the context is valid) and set @scontext_len to
> + * the length of the string (or 0 if the context is valid).
> + */
> +int security_sid_to_context_inval(struct selinux_state *state, u32 sid,
> +                                 char **scontext, u32 *scontext_len)
> +{
> +       return security_sid_to_context_core(state, sid, scontext,
> +                                           scontext_len, 1, 1);
>  }
>
>  /*
> --
> 2.20.1
>


-- 
paul moore
www.paul-moore.com