| Message ID | 20220121084012.GS7643@suse.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libselinux: Cached security context not accurate | expand |
On Fri, 21 Jan 2022 at 09:40, Johannes Segitz <jsegitz@suse.de> wrote: > > 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) > > > > - if (prev_context && prev_context != UNSET) { > + if (prev_context && prev_context != UNSET && !pid) { Wouldn't it make logically more sense to first check if pid is zero and then check if the cache is set, cause we never want to access the cache if not operating on out own process? Also isn't setprocattrcon_raw() affected too? diff --git a/libselinux/src/procattr.c b/libselinux/src/procattr.c index 142fbf3a..c7a842ed 100644 --- a/libselinux/src/procattr.c +++ b/libselinux/src/procattr.c @@ -242,9 +242,9 @@ static int setprocattrcon_raw(const char * context, return -1; } - if (!context && !*prev_context) + if (pid == 0 && !context && !*prev_context) return 0; - if (context && *prev_context && *prev_context != UNSET + if (pid == 0 && context && *prev_context && *prev_context != UNSET && !strcmp(context, *prev_context)) return 0; @@ -272,9 +272,11 @@ out: free(context2); return -1; } else { - if (*prev_context != UNSET) - free(*prev_context); - *prev_context = context2; + if (pid == 0) { + if (*prev_context != UNSET) + free(*prev_context); + *prev_context = context2; + } return 0; } }
On Fri, Jan 21, 2022 at 01:06:16PM +0100, Christian Göttsche wrote: > Wouldn't it make logically more sense to first check if pid is zero > and then check if the cache is set, cause we never want to access the > cache if not operating on out own process? Yes, I changed that > Also isn't setprocattrcon_raw() affected too? Of course. I managed to attach the wrong file that only had the change for getprocattrcon_raw. Attached is the full patch Johannes
Johannes Segitz <jsegitz@suse.de> writes: > On Fri, Jan 21, 2022 at 01:06:16PM +0100, Christian Göttsche wrote: >> Wouldn't it make logically more sense to first check if pid is zero >> and then check if the cache is set, cause we never want to access the >> cache if not operating on out own process? > > Yes, I changed that > >> Also isn't setprocattrcon_raw() affected too? > > Of course. I managed to attach the wrong file that only had the change for > getprocattrcon_raw. Attached is the full patch Hello, thanks for the patch. I have only comments on the format. The best way how to send a patch to the mailing list is to use `git send-email`, e.g. $ git send-email --from='Johannes Segitz <jsegitz@suse.de>' --to=selinux@vger.kernel.org --smtp-server=your.smtp.server --confirm=auto -1 Also as stated in CONTRIBUTING.md, the patch description must have signed-off. See https://github.com/SELinuxProject/selinux/pull/336/checks?check_run_id=4559976491 for the guidance. Petr > 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) > 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 (pid == 0 && prev_context && prev_context != UNSET) { > *context = strdup(prev_context); > if (!(*context)) { > return -1; > @@ -242,9 +242,9 @@ static int setprocattrcon_raw(const char > return -1; > } > > - if (!context && !*prev_context) > + if (pid == 0 && !context && !*prev_context) > return 0; > - if (context && *prev_context && *prev_context != UNSET > + if (pid == 0 && context && *prev_context && *prev_context != UNSET > && !strcmp(context, *prev_context)) > return 0; > > @@ -272,9 +272,11 @@ out: > free(context2); > return -1; > } else { > - if (*prev_context != UNSET) > - free(*prev_context); > - *prev_context = context2; > + if (pid == 0) { > + if (*prev_context != UNSET) > + free(*prev_context); > + *prev_context = context2; > + } > return 0; > } > }
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;
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