All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] libselinux: Cached security context not accurate
@ 2022-01-21  8:40 Johannes Segitz
  2022-01-21 12:06 ` Christian Göttsche
  0 siblings, 1 reply; 4+ messages in thread
From: Johannes Segitz @ 2022-01-21  8:40 UTC (permalink / raw)
  To: selinux


[-- 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 --]

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-25 15:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-01-21  8:40 [PATCH] libselinux: Cached security context not accurate Johannes Segitz
2022-01-21 12:06 ` Christian Göttsche
2022-01-24  8:51   ` Johannes Segitz
2022-01-25 15:06     ` Petr Lautrbach

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.