SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] libselinux: fix string conversion of unknown perms
@ 2019-09-10 19:53 Mike Palmiotto
  2019-09-16 20:01 ` Stephen Smalley
  0 siblings, 1 reply; 3+ messages in thread
From: Mike Palmiotto @ 2019-09-10 19:53 UTC (permalink / raw)
  To: selinux; +Cc: Mike Palmiotto

Commit c19395d72295f5e69275d98df5db22dfdf214b6c fixed some handling of unknown
classes/permissions, but missed the case where an unknown permission is loaded
and then subsequently logged, either via denial or auditallow. If a permission
set has some valid values mixed with unknown values, say `{ read write foo }`,
a check on `{ read write foo }` would fail to log the entire set.

To fix this, skip over the bad permissions/classes when expanding them to
strings. The unknowns should be logged during `selinux_set_mapping`, so
there is no need for further logging of the actual unknown permissions.

Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
---
 libselinux/src/stringrep.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
index ad29f76d..85579422 100644
--- a/libselinux/src/stringrep.c
+++ b/libselinux/src/stringrep.c
@@ -276,19 +276,15 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
 	char *ptr;
 
 	/* first pass computes the required length */
-	while (tmp) {
+	for (i = 0; tmp; tmp >>= 1, i++) {
 		if (tmp & 1) {
 			str = security_av_perm_to_string(tclass, av & (1<<i));
-			if (str)
-				len += strlen(str) + 1;
-			else {
-				rc = -1;
-				errno = EINVAL;
-				goto out;
+			if (!str) {
+				continue;
 			}
+
+			len += strlen(str) + 1;
 		}
-		tmp >>= 1;
-		i++;
 	}
 
 	*res = malloc(len);
@@ -298,7 +294,6 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
 	}
 
 	/* second pass constructs the string */
-	i = 0;
 	tmp = av;
 	ptr = *res;
 
@@ -308,12 +303,13 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
 	}
 
 	ptr += sprintf(ptr, "{ ");
-	while (tmp) {
-		if (tmp & 1)
-			ptr += sprintf(ptr, "%s ", security_av_perm_to_string(
-					       tclass, av & (1<<i)));
-		tmp >>= 1;
-		i++;
+	for (i = 0; tmp; tmp >>= 1, i++) {
+		if (tmp & 1) {
+			str = security_av_perm_to_string(tclass, av & (1<<i));
+			if (str) {
+				ptr += sprintf(ptr, "%s ", str);
+			}
+		}
 	}
 	sprintf(ptr, "}");
 out:
-- 
2.21.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] libselinux: fix string conversion of unknown perms
  2019-09-10 19:53 [PATCH] libselinux: fix string conversion of unknown perms Mike Palmiotto
@ 2019-09-16 20:01 ` Stephen Smalley
  2019-09-17 13:20   ` Mike Palmiotto
  0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2019-09-16 20:01 UTC (permalink / raw)
  To: Mike Palmiotto, selinux

On 9/10/19 3:53 PM, Mike Palmiotto wrote:
> Commit c19395d72295f5e69275d98df5db22dfdf214b6c fixed some handling of unknown
> classes/permissions, but missed the case where an unknown permission is loaded
> and then subsequently logged, either via denial or auditallow. If a permission
> set has some valid values mixed with unknown values, say `{ read write foo }`,
> a check on `{ read write foo }` would fail to log the entire set.
> 
> To fix this, skip over the bad permissions/classes when expanding them to
> strings. The unknowns should be logged during `selinux_set_mapping`, so
> there is no need for further logging of the actual unknown permissions.
> 
> Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> ---
>   libselinux/src/stringrep.c | 28 ++++++++++++----------------
>   1 file changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> index ad29f76d..85579422 100644
> --- a/libselinux/src/stringrep.c
> +++ b/libselinux/src/stringrep.c
> @@ -276,19 +276,15 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
>   	char *ptr;
>   
>   	/* first pass computes the required length */
> -	while (tmp) {
> +	for (i = 0; tmp; tmp >>= 1, i++) {

Remove the redundant initialization in the declaration now that you are 
doing it here (which is better, I agree).

>   		if (tmp & 1) {
>   			str = security_av_perm_to_string(tclass, av & (1<<i));
> -			if (str)
> -				len += strlen(str) + 1;
> -			else {
> -				rc = -1;
> -				errno = EINVAL;
> -				goto out;
> +			if (!str) {
> +				continue;
>   			}

No need to bracket it when it is a single statement.

> +
> +			len += strlen(str) + 1;

Might be clearer as:
	if (str)
		len += strlen(str) + 1;
And just let it fall through to the end of the loop otherwise - no need 
for explicit continue here.

>   		}
> -		tmp >>= 1;
> -		i++;
>   	}
>   
>   	*res = malloc(len);
> @@ -298,7 +294,6 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
>   	}
>   
>   	/* second pass constructs the string */
> -	i = 0;
>   	tmp = av;
>   	ptr = *res;
>   
> @@ -308,12 +303,13 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
>   	}
>   
>   	ptr += sprintf(ptr, "{ ");
> -	while (tmp) {
> -		if (tmp & 1)
> -			ptr += sprintf(ptr, "%s ", security_av_perm_to_string(
> -					       tclass, av & (1<<i)));
> -		tmp >>= 1;
> -		i++;
> +	for (i = 0; tmp; tmp >>= 1, i++) {
> +		if (tmp & 1) {
> +			str = security_av_perm_to_string(tclass, av & (1<<i));
> +			if (str) {
> +				ptr += sprintf(ptr, "%s ", str);
> +			}

No need for { } around a single statement.

> +		}
>   	}
>   	sprintf(ptr, "}");
>   out:
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] libselinux: fix string conversion of unknown perms
  2019-09-16 20:01 ` Stephen Smalley
@ 2019-09-17 13:20   ` Mike Palmiotto
  0 siblings, 0 replies; 3+ messages in thread
From: Mike Palmiotto @ 2019-09-17 13:20 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: selinux

On Mon, Sep 16, 2019 at 4:01 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 9/10/19 3:53 PM, Mike Palmiotto wrote:
> > Commit c19395d72295f5e69275d98df5db22dfdf214b6c fixed some handling of unknown
> > classes/permissions, but missed the case where an unknown permission is loaded
> > and then subsequently logged, either via denial or auditallow. If a permission
> > set has some valid values mixed with unknown values, say `{ read write foo }`,
> > a check on `{ read write foo }` would fail to log the entire set.
> >
> > To fix this, skip over the bad permissions/classes when expanding them to
> > strings. The unknowns should be logged during `selinux_set_mapping`, so
> > there is no need for further logging of the actual unknown permissions.
> >
> > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > ---
> >   libselinux/src/stringrep.c | 28 ++++++++++++----------------
> >   1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> > index ad29f76d..85579422 100644
> > --- a/libselinux/src/stringrep.c
> > +++ b/libselinux/src/stringrep.c
> > @@ -276,19 +276,15 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
> >       char *ptr;
> >
> >       /* first pass computes the required length */
> > -     while (tmp) {
> > +     for (i = 0; tmp; tmp >>= 1, i++) {
>
> Remove the redundant initialization in the declaration now that you are
> doing it here (which is better, I agree).
>
> >               if (tmp & 1) {
> >                       str = security_av_perm_to_string(tclass, av & (1<<i));
> > -                     if (str)
> > -                             len += strlen(str) + 1;
> > -                     else {
> > -                             rc = -1;
> > -                             errno = EINVAL;
> > -                             goto out;
> > +                     if (!str) {
> > +                             continue;
> >                       }
>
> No need to bracket it when it is a single statement.
>
> > +
> > +                     len += strlen(str) + 1;
>
> Might be clearer as:
>         if (str)
>                 len += strlen(str) + 1;
> And just let it fall through to the end of the loop otherwise - no need
> for explicit continue here.
>
> >               }
> > -             tmp >>= 1;
> > -             i++;
> >       }
> >
> >       *res = malloc(len);
> > @@ -298,7 +294,6 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
> >       }
> >
> >       /* second pass constructs the string */
> > -     i = 0;
> >       tmp = av;
> >       ptr = *res;
> >
> > @@ -308,12 +303,13 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
> >       }
> >
> >       ptr += sprintf(ptr, "{ ");
> > -     while (tmp) {
> > -             if (tmp & 1)
> > -                     ptr += sprintf(ptr, "%s ", security_av_perm_to_string(
> > -                                            tclass, av & (1<<i)));
> > -             tmp >>= 1;
> > -             i++;
> > +     for (i = 0; tmp; tmp >>= 1, i++) {
> > +             if (tmp & 1) {
> > +                     str = security_av_perm_to_string(tclass, av & (1<<i));
> > +                     if (str) {
> > +                             ptr += sprintf(ptr, "%s ", str);
> > +                     }
>
> No need for { } around a single statement.
>
> > +             }
> >       }
> >       sprintf(ptr, "}");
> >   out:
> >
>

Thanks for the review. Fixed all of the above in v2.

-- 
Mike Palmiotto
https://crunchydata.com

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-10 19:53 [PATCH] libselinux: fix string conversion of unknown perms Mike Palmiotto
2019-09-16 20:01 ` Stephen Smalley
2019-09-17 13:20   ` Mike Palmiotto

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org selinux@archiver.kernel.org
	public-inbox-index selinux


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/ public-inbox