[PATCH] libselinux: Cached security context not accurate

[PATCH] libselinux: Cached security context not accurate

[Johannes Segitz]

[-- Attachment #1.1: Type: text/plain, Size: 4383 bytes --]

Hello,

a colleague of mine (Olaf Kirch) identified a problem with libselinux that
has a small security impact. I discussed this with some people on this list
privately before and we think it doesn't have a big impact and can be
discussed here without harm.

The reproducer below works on SUSE and RH systems.
# gcc -Wall -o test -lselinux test.c
# ./test
Parent: child context according to library unconfined_u:unconfined_r:unconfined_t:s0
Parent: child context according to procfs: unconfined_u:unconfined_r:passwd_t:s0
bummer, context mismatch

You need to make you system permissive to allow the initial context change.

The problem is the procattr cache in libselinux, which doesn't work properly.
Attached is a small patch that "fixes" the issue at the cost of not using the
cache as soon as a pid is specified.

With this applied we see the correct result:
# ./test
Parent: child context according to library unconfined_u:unconfined_r:passwd_t:s0
Parent: child context according to procfs: unconfined_u:unconfined_r:passwd_t:s0

In most use cases this issue will not be much of a problem, but it's still
a small security issue, since this incorrect information might lead to
incorrect access decisions. This way of doing it is inherently racy and
should not be used for security decision, it's also probably pretty rare to
see this. ATM not CVE was assigned. Strictly speaking we probably need one,
but I won't push for it given the low impact.

Thanks,
Johannes

/*
 * This demonstrates some odd behavior in libselinux.
 *
 * procattr query functions seem to return cached content from
 * previous calls to the corresponding set function, but without
 * taking the pid into account.
 *
 * This means that getpidcon() returns the context installed
 * by the most recent setcon() call, rather than the actual
 * context of the process we wanted to query.
 *
 * Apparently, this was introduced by commit 1d403326a
 *
 * Enjoy,
 * Olaf Kirch <okir at suse.de>
 */
#include <selinux/selinux.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <signal.h>
#include <unistd.h>

static void
fatal(const char *msg)
{
        perror(msg);
        exit(1);
}

static char *
getpidcon_sinatra(pid_t pid)
{
        static char linebuf[128];
        char path[1024];
        FILE *fp;

        snprintf(path, sizeof(path), "/proc/%d/attr/current", pid);
        if ((fp = fopen(path, "r")) == NULL)
                fatal(path);

        if (fgets(linebuf, sizeof(linebuf), fp) == NULL)
                fatal("read from /proc file");
        fclose(fp);

        return linebuf;
}

int
main(void)
{
        char *lib_context = NULL, *actual_context;
        pid_t pid;

        pid = fork();
        if (pid < 0)
                fatal("fork");

        if (pid == 0) {
                if (setcon("unconfined_u:unconfined_r:passwd_t:s0") < 0)
                        fatal("child setcon");

                sleep(15);
                exit(0);
        }

        /* Set my own process context.
         * This will addle the library's brain by setting prev_current
         * in procattr.c
         */
        if (setcon("unconfined_u:unconfined_r:unconfined_t:s0") < 0)
                fatal("parent setcon");

        /* Wait for the child process to complete initialization */
        sleep(1);

        /* Ask the library about the security context */
        if (getpidcon(pid, &lib_context) < 0)
                fatal("getpidcon");
        printf("Parent: child context according to library %s\n", lib_context);

        /* Query /proc/$pid/attr/current directly, which is what
         * the library is supposed to do. */
        actual_context = getpidcon_sinatra(pid);
        printf("Parent: child context according to procfs: %s\n", actual_context);

        kill(pid, 9);

        if (strcmp(lib_context, actual_context) != 0) {
                printf("bummer, context mismatch\n");
                return 1;
        }

        return 0;
}


Johannes
-- 
GPG Key                EE16 6BCE AD56 E034 BFB3  3ADD 7BF7 29D5 E7C8 1FA0
Subkey fingerprint:    250F 43F5 F7CE 6F1E 9C59  4F95 BC27 DD9D 2CC4 FD66
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg
Geschäftsführer: Ivo Totev (HRB 36809, AG Nürnberg)

[-- Attachment #1.2: libselinux_procattr_cache.patch --]
[-- Type: text/x-patch, Size: 448 bytes --]

Index: libselinux-3.3/src/procattr.c
===================================================================
--- libselinux-3.3.orig/src/procattr.c
+++ libselinux-3.3/src/procattr.c
@@ -148,7 +148,7 @@ static int getprocattrcon_raw(char ** co
 			return -1;
 	}
 
-	if (prev_context && prev_context != UNSET) {
+	if (prev_context && prev_context != UNSET && !pid) {
 		*context = strdup(prev_context);
 		if (!(*context)) {
 			return -1;

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libselinux: Cached security context not accurate

[Christian Göttsche]

On Fri, 21 Jan 2022 at 09:40, Johannes Segitz <jsegitz@suse.de> wrote:
>
> Hello,
>
> a colleague of mine (Olaf Kirch) identified a problem with libselinux that
> has a small security impact. I discussed this with some people on this list
> privately before and we think it doesn't have a big impact and can be
> discussed here without harm.
>
> The reproducer below works on SUSE and RH systems.
> # gcc -Wall -o test -lselinux test.c
> # ./test
> Parent: child context according to library unconfined_u:unconfined_r:unconfined_t:s0
> Parent: child context according to procfs: unconfined_u:unconfined_r:passwd_t:s0
> bummer, context mismatch
>
> You need to make you system permissive to allow the initial context change.
>
> The problem is the procattr cache in libselinux, which doesn't work properly.
> Attached is a small patch that "fixes" the issue at the cost of not using the
> cache as soon as a pid is specified.
>
> With this applied we see the correct result:
> # ./test
> Parent: child context according to library unconfined_u:unconfined_r:passwd_t:s0
> Parent: child context according to procfs: unconfined_u:unconfined_r:passwd_t:s0
>
> In most use cases this issue will not be much of a problem, but it's still
> a small security issue, since this incorrect information might lead to
> incorrect access decisions. This way of doing it is inherently racy and
> should not be used for security decision, it's also probably pretty rare to
> see this. ATM not CVE was assigned. Strictly speaking we probably need one,
> but I won't push for it given the low impact.
>
> Thanks,
> Johannes
>
> /*
>  * This demonstrates some odd behavior in libselinux.
>  *
>  * procattr query functions seem to return cached content from
>  * previous calls to the corresponding set function, but without
>  * taking the pid into account.
>  *
>  * This means that getpidcon() returns the context installed
>  * by the most recent setcon() call, rather than the actual
>  * context of the process we wanted to query.
>  *
>  * Apparently, this was introduced by commit 1d403326a
>  *
>  * Enjoy,
>  * Olaf Kirch <okir at suse.de>
>  */
> #include <selinux/selinux.h>
> #include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
> #include <signal.h>
> #include <unistd.h>
>
> static void
> fatal(const char *msg)
> {
>         perror(msg);
>         exit(1);
> }
>
> static char *
> getpidcon_sinatra(pid_t pid)
> {
>         static char linebuf[128];
>         char path[1024];
>         FILE *fp;
>
>         snprintf(path, sizeof(path), "/proc/%d/attr/current", pid);
>         if ((fp = fopen(path, "r")) == NULL)
>                 fatal(path);
>
>         if (fgets(linebuf, sizeof(linebuf), fp) == NULL)
>                 fatal("read from /proc file");
>         fclose(fp);
>
>         return linebuf;
> }
>
> int
> main(void)
> {
>         char *lib_context = NULL, *actual_context;
>         pid_t pid;
>
>         pid = fork();
>         if (pid < 0)
>                 fatal("fork");
>
>         if (pid == 0) {
>                 if (setcon("unconfined_u:unconfined_r:passwd_t:s0") < 0)
>                         fatal("child setcon");
>
>                 sleep(15);
>                 exit(0);
>         }
>
>         /* Set my own process context.
>          * This will addle the library's brain by setting prev_current
>          * in procattr.c
>          */
>         if (setcon("unconfined_u:unconfined_r:unconfined_t:s0") < 0)
>                 fatal("parent setcon");
>
>         /* Wait for the child process to complete initialization */
>         sleep(1);
>
>         /* Ask the library about the security context */
>         if (getpidcon(pid, &lib_context) < 0)
>                 fatal("getpidcon");
>         printf("Parent: child context according to library %s\n", lib_context);
>
>         /* Query /proc/$pid/attr/current directly, which is what
>          * the library is supposed to do. */
>         actual_context = getpidcon_sinatra(pid);
>         printf("Parent: child context according to procfs: %s\n", actual_context);
>
>         kill(pid, 9);
>
>         if (strcmp(lib_context, actual_context) != 0) {
>                 printf("bummer, context mismatch\n");
>                 return 1;
>         }
>
>         return 0;
> }
>
>
> Johannes
> --
> GPG Key                EE16 6BCE AD56 E034 BFB3  3ADD 7BF7 29D5 E7C8 1FA0
> Subkey fingerprint:    250F 43F5 F7CE 6F1E 9C59  4F95 BC27 DD9D 2CC4 FD66
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg
> Geschäftsführer: Ivo Totev (HRB 36809, AG Nürnberg)
>
>
>
> - if (prev_context && prev_context != UNSET) {
> + if (prev_context && prev_context != UNSET && !pid) {

Wouldn't it make logically more sense to first check if pid is zero
and then check if the cache is set, cause we never want to access the
cache if not operating on out own process?

Also isn't setprocattrcon_raw() affected too?

diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
index 142fbf3a..c7a842ed 100644
--- a/libselinux/src/procattr.c
+++ b/libselinux/src/procattr.c
@@ -242,9 +242,9 @@ static int setprocattrcon_raw(const char * context,
                       return -1;
       }

-       if (!context && !*prev_context)
+       if (pid == 0 && !context && !*prev_context)
               return 0;
-       if (context && *prev_context && *prev_context != UNSET
+       if (pid == 0 && context && *prev_context && *prev_context != UNSET
           && !strcmp(context, *prev_context))
               return 0;

@@ -272,9 +272,11 @@ out:
               free(context2);
               return -1;
       } else {
-               if (*prev_context != UNSET)
-                       free(*prev_context);
-               *prev_context = context2;
+               if (pid == 0) {
+                       if (*prev_context != UNSET)
+                               free(*prev_context);
+                       *prev_context = context2;
+               }
               return 0;
       }
}

Re: [PATCH] libselinux: Cached security context not accurate

[Johannes Segitz]

[-- Attachment #1.1: Type: text/plain, Size: 749 bytes --]

On Fri, Jan 21, 2022 at 01:06:16PM +0100, Christian Göttsche wrote:
> Wouldn't it make logically more sense to first check if pid is zero
> and then check if the cache is set, cause we never want to access the
> cache if not operating on out own process?

Yes, I changed that

> Also isn't setprocattrcon_raw() affected too?

Of course. I managed to attach the wrong file that only had the change for
getprocattrcon_raw. Attached is the full patch

Johannes
-- 
GPG Key                EE16 6BCE AD56 E034 BFB3  3ADD 7BF7 29D5 E7C8 1FA0
Subkey fingerprint:    250F 43F5 F7CE 6F1E 9C59  4F95 BC27 DD9D 2CC4 FD66
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg
Geschäftsführer: Ivo Totev (HRB 36809, AG Nürnberg)

[-- Attachment #1.2: libselinux_procattr_cache.patch --]
[-- Type: text/x-patch, Size: 1124 bytes --]

Index: libselinux-3.3/src/procattr.c
===================================================================
--- libselinux-3.3.orig/src/procattr.c
+++ libselinux-3.3/src/procattr.c
@@ -148,7 +148,7 @@ static int getprocattrcon_raw(char ** co
 			return -1;
 	}
 
-	if (prev_context && prev_context != UNSET) {
+	if (pid == 0 && prev_context && prev_context != UNSET) {
 		*context = strdup(prev_context);
 		if (!(*context)) {
 			return -1;
@@ -242,9 +242,9 @@ static int setprocattrcon_raw(const char
 			return -1;
 	}
 
-	if (!context && !*prev_context)
+	if (pid == 0 && !context && !*prev_context)
 		return 0;
-	if (context && *prev_context && *prev_context != UNSET
+	if (pid == 0 && context && *prev_context && *prev_context != UNSET
 	    && !strcmp(context, *prev_context))
 		return 0;
 
@@ -272,9 +272,11 @@ out:
 		free(context2);
 		return -1;
 	} else {
-		if (*prev_context != UNSET)
-			free(*prev_context);
-		*prev_context = context2;
+		if (pid == 0) {
+			if (*prev_context != UNSET)
+				free(*prev_context);
+			*prev_context = context2;
+		}
 		return 0;
 	}
 }

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libselinux: Cached security context not accurate

[Petr Lautrbach]

Johannes Segitz <jsegitz@suse.de> writes:

> On Fri, Jan 21, 2022 at 01:06:16PM +0100, Christian Göttsche wrote:
>> Wouldn't it make logically more sense to first check if pid is zero
>> and then check if the cache is set, cause we never want to access the
>> cache if not operating on out own process?
>
> Yes, I changed that
>
>> Also isn't setprocattrcon_raw() affected too?
>
> Of course. I managed to attach the wrong file that only had the change for
> getprocattrcon_raw. Attached is the full patch

Hello,

thanks for the patch. I have only comments on the format.

The best way how to send a patch to the mailing list is to use `git send-email`, e.g.

   $ git send-email --from='Johannes Segitz <jsegitz@suse.de>' --to=selinux@vger.kernel.org --smtp-server=your.smtp.server --confirm=auto -1

Also as stated in CONTRIBUTING.md, the patch description must have signed-off.
See
https://github.com/SELinuxProject/selinux/pull/336/checks?check_run_id=4559976491
for the guidance.


Petr

> Johannes
> -- 
> GPG Key                EE16 6BCE AD56 E034 BFB3  3ADD 7BF7 29D5 E7C8 1FA0
> Subkey fingerprint:    250F 43F5 F7CE 6F1E 9C59  4F95 BC27 DD9D 2CC4 FD66
> SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg
> Geschäftsführer: Ivo Totev (HRB 36809, AG Nürnberg)
> Index: libselinux-3.3/src/procattr.c
> ===================================================================
> --- libselinux-3.3.orig/src/procattr.c
> +++ libselinux-3.3/src/procattr.c
> @@ -148,7 +148,7 @@ static int getprocattrcon_raw(char ** co
>  			return -1;
>  	}
>  
> -	if (prev_context && prev_context != UNSET) {
> +	if (pid == 0 && prev_context && prev_context != UNSET) {
>  		*context = strdup(prev_context);
>  		if (!(*context)) {
>  			return -1;
> @@ -242,9 +242,9 @@ static int setprocattrcon_raw(const char
>  			return -1;
>  	}
>  
> -	if (!context && !*prev_context)
> +	if (pid == 0 && !context && !*prev_context)
>  		return 0;
> -	if (context && *prev_context && *prev_context != UNSET
> +	if (pid == 0 && context && *prev_context && *prev_context != UNSET
>  	    && !strcmp(context, *prev_context))
>  		return 0;
>  
> @@ -272,9 +272,11 @@ out:
>  		free(context2);
>  		return -1;
>  	} else {
> -		if (*prev_context != UNSET)
> -			free(*prev_context);
> -		*prev_context = context2;
> +		if (pid == 0) {
> +			if (*prev_context != UNSET)
> +				free(*prev_context);
> +			*prev_context = context2;
> +		}
>  		return 0;
>  	}
>  }