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@xxxxxxxx/ and a
patch to fix it was sent in
https://patchwork.kernel.org/project/selinux/patch/20220127130741.31940-1-jsegitz@xxxxxxx/
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@xxxxxxx>
Cc: Johannes Segitz <jsegitz@xxxxxxx>
---
libselinux/src/procattr.c | 147 +++++++++++++-------------------------
1 file changed, 50 insertions(+), 97 deletions(-)
diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c
index 142fbf3a80e0..6f4cfb82479d 100644
--- a/libselinux/src/procattr.c
+++ b/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 @@ out:
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,8 +236,8 @@ out:
}
}
-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;
@@ -288,62 +245,58 @@ static int setprocattrcon(const char * context,
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);
+}
--
2.36.1
