On Mon, May 30, 2022 at 3:09 AM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
>
> 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>
Acked-by: James Carter <jwcart2@xxxxxxxxx>
> ---
> 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
>
