On Thu, Jun 2, 2022 at 9:56 AM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> 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>
>
Merged.
Thanks,
Jim
> > ---
> > 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
> >
