libselinux: do not return the cached prev_current value when using getpidcon()

libselinux implements a cache mechanism for get*con() functions, such
that when a thread calls setcon(...) then getcon(...), the context is
directly returned. Unfortunately, getpidcon(pid, &context) uses the same
cached variable, so when a program uses setcon("something"), all later
calls to getpidcon(pid, ...) returns "something". This is a bug.

Here is a program which illustrates this bug:

    #include <stdio.h>
    #include <selinux/selinux.h>

    int main() {
        char *context = "";
        if (getpidcon(1, &context) < 0) {
            perror("getpidcon(1)");
        }
        printf("getpidcon(1) = %s\n", context);

        if (getcon(&context) < 0) {
            perror("getcon()");
        }
        printf("getcon() = %s\n", context);
        if (setcon(context) < 0) {
            perror("setcon()");
        }
        if (getpidcon(1, &context) < 0) {
            perror("getpidcon(1)");
        }
        printf("getpidcon(1) = %s\n", context);

        return 0;
    }

On an Arch Linux system using unconfined user, this program displays:

    getpidcon(1) = system_u:system_r:init_t
    getcon() = unconfined_u:unconfined_r:unconfined_t
    getpidcon(1) = unconfined_u:unconfined_r:unconfined_t

With this commit, this program displays:

    getpidcon(1) = system_u:system_r:init_t
    getcon() = unconfined_u:unconfined_r:unconfined_t
    getpidcon(1) = system_u:system_r:init_t

This bug was present in the first commit of
https://github.com/SELinuxProject/selinux git history. It was reported
in https://lore.kernel.org/selinux/20220121084012.GS7643@suse.com/ and a
patch to fix it was sent in
https://patchwork.kernel.org/project/selinux/patch/20220127130741.31940-1-jsegitz@suse.de/
without a clear explanation. This patch added pid checks, which made
sense but were difficult to read. Instead, it is possible to change the
way the functions are called so that they directly know which cache
variable to use.

Moreover, as the code is not clear at all (I spent too much time trying
to understand what the switch did and what the thread-local variable
contained), this commit also reworks libselinux/src/procattr.c to:
- not use hard-to-understand switch/case constructions on strings (they
  are replaced by a new argument filled by macros)
- remove getpidattr_def macro (it was only used once, for pidcon, and
  the code is clearer with one less macro)
- remove the pid parameter of setprocattrcon() and setprocattrcon_raw()
  (it is always zero)

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Cc: Johannes Segitz <jsegitz@suse.de>

Author: fishilico

libselinux/src/procattr.c

@@ -11,11 +11,14 @@
 
 #define UNSET (char *) -1
 
+/* Cached values so that when a thread calls set*con() then gen*con(), the value
+ * which was set is directly returned.
+ */
 static __thread char *prev_current = UNSET;
-static __thread char * prev_exec = UNSET;
-static __thread char * prev_fscreate = UNSET;
-static __thread char * prev_keycreate = UNSET;
-static __thread char * prev_sockcreate = UNSET;
+static __thread char *prev_exec = UNSET;
+static __thread char *prev_fscreate = UNSET;
+static __thread char *prev_keycreate = UNSET;
+static __thread char *prev_sockcreate = UNSET;
 
 static pthread_once_t once = PTHREAD_ONCE_INIT;
 static pthread_key_t destructor_key;
@@ -111,43 +114,18 @@ static int openattr(pid_t pid, const char *attr, int flags)
 	return fd;
 }
 
-static int getprocattrcon_raw(char ** context,
-			      pid_t pid, const char *attr)
+static int getprocattrcon_raw(char **context, pid_t pid, const char *attr,
+			      const char *prev_context)
 {
 	char *buf;
 	size_t size;
 	int fd;
 	ssize_t ret;
 	int errno_hold;
-	char * prev_context;
 
 	__selinux_once(once, init_procattr);
 	init_thread_destructor();
 
-	switch (attr[0]) {
-		case 'c':
-			prev_context = prev_current;
-			break;
-		case 'e':
-			prev_context = prev_exec;
-			break;
-		case 'f':
-			prev_context = prev_fscreate;
-			break;
-		case 'k':
-			prev_context = prev_keycreate;
-			break;
-		case 's':
-			prev_context = prev_sockcreate;
-			break;
-		case 'p':
-			prev_context = NULL;
-			break;
-		default:
-			errno = ENOENT;
-			return -1;
-	}
-
 	if (prev_context && prev_context != UNSET) {
 		*context = strdup(prev_context);
 		if (!(*context)) {
@@ -194,13 +172,13 @@ static int getprocattrcon_raw(char ** context,
 	return ret;
 }
 
-static int getprocattrcon(char ** context,
-			  pid_t pid, const char *attr)
+static int getprocattrcon(char **context, pid_t pid, const char *attr,
+			  const char *prev_context)
 {
 	int ret;
 	char * rcontext;
 
-	ret = getprocattrcon_raw(&rcontext, pid, attr);
+	ret = getprocattrcon_raw(&rcontext, pid, attr, prev_context);
 
 	if (!ret) {
 		ret = selinux_raw_to_trans_context(rcontext, context);
@@ -210,45 +188,24 @@ static int getprocattrcon(char ** context,
 	return ret;
 }
 
-static int setprocattrcon_raw(const char * context,
-			      pid_t pid, const char *attr)
+static int setprocattrcon_raw(const char *context, const char *attr,
+			      char **prev_context)
 {
 	int fd;
 	ssize_t ret;
 	int errno_hold;
-	char **prev_context, *context2 = NULL;
+	char *context2 = NULL;
 
 	__selinux_once(once, init_procattr);
 	init_thread_destructor();
 
-	switch (attr[0]) {
-		case 'c':
-			prev_context = &prev_current;
-			break;
-		case 'e':
-			prev_context = &prev_exec;
-			break;
-		case 'f':
-			prev_context = &prev_fscreate;
-			break;
-		case 'k':
-			prev_context = &prev_keycreate;
-			break;
-		case 's':
-			prev_context = &prev_sockcreate;
-			break;
-		default:
-			errno = ENOENT;
-			return -1;
-	}
-
 	if (!context && !*prev_context)
 		return 0;
 	if (context && *prev_context && *prev_context != UNSET
 	    && !strcmp(context, *prev_context))
 		return 0;
 
-	fd = openattr(pid, attr, O_RDWR | O_CLOEXEC);
+	fd = openattr(0, attr, O_RDWR | O_CLOEXEC);
 	if (fd < 0)
 		return -1;
 	if (context) {
@@ -279,71 +236,67 @@ static int setprocattrcon_raw(const char * context,
 	}
 }
 
-static int setprocattrcon(const char * context,
-			  pid_t pid, const char *attr)
+static int setprocattrcon(const char *context, const char *attr,
+			  char **prev_context)
 {
 	int ret;
 	char * rcontext;
 
 	if (selinux_trans_to_raw_context(context, &rcontext))
 		return -1;
 
-	ret = setprocattrcon_raw(rcontext, pid, attr);
+	ret = setprocattrcon_raw(rcontext, attr, prev_context);
 
 	freecon(rcontext);
 
 	return ret;
 }
 
-#define getselfattr_def(fn, attr) \
+#define getselfattr_def(fn, attr, prev_context) \
 	int get##fn##_raw(char **c) \
 	{ \
-		return getprocattrcon_raw(c, 0, #attr); \
+		return getprocattrcon_raw(c, 0, attr, prev_context); \
 	} \
 	int get##fn(char **c) \
 	{ \
-		return getprocattrcon(c, 0, #attr); \
+		return getprocattrcon(c, 0, attr, prev_context); \
 	}
 
-#define setselfattr_def(fn, attr) \
+#define setselfattr_def(fn, attr, prev_context) \
 	int set##fn##_raw(const char * c) \
 	{ \
-		return setprocattrcon_raw(c, 0, #attr); \
+		return setprocattrcon_raw(c, attr, &prev_context); \
 	} \
 	int set##fn(const char * c) \
 	{ \
-		return setprocattrcon(c, 0, #attr); \
+		return setprocattrcon(c, attr, &prev_context); \
 	}
 
-#define all_selfattr_def(fn, attr) \
-	getselfattr_def(fn, attr)	 \
-	setselfattr_def(fn, attr)
+#define all_selfattr_def(fn, attr, prev_context) \
+	getselfattr_def(fn, attr, prev_context)	 \
+	setselfattr_def(fn, attr, prev_context)
 
-#define getpidattr_def(fn, attr) \
-	int get##fn##_raw(pid_t pid, char **c)	\
-	{ \
-		if (pid <= 0) { \
-			errno = EINVAL; \
-			return -1; \
-		} else { \
-			return getprocattrcon_raw(c, pid, #attr); \
-		} \
-	} \
-	int get##fn(pid_t pid, char **c)	\
-	{ \
-		if (pid <= 0) { \
-			errno = EINVAL; \
-			return -1; \
-		} else { \
-			return getprocattrcon(c, pid, #attr); \
-		} \
-	}
+all_selfattr_def(con, "current", prev_current)
+    getselfattr_def(prevcon, "prev", NULL)
+    all_selfattr_def(execcon, "exec", prev_exec)
+    all_selfattr_def(fscreatecon, "fscreate", prev_fscreate)
+    all_selfattr_def(sockcreatecon, "sockcreate", prev_sockcreate)
+    all_selfattr_def(keycreatecon, "keycreate", prev_keycreate)
 
-all_selfattr_def(con, current)
-    getpidattr_def(pidcon, current)
-    getselfattr_def(prevcon, prev)
-    all_selfattr_def(execcon, exec)
-    all_selfattr_def(fscreatecon, fscreate)
-    all_selfattr_def(sockcreatecon, sockcreate)
-    all_selfattr_def(keycreatecon, keycreate)
+int getpidcon_raw(pid_t pid, char **c)
+{
+	if (pid <= 0) {
+		errno = EINVAL;
+		return -1;
+	}
+	return getprocattrcon_raw(c, pid, "current", NULL);
+}
 
+int getpidcon(pid_t pid, char **c)
+{
+	if (pid <= 0) {
+		errno = EINVAL;
+		return -1;
+	}
+	return getprocattrcon(c, pid, "current", NULL);
+}